test(bench): split the fixed-model benchmarks into four timed operations - #244
Open
diagonal-hamiltonian wants to merge 14 commits into
Open
test(bench): split the fixed-model benchmarks into four timed operations#244diagonal-hamiltonian wants to merge 14 commits into
diagonal-hamiltonian wants to merge 14 commits into
Conversation
Adds a memory arm to the random suite and makes `propagate` measurable on its own, so an A/B can attribute time and memory to `build_graph`, `propagate`, `energy` and `gradient` separately rather than to a test that fuses them. - `test_random_propagate` builds a fresh propagator per round, because `propagate()` raises `GraphStateConflict` on an instance already holding a graph, and carries `lower_atol=None` so it lands on the same operator as `build_graph`. `test_random_inplace` fused it with `expectation_value` behind a 1e-5 truncation and could not be read as either operation. - New per-op artifact sections: `opmemdelta` (peak above the operation's own floor -- the headline), `opmempeak`/`opmembase`, `opbytes` from the engine's own accounting, and `opmembreak`. Windows cover only the timed call. - `HighWaterMark` gains `open()`/`close()` so a window can be opened inside a pedantic `setup=` and closed after `pedantic()` returns, and `pinned_thread_summary()` reads placement from /proc/self/task/*/status:Cpus_allowed_list -- deliberately engine-independent, since a build without monoprop_COMM_PROFILE reports nothing about itself. - `PssSampler` is now opt-in (`--bench-pss-sampler`). Above a few GiB its smaps_rollup read is a page-table walk under mmap_lock, and it samples only on GIL release, so the faster arm reports the lower peak -- a confound, not a lower bound. Assisted-by: ClaudeCode:claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 3a1a897)
bench_models.py had one fused test -- steps x propagate plus an expectation value -- with no op_memory window and no operation split, so Hubbard and Pauli had no per-operation time or memory at all. Adds build_graph, propagate, energy and gradient, mirroring how bench_random.py splits the random problem, and reusing op_memory, record_opsize, barriered, bench_rounds and bench_comm unchanged. test_model stays exactly as it was: it is a tracked Bencher series, and replacing it would orphan the history. build_graph and propagate issue `steps` successive calls, because Hubbard's circuit is a single Trotter step the driver re-applies 29 times. Fewer would build a shorter graph than propagate evolves. Pauli's step count is 1, so the pair attributes per-call cost 29x against 1x between the two models without any extra code. conftest gains model_graph, the fixed-model analogue of built_graph: session-scoped and cached per model so energy and gradient share one build. It is a factory rather than a parametrized fixture because the benchmarks select their model with @pytest.mark.parametrize, which a session-scoped fixture cannot see. It records the config as well as the stats -- the graph cell runs nothing that calls record_model_config, and a sweep over cutoff and system size is unreadable without them. gc.freeze() as in random_problem: at these term counts the model's own objects are millions of GC roots that can never become garbage. record_model_stats' body moves to a module-level _record_model_stats so the fixture and model_graph cannot drift apart. The two groups do not belong in one process -- build_graph and propagate each hold their own operator while energy and gradient share one, and all four together do not fit a node at these sizes. Documented on the module. Assisted-by: ClaudeCode:claude-opus-5 (cherry picked from commit 73fa44d)
`build_graph` extends the graph rather than replacing it, so a model whose driver re-applies its circuit retains every step's layer-set where `propagate` retains one. Measured on hubbard at 1 rank x 16 partitions, lower_atol=1e-4: propagate 29 steps, 1,887,255 terms -> 379 MiB build_graph 29 steps, 1,887,255 terms -> OOM > 242 GiB That operator is ~110 MB at the 58 B/term a 2-step run measures, so it is the retained graph, and it does not shrink with rank count -- 1, 2 and 4 nodes all died the same way. `just bench-ci` passes -m "not slow" and never reached these, but `just bench` does not, so without this the three graph-holding benchmarks this PR adds would OOM the machine of anyone who ran them. They now skip with the measurement in the message. The limit is 2 rather than 1 because a 2-step hubbard reaches 278M terms in 15.4 GiB -- two steps is measured to fit, not assumed. Pauli's step count is 1 and is unaffected; set monoprop_BENCH_ALLOW_BIG_GRAPH=1 to override. Assisted-by: ClaudeCode:claude-opus-5 (cherry picked from commit 344fe37)
Merges pr/bench-record-placement (#230) into the per-operation profiling lineage. The two were siblings off the same commit, each carrying something the other lacked, and shipping either alone loses half the work. From #230: barriered()'s entry barrier moves into a new barrier_setup() so it runs inside pedantic's setup= rather than inside the timed call, and PssSampler becomes opt-in behind --bench-pss-sampler with HighWaterMark as the default. #230's own note that the barrier was worth ~1.44x of a 2.86x observed spread on build_graph is an unreplicated single-run observation -- it traces to no job id and no RESULTS doc -- and should be read as one. _record_placement is called from _record_model_stats rather than from either caller's body, which is where #230 put it. That reaches both the per-operation fixture and the session-scoped model_graph fixture; the fixed-model benchmarks open no memory window, so before this they recorded no placement at all and ab_summary.py refused their results -- correctly, but for an instrumentation gap rather than a bad run. Drops the "Placement must not divide an already-divided machine" paragraph this lineage had added to AGENTS.md. It is placement documentation carried by a benches commit, it is not on main, and it describes a design that exists on no branch it was ever committed to -- it names classify_node_mask, NodeMask::PerRank, Shared and a test, none of which exist on pr/partition-cgroup-placement either. It belongs with the placement fix, rewritten to that branch's actual vocabulary and without the 437 -> 15.5 us/sync figure, which is a pair of range midpoints measured on the pre-hwloc tree with a different branch as the arm. Assisted-by: ClaudeCode:claude-opus-5
…n stamp An A/B whose two arms cannot be told apart prints a full table of 1.00x and calls it flat, which is the most expensive way to fail here: the output looks exactly like a real negative result. __version__ cannot tell them apart. It is a git describe of the worktree, stamped into the dist-info when the package is installed, and a later `cmake --build` plus a copy of the .so into the venv does not rewrite it -- so an editable-install arm advertises the commit it was FIRST installed at while serving a binary from several commits later. Keying on it refused a run whose arms were provably distinct, and would equally have PASSED two arms sharing one .so across differing checkouts. The hash catches both directions; the version catches neither. So record the md5 of whatever `_core` actually resolved to -- not a path rebuilt from the source tree, since site-packages holds its own copy and the point is to fingerprint the file that was loaded. Versions stay in the report as a checkout-level breadcrumb. Assisted-by: ClaudeCode:claude-opus-5
CTest runs every Boost case as its own process, so an MPI build pays a full MPI_Init per case, and MPI_Init initialises every fabric device present even though a single-process test never sends a message: 8.8 s per process against 0.2 s of user CPU on a login node with 8 HCAs, i.e. 34 minutes for 224 cases. Excluding the fabric components takes each process to 1.9 s and the suite to 6.8 minutes, 224/224 passing. Scoped to the per-case `serial` variants through a new SERIAL_ENVIRONMENT argument on discover_tests, and deliberately kept off the multi-rank ones: a per-case launch has world size 1, so no transport is used and the fabric can only cost startup time, whereas OMPI_MCA_pml=^ucx makes a 2-rank run of zero_cutoff_upper_atol_zero_is_exact_World hang indefinitely where it otherwise passes in 29 ms. That hang reproduces on the pre-branch commit, so it is component selection rather than engine code. Assisted-by: ClaudeCode:claude-opus-5 (cherry picked from commit d8755f9)
…login node
The suite-wall figure this branch shipped ("34 min to 5.4") was never measured. It was
224 x 8.8 s of arithmetic over a login-node per-case cost and a case count that is now
stale, and the tree disagreed with itself: hpc/deucalion/README.md said 6.8 min for the
same change while AGENTS.md said 5.4.
Fabric init is much cheaper on the compute nodes the gate actually runs on, so the
login-node measurement sizes the mechanism but not the saving. Measured there instead:
ctest -L serial costs 0.61 s/case with the exclusion (208 cases in 126.03 s, job 1828011)
against 2.03 s/case without it (214 cases in 435.31 s, job 1828023) -- 1.42 s/case, a
median paired ratio of 3.42x, faster on 208 of the 208 cases the two runs share.
The comparison is across two branches rather than two arms of one experiment. The one
case that receives no exclusion on either side, the 2-rank monoprop_unit_tests.x_mpi_2,
differs by 1.3% (6.36 s against 6.44 s), which bounds the branch effect.
The login-node per-case table is kept and is now labelled as such.
Assisted-by: ClaudeCode:claude-opus-5
pinned_thread_summary() already parsed Cpus_allowed_list into a set of CPU ids and returned only its cardinality. Counts cannot distinguish sixteen threads on one NUMA domain from sixteen spread over four, which is the distinction a placement investigation turns on, so return the ids too. _record_placement gathers them per rank rather than keeping rank 0's alone: a partial placement failure -- some ranks placed, some not -- is the shape a Slurm cpuset confinement takes, and it is invisible in any single rank's view. The gather sits above the rank-0 check so every rank enters the collective, matching _reduce_min/_reduce_max.
_record_placement's docstring already gives the reason -- "a partial failure -- some ranks placed, some not -- is the interesting case, and it is the shape a Slurm cpuset confinement takes" -- but only single_cpu_threads was reduced. affinity_cpus came through **summary unreduced, so it carried rank 0's mask width alone and could not distinguish a node-wide confinement from one that confined rank 0 and left the other ranks free. Both reductions are unconditional collectives placed with the existing ones, before the rank-0 guard, so every rank still executes the same sequence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings in 1b7533f, which releases the benchmark harness as the monoprop-bench-tools package. Three conflicts, all resolved mechanically: benches/bench_random.py import block -> monoprop_bench_tools.models, keeping PR 3's barrier_setup alongside barriered benches/bench_models.py same import move, plus resting_rss_bytes from monoprop_bench_tools.memory.cpu; docstring now points at :data:`monoprop_bench_tools.models.MODELS` (main's wording) with PR 3's paragraphs on the four split operations kept AGENTS.md main's rewritten renderer bullet taken verbatim; PR 3's two additive bullets kept -- pedantic setup= doubling peak memory, and PssSampler's smaps_rollup cost, the latter repathed to monoprop_bench_tools.memory.cpu.PssSampler Main renamed benches/_builders.py -> monoprop_bench_tools.models (R100) and benches/_memory_cpu.py -> monoprop_bench_tools.memory.cpu (R091); PR 3's additions to both survive at the new paths. THIS TREE NOW REQUIRES monoprop-bench-tools TO BE INSTALLED, and no venv in this project has it: benches/conftest.py and benches/bench_models.py import monoprop_bench_tools at module scope, so the suite fails at collection without it. Installing it is a separate, deliberate step -- it touches venvs that hold declared A/B baseline binaries.
diagonal-hamiltonian
requested review from
fpietra,
ludmilaasb and
robertodr
as code owners
August 19, 2026 09:30
github-actions
Bot
added
documentation
|
Docs preview: https://pr-244.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #244 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
AGENTS.md carried a third copy of the MPI-fabric rationale already written above the option in cpp/tests/CMakeLists.txt and in the testing guide, Slurm job ids included, and two bullets unrelated to either: a test-methodology essay, and the editable-tree recipe that docs/content/docs/building.mdx already holds. One bullet was also wrong -- it claimed the `just bench` recipes pass `-s`, and no recipe does. README.md's new paragraph linked the testing guide four lines above the pre-existing sentence that links the same page, and testing.mdx quoted six site-specific numbers a reader cannot act on. cspell loses Deucalion/HCAs/Mellanox, which existed only to spell-check that prose. The `OMPI_MCA_*` component names stay: they are literal strings in the CMake. Assisted-by: ClaudeCode:claude-opus-5
…ield AGENTS.md asks comments to state what the code cannot and not to restate it, narrate history, or repeat a fact with a home elsewhere. These did all three: `_record_placement` explained why it had moved between two functions, `_core_md5` retold the incident that motivated it, and `barriered` carried a twelve-line measurement log ending in an open question -- in a published package's public docstring. The invariants they were buried in survive: which calls are collective, why the memory window opens inside `setup`, why `settle=False`, and why the exclusion cannot reach the MPI variants. `meta.transparent_hugepage` goes with them. Nothing reads it, and `_meta()` runs on rank 0 only, so it never described more than one node of a multi-node job anyway. Prose added by this branch, with the docs commit beneath: 454 lines -> 295, from 49% of the diff to 39%. Assisted-by: ClaudeCode:claude-opus-5
diagonal-hamiltonian
changed the title
Panadestein
approved these changes
Aug 19, 2026
Panadestein
left a comment
Panadestein
left a comment
Member
There was a problem hiding this comment.
Thanks! Take a look at the comments.
Comment on lines
+41
to
+43
| # layer-set per step: 3 steps of hubbard exceed a 242 GiB node where 2 fit in 15.4 GiB. | ||
| # Skipping beats OOM-killing the machine of whoever runs `just bench`, which does not | ||
| # pass -m "not slow". Pauli's step count is 1 and is unaffected. |
Member
There was a problem hiding this comment.
Suggested change
| # layer-set per step: 3 steps of hubbard exceed a 242 GiB node where 2 fit in 15.4 GiB. | |
| # Skipping beats OOM-killing the machine of whoever runs `just bench`, which does not | |
| # pass -m "not slow". Pauli's step count is 1 and is unaffected. |
| from monoprop_bench_tools.models import MODELS, barriered | ||
| from monoprop_bench_tools.models import MODELS, barrier_setup, barriered | ||
|
|
||
| # `build_graph` extends the graph, so a driver that re-applies its circuit retains one |
Member
There was a problem hiding this comment.
Suggested change
| # `build_graph` extends the graph, so a driver that re-applies its circuit retains one | |
| # `build_graph` extends the graph, so a driver that re-applies its circuit retains one layer-set per step |
Comment on lines
+94
to
+118
| def _reduce_max(comm: Any, value: int) -> int: | ||
| """Return the largest ``value`` over ranks. Collective; serial returns ``value``.""" | ||
| if comm is not None and comm.Get_size() > 1: | ||
| return comm.allreduce(value, op=MPI.MAX) | ||
| return value | ||
|
|
||
|
|
||
| def _reduce_min(comm: Any, value: int) -> int: | ||
| """Return the smallest ``value`` over ranks. Collective; serial returns ``value``.""" | ||
| if comm is not None and comm.Get_size() > 1: | ||
| return comm.allreduce(value, op=MPI.MIN) | ||
| return value | ||
|
|
||
|
|
||
| def _gather_lists(comm: Any, values: list[int]) -> list[list[int]]: | ||
| """Gather per-rank CPU-id lists to rank 0. Collective; off root returns ``[]``.""" | ||
| if comm is None or comm.Get_size() == 1: | ||
| return [values] | ||
| gathered = comm.gather(values, root=0) | ||
| return gathered if gathered is not None else [] | ||
|
|
||
|
|
||
| def _spread(comm: Any, value: int) -> dict[str, int]: | ||
| """Reduce a per-rank number to ``sum`` (bounds the job) and ``max`` (bounds a node).""" | ||
| return {"sum": _reduce_sum(comm, value), "max": _reduce_max(comm, value)} |
Member
There was a problem hiding this comment.
Nonblocking: Do you think this bunch of MPI wrappers could be reused somewhere else in the codebase?
Comment on lines
+54
to
+65
| # CTest runs every Boost case as its own process, so each pays a full MPI_Init -- which | ||
| # initialises every fabric device present even though a single-process test never sends a | ||
| # message. Measured on a compute node: 2.03 s per case without the exclusion, 0.61 s with it. | ||
| # | ||
| # A per-case launch is ONE process (world size 1, `*_World` cases skip themselves, every | ||
| # other communicator is MPI_COMM_SELF), so the fabric can only cost startup time. The MPI | ||
| # variants exchange real messages and MUST keep the full component set -- `OMPI_MCA_pml=^ucx` | ||
| # hangs a 2-rank run indefinitely -- hence SERIAL_ENVIRONMENT rather than a global setting. | ||
| # | ||
| # Exclusions (`^`), never a positive component list: `vader` became `sm` in Open MPI 5, so | ||
| # naming a component breaks on the next machine. Unknown OMPI_MCA_* variables are ignored by | ||
| # other MPIs, so this is inert under MPICH. |
Member
There was a problem hiding this comment.
I would condense this to two lines, really:
Suggested change
| # CTest runs every Boost case as its own process, so each pays a full MPI_Init -- which | |
| # initialises every fabric device present even though a single-process test never sends a | |
| # message. Measured on a compute node: 2.03 s per case without the exclusion, 0.61 s with it. | |
| # | |
| # A per-case launch is ONE process (world size 1, `*_World` cases skip themselves, every | |
| # other communicator is MPI_COMM_SELF), so the fabric can only cost startup time. The MPI | |
| # variants exchange real messages and MUST keep the full component set -- `OMPI_MCA_pml=^ucx` | |
| # hangs a 2-rank run indefinitely -- hence SERIAL_ENVIRONMENT rather than a global setting. | |
| # | |
| # Exclusions (`^`), never a positive component list: `vader` became `sm` in Open MPI 5, so | |
| # naming a component breaks on the next machine. Unknown OMPI_MCA_* variables are ignored by | |
| # other MPIs, so this is inert under MPICH. | |
| # CTest launches each Boost case as a world-size-1 process, so excluding fabric components cuts MPI_Init from 2.03 s to 0.61 s without affecting communication; real MPI tests must keep the full component set. | |
| # Use SERIAL_ENVIRONMENT with exclusions (`^`), not a positive component list: Open MPI component names change across versions, and unknown OMPI_MCA_* variables are harmless under other MPIs. | |
|
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.
🤖 AI text below 🤖
Branch
pr/bench-and-test-harness@302767e, merged up toorigin/main@1b7533f.main's #227 moved the shared bench modules intopackages/monoprop-bench-tools/, so theresting_rss_bytesandMODELSedits land inside that package — visible as renames withdetection on. This branch now requires
monoprop-bench-toolsinstalled, asmaindoes.What this changes
1. The four fixed-model benchmarks become four separately timed operations —
build_graph,propagate,energy,gradient— instead of one blended figure, each withits own time and its own memory window. That split is what makes the operator/graph memory
question answerable:
build_graphandpropagateare mutually exclusive per process, so apropagaterow's peak RSS is operator memory (its graph ledger reads exactly0) and abuild_graphrow's is graph memory. No new instrument was needed.test_modelisuntouched — it is a tracked Bencher series, and renaming it would orphan its history.
2.
monoprop_TEST_EXCLUDE_MPI_FABRIC, a new CMake option, ON for MPI builds: thesingle-process test variants stop initialising every fabric device in
MPI_Init. 2.03 s →0.61 s per case, faster on all 208 cases the two runs share. This changes the CI default
there, and it is safe only there — the same setting hangs multi-rank runs, so it is scoped
to the
serialvariants throughdiscover_tests' newSERIAL_ENVIRONMENTargument.Supporting: the entry barrier comes out of the timed region, where it was charging each
measurement for skew in the preceding setup; an A/B arm is identified by its
_core.sohash,not by a dist-info version that goes stale after a rebuild; per-rank kernel peak RSS and
thread affinity are read from
/procand/usr/bin/time -v, neither cluster-specific; andmodel benchmarks whose graph cannot fit skip instead of OOM-killing the run.
Measurement — this PR is the null control for the rest of the stack
It adds no library code.
pr3andmaindiffer only by an upstreamclass→typenamesweepthat is provably codegen-identical, yet their extension modules hash differently, so the A/B
harness treats them as two genuine arms and measures them like any other pair — which
measures the harness's own false-positive rate instead of assuming it.
0 of 24 timing tests resolved, best agreement anywhere 8 of 10 reps. Every operation flat
on both workloads, both layouts, both node counts; term counts identical in all 12 cells.
That is what the rest of the stack is read against.
The grid: layout A = 1 rank/node × 128 partitions, B = 8 × 16; N = 1 and 2; hubbard c10 /
lower_atol=1.25e-5, pauli c14 /5e-5, plus abuild_graph[hubbard]rung at--hubbard-trotter-steps=2. A and B hold the flat world P constant (128 at N=1, 256 at N=2),so A-vs-B is transport alone.
--cpu-bind=cores --distribution=block:block, both arms,recorded per cell. 10 interleaved reps, order flipped per (rep, cell); median of paired
per-rep ratios, sign test, Holm across the 24-test family. Ten reps because six floors the
sign test at p=0.031, which Holm over 24 tests makes 0.75.
Not reproducible from this diff. Measured at
cd58352, before the merge, on Deucalion bya private harness driving two prebuilt venvs whose hashes are above. The batch scripts and
report generators live in a separate private repo and are not proposed for this one.