Remove legacy HFS writer - #50
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues: None.
Nonblocking issues: None.
Questions: None.
Tests run and outcomes:
- Verified PR #50 against issue #48; the removal path fully resolves the requested decision and
Closes #48is appropriate. - Confirmed the local merge-base diff totals match GitHub: 5 files changed, 5 insertions, 64 deletions.
julia --project=. -e 'using Pkg; Pkg.instantiate(); include("test/chimera.jl")'passed in clean worktree/tmp/dwave-pr50-review-50.julia --project=. -e 'using Pkg; Pkg.test()'passed in clean worktree/tmp/dwave-pr50-review-50. LocalDWave.Optimizercloud tests were skipped by the existing harness becauseDWAVE_API_TOKENis unset.- CI is green on Julia 1 and 1.10 across Ubuntu and Windows.
Merge-readiness: I found no actionable issues and would consider this merge-ready. Because I authored this PR, this review is submitted as COMMENT; formal approval must come from another maintainer.
bernalde
left a comment
There was a problem hiding this comment.
Review of #50 (remove legacy HFS writer, Closes #48).
Note on event type: I am the PR author, so GitHub only permits a COMMENT review. A formal APPROVE / REQUEST_CHANGES must come from another maintainer. There are no blocking issues.
Intent vs issue #48
Issue #48 asked to either remove src/wrapper/hfs/ or revive it behind a guarded load with a round-trip test. This PR takes the remove path, which is a valid resolution. The writer was already dead code: architecture.jl has not included hfs/format.jl since #46, and hfs/printer.jl did not compile against the current API (it called a nonexistent Chimera(model, ...) constructor and read chimera.linear_terms/quadratic_terms fields the struct does not have). So removal carries no behavioral risk. Closes #48 is appropriate.
Verification
- Confirmed the removal is complete:
git grepforhfs,HFS,write_model,:hfs,AbstractFormatat head finds no remaining references except the legitimate[^alex1770]citation in the Chimera docstring (the source of the coordinate convention, not writer code). architecture.jlhas no orphan include after removing the trailing comment.src/wrapper/hfs/contained onlyformat.jlandprinter.jl, so deleting both removes the directory; verified absent at head.src/wrapper/README.mddoes not mention HFS (rewritten in #46), so no stale doc reference.
Nonblocking
- The new guard test is fine but lightly coupled to the source tree.
!isdir(joinpath(pkgdir(DWave), "src", "wrapper", "hfs"))asserts on repo layout rather than behavior, so it would spuriously fail or pass if the wrapper directory is ever reorganized; and!isdefined(DWave, :HFS)is trivially true today sinceHFSwas never loaded into the module. Both are harmless as regression guards against re-introduction, so this is optional - consider whether the directory-existence assertion earns its keep, or rely on theisdefinedsymbol guard alone.
Observation (pre-existing, optional follow-up)
With the writer gone, the Chimera docstring still opens with "The format of the instance-description file starts with a line giving the size of the Chimera graph" - describing a file format the package no longer reads or writes. That line is pre-existing and not modified by this PR, so it is out of scope here, but a future docs pass could reframe it to document the coordinate convention rather than implying the package handles that file format.
Tests run (clean worktree at head d412614)
julia --project=. test/chimera.jl: all pass - new "Legacy HFS writer is removed" guard 2/2, plus 13/13, 12/12, dnx-equivalence 1/1, 4/4, 7/7.julia --project=. -e 'using Pkg; Pkg.test()': passed (exit 0); package precompiles and loads after the removals; cloudDWave.Optimizertests skipped (no token, expected).
Merge-readiness
No blocking issues. Clean, complete, well-scoped removal of dead code with a regression guard; tests pass on a clean checkout. Mergeable on the merits, pending another maintainer's formal verdict.
|
Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up items:
|
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues: none.
Nonblocking issues: none.
Questions: none.
Tests run:
- Targeted PR-head Chimera tests passed:
include("test/chimera.jl"). - Full PR-head package suite passed:
Pkg.test(), including the live D-Wave quantum sampler checks. - Targeted merge-result Chimera tests passed against current
main, resolving withDWave v0.7.1,QUBOTools v0.15.1, andQUBODrivers v0.6.4.
Merge-readiness: the PR diff matches GitHub's reported 5 changed files, +4/-64; Closes #48 is complete for the removal path; a merge simulation against current main was clean; CI is green. This is approvable on the merits, but because this PR is authored by bernalde, formal approval needs to come from another maintainer.
bernalde
left a comment
There was a problem hiding this comment.
Reviewed as maintainer. Note: I am the PR author, so GitHub only permits a COMMENT event here; a formal APPROVE/REQUEST_CHANGES must come from another maintainer. There are no blocking issues, so this is approvable on the merits.
Scope and intent
The PR removes the legacy HFS writer (src/wrapper/hfs/format.jl, printer.jl), the dead include-tracking comment in src/wrapper/architecture.jl, and the related Chimera doc comment, and adds a guard test. This matches one of the two paths issue #48 explicitly offered ("remove src/wrapper/hfs/ and the commented include if there is no near-term plan to revive the writer"). Closes #48 is appropriate: the issue asks for a decision, and removal fully resolves it, including its request that the architecture.jl comment no longer imply an opt-in mechanism exists.
Correctness verification
- Confirmed the removed code was dead, not live: on the merge-base (
091d47e),src/wrapper/architecture.jlincludes onlydevice.jl,topology.jl, andchimera.jl. Thehfs/files were neverincluded, soHFS,format(::Val{:hfs}), andwrite_model(..., ::HFS)were never inDWave's namespace. Removing them changes no public or loaded behavior. - Confirmed no dangling references remain at the head:
git grep -i hfsover the whole tree returns only the new test. Thesrc/wrapper/hfs/directory is fully gone. No docs, includes, orformat/write_modelcall sites reference it.
Nonblocking observations (not actionable, recorded for context)
- The new guard
Test.@test !isdefined(DWave, :HFS)would already have passed before this PR (the symbol was never defined), so it is a forward regression guard rather than a test of behavior changed by this PR. That is reasonable and the right intent. - The second commit ("test: avoid HFS source layout assertion") deliberately replaced a source-file-layout assertion with the symbol-definition check. That is the better choice (asserts behavior, not on-disk layout), and I would not suggest reverting to a path-based check.
Tests run
- Clean detached worktree at the PR head (
f5cc87b), isolatedJULIA_DEPOT_PATH,DWAVE_API_TOKENunset. - This PR does not touch
Project.toml, so it resolved against the base compat (QUBOTools 0.13.x), which is the intended environment for this branch. Pkg.test()passed (exit 0). The new "Legacy HFS writer is not loaded" testset passed (1/1), and the full Neal/Greedy/Random/Tabu conformance, parity, metadata, chimera, and topology testsets passed. The hardware-backedDWave.Optimizerset is token-gated and was skipped, matching the PR description.
Merge-readiness
- No blocking issues. Diff verified against merge-base
091d47e: 5 files, +4/-64, matching the PR's reported totals. Clean, well-scoped dead-code removal with a sensible forward guard. Approvable on the merits; because this is my own PR, the formal approval must come from a second maintainer.
Summary
src/wrapper/hfs.Tests
julia --project=. -e 'using Pkg; Pkg.instantiate(); include("test/chimera.jl")'passed.julia --project=. -e 'using Pkg; Pkg.test()'passed.Notes: local tests warned that
DWAVE_API_TOKENis unset, so theDWave.Optimizercloud tests were skipped by the existing test harness.Branch Hygiene
main091d47e(origin/mainaftergit pull --ff-only origin main)mainCloses #48