Introduce solver backend interface#43
Conversation
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
GitHub rejected REQUEST_CHANGES because this authenticated account is the pull request author, so I am submitting this as COMMENT. Maintainer recommendation: do not merge as-is.
Review summary:
- Blocking issues: 1. The symbolic backend normalization is not extensible for the later optional
backend = :pepspath described by the docs. - Nonblocking issues: none.
- Questions: none.
- Tests run and outcomes:
julia --project=. -e 'using Pkg; Pkg.instantiate()': passed.- Targeted
test/backend.jlon Julia 1.11.5: passed, 13/13. - Custom backend object dispatch probe: passed.
- Symbol-extension dispatch probe: confirmed value-specific symbol extension is not reachable.
git diff --check origin/feature/issue-36-qubo-ising-conversion...HEAD: passed.- Targeted
test/backend.jlon Julia 1.10.11: passed, 13/13. julia --project=. -e 'using Pkg; Pkg.test()': passed.- Documented docs setup command: passed.
julia --project=docs/ docs/make.jl local: passed.- GitHub CI checks inspected: all current checks are passing.
- Merge as-is: no.
I would not merge this until the blocking issue above is addressed.
|
|
||
| _normalize_backend(backend::DMRGBackend) = backend | ||
| _normalize_backend(backend::AbstractTenSolverBackend) = backend | ||
| function _normalize_backend(backend::Symbol) |
There was a problem hiding this comment.
Blocking: This catches every Symbol backend in one concrete method, so an optional package extension cannot later implement the documented backend = :peps path with normal Julia dispatch. minimize(...; backend = :peps) will always enter this method and throw before backend-specific _minimize methods can run, which makes the new backend interface much harder to extend without editing core TenSolver again or overwriting this method.
Use a value-dispatch hook instead, for example _normalize_backend(backend::Symbol) = _normalize_backend(Val(backend)), _normalize_backend(::Val{:dmrg}) = default_backend, and a fallback _normalize_backend(::Val{backend}) where backend = throw(_backend_error(backend)). Then a PEPS extension can add _normalize_backend(::Val{:peps}) = PEPSBackend(...). Please also add a regression test with a fake symbolic backend so this extension path stays covered.
There was a problem hiding this comment.
Addressed in f4a3769. Symbol normalization now dispatches through Val(backend), :dmrg stays mapped to the default backend, and test/backend.jl covers a fake symbol-specific backend reaching its _minimize method.
|
Pushed commit Main changes:
Tests run:
Comments intentionally not addressed: none. Remaining risks/follow-up:
|
|
Blocking: The PR introduces the backend boundary, but the committed symbol normalization currently hard-codes A concrete fix is to route symbols through _normalize_backend(backend::Symbol) = _normalize_backend(Val(backend))
_normalize_backend(::Val{:dmrg}) = default_backend
function _normalize_backend(::Val{backend}) where {backend}
throw(_backend_error(backend))
endThen a later PEPS bridge can implement |
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
GitHub rejected APPROVE because this authenticated account is the pull request author, so I am submitting this as COMMENT. Maintainer recommendation: merge-ready as-is.
Review summary:
- Blocking issues: none.
- Nonblocking issues: none.
- Questions: none.
- Tests run and outcomes:
julia --project=. -e 'using Pkg; Pkg.instantiate()': passed.- Targeted
test/backend.jlon Julia 1.11.5: passed, 19/19. - Targeted
test/backend.jlon Julia 1.10.11: passed, 19/19. - Module-scoped fake backend extension probe: passed.
git diff --check origin/feature/issue-36-qubo-ising-conversion...HEAD: passed.julia --project=. -e 'using Pkg; Pkg.test()': passed.- Documented docs setup command: passed.
julia --project=docs/ docs/make.jl local: passed.- GitHub CI checks inspected: all current checks are passing.
- Merge as-is: yes.
The latest head uses Val dispatch for symbolic backend normalization and covers the extension path with a fake symbolic backend test. I found no actionable issues requiring inline comments.
bernalde
force-pushed
the
feature/issue-37-backend-interface
branch
from
f4a3769 to
009bcd1
Compare
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: confirmed against the stacked base feature/issue-36-qubo-ising-conversion (not main). merge-base 03eb8d8; git diff <merge-base>..HEAD --stat = 6 files, +155 / -33, matching the PR's reported totals.
Issue intent (#37): introduce a minimal backend abstraction (AbstractTenSolverBackend, DMRGBackend, a default) and refactor minimize to delegate to a backend-specific _minimize while keeping DMRG default and not changing behavior. The PR delivers exactly this: the abstract type, DMRGBackend, const default_backend = DMRGBackend(), public minimize delegating via _minimize(_normalize_backend(backend), ...), plus Symbol (:dmrg) and object support with clear errors.
Verified, no behavior change: delegation forwards cutoff, kwargs... intact; _minimize(::DMRGBackend, ...) rebuilds obj/H exactly as before and calls the renamed _minimize_mpo (body byte-identical apart from the rename); maximize and the 2-arg minimize convenience forward backend transparently; the QUBODrivers sample path uses the default backend unchanged. No method ambiguity (Aqua passed); the typed _minimize(::DMRGBackend, ...) is strictly more specific than the args... fallback. default_backend is intentionally not exported, matching #37's guidance to avoid committing public names early. Behavioral equivalence confirmed: default vs backend=DMRGBackend() vs backend=:dmrg give identical energy and sample on a 4-var QUBO.
Nonblocking
_backend_error(src/solver.jl:54) always says "Install/load the PEPS extension or use backend = :dmrg." A user passing an arbitrary symbol (e.g.:foo) or a customAbstractTenSolverBackendsubtype without a_minimizemethod gets PEPS-specific wording. Reserve the PEPS message for the:pepscase and give a generic "no_minimizemethod for backend ..." message in the object/symbol fallbacks.
Question
2. The PR body says test/backend.jl has "13 tests"; my isolated run reports 19 Pass / 19 Total. Not a defect, but the body's count is inaccurate.
Tests run (Julia 1.10.11, clean worktree on the PR head):
julia --project=. -e 'using Pkg; Pkg.instantiate(); Pkg.test()'— passed (Backend interface, ising_conversion, QUBODrivers.jl 130, Aqua.jl 11, VRP, HDF5, Doctests 1).test/backend.jlin isolation: Backend interface 19/19.- Equivalence check on a 4-var QUBO: default /
DMRGBackend()/:dmrgreturned identical energy and sample.
Merge readiness: Ready to merge (pending normal human review). Faithful, minimal implementation of #37; delegation preserves all signatures, defaults, kwargs, dispatch, and QUBODrivers/maximize behavior; suite and doctests pass; no ambiguities. Only nonblocking polish: the reused PEPS-specific error wording, and the body's "13 tests" should read 19. Stacked on #42 (feature/issue-36) and must merge after it. The formal verdict must come from another maintainer.
|
|
||
| const default_backend = DMRGBackend() | ||
|
|
||
| _backend_error(backend) = ArgumentError("backend $(repr(backend)) is not available. Install/load the PEPS extension or use backend = :dmrg.") |
There was a problem hiding this comment.
Nonblocking: this error always mentions the PEPS extension, but it also fires for an arbitrary unknown symbol (e.g. :foo) or a custom AbstractTenSolverBackend without a _minimize method (via the fallback at lines 61/216). Reserve the PEPS wording for the :peps case and give a generic "no _minimize method for backend ..." message otherwise.
There was a problem hiding this comment.
Addressed in 0345560. src/solver.jl now reserves the PEPS-extension wording for :peps; unknown symbols and custom backends without _minimize get a generic no-_minimize message. Covered in test/backend.jl.
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Senior maintainer review of PR #43 ("Introduce solver backend interface"), stacked on feature/issue-36-qubo-ising-conversion (PR #42). I reviewed the diff against the PR's actual base (gh-computed), which reproduces the reported totals (+155/-33, 6 files). Findings are limited to code introduced by #43 itself; the QUBO/Ising lowering and _single_variable_minimize paths are inherited from the #41/#42 base chain and are out of scope here.
Process note: I am the PR author, so GitHub will not let me APPROVE/REQUEST_CHANGES my own PR. This is posted as a COMMENT; a formal verdict must come from another maintainer.
Assessment
The abstraction is coherent and minimal and matches issue #37's recommended shape: AbstractTenSolverBackend / DMRGBackend markers, a default_backend const, _normalize_backend to accept both symbols and backend objects, and a _minimize(backend, ...) dispatch layer with the unchanged DMRG body renamed to _minimize_mpo. The public API is preserved: minimize(Q[, l[, c]]), the minimize(Q, c) 2-arg overload, the polynomial overload, and maximize(qs...; kwargs...) all forward backend correctly (maximize forwards it through kwargs). The DMRG body is moved verbatim, so existing keywords (iterations, maxdim, mindim, noise, vtol, device, callbacks) keep working. Tests genuinely exercise the new boundary, including extension-style registration via _normalize_backend(::Val{...}) + _minimize(::CustomBackend, ...), default-equivalence, maximize forwarding, and both error paths. CI is green on all 9 Julia jobs + build + docs at the exact head SHA 009bcd1.
No blocking correctness issues found in #43's own changes. The blocking item below is a merge-readiness/process gate, not a code defect.
Blocking
- Cannot merge as-is: stacked-base dependency + staleness vs current
main. This PR targetsfeature/issue-36-qubo-ising-conversion(PR #42), and that base chain (#41/#42) is not yet merged, so #43 cannot merge until its base merges (or is retargeted). Separately, the branch is cut from old main and is now stale: headProject.tomlisversion = "0.1.0"withQUBODrivers = "0.3", whereas currentorigin/mainis0.2.0withQUBODrivers = "0.6.1"(#49/#53/#54 merged).git compare main...009bcd1reports diverged: 6 ahead, 5 behind. The CI that passed ran against the stale base, so it does not validate post-rebase behavior. Concretely: rebase/merge the #41..#46 stack onto current main, resolve the version/QUBODrivers bump, and confirm this stack does not duplicate the already-merged single-variable QUBO fix (#48) that the inherited_single_variable_minimizepath appears to overlap. Why it matters: merging would regressProject.tomlversion and the QUBODrivers compat bound and may re-introduce or conflict with already-merged fixes.
Nonblocking
-
Closes #37is accurate for scope. Issue #37 explicitly lists QUBODrivers backend attributes and a PEPS backend as out of scope, and the PR delivers the abstraction, default preservation, clear unknown-backend error, and extension room that #37's acceptance criteria require. No change requested; flagging only that #37 is part of a 6-PR epic and remains open until the rest of the stack lands. -
Redundant error surfaces in the backend-normalization layer.
_normalize_backend(::Val{backend})throws_backend_error(backend)and the generic_minimize(backend::AbstractTenSolverBackend, args...)fallback also throws_backend_error(backend), while the catch-all_normalize_backend(backend)throws a differently-worded"Unsupported backend ...". The two messages diverge for conceptually similar failures (unknown symbol vs unknown type). Consider consolidating on one message/builder so users see consistent guidance. Minor maintainability only.
Questions
- The generic
_minimize(backend::AbstractTenSolverBackend, args...; kwargs...)fallback errors for any registered backend subtype lacking a concrete_minimizemethod. Is that the intended contract for a future PEPS extension that registers a backend type but only implements the matrix path (not the polynomial path)? If so, a brief note in theAbstractTenSolverBackenddocstring stating that backends must implement_minimizefor each input form they support (matrix and/or polynomial) would help extension authors; the current docstring says "backend-specific methods for the normalized optimization inputs they support" but does not name_minimizeas the extension hook.
Summary
- Blocking: stacked-base dependency on unmerged #42 plus staleness vs current main (head 0.1.0/QUBODrivers 0.3 vs main 0.2.0/0.6.1; diverged 6 ahead/5 behind; possible overlap with merged #48). No blocking defects in #43's own code.
- Nonblocking: redundant/divergent error messages across
_normalize_backend/_minimizefallbacks;Closes #37is fine but #37 is one of a 6-PR epic. - Questions: document
_minimizeas the per-input-form extension hook in theAbstractTenSolverBackenddocstring. - Tests run: none locally; the sandbox denied
git fetchandjulia(consistent with sibling reviews). Fell back to static review of the head sources (gh api .../contents/src/solver.jl?ref=009bcd1) plus CI verification against the exact head SHA009bcd1(all 9 Julia matrix jobs, build, and Documenter deploy: success). Note CI ran against the stale base and does not validate post-rebase behavior. - Merge-readiness: Not mergeable yet. The code change for the backend interface itself is sound and well-tested, but the PR is blocked on (a) its unmerged base #42 and the rest of the #41..#46 stack, and (b) rebasing onto current main to resolve the version/QUBODrivers regression and confirm no duplication with merged fixes.
I would not merge this until the blocking issues above are addressed.
| throw(_backend_error(backend)) | ||
| end | ||
| _normalize_backend(backend) = throw(ArgumentError("Unsupported backend $(repr(backend)). Use backend = :dmrg or backend = DMRGBackend().")) | ||
|
|
There was a problem hiding this comment.
Nonblocking (maintainability): this catch-all throws "Unsupported backend ...", while _normalize_backend(::Val{backend}) above and the _minimize(::AbstractTenSolverBackend, args...) fallback both throw _backend_error ("backend X is not available..."). Two divergent messages for conceptually similar "unknown backend" failures. Consider consolidating on a single message builder.
There was a problem hiding this comment.
Addressed in 0345560. The unsupported-value path, unknown symbolic backend path, and missing-_minimize backend fallback now share _backend_error. Covered in test/backend.jl.
|
|
||
| """ | ||
| DMRGBackend() | ||
|
|
There was a problem hiding this comment.
Question (docs): consider naming _minimize here as the extension hook backends must implement, and noting it is per input form (matrix and/or polynomial). The generic _minimize(::AbstractTenSolverBackend, args...) fallback errors for any registered backend that omits a concrete method, so extension authors need to know which method(s) to define.
There was a problem hiding this comment.
Addressed in 0345560. The backend docstring now names _minimize as the extension hook for matrix and polynomial inputs, and notes the Val normalization hook for symbolic backends. Docs build passed.
…ersion' into feature/issue-37-backend-interface
|
Pushed commits:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/issue-36-qubo-ising-conversion #43 +/- ##
==========================================================================
- Coverage 97.89% 97.05% -0.84%
==========================================================================
Files 5 5
Lines 237 272 +35
==========================================================================
+ Hits 232 264 +32
- Misses 5 8 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
AbstractTenSolverBackendandDMRGBackendas the core backend boundary.minimizecalls through backend-specific dispatch while keeping the current DMRG implementation as the default._normalize_backend(::Val{...})so extensions can register symbolic backend names without core edits._minimizeas the backend extension hook for matrix and polynomial input forms.:pepson the PEPS-extension-specific message._minimizemethods.This is stacked on PR #42 and targets
feature/issue-36-qubo-ising-conversion.Tests run
git diff --check- passed.julia +1.12 --project=. -e 'using Pkg; Pkg.instantiate()'- passed.julia +1.12 --project=. -e 'using Test, Random, LinearAlgebra, TenSolver; include("test/backend.jl")'- passed, 26 tests.julia +1.12 --project=docs/ -e 'using Pkg; Pkg.instantiate()'- passed.julia +1.12 --project=docs/ docs/make.jl- passed; deployment skipped outside CI.julia +1.12 --project=. -e 'using Pkg; Pkg.test(; coverage=false)'- passed.Notes
Pkg.instantiate()the rerun passed.Valdispatch path.Closes #37