CI: exercise the flexible-rank RMA path on two MPI implementations - #827
Draft
DLWoodruff wants to merge 4 commits into
Draft
CI: exercise the flexible-rank RMA path on two MPI implementations#827DLWoodruff wants to merge 4 commits into
DLWoodruff wants to merge 4 commits into
Conversation
The flexible (unequal) rank path is the only place mpi-sppy uses passive-target MPI RMA (Lock/Get/Put), the least portable corner of the spec, and the design doc gates recommending the feature for production on running it against at least two MPI implementations and more than one mpi4py/MPI version. Until now the one job that runs those tests did a bare `conda install mpi4py`, which resolves to MPICH -- so only MPICH was ever covered, and nothing in the logs said so. `test-cylinders` becomes a three-cell matrix: MPICH, OpenMPI, and MPICH with mpi4py pinned to 3.1. Dry-run solves give OpenMPI 5.0.10 with mpi4py 4.1.2, MPICH 5.0.1 with mpi4py 4.1.2, and MPICH 4.3.2 with mpi4py 3.1.6 -- two implementations, two MPICH versions, two mpi4py versions. `fail-fast: false`, because the point is to learn which builds the path works on, so every cell has to report. A new step prints `MPI.Get_library_version()`, making the log the evidence the gate asks for. The job now installs from conda-forge exclusively. That is the channel maintaining per-implementation MPI variants: asking for `openmpi` there also pulls the openmpi-flavored mpi4py through the `mpi` metapackage, so each cell exercises the MPI it names. On defaults the mpi4py build is not variant-selected, and an "openmpi" cell could silently keep linking MPICH. `-oversubscribe` is OpenMPI-only and the np=6 runs want more ranks than a runner has cores, so it is applied conditionally. The existing detection in run_coverage.bash parses the mpiexec banner for "Open MPI"/"OpenRTE", which OpenMPI 5 breaks -- it launches through PRRTE. Both the workflow and run_coverage.bash now ask mpi4py what it is linked against instead, which is stable across versions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #827 +/- ##
=======================================
Coverage 76.82% 76.83%
=======================================
Files 177 177
Lines 23498 23498
=======================================
+ Hits 18053 18055 +2
+ Misses 5445 5443 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The first run of the matrix failed only in the openmpi cell, and not on anything to do with RMA. It died in the pre-existing *equal-rank* step (test_with_cylinders.py at np=2), which then skipped the flexible-rank step entirely -- leaving the question the matrix exists to ask, does the RMA path work under OpenMPI, unanswered. test_xhatshuffle_coverage compares the value its helper extension records on iteration 0 against the one from iteration 1, so the hub has to run both. It asks for max_iterations=2 but never overrides cfg.rel_gap, which defaults to 0.05, and the hub stops as soon as the inter-cylinder gap drops below it. If the xhatshuffle spoke lands an incumbent before the iteration-1 convergence check, PH terminates after one iteration, only one value is recorded, and v[1] raises IndexError. The CI log shows exactly that: "Terminating based on inter-cylinder relative gap 4.595%" immediately before the traceback. That is a race between the spoke and the hub, so it fires on some MPI/host combinations and not others -- OpenMPI on a runner wins the race, MPICH there and OpenMPI 4.1.6 locally lose it. Pinning rel_gap to 0 makes the two iterations happen unconditionally, which is what the assertion assumes; the run now ends on "Reached user-specified limit=2" instead. Guarding the assertion on len(v) >= 2 would have hidden the check instead of fixing it. The flexible-rank step also gets `if: !cancelled()` so it runs even when the equal-rank step above fails. Both still fail the job -- the point is only that a pre-existing equal-rank failure must not be able to conceal the flexible-rank result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design doc held that the fullcomm path must not be documented or recommended for production use until it had run on at least two MPI implementations and more than one mpi4py/MPI version, that being where the RMA-portability risk lives. The matrix added in this PR does that on every pull request, and its first green run covered: mpich MPICH 5.0.1 mpi4py 4.1.2 openmpi OpenMPI 5.0.10 mpi4py 4.1.2 mpich, mpi4py 3.1 MPICH 4.3.2 mpi4py 3.1.6 Two implementations, two MPICH versions, two mpi4py versions, each running the whole unequal-rank suite at np=6. The OpenMPI cell's log carries the coherence-diagnostic lines from all four cylinder types, which only print when multi-source reads actually happen -- so the unequal-rank path demonstrably ran there rather than being skipped. That was the last item on the list, so the doc now records that no release gates remain, and states what the matrix covers rather than what someone still has to do. With the feature no longer gated, the read-outcome diagnostic gets a user-facing entry in secretmenu.rst: the always-on finalization summary, what each of the five buckets means, how to tell a coherence problem from a slow upstream sender, and the `coherence_diagnostics_period` knob for watching a run live. It is set in opt_kwargs["options"], the dict every cylinder has. Three caveats are documented rather than fixed, since they affect how the numbers read and not whether they are collected: counters aggregate per Field with no peer-cylinder attribution, the periodic line's buckets sum to total-1 because it prints before the current read is bucketed, and equal-rank runs print nothing at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The feature has had no page under doc/src at all -- it was documented only in doc/designs/flexible_rank_assignments.md, which is not part of the Sphinx build. That was correct while the design doc's release gate said the fullcomm path must not be documented until it had run on two MPI implementations. This PR closes that gate, so the page can exist. It is written for someone deciding whether to use the feature rather than someone implementing it: why unequal splits are worth wanting, the eight --<spoke>-rank-ratio options, how ratios are apportioned (largest-remainder with a floor of one, summing to exactly -np), and the fact that all-1.0 ratios take the same code path as before, which is also what makes turning it back off a complete fallback. Limitations are stated plainly: reduced_costs has no rank-ratio option because its per-scenario field never got a multi-source assembler, and any other unsupported field fails at startup during window creation with a message naming the cylinder and the field, rather than mis-assembling a buffer. The diagnostics section explains why a bounds cylinder can look like it reports rarely on an unequal-rank run, and points at coherence_diagnostics_period in secretmenu for the details; secretmenu gains the label that reference needs. The worked example is the one from the design doc, run end to end first: 14 ranks at 1.0/0.5/0.25 does print "per-cylinder rank counts [8, 4, 2]". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DLWoodruff
marked this pull request as draft
August 10, 2026 22:58
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
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.
Closes the last release gate for flexible (unequal) rank assignments.
The flexible-rank path is the only place mpi-sppy uses passive-target MPI RMA (
Lock/Get/Put), the least portable corner of the spec.doc/designs/flexible_rank_assignments.mdtherefore gates recommending the feature for production on exercising it against "at least two MPI implementations (e.g. OpenMPI and MPICH) and more than one mpi4py / MPI version".Until now the one job that runs those tests —
test-cylinders— did a bareconda install mpi4py, which resolves to MPICH. So only MPICH was ever covered, and nothing in the logs said which implementation had been used.The matrix
test-cylindersbecomes a three-cellincludematrix. Resolved versions, from dry-run solves:mpichopenmpimpich, mpi4py 3.1Two implementations, two MPICH versions, two mpi4py versions.
fail-fast: false, because the point is to learn which builds the RMA path works on — every cell has to report rather than being cancelled by a sibling.A new step prints
mpiexec --versionandMPI.Get_library_version(), so the job log is the evidence the gate asks for instead of an assumption about what conda picked.conda-forge, exclusively
The job now sets
channels: conda-forgewithconda-remove-defaults. conda-forge is the channel that maintains per-implementation MPI variants: asking foropenmpithere also pulls the openmpi-flavored mpi4py through thempimetapackage, so each cell exercises the MPI it names.On the defaults channel the mpi4py build is not variant-selected, so an "openmpi" cell could install OpenMPI beside an mpi4py still linked against MPICH — a cell that looks like coverage and is not.
A bug this turned up
-oversubscribeis OpenMPI-only (MPICH rejects it) and thenp=6runs ask for more ranks than a runner has cores, so it has to be applied conditionally. The existing detection inrun_coverage.bashgreps thempiexec --versionbanner forOpen MPI/OpenRTE.That breaks on OpenMPI 5, which launches through PRRTE and no longer reliably prints either string. The openmpi cell would have silently dropped the flag and failed at
np=6with "not enough slots available".Both the workflow and
run_coverage.bashnow ask mpi4py which library it is linked against (MPI.Get_library_version()), which is stable across versions. Verified locally on OpenMPI 4.1.6: detection fires, and single-dash-oversubscribeworks atnp=6.Notes for review
tests on some cylinderstotests on some cylinders (mpich)/(openmpi)/(mpich, mpi4py 3.1). If branch protection lists the old name as a required check, that entry needs updating.upload-artifact@v4errors on duplicates); the aggregator globscoverage-*, so they are still collected.Follow-ups folded in after the first run
The first run failed in the
openmpicell — not on RMA. It died in the pre-existing equal-rank step (test_with_cylinders.pyat np=2), which then skipped the flexible-rank step, leaving the question this job exists to ask unanswered.test_xhatshuffle_coveragecompares the value its helper extension records on iteration 0 against iteration 1, so the hub has to run both. It asks formax_iterations=2but never overridescfg.rel_gap, which defaults to0.05, and the hub stops as soon as the inter-cylinder gap drops below it. The log showsTerminating based on inter-cylinder relative gap 4.595%immediately before theIndexError. That is a race between spoke and hub, so it fires on some MPI/host combinations and not others.rel_gapis now pinned to 0 in that test, which is what the assertion already assumed; guarding onlen(v) >= 2would have hidden the check instead of fixing it.The flexible-rank step also gets
if: !cancelled(), so a pre-existing equal-rank failure can no longer conceal the flexible-rank result. Both still fail the job.Result
All three cells green, and the OpenMPI cell's log carries
coherence diagnosticlines fromPHHub,LagrangianOuterBound,XhatShuffleInnerBoundandCGHub— those only print when multi-source reads actually happen, so the unequal-rank path demonstrably ran there rather than being skipped.That was the last item on the "finish before recommending it" list, so the design doc now records that no release gates remain, and states what the matrix covers instead of what someone still has to do.
With the feature no longer gated, flexible ranks gets its first user-facing page,
doc/src/flexible_ranks.rst(Advanced Topics). Until now the feature was documented only indoc/designs/, which is not part of the Sphinx build. The page covers the eight--<spoke>-rank-ratiooptions, the largest-remainder apportionment, the fact that all-1.0 ratios take the same code path as before (which is what makes reverting a complete fallback), and the limitations —reduced_costshas no ratio option, and any other unsupported field fails at startup rather than mis-assembling. The worked example was run end to end first: 14 ranks at 1.0/0.5/0.25 does printper-cylinder rank counts [8, 4, 2].The read-outcome diagnostic from #816 also gets an entry in
doc/src/secretmenu.rst: the always-on finalization summary, what each of the five buckets means, how to tell a coherence problem from a slow upstream sender, and thecoherence_diagnostics_periodknob. Three review caveats are documented rather than fixed, since they affect how the numbers read and not whether they are collected — counters aggregate perFieldwith no peer attribution, the periodic line's buckets sum tototal - 1, and equal-rank runs print nothing.🤖 Generated with Claude Code