Move examples out of the package, split install docs into INSTALL.md - #44
Merged
Merged
Conversation
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>
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>
…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>
…vented 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>
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>
…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>
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>
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>
Sophia Wen (hfwen0502)
force-pushed
the
examples-reorg
branch
from
September 29, 2026 20:12
7a40069 to
00d16cc
Compare
…<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>
Jim Garrison (garrison)
approved these changes
Sep 30, 2026
Jim Garrison (garrison)
left a comment
Jim Garrison (garrison)
left a comment
Member
There was a problem hiding this comment.
Thank you -- this looks reasonable to me.
The addition of test/test_build_detection.py is unrelated to the PR description I think, but it looks reasonable to me as well.
Jim Garrison (garrison)
added this pull request to stack #47
September 30, 2026 16:55
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs and layout only — no code changes, no behaviour changes.
What changed
examples/, grouped by basis:examples/tpb/. Runnable MPI scripts were shipping inside the installed wheel next to__init__.pyandbindings.cpp.examples/tpb/is the same depth as before, so the drivers'../../vendor/...paths are untouched. A signpost stays atpython/examples/README.mdso old links don't 404.INSTALL.md— they were 190 of the README's 424 lines. The README keeps a quickstart (conda env,pip install,available_backends()check) because it is also the PyPI long_description.examples/README.md— each solver folder's README stands alone.Corrections found while moving
Worth a look, since they go slightly beyond a move:
SBD_NON_CUDA_AWARE_MPIexists for Thrust. Backend Architecture already states it accurately.OMP_NUM_THREADS=1was advised for GPU runs. Helper construction is host-threaded in both solvers, so that starves it; now "divide the node's cores among the ranks".MANIFEST.ingained*.json, socount_dict_h2o.json— whichrun_sqd_sbd.pydefaults to — now reaches the sdist.MPI_HOMEentry was stale (the build now hard-errors with a specific message); added the macOSOMP: Error #15symptom (macOS: OMP Error #15 (duplicate libomp) aborts at first parallel region #27) and multi-rank GPU GDB failing on the helper dimension.b_comm_size"must be 1" without saying whose limit that is. It's ours — upstream's in-memorygdb::diagexpects a per-rank shard, and upstream's ownrun.shuses--b_comm_size 2.t_comm_size == 1then follows from upstream's algorithm.Verification
run_sbd_diag.pyruns from its new location, resolving its default vendored paths:-76.2359465468examples/,examples/tpb/with all six files,INSTALL.md, and the signpostpython/examplesreference survives except the MANIFEST line that ships the signpostpytest test/unchanged: 15 passedA follow-up PR (
gdb-sharding) builds on this and addsexamples/gdb/. Reviewing this one first keeps the rename out of that diff.🤖 Generated with Claude Code