Relax DWave NumPy pin for QiskitOpt compatibility - #53
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Reviewing as maintainer. Note: I am the PR author, so GitHub only permits a COMMENT event from me; the formal verdict (approve/request-changes) must come from another maintainer. There are no blocking issues; on the merits this would be an approve.
The change removes the numpy ==2.4.6 pip pin from CondaPkg.toml and updates the metadata tests. I confirmed against the merge-base (7138d95): 2 files, 26 additions, 2 deletions, matching the PR totals. The change correctly targets issue #52 (the hard numpy pin blocking a shared DWave+QiskitOpt env). DWave's source only uses stable numpy APIs (np.array, BinaryQuadraticModel.from_numpy_vectors in src/neal/sampler.jl and src/DWave.jl), so dropping the version pin and relying on dwave-ocean-sdk's transitive numpy constraint does not risk an API break.
Blocking issues:
- None.
Nonblocking issues:
- The new "shared QiskitOpt benchmark env" testset is a metadata-only proxy. It never calls
CondaPkg.resolve(), which is the actual acceptance criterion in #52. It checks that DWave's direct pip pins don't collide with a hardcoded QiskitOpt snapshot, which does guard against re-introducing a conflicting numpy pin, but it cannot detect a transitive numpy conflict fromdwave-ocean-sdk. The real resolve (numpy=2.2.6) was only verified manually in the PR description, not in CI. - The QiskitOpt dependency set is hardcoded as a literal dict rather than read from QiskitOpt's actual
CondaPkg.toml, so it will silently drift as QiskitOpt evolves, giving false confidence (or a false failure) without any signal tying it to the real upstream metadata.
Questions:
- The repo otherwise follows an "audited stable stack" with exact pins for every Python dependency. Full removal lets numpy float to whatever
dwave-ocean-sdkresolves, which is unaudited. #52 offered "relax the bound to a range ... or remove". Was a range pin (e.g. a bound that coexists with QiskitOpt's~=2.2.0while staying audited) considered, or is reliance on the ocean-sdk transitive constraint the intended policy? - The PR uses
Closes #52. The issue's acceptance criteria ask forCondaPkg.resolve()success (verified manually here) plus a minimal smoke check. The automated check is a metadata proxy rather than a resolve. Is that considered sufficient to auto-close #52, or should the resolve smoke check be wired into CI before closing?
Tests run:
- Extracted and ran the two new TOML-parsing testsets standalone against the current
CondaPkg.toml: both pass (audited 4/4, shared env 5/5). - Did not run the full
Pkg.test()suite here, as it requires a live CondaPkg/Python resolve and (for the cloud sampler) network/credentials. The PR description reports it green; I verified the diff's own logic directly.
Merge-readiness: Mergeable. No blocking issues; the nonblocking items and questions are about test strength and policy, not correctness. I would defer the formal approval to another maintainer since I authored this PR.
|
Pushed review-response commit:
Main changes:
Tests run:
Comments intentionally not fully addressed:
Remaining risks or follow-up:
|
bernalde
left a comment
There was a problem hiding this comment.
Second review round, at head 95b91d2. I am the PR author, so GitHub permits only a COMMENT event from me; a formal approve must come from another maintainer. There are no blocking issues; on the merits this is an approve.
This commit changes the approach from the previous round: instead of removing numpy, it pins a direct audited range numpy = "~=2.2.0" in CondaPkg.toml, matching QiskitOpt v0.7.0's bound. Verified against merge-base 7138d95: 2 files, 30 additions, 2 deletions, matching the PR totals.
Correctness verification (focus of this round: does dwave-ocean-sdk ==9.3.0 resolve under the numpy <2.3.0 ceiling):
- Inspected the CondaPkg cache generated from the head spec (
.CondaPkg/pixi.tomllists exactlynumpy = "~=2.2.0",dwave-ocean-sdk = "==9.3.0",dwave-networkx = "==0.8.18", no qiskit packages, i.e. DWave standalone). It resolves:pixi.lockand the on-disk env containnumpy 2.2.6,dwave-ocean-sdk 9.3.0,dwave-system 1.34.0,dwave_networkx 0.8.18. - Confirmed at runtime:
PYTHONNOUSERSITE=1 .CondaPkg/.pixi/envs/default/bin/python -c "import numpy, dwave.system, dwave_networkx"reports numpy 2.2.6 and dwave_networkx 0.8.18, with dwave.system importing cleanly. So DWave standalone resolves correctly under the~=2.2.0ceiling, landing on 2.2.6 (a deliberate downgrade from the old 2.4.6 pin). DWave's source only uses stable numpy APIs (np.array,from_numpy_vectors), so the downgrade carries no API-break risk. - Note for other reviewers: importing through PythonCall on this machine without
PYTHONNOUSERSITEpicks up a stray numpy 2.5.0 from~/.local/lib/python3.13/site-packages, shadowing the env's 2.2.6. That is a local user-site leak on my machine, not a property of this PR or of CI; the CondaPkg-resolved environment itself is correct.
Blocking issues:
- None.
Nonblocking issues:
- The shared-env testset still verifies coexistence by string-matching a hardcoded QiskitOpt v0.7.0 snapshot rather than calling
CondaPkg.resolve(). This was raised last round; the author gave a reasoned decline (a directnumpy = "~=2.2.0"pin means DWave's own package tests would fail on a transitive Ocean conflict with that range, and a registered-QiskitOpt resolve in CI risks cross-package resolver drift). Acceptable as-is; noting for the record, not asking for a change.
Questions:
- None outstanding. Both questions from the prior round are resolved: the range-vs-removal question is answered by the direct audited
~=2.2.0pin, and the snapshot now names QiskitOpt v0.7.0 with a tightened overlap assertion.
Tests run:
- Ran the two updated TOML testsets from
test/compat_metadata.jlstandalone against headCondaPkg.toml: both pass (audited 4/4, shared env 5/5). - Did not run the full
Pkg.test()suite locally: it imports DWave through PythonCall, which on this machine picks up the leaked user-site numpy 2.5.0 and would produce environment-specific noise unrelated to the PR. The dependency resolution itself is verified above; the PR description reports the full suite green in a clean env.
Merge-readiness: Mergeable. The change is correct and resolution is verified; the one nonblocking item is acknowledged and reasonably declined. I defer the formal approval to another maintainer since I authored this PR.
|
Review comments processed for the latest round. Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
Summary
numpy ==2.4.6pip pin fromCondaPkg.toml.dwave-ocean-sdk ==9.3.0anddwave_networkx ==0.8.18pins.Closes #52
Tests
julia --project=. -e 'include("test/compat_metadata.jl")'julia --startup-file=no --project=/tmp/dwave-qiskitopt-condapkg-smoke -e 'import Pkg; Pkg.develop(path="/home/bernalde/repos/DWave.jl"); Pkg.add(Pkg.PackageSpec(name="QiskitOpt", version="0.7.0")); Pkg.add("CondaPkg"); import CondaPkg; CondaPkg.resolve()'julia --startup-file=no --project=/tmp/dwave-qiskitopt-condapkg-smoke -e 'import DWave; import QiskitOpt; numpy = DWave.PythonCall.pyimport("numpy"); println("numpy=", numpy.__version__); println("dwave_system=", DWave.PythonCall.pyimport("dwave.system").__name__); println("qiskit=", DWave.PythonCall.pyimport("qiskit").__version__)'numpy=2.2.6,dwave_system=dwave.system,qiskit=2.3.1julia --project=. -e 'import Pkg; Pkg.test()'Branch Hygiene
mainorigin/mainat7138d95b559f4e1000fd14540313f0d981c0a9c4