Test the qiskit-addon-sqd entry points, serially and under MPI by garrison · Pull Request #18 · Qiskit/sbd-eigensolver-python

Jim Garrison (garrison) · 2026-09-04T21:03:17Z

Nothing in the suite covered the solver wrapper that qiskit-addon-sqd is
meant to be handed. test_reference_energies goes through
tpb_diag_from_files, and the only place diagonalize_fermionic_hamiltonian
ran was run_sqd_sbd.ipynb under nbmake — which asserts nothing, so it
caught a raised exception and nothing else, on a single rank.

What this adds

test/test_sqd_integration.py, three checks each in a _standalone and an
_mpi variant sharing one body:

Test	Subspace	Assertion
`test_small_subspace_*`	fixed, 40 dets	energy pinned to `-85.29400074571684`
`test_published_energy_*`	full 1em3 selection	the published `-76.23594663` (`slow`)
`test_diagonalize_fermionic_hamiltonian_*`	sampled by upstream	bracketed, plus shape/occupancy consistency

The first two are deterministic: the determinants are given rather than
sampled, so the answer does not depend on the process count, which is the
property worth pinning. The third runs the real self-consistent loop, so a
change to the sci_solver contract or to SCIResult/SCIState fails here
instead of in a user's script; its energy is bracketed because the subspace
comes from upstream's sampling and recovery.

All three leave fcidump_path unset, so rank 0 regenerates the FCIDUMP into
a temp dir and broadcasts the path for every rank to open — the default, what
a caller coming through diagonalize_fermionic_hamiltonian gets, and the
multi-rank path 7c7cc09 fixed.

Verified identical to ten digits on 1, 2 and 4 ranks.

Why two variants instead of one test

pytest-mpi filters on the mpi marker in opposite directions: --only-mpi
skips what is not marked, and no flag skips what is. So one function cannot
run in both modes. The bodies size the alpha-determinant grid from
MPI.COMM_WORLD, which is 1 in a single process, so the same code serves
both — tox -e py runs the _standalone variants and tox -e mpi the
_mpi ones, with no new tox env or CI plumbing needed.

bit_length

Set to 63 rather than the wrapper's default of 20. SBD fixes the packed word
count in a process-wide inline static on the first diagonalization and throws
det_vector: elem_size mismatch for any later one implying a different
count, so every test sharing a process must agree; 63 gives h2o the single
word that test_reference_energies already gets from 64.

63 is also the largest legal value. As #12 established, bitadvance()
computes (((size_t) 1) << bit_length) - 1, so 64 shifts a 64-bit size_t
by 64 — undefined behavior, reached from mpi_redistribution() and
mpi_sort_bitarray(), which is exactly what the _mpi tests exercise.

Two known gaps, both noted in the module:

Multi-word packing goes uncovered, and that is what a caller taking
SBD_DEFAULT_BIT_LENGTH (20) actually gets. Covering it needs a module
that does not share a process with these.
test_reference_energies still pins 64. Same UB, but on
tpb_diag_from_files rather than the MPI paths, and out of scope here.

Not a performance question either way: at 275 determinants the solve takes
1.9s at 63 against 2.1s at 20.

Other changes

conftest.py: device_config and counts_path fixtures. The solver
wrappers take a DeviceConfig rather than a backend module, so the
"was this backend built?" skip is factored into a helper both fixtures
call instead of being duplicated.
pyproject.toml: qiskit-addon-sqd and pyscf move into the test
extra so tox -e py and tox -e mpi both cover this path. The tests
skip themselves if the imports are unavailable.

This PR was generated by Claude Opus 5 under my guidance.

Nothing covered the solver wrapper qiskit-addon-sqd is meant to be handed. test_reference_energies goes through tpb_diag_from_files, and the one place diagonalize_fermionic_hamiltonian ran was run_sqd_sbd.ipynb under nbmake -- which asserts nothing, so it only ever caught a raised exception, and only on a single rank. Three checks, each in a _standalone and an _mpi variant sharing one body: - solve_sci_batch over a fixed 40-determinant subspace, energy pinned. The determinants are given rather than sampled, so the answer is deterministic and the same on any process count -- which is the property worth pinning. - The same over the full 1em3 selection, asserting the energy published in the vendored data. Marked slow. - diagonalize_fermionic_hamiltonian itself, so a change to the sci_solver contract or to SCIResult/SCIState fails here rather than in a user script. Bracketed rather than pinned: the subspace comes from upstream's sampling. All leave fcidump_path unset, so rank 0 regenerates the FCIDUMP and broadcasts it -- the default path, and the multi-rank one 7c7cc09 fixed. Two variants rather than one test because pytest-mpi filters on the mpi marker in opposite directions (--only-mpi skips what is not marked, no flag skips what is), so a single function cannot run in both modes. The bodies size the determinant grid from MPI.COMM_WORLD, which is 1 in one process. bit_length is 63, not the wrapper's default of 20: SBD fixes the packed word count process-wide on the first diagonalization, so every test in a process must imply the same one, and 63 gives the single word test_reference_energies already gets from 64. It is also the largest legal value -- 64 shifts a 64-bit size_t by 64 in bitadvance(), which is undefined behavior on the very paths these tests exercise (#12). That leaves multi-word packing uncovered, which is what a default caller gets; noted in the module for a follow-up. Verified identical to ten digits on 1, 2 and 4 ranks. qiskit-addon-sqd and pyscf move into the test extra so tox -e py and tox -e mpi both cover this; the tests skip if the imports are missing. Assisted-by Claude Opus 5

Both fixtures guarded their path with a skip. For the counts file that was plainly wrong -- it is tracked in the repository, so a skip meant moving or deleting it broke nothing. The submodule case turns out to be no different. The sdist ships neither test/ nor the reference data, only the vendored headers needed to compile, so the tests only ever run from a git checkout; and that checkout must have the submodule initialized regardless, since setup.py compiles against vendor/sbd-upstream/include. Without it the extension does not build and `import sbd` fails long before a fixture is asked for a path. So neither skip could fire for the reason it claimed, and both could hide a broken checkout by reporting success. Assisted-by Claude Opus 5

PR #18 landed test/test_sqd_integration.py while this branch was open. Its conftest resolves the curated h2o counts under python/examples/, which this branch moves to examples/tpb/. Neither change conflicts textually, so the merge was clean and the merged tree still broke -- every CI job failed on FileNotFoundError: .../python/examples/count_dict_h2o.json which is exactly what the comment above COUNTS_PATH says should happen when a move does not update it. Retarget the constant and drop the MANIFEST.in line for the shared examples README this branch removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…44) * examples: move out of the package to a top-level examples/ directory Pure reorganization, no behaviour change, so that a later PR adding a second solver's drivers is additive rather than a rename plus a feature. Runnable MPI scripts were living inside the installed package: they shipped in the wheel, sat beside __init__.py and bindings.cpp, and a path like python/examples/run_sbd_diag.py reads as package internals. They move to a top-level examples/, which is the common convention for runnable scripts, and are grouped by basis type -- examples/tpb/ -- since the solvers take different subspaces and decompose over MPI differently. (The sibling qiskit-addon-sqd has no examples/ only because its tutorials are notebooks rendered into Sphinx docs; ours are CLI drivers.) examples/tpb/ is the same depth as the old location, so every ../../vendor/sbd-upstream/... path inside the drivers keeps working untouched. What is package-level rather than example-level -- backend selection, the --device table, bundled test data, performance notes -- moves to examples/README.md, which the solver README links to instead of restating. The GDB decomposition paragraph goes there too so nothing is lost; a later PR relocates and expands it. python/examples/README.md stays behind as a signpost, since git mv removes the directory outright and links to python/examples/... would otherwise 404. It can be deleted once those links age out. The top README now names the folders and defers to their READMEs instead of listing every example file, which is the part that rots whenever a driver is added. Reference updates: MANIFEST.in, tox.ini's notebook env, python/__init__.py's citation of the rank-to-device rule. Two small fixes ride along: MANIFEST.in gained *.json so count_dict_h2o.json -- which run_sqd_sbd.py defaults to -- now reaches the sdist, and the bundled-data paths in the shared README are stated relative to a solver folder, matching how the drivers' own defaults are spelled. Verified: run_sbd_diag.py runs from examples/tpb/ resolving its default vendored paths (-76.2359465468); the sdist contains examples/, examples/tpb/ with all six files, and the signpost; no reference to python/examples survives except the MANIFEST line that ships the signpost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: split the install matrix out to INSTALL.md Installation was 190 of the README's 424 lines -- 45% of the landing page spent on prerequisites, every environment variable the build reads, host-MPI builds and verification, before a reader learned what the package does. It moves to INSTALL.md, taking the README to 264 lines. The README keeps a quickstart rather than deferring everything, because it is also the PyPI long_description: a conda env, pip install, and the available_backends() check that tells you which backends the build produced, plus the GPU-aware MPI requirement, which is the one prerequisite easy to miss and expensive to diagnose. Everything else -- per-platform prerequisites, installing from a checkout with the submodule, the full environment-variable matrix, narrowing which backends get built, building against an existing host MPI -- is one link away. Extracted subsections are promoted to top-level headings, the one intra-README reference ("see Backend Architecture below") now names the README explicitly, and INSTALL.md is added to MANIFEST.in so it reaches the sdist. Verified: every relative link in both files resolves, the three referenced anchors exist, and the sdist contains INSTALL.md alongside the READMEs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * README: drop an over-general MPI claim, and say whose limit each GDB constraint is Two corrections. The install quickstart asserted that GPU backends "need a GPU-aware MPI ... because every one of them hands MPI device pointers". That is wrong twice over: it is not every backend -- the OpenMP-offload path works with an MPI that cannot address device memory, since ROCm device allocations are host-addressable -- and it is not a hard requirement even for Thrust, which has two documented build-time escape hatches (SBD_NON_CUDA_AWARE_MPI, SBD_THRUST_SAFE_MPI_ALLREDUCE). The Backend Architecture section already states it accurately as an assumption of the Thrust build with those hatches, and INSTALL.md carries the detail, so the quickstart line is removed rather than reworded. The GDB config table said b_comm_size "must be 1" and left t_comm_size unexplained, which invites the reader to assume both are SBD limitations. They are not the same kind of thing. b_comm_size == 1 is OUR calling convention: upstream's in-memory gdb::diag expects each rank to pass its own shard -- that is what its file-based path feeds it after distributing files over b_comm -- while this wrapper passes the whole list from every rank, and upstream's own run.sh for the GDB app uses --b_comm_size 2. t_comm_size == 1 is then UPSTREAM's algorithm: the matvec rotates the ket around b_comm as a ring, so there are exactly b_comm_size stations and one task is one station, hence t_comm_size cannot exceed b_comm_size. Also notes that the derived helper dimension still absorbs the remaining ranks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * README: use the documented conda command in the quickstart, not an invented one When splitting install out to INSTALL.md I wrote a shortened environment command from scratch instead of reusing the one the project documents. It diverged four ways: python=3.12 instead of the pinned 3.13.12, no -y, no pybind11/numpy/setuptools/wheel/ pip/pyscf, and -- the one that actually matters -- it dropped the "plus llvm-openmp on macOS" note. setup.py's Darwin path looks for omp.h in the conda env, so a macOS reader following the quickstart would have hit exactly the OpenMP problem issue #27 is about. The build-tool packages are arguably redundant under pip's build isolation, since pyproject's [build-system] requires supplies them, but that is not a judgement to make silently in a quickstart -- and two different install commands in one repository is a maintenance trap regardless. Now verbatim from INSTALL.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * README: audit the troubleshooting entries against the code Checked all five against what the build and bindings actually do now. Three hold unchanged, one was stale, and two symptoms we know about were missing. Still accurate, verified: the conda CC/CXX entry (setup.py uses os.environ.setdefault, so a caller-set compiler still suppresses nvc++/amdclang++); the GPU-not-building entry; and the Bus error / SIGSEGV entry, including both of its misleading cases. NOT resolved, contrary to how it might read: the "all ranks land on GPU 0" entry. The omp_get_num_devices() fallback to counting the vendor visibility variable is still there, and so is omp_set_default_device(mpi_rank % n_dev). Added the caveat that the index is the GLOBAL rank, so the assignment is only even when the launcher places ranks on nodes in contiguous blocks; round-robin placement leaves each node on a strided subset of its GPUs. Stale: "MPI errors: verify MPI_HOME, check MPI.Get_version()" predates the build learning to diagnose this. setup.py now stops with "MPI_HOME=... is not the MPI that mpi4py is linked against", so the entry now names that message, explains why it is fatal rather than a warning, and gives the two resolutions. Added, both real and previously undocumented here: the macOS OMP Error #15 duplicate libomp abort, which kills the first diagonalization while letting the import succeed (issue #27); and multi-rank GPU GDB failing with "GDB Thrust mult does not support h_comm_size > 1", which is unavoidable in this release because b_comm_size is pinned to 1 and every added rank therefore lands in the helper dimension. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * examples/README: explain the cpu/gpu-omp conflict, and stop advising one thread on GPU Two things a reader could not act on. "# NOT in the same process as 'cpu'" said don't without saying what happens. Both backends link the same OpenMP runtime and _core_cpu is built without offload support, so whichever loads first initializes it host-only; if that is the CPU backend, the OMP-offload backend cannot acquire a device and silently runs its target regions on the host -- right answers, exit 0, idle GPU, nothing to catch. Now stated, with loaded_backends() as the way to check, and with the note that 'cpu' and 'gpu' (Thrust) do coexist because Thrust does not route device work through OpenMP. "GPU: One MPI rank per GPU, OMP_NUM_THREADS=1" is wrong advice. One rank per GPU is about device ownership; it says nothing about threads, and a GPU build still does real host work. Helper construction is host-threaded in both solvers (tpb/helper.h has 15 omp regions, gdb/helper.h one), and for GDB the heatbath expansion and carryover selection have no device implementation at all -- gdb/expansion.h and gdb/carryover.h contain no thrust:: code and are included outside any SBD_THRUST guard. One thread per rank single-threads all of that; the guidance is now to divide the node's physical cores among the ranks exactly as for a CPU run, and to expect GPU utilisation below 100% because of the host phases rather than read it as a fault. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * examples: drop the shared README; each solver folder stands alone An intermediate examples/README.md meant a reader looking for how to run something had to visit two files. Removed, with its content moved to where it is used rather than deleted: - Backend Selection (the --device values, available_backends()/loaded_backends(), and why 'cpu' and 'gpu-omp' must not share a process) -> examples/tpb/README.md - Available Test Data (the bundled h2o and n2 alpha lists) -> examples/tpb/README.md, whose drivers are what consume them - Performance Tips (thread counts for CPU and GPU runs) -> examples/tpb/README.md Its GDB paragraph is dropped rather than moved: the top README's Configuration section now explains the b_comm_size and t_comm_size constraints more fully, including whose limitation each one is, so the shorter version was redundant. References repointed: the top README indexes the folder directly, INSTALL.md's See Also, python/__init__.py's citation of the rank-to-device rule, and the python/examples signpost. Verified every relative link in all four files resolves and the sdist still ships the examples tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: point COUNTS_PATH at the moved counts file PR #18 landed test/test_sqd_integration.py while this branch was open. Its conftest resolves the curated h2o counts under python/examples/, which this branch moves to examples/tpb/. Neither change conflicts textually, so the merge was clean and the merged tree still broke -- every CI job failed on FileNotFoundError: .../python/examples/count_dict_h2o.json which is exactly what the comment above COUNTS_PATH says should happen when a move does not update it. Retarget the constant and drop the MANIFEST.in line for the shared examples README this branch removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * setup.py: find nvc++ under NVHPC_ROOT and under compilers/, not just <root>/bin Detection probed exactly one path, $NVHPC_HOME/bin/nvc++. NVHPC does not put the compiler there: the version root holds compilers/ cuda/ math_libs/ comm_libs/ and the binary is at <root>/compilers/bin/nvc++. NVIDIA's own modulefile does setenv NVHPC_ROOT $nvhome/$target/$version prepend-path PATH $nvcompdir/bin so NVHPC_ROOT names the version root, one level above the compiler. Setting NVHPC_HOME to that root -- the natural guess, and what copying $NVHPC_ROOT gives you -- found nothing, and NVHPC_ROOT was never consulted. Neither failure is loud. SBD_BUILD_BACKEND=auto falls back to a CPU-only build, pip hides setup.py's output without -v, and the install exits 0; the user finds out later from "Device 'gpu' requested but its backend is not usable". Verified on a real NVHPC 26.3 tree with nvc++ off PATH: before, both NVHPC_HOME=<version root> and NVHPC_ROOT=<version root> gave "will build CPU backend only"; after, both give "will build CPU, Thrust GPU and OMP-offload GPU backends". Read NVHPC_ROOT as well as NVHPC_HOME, and under each try bin/ and compilers/bin/, so the compilers directory, the version root, and a bare `module load nvhpc` all work. Also compare PATH entries rather than substrings when deciding whether the directory is already there. The "Found NVIDIA HPC SDK at: <path>" prefix is kept because the troubleshooting section quotes it; the variable name is appended. test/test_build_detection.py is new -- setup.py had no tests. It extracts the function rather than importing setup.py, which would run a build, and covers both variables against both layouts, a stale NVHPC_HOME not masking a good NVHPC_ROOT, and a negative control so the suite cannot pass against a function that always claims success. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: drop test_build_detection.py, out of scope for this PR Flagged in review as unrelated to the PR description, which it is -- this branch moves examples and splits the install docs. The setup.py NVHPC_ROOT fix stays, since INSTALL.md documents the variable and the two belong together, but its test does not need to ride along here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Jim Garrison (garrison) commented Sep 11, 2026

View reviewed changes

Comment thread test/conftest.py Outdated

Jim Garrison (garrison) added 2 commits September 14, 2026 16:56

Merge remote-tracking branch 'origin/main' into test-sqd-under-mpi

ed90330

Jim Garrison (garrison) marked this pull request as ready for review September 14, 2026 21:23

Jim Garrison (garrison) requested a review from Sophia Wen (hfwen0502) September 14, 2026 21:25

Jim Garrison (garrison) added 2 commits September 16, 2026 14:40

Merge remote-tracking branch 'origin/main' into test-sqd-under-mpi

9594281

Merge branch 'main' into test-sqd-under-mpi

0760214

Jim Garrison (garrison) merged commit f37c430 into main Sep 25, 2026
11 checks passed

Jim Garrison (garrison) deleted the test-sqd-under-mpi branch September 25, 2026 20:29

Navigation Menu

Repository navigation

Test the qiskit-addon-sqd entry points, serially and under MPI - #18

Test the qiskit-addon-sqd entry points, serially and under MPI#18
Jim Garrison (garrison) merged 5 commits into
mainfrom
test-sqd-under-mpi

Jim Garrison (garrison) commented Sep 4, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Jim Garrison (garrison) commented Sep 4, 2026

What this adds

Why two variants instead of one test

bit_length

Other changes

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant