Add optional GenericTensorNetworks backend#31
Conversation
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
GitHub would not allow REQUEST_CHANGES because the authenticated account owns this PR, but the findings below are blocking and should be treated as request-changes.
Review summary:
- Blocking issues:
GTNBackend.k > 1paths exposed by the public API are broken for Float64 QUBOs.:kbest_sizesfails when combining GTN/Tropical sizes with the objective constant, and:configs/:countfail before TenSolver can return a result. These need either implementation plus tests or explicit rejection of unsupported combinations. - Nonblocking issues: the one-variable DMRG path silently drops unknown kwargs, and
solution_spacegives an opaque failure when used with DMRG. - Questions: none.
- Tests run and outcomes:
julia +1.12 --project=. -e 'using Pkg; Pkg.test(; coverage=false)'passed;julia +1.12 --project=docs/ docs/make.jlpassed; targetedGTNBackend(k=2)smoke tests failed as noted inline. - Merge recommendation: I would not merge this until the blocking issues above are addressed.
|
Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up items:
|
bernalde
changed the title
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Senior maintainer review. Note: I am the PR author, so GitHub only permits a COMMENT event from me; a formal approval or change-request must come from another maintainer. No blocking issues found.
Diff verification: merge-base ca27880; git diff <merge-base>..HEAD --stat = 16 files, +720 / -15, matching the PR's reported totals. No issue is referenced (no Closes/Fixes #N); reviewed on the merits.
Optionality / extension correctness — clean:
- GenericTensorNetworks and ProblemReductions are in
[weakdeps]+[extensions], NOT in core[deps]. CorePkg.instantiate()succeeds without them. - Extension key, module name (
TenSolverGenericTensorNetworksExt), and filename all match;julia = "1.10"satisfies the >= 1.9 requirement for extensions. - When the deps are absent, the
GTNBackendstub insrc/backends.jlthrows a helpfulArgumentErrordirecting the user tousing GenericTensorNetworks, ProblemReductions.
Correctness — validated empirically (in an isolated env with the extension deps): QUBO min E=2.0 and maximize E=3.0 with correct configs (sign/sense preserved through _with_objective); :count/:configs/:kbest correct; degree-3 PUBO matches brute force; asymmetric Q matches brute force (confirms Q[i,j]+Q[j,i] symmetrization into ProblemReductions.QUBO). DMRG remains the default everywhere.
Nonblocking
ext/TenSolverGenericTensorNetworksExt.jl:166:metadata["contraction_complexity"] = GenericTensorNetworks.contraction_complexity(gtn)is not wrapped in try/catch, unlike the adjacentestimate_memorycall. If it ever throws for some optimizer/problem the whole solve fails after the answer is computed. Wrap it defensively likeestimate_memory.test/gtn.jlguards the GTN tests withtry import GenericTensorNetworks, but the deps are listed intest/Project.toml [deps], so they are always installed and the skip branch is dead in CI. The "graceful skip on missing dep" path is therefore never actually exercised; if graceful degradation is a goal, it isn't tested.src/objective.jlevaluate:selected &= !iszero(xs[i])treats any nonzero entry as 1, so fractional/invalid input silently yields wrong energies. Document that inputs must be 0/1, or validate.
Questions
4. There is no test exercising the JuMP/MOI path with backend = "gtn" — only the direct-API GTN path is tested. The gtn_T :: Type raw attribute and attribute plumbing are untested end-to-end. Add one JuMP-level test selecting the GTN backend.
5. The PR body says validation used Julia 1.12 "because the manifest is generated for 1.12." No Manifest.toml is tracked and Project.toml declares julia = "1.10"; I validated on 1.10.11. The body's claim is inaccurate.
Tests run (Julia 1.10.11):
julia --project=. -e 'using Pkg; Pkg.instantiate()'— success (core env, no GTN deps needed).julia --project=. -e 'using Pkg; Pkg.test()'— passed (exit 0,Testing TenSolver tests passed); with GTN deps installed viatest/Project.toml,test/gtn.jlran for real. QUBODrivers.jl 130, Aqua.jl 11, VRP 2, HDF5 16, Doctests 1.- Direct GTN validation in an isolated env: min/max/count/configs/kbest/PUBO/asymmetric-Q all matched expected/brute-force.
Merge readiness: Ready to merge from a correctness/optionality standpoint — optional backend done correctly via weakdeps+extension (not a hard dep), results correct and sign-consistent with DMRG, DMRG remains default, full suite green on 1.10. Recommended follow-ups (non-blocking): a JuMP-level backend = "gtn" test, wrap contraction_complexity in try/catch, and note the gtn.jl skip-guard never runs the missing-dep path in CI. The formal verdict must come from another maintainer.
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Senior maintainer review of PR #31 "Add optional GenericTensorNetworks backend". Note: I am the PR author under this posting identity, so GitHub will not let me file APPROVE/REQUEST_CHANGES on my own PR. This is submitted as COMMENT; the formal approving/blocking verdict must come from another maintainer. All findings, including blocking ones, are below.
Scope check: diff matches the reported totals (16 files, +720/-15) against merge-base ca27880. CI is green on all 9 Julia jobs plus build/docs at head c96eb40. The optional backend is correctly implemented as a Julia package extension via [weakdeps] + [extensions] (GenericTensorNetworks + ProblemReductions are weakdeps, not hard deps), and both are added to test/Project.toml so the extension loads under CI. GTNBackend without the extension loaded throws a clear ArgumentError. That part of the design is sound.
Blocking 1 — PR is stale and conflicts with current main (v0.2.0). GitHub reports this PR mergeable=CONFLICTING / mergeStateStatus=DIRTY. It was branched from old main ca27880 and is now behind_by 5 commits. The branch still carries version = "0.1.0", QUBODrivers = "0.3", and QUBOTools = "0.10" in Project.toml, whereas main is at version = "0.2.0", QUBODrivers = "0.6.1", QUBOTools = "0.13" (merged via #53/#54). These exact compat lines are the conflict. Why it matters: it cannot merge as-is, and if force-merged it would silently regress the QUBODrivers/QUBOTools bump and the version. Fix: rebase onto current origin/main, drop the version/QUBODrivers/QUBOTools changes (keep main's), and re-resolve. The green CI here ran against the stale base and does not reflect post-rebase behavior.
Blocking 2 — Overlap/likely regression with already-merged #49 (single-variable QUBO fix). This branch re-implements single-variable handling independently: _solve_backend(::DMRGBackend, ...) dispatches to a new _minimize_single_variable, and test/qubo.jl adds "Single variable"/"Single variable tie" cases. Meanwhile #49 (commit b67de66) already fixed single-variable QUBOs in src/solver.jl on main. After rebasing, src/solver.jl will conflict and the two implementations must be reconciled into one. Why it matters: this is the kind of duplicate/divergent fix that produces subtle regressions or a lost fix on merge. Fix: rebase, resolve src/solver.jl against main's #49 implementation, and keep a single canonical single-variable path with the union of tests.
Issue intent: the PR body references no Closes/Fixes/Refs #N and there is no linked closing issue. Per review policy I am flagging the missing issue link (Nonblocking) — if this tracks an issue, add Refs #N.
Nonblocking 1 — Compat bound risk after rebase. The PR pins GenericTensorNetworks = "4" and ProblemReductions = "0.3". These are reasonable, but they were resolved against the old manifest/Julia 1.12. After rebasing onto v0.2.0 (which bumped QUBODrivers to 0.6.1 / QUBOTools 0.13), re-run Pkg.resolve() to confirm GenericTensorNetworks 4 + ProblemReductions 0.3 still co-resolve with the new QUBOTools 0.13 stack; tighten if needed.
Nonblocking 2 — GTNSolution.sample() for property=:single with no enumerated configs falls back to rand(psi.configs) or throws if configs is empty. For :size/:count properties there are legitimately no configs and sample throws ArgumentError, which is correct, but it is worth a docstring note that sample is only meaningful for config-producing properties (:single, :configs). Low priority.
Question 1 — QUBO matrix mapping correctness. In ext/TenSolverGenericTensorNetworksExt.jl, _qubo_matrix writes a quadratic term's combined coefficient only into the upper entry matrix[vars[1], vars[2]] and leaves matrix[vars[2], vars[1]] = 0. The canonical PseudoBooleanModel already folds Q[i,j]+Q[j,i] into one term (confirmed by test/objective.jl: model.terms[[1,2]] == Q[1,2]+Q[2,1]). So this is correct only if ProblemReductions.QUBO evaluates the full asymmetric sum_{i,j} M_ij x_i x_j (counting each off-diagonal once). If QUBO instead symmetrizes or only reads the upper triangle differently, the pairwise energy would be off by a factor. The existing gtn.jl test (Q=[0 2;0 0]) does not discriminate a doubling error because the quadratic term is inactive at the minimum. The higher-order brute-force test exercises the CSP path, not the QUBO path. Please confirm against ProblemReductions' QUBO energy convention and add a test where the quadratic term is active at the optimum and would differ under doubling (e.g. a 2-var case whose minimizer is [1,1]).
Question 2 — maximize for GTN returns _with_objective(sol, -E) which rebuilds a GTNSolution carrying the negated objective but the SAME configs/result computed for the negated (minimization) problem. The test asserts sample(max_sol) lands in the maximizing set, which passes, but max_sol.result and max_sol.metadata["size"] still describe the internal minimized (sign-flipped) problem. Intended? If so, document that raw result/size metadata is in the internal-minimization sign convention.
Tests run: julia and git are denied in this review sandbox (even with the sandbox disabled), so I could not execute Pkg.test() locally. I verified via the GitHub checks API against the exact head SHA c96eb40: all 9 Julia matrix jobs (1/lts/pre x ubuntu/macOS/windows), build, and documenter/deploy pass. The GTN extension is exercised in CI because test/Project.toml carries GenericTensorNetworks + ProblemReductions. Caveat: this CI ran against the stale base ca27880, so it does not validate behavior after the required rebase onto v0.2.0.
Summary
- Blocking: (a) PR is stale/CONFLICTING against main v0.2.0 — must rebase and drop the version/QUBODrivers/QUBOTools downgrades; (b) duplicate/divergent single-variable implementation overlapping already-merged #49 — must reconcile src/solver.jl on rebase.
- Nonblocking: re-resolve compat after rebase (GenericTensorNetworks 4 / ProblemReductions 0.3 vs new QUBOTools 0.13); document that GTNSolution
sampleis only meaningful for config-producing properties. - Questions: confirm
_qubo_matrixupper-triangular placement matches ProblemReductions.QUBO's energy convention and add a discriminating test; clarify the sign convention ofresult/sizemetadata returned by GTNmaximize. - Tests run: none locally (julia/git sandbox-denied); fell back to static review + CI verification at head c96eb40 — all checks green, but against the stale base.
- Merge-readiness: not ready. Blocked on the rebase and the #49 reconciliation, after which CI must be re-run and the QUBO-mapping question resolved.
I would not merge this until the blocking issues above are addressed.
|
Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up items:
|
|
We should wait for #43 to land and then adapt this PR to use that API. |
|
Context refresh for whoever picks this up: beyond the earlier note to adapt to the #43 backend interface, the constraint/PEPS review cycles on closed PRs hardened several conventions the re-integration must follow:
Given the drift already noted, these together reinforce the earlier assessment that a fresh branch porting Cross-referenced from a sweep of review comments on closed PRs, 2026-07-16. |
|
Refreshed this draft against current Key changes:
Validation passed: full |
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Maintainer review — optional GenericTensorNetworks backend (draft)
The re-integration landed well. Today's fa236b1 ("Merge main and port GTN backend architecture") does exactly what this branch needed: GTNBackend <: AbstractTenSolverBackend in src/backends/gtn.jl, backend-specific minimize(::GTNBackend, …) in the extension, the old AbstractBackend/PseudoBooleanModel/duplicate-DMRG layer gone, stateless backend with settings as solve-call keywords, and — unlike the PEPS extension — the GTN deps resolve so test/gtn.jl actually runs in CI (12/12 green). Domains and native constraints are rejected explicitly (validate_gtn_inputs), not silently ignored. Good.
This is a draft, so this review is a COMMENT; I'm also the author, so it wouldn't be a formal APPROVE regardless. No correctness blockers found. The one merge-gating item is the public-API surface, below — worth settling before the PR leaves draft.
Findings
Blocking (for merge, not for draft) — the new exported solution_space is a redundant public interface. solution_space(::GTNBackend, args…) just calls minimize(args…; backend, property, kwargs…) (src/backends/gtn.jl:46), so it is a middle-man over minimize(…; backend = :gtn, property = …) — which already returns the same GTNSolution. It's exported (src/TenSolver.jl:27) alongside GTNBackend, adding a third solver verb to a public surface a maintainer has deliberately kept at minimize/maximize. Two clean options: drop solution_space and let callers pass property to minimize (the property machinery already lives there), or, if a distinct query API is genuinely wanted, design it as a real cross-backend interface rather than a GTN-defaulting delegator (right now backend = GTNBackend() is its default and every non-GTN backend just errors). Relatedly, this branch exports GTNBackend while the sibling optional backend PEPSBackend (#44) was deliberately kept unexported — GTN is more defensible since its extension is CI-exercised, but the two optional backends should be consistent; pick one policy.
Question — objective value for non-size properties. In solve_gtn, objective = isnothing(raw_size) ? constant : constant + primary_size(raw_size) (ext:224). For property = :count (and any property whose result carries no readable size), this reports just the additive constant as the objective, which is misleading. Is that intended, or should minimize(…; property = :count) still compute the true optimum size, or refuse to return an objective? Worth pinning down what the first return value means per property.
Nonblocking — the try_read_size/try_read_count/try_read_configs helpers (ext:175+) swallow every exception and return nothing/[] with no record. Unlike the adjacent contraction_complexity/estimate_memory blocks (which store a *_error in metadata), a genuine read failure here vanishes and surfaces later as a confusing "GTNSolution … does not contain sampleable configurations." Narrow the catch to the expected "property has no size/config" case, or record the error in metadata as the other blocks do.
Nonblocking / coordination — GTNSolution is a standalone struct re-implementing sample/sample(_,n)/in. AbstractSolution (which provides those generically) is not on main yet — it's in the still-open #44 — so it can't subtype it today. When #44 lands, GTNSolution and PEPSSolution should both become AbstractSolution subtypes and drop the duplicated methods; whichever merges second reconciles.
Tests / CI
Full CI is green on fa236b1 (12/12), and the GTN extension is genuinely exercised — test/gtn.jl is wired into runtests.jl and gated with a skip only if the deps are missing (they're in test/Project.toml, so CI runs the real solves). I relied on that authoritative same-head CI plus static review rather than installing GenericTensorNetworks locally.
What remains before leaving draft
- Settle the
solution_space/export surface (the Blocking item). - Decide the objective semantics for non-size properties.
- Post-#44: subtype
AbstractSolution.
The core lowering, domain/constraint rejection, and extension packaging are in good shape.
| )) | ||
| end | ||
|
|
||
| function solution_space( |
There was a problem hiding this comment.
Blocking (for merge): this is a thin wrapper — solution_space(::GTNBackend, args…) just forwards to minimize(args…; backend, property, kwargs…), which already returns the same GTNSolution. So solution_space(Q; property=:size) is exactly minimize(Q; backend=:gtn, property=:size). Exporting it (with GTNBackend() as the default backend and every other backend erroring) adds a third public solver verb that duplicates minimize. Prefer dropping it and routing property through minimize; if a distinct query API is really wanted, design it as a genuine cross-backend interface rather than a GTN-defaulting delegator.
There was a problem hiding this comment.
Addressed in 2681717. I removed the solution_space definitions, export, documentation entry, and call sites. Exact queries now use the existing solver verb directly, e.g. minimize(Q; backend=:gtn, property=:count), with no GTN-specific default path.
| include("solver.jl") | ||
| export minimize, maximize | ||
| export DMRGBackend | ||
| export DMRGBackend, GTNBackend, solution_space |
There was a problem hiding this comment.
Part of the API-surface finding: GTNBackend and solution_space are exported here, but the sibling optional backend PEPSBackend (#44) was deliberately kept unexported. GTN is more defensible than PEPS since its extension actually resolves and is CI-exercised, but the two optional backends should follow one export policy — reconcile which (if any) of these names belong in the public surface.
There was a problem hiding this comment.
Addressed in 2681717. GTNBackend is no longer exported, matching the optional PEPSBackend convention. backend=:gtn is the preferred user-facing selector; TenSolver.GTNBackend() remains documented for advanced/object dispatch use. The backend-interface test now pins the unexported policy.
| item = scalar_result(raw) | ||
|
|
||
| raw_size = try_read_size(item) | ||
| objective = isnothing(raw_size) ? constant : constant + primary_size(raw_size) |
There was a problem hiding this comment.
Question: when raw_size is nothing (e.g. property = :count, whose result carries no readable size), the objective becomes just the additive constant, which misreports the optimum. Should minimize(…; property = :count) still compute the true size, or should the first return value be undefined/omitted for size-less properties? Please pin down what the returned objective means per property.
There was a problem hiding this comment.
Addressed in 2681717. All supported GTN properties expose read_size, including CountingMin, so solve_gtn now reads it unconditionally and returns constant + primary_size(raw_size). The regression coverage checks the returned optimum for size, count, configs, k-best sizes, and representative k-best configurations; a size-only result is also verified to be non-sampleable.
|
|
||
| configs_from_result(config) = append_configs!(Vector{Int}[], config) | ||
|
|
||
| function try_read_size(item) |
There was a problem hiding this comment.
Nonblocking: this (and try_read_count/try_read_configs) catches every exception and returns nothing/[] with no trace. A real read failure then vanishes and resurfaces downstream as a confusing "GTNSolution … does not contain sampleable configurations." The contraction_complexity/estimate_memory blocks just below record a *_error in metadata — do the same here, or narrow the catch to the expected "this property has no size/config" case, so genuine failures aren't silently hidden.
There was a problem hiding this comment.
Addressed in 2681717. I removed the three catch-all reader helpers. Size is always read directly, while configurations and counts are read directly only for properties that define those payloads. A genuine GTN reader failure now propagates. The separate contraction-complexity and memory-estimate catches remain intentional metadata diagnostics and record their errors.
| when the selected GTN property produces them, together with the raw backend | ||
| result and metadata. | ||
| """ | ||
| struct GTNSolution{C, R} |
There was a problem hiding this comment.
Nonblocking / coordination: GTNSolution re-implements sample, sample(_, n), and in standalone. AbstractSolution — which provides those generically — isn't on main yet (it's in the open #44), so this can't subtype it today. When #44 lands, make GTNSolution (and PEPSSolution) <: AbstractSolution and drop the duplicated methods; whichever of #31/#44 merges second does the reconciliation.
There was a problem hiding this comment.
|
Addressed the latest review in
The Local validation on Julia 1.12 is green: focused backend tests (32/32), focused GTN tests (27/27), full |
2 participants
Summary
TenSolver.GTNBackend <: AbstractTenSolverBackendinsrc/backends/gtn.jlbackend = :gtn(orbackend = TenSolver.GTNBackend()for advanced use)minimize(...; backend = :gtn, property = ...)GTNSolutionwrapper with configurations, the raw GTN result, and contraction metadataArchitecture refresh
This branch now merges current
mainand follows the backend API that landed after the draft was opened:src/backends/minimize(::Backend, ...)directlyAbstractBackend, DMRG implementation, andPseudoBooleanModellayer are no longer part of this PRbackend = :gtnis the preferred user-facing selectorproperty,k,usecuda,element_type, optimizer/slicer, and storage settings) are solve-call keywordsThe existing
TenSolver.OptimizerJuMP/MOI path remains DMRG-backed. This PR does not introduce GTN-specific raw optimizer attributes; optional backend forwarding should be added through a generic optimizer architecture rather than special-casing one extension.GTNSolutionremains standalone while PR #44 is open. Once the sharedAbstractSolutionhierarchy lands, this result type can be reconciled in a focused follow-up.Supported exact properties
:size:single:count(k == 1):configs(k == 1):kbest_sizesFor representative k-best configurations, use
property = :single, k = n.Validation
Pkg.test()suite on Julia 1.12: passed