Conversation
The first step of #539. `mpi-pytest` runs pytest itself under `mpiexec` and executes each `@pytest.mark.parallel(N)` test on `MPI.COMM_WORLD` of a right-sized job, so the whole suite pays one interpreter start instead of one per test. Measured on this file (72 cases, 2 ranks, median of 3): **78.0 s -> 2.7 s**. The cost being amortised is the pySDC/numpy/scipy import, not `mpirun` -- locally `mpirun -np 2 python -c pass` is 0.03 s while a full child is 0.58 s. The launcher, the `launch=` flag and the `__main__` argparse block that existed only so the file could re-enter itself under `mpirun` are all gone; the test body is what used to be the child. `pytest-isolate-mpi` 0.3 refuses to load inside an `mpiexec` session (fixed upstream in 0.4, which we cannot take yet), so while both plugins are installed the mpi-pytest invocations need `-p no:pytest_isolate_mpi`. That and the pin go away with the last ported test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mpi4py job now makes one `mpiexec -n N` pytest launch per rank count in use (2..5) plus one ordinary launch, instead of one `mpirun` per test case. The five passes partition the suite exactly -- verified by collection count: 75 (n=2) + 3 (n=3) + 0 + 0 + 429 (serial) = 507 = every mpi4py-marked test. `parallel[1]` selects tests that are unmarked or explicitly serial, so the ordinary pass picks up everything not yet ported without re-running anything that is, and nothing falls back to mpi-pytest's forking mode (which measured 2.1x slower than the subprocess status quo). That count is the invariant to keep: as files are ported, tests move from the serial pass into a rank pass and the sum stays at the total. Ported: test_compute_end_point, test_base_transfer_MPI. Both drop their launcher and their `__main__` re-entry block; in the latter the `_test_`/`test_` wrapper pair collapses into one function, since the indirection only existed to be called back from `mpirun`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Marker swap plus dropping the mandatory `mpi_ranks` fixture argument. `mpi-pytest` has no `comm` fixture -- it runs the test on `MPI.COMM_WORLD` of a right-sized job -- so `test_loggingMPI` takes the communicator from `MPI` directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e-mpi Verified at every rank count they declare: n=1 157 passed, n=2 130, n=4 25, n=5 1. pySDC/tests no longer uses pytest-isolate-mpi at all; only pySDC/projects/RayleighBenard/tests/test_sweeper.py still does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both had the same `individual_test(..., launch=True)` shape; the launcher branch and the `__main__` argument-parsing block go, and the comparison becomes the test body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… communicator The remaining `run_MPI_test` launcher and the `__main__` argument parser are gone, so `axes` is passed as a tuple rather than a string that argparse had to convert back. While porting: `test_differentiation_matrix2D` never accepted a communicator. It built `SpectralHelper(comm=None)` unconditionally, so `test_differentiation_MPI` was running N redundant *serial* copies under `mpirun` -- paying the launch cost while testing nothing about the distributed path. It now takes `useMPI` and passes `MPI.COMM_WORLD`, exactly as its sibling `test_integration_matrix2D` already did. Verified passing on 1 and 2 ranks with a real communicator, so this closes the gap rather than papering over it. Verified: n=1 114 passed, n=2 125, n=4 21, and the 657 `base` tests in the same file unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_extrapolation_within_Q, test_polynomial_error, test_adaptive_collocation and test_basic_restarting. In the last one `single_test` goes too: it existed only to dispatch between the two test bodies based on an integer passed on the command line. test_crash is deliberately left alone -- it aborts a rank on purpose, which needs isolation. Verified at every rank count each declares. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_log_work, test_entry_class, test_stats_helper and the mpi4py half of test_datatypes. test_datatypes keeps its launcher and __main__ block for test_PyTorch_dtype: that runs in the pytorch CI job, which is plain pytest in an environment without mpi-pytest, so it cannot use the parallel marker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_controller_ParaDiag_MPI and test_controller_conformance. Both used to launch `mpirun` and hand results back through a file; with pytest already running on the ranks, both transports run in-process and the file round-trip goes. test_controller_conformance had batched every case into a single launch precisely because starting an MPI job dominated the module (#675). There is no launch left to amortise, so the fixture just runs the cases. ParaDiag's checks only hold on the last rank, which is where the end of the block lives, so they use mpi-pytest's `parallel_assert(..., participating=last)` rather than a rank-divergent plain assert. Mutation-checked: perturbing the compared value fails 4 of 6 at n=2 and 3 of 4 at n=4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The write batching from #675 -- one mpirun for all grid sizes of a case -- is no longer needed: pytest is already on the ranks, so the writes just happen. Two things the shared session forces into the open: - `tmpdir` is per-rank, so rank 0's path is broadcast; every rank has to write the same files. - `Rectilinear.setupMPI` is class-level state. After the writes, every `Rectilinear` in the interpreter reads back only its local block, which made the verification read fail with (1, 16, 8) instead of (1, 61, 61). The old model got the global read for free because the checks ran in the parent process, which had never called `setupMPI`. The class is now put back into serial mode explicitly, in a `finally` so a failing write cannot leave it that way for the rest of the session. Verified: n=2 32 passed, n=4 32 passed, and the 370 serial tests in the file unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e entry point `C_pySDC_with_PETSc.main()` read `sys.argv` directly, so the only way to drive it was to launch it as a script. It now takes `num_procs_space` and `fname` as arguments, with the argv parsing moved into the `__main__` block -- so it is still runnable exactly as the tutorial text describes, and the tests can call it on the ranks pytest already has. The petsc CI env gets mpi-pytest and the same per-rank-count invocation as mpi4py. Verified: parallel[1] 1 passed, n=2 1 passed, n=4 1 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing uses that plugin any more, so the dependency, the 0.3 pin and the transitional `-p no:pytest_isolate_mpi` flag all go. The pin existed because 0.4 broke module teardown; the flag existed because 0.3 refuses to load inside an `mpiexec` session. Neither problem is ours now. The RayleighBenard project job gets the same per-rank-count invocation as the mpi4py and petsc jobs. Verified with a environment holding mpi-pytest and no isolate-mpi: mpi4py n=2 250 passed, n=3 17, n=4 82, n=5 3 partition exact: 250+17+82+3+339 serial = 691 = every mpi4py-marked test petsc partition exact: 1+1+1 = 3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A `PFFT` allocates MPI communicators -- `MPI_Cart_create` plus an `MPI_Cart_sub` per axis -- and
never releases them when the Python object is collected. Measured on MPICH 4.3.2: creating and
dropping `IMEX_Laplacian_MPIFFT` fails at **1022 instances** with
MPI_Cart_create(MPI_COMM_WORLD, ndims=3, ...) failed
`PFFT.destroy()` is the release path, it is idempotent, and arrays already made with
`newDistArray` stay usable afterwards -- so `pySDC/helpers/fft_helper.py` subclasses `PFFT` to call
it on garbage collection, and the five construction sites now build that subclass. One definition
rather than a guard per owner, since the owners (problem classes, a transfer class and
`SpectralHelper`'s cache) have no common base.
This is not a test-only concern: any long-running driver that builds many problems hits the same
ceiling. It stayed invisible because every MPI test used to get its own short-lived `mpirun`, so no
process lived long enough to reach 1022.
Verified: the reproducer reaches 4000 creations clean (was 1022), `SpectralHelper` reaches 2000, and
the mpi4py serial pass goes from **108 failed to 339 passed**. Rank passes unchanged
(n=2 250, n=3 17, n=4 82, n=5 3); petsc 1/1/1; the 3443 `base` tests still pass.
Also drops `--oversubscribe` from the two mpiexec invocations: PR #671 already sets
`PRTE_MCA_rmaps_default_mapping_policy=':oversubscribe'` workflow-wide, which is the OpenMPI 5
spelling, and the bare flag is deprecated in PRRTE (and rejected outright by MPICH locally).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`shell: bash -l {0}` carries no `-e` -- GitHub only adds that for the `bash` shorthand -- so with
the per-rank-count loop a failing pass in the middle was swallowed and the step's status came from
the last command alone. Before the loop the step was a single command, so this could not happen.
Each pass now goes through `run_pass`, which propagates the failure but tolerates pytest's exit 5
("no tests collected"), expected whenever an environment has no test asking for that rank count.
Verified against a stubbed pytest: green -> 0; empty selections -> 0; a failure in the first,
middle or last pass, and a failure after an empty pass -> 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#693 landed the `rtol=0` fix on three files this branch had already ported. The one real conflict was in `test_MPI_sweeper`, where their copy still sits inside the `if launch:` branch this branch removes -- resolved by keeping our structure and their tolerance. All of #693's `rtol=0` additions survive, matching master's counts file for file (spectral_helper 4, ParaDiag 1, MPI_sweeper 2). Verified under the tighter tolerances: n=1 117 passed, n=2 203, n=4 25, and 667 `base` tests.
That module also holds `base`-marked tests, so it has to import in environments that have neither mpi4py nor mpi-pytest -- which is every job except mpi4py and petsc. Porting it to mpi-pytest put both imports at module level and broke collection there: the `base` and `fenics` matrix jobs failed with `ModuleNotFoundError: No module named 'pytest_mpi'` before running a single test. Both imports now sit in the four MPI tests that use them, which is how the rest of pySDC handles optional backends. Verified in an environment without mpi-pytest: 10 base tests pass where collection previously aborted, and the whole of pySDC/tests collects with no error attributable to this branch. With mpi-pytest: n=1 3 passed, n=2 6, n=4 4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The last three launchers in pySDC/tests: test_firedrake_mesh's communication test (2 ranks), step_7's test_E_MPI (3) and gusto's MSSDC coupling test (4). The firedrake job now uses the same per-rank-count invocation as the others, and installs mpi-pytest explicitly rather than relying on the image carrying Firedrake's own test extras. `test_pySDC_integrator_MSSDC` is split in two rather than given one marker, because its branches want different sizes: the MPI controller divides COMM_WORLD between space and time and needs a real 4-rank job, while the serial controller runs the whole block itself and belongs on one rank. The shared body moves into `_run_MSSDC`. The MPI half passes `setup=None` and builds its own on the space communicator, which is what the subprocess it used to launch did -- that child received `setup=None` whenever it ran on more than one rank. Verified locally as far as is possible without firedrake: the partition is exact by collection (2 + 1 + 2 + 32 serial = 37 = every firedrake-marked test), all three modules still import with neither firedrake nor mpi-pytest present, so the base/fenics jobs keep collecting, and black/ruff are clean. The tests themselves can only be exercised by the firedrake job. Only two launchers remain in pySDC/tests, both deliberate: test_crash (aborts a rank on purpose) and test_datatypes' pytorch case (its job has no mpi-pytest). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iner The firedrake job's n=2 pass ran fine and then n=3 died with OpenMPI 4's "There are not enough slots available in the system to satisfy the 3 slots that were requested". PRRTE counts a slot as a physical core, so 3 ranks do not fit on a 4-vCPU runner. #671 already set `PRTE_MCA_rmaps_default_mapping_policy`, but that is the OpenMPI 5 spelling and the Firedrake image ships OpenMPI 4, which reads `OMPI_MCA_rmaps_base_oversubscribe` instead. The conda envs resolve OpenMPI 5 and were therefore unaffected -- which is why this only showed up once the firedrake job started launching more than two ranks. Each version ignores the other's variable, so both can be set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…re coverage Two gaps this branch left behind. **The partition was only ever checked by hand.** The rank counts live in the workflow while `@pytest.mark.parallel(n)` lives in the test, so a test asking for a count nobody launches is deselected by every pass and silently never runs -- invisible in a green pipeline. `etc/check_parallel_partition.sh` asserts that the passes claim exactly as many tests as the marker selects, and runs in the mpi4py, petsc, firedrake and RayleighBenard jobs before their tests do. It needs no MPI: mpi-pytest attaches the `parallel[n]` markers at collection time, so five plain collections settle it in seconds. Checked both ways: it passes on this tree (mpi4py 339+218+17+50+3 = 627 = 627, petsc 3 = 3, RayleighBenard 25 = 25) and fails with the intended message when a `@pytest.mark.parallel(6)` test is added while the workflow launches only 2..5. **The firedrake job has never measured anything.** Its coverage database carries 365 file rows and exactly one row of line data on master, and zero on this branch -- dropping to zero because the `COVERAGE_PROCESS_START` subprocesses that produced that single row are what the migration removes. The cause is older than this branch: the container mounts the workspace at /repositories, the job sits in ./pySDC, and pySDC is installed editable from /repositories/pySDC, so the path its modules report never matches the `source = ['pySDC']` resolved against the working directory. Running from /repositories/pySDC makes the two agree. `..` is the workspace from either path, so the artifact moves are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
That was the last launcher in pySDC/tests other than test_crash, and it only existed because the pytorch job had no mpi-pytest. It has one now, and that job gets the same per-rank-count invocation, so `launch_test` and the `__main__` block go with it. `test_PyTorch_dtype` is split rather than given one marker, for the same reason the gusto one was: its `useMPI` parametrisation wanted 4 ranks in one branch and 1 in the other. Partition checked for the new marker: 3 = 3. Co-Authored-By: Claude Opus 5 <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
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.
Addresses #539.
pySDC ran MPI tests by having pytest
subprocessa freshmpirunper test case, with each testfile re-entering itself through a
__main__argparse block. That is slow, and the useful part of afailure — the assertion, the rank it happened on — was reduced to a return code.
This moves
pySDC/teststompi-pytest, whichruns pytest itself under
mpiexecand gives each@pytest.mark.parallel(n)testMPI.COMM_WORLDof a right-sized job. The suite pays one interpreter start per rank count insteadof one per test.
Measured on this CI
mpi4pyjob (each of 5)Per the earlier CI-duration work the
mpi4pyjobs were what set the pipeline's wall clock; theyare no longer on the critical path, which is now
parallelSDC_reloadedand its floor of genuine SDCnumerics. The
firedrakejob is unchanged at 9.7 min — it only has five MPI tests, so there wasnever much launch cost there; porting it was about consistency and dropping
pytest-isolate-mpi.Why this plugin
Measured on
test_MPI_sweeper.py(72 cases, 2 ranks, median of 3, one machine):subprocess+mpirunper casempi-pytestnon-forkingpytest_parallel(ONERA), static schedulerpytest-isolate-mpi0.3mpi-pytestforking modeTwo things worth stating plainly, because #539 bundles "slow" and "bad error messages" as one
problem and they pull in opposite directions:
to start than the plain
python script.pychild the current code spawns. Migrating topytest-isolate-mpifor the diagnostics would have been a ~15% wall-clock tax.mpirun. Locallympirun -np 2 python -c passis 0.03 swhile a full child is 0.58 s. The win comes from amortising the pySDC/numpy/scipy import.
mpi-pytestoverpytest_parallelon operational grounds rather than speed: it is on PyPI, it doesnot collide with
pytest-timeout(our only deadlock guard), its isolate-mode equivalents work onmacOS, it was already in the Firedrake image, and it ships
parallel_assertfor rank-divergentassertions that otherwise deadlock.
Coverage
Model B needs no setup —
coverage runwraps each rank, so coverage measures the process thatexecutes the test. Measured on
generic_implicit_MPI.py:COVERAGE_PROCESS_START?That last row is a silent failure: tests pass, the run looks healthy, the number quietly drops.
CI shape
For the
mpi4py,petsc,firedrakeandRayleighBenardjobs: onempiexec -n Npytest launchper rank count in use, plus one ordinary pass.
parallel[1]selects tests that are unmarked orexplicitly serial, so nothing runs twice and nothing falls back to forking mode.
run_passexists becauseshell: bash -l {0}carries no-e— GitHub adds that only for thebashshorthand — so a failing pass mid-loop would otherwise be swallowed and the step's statuswould come from the last command alone. It propagates failures but tolerates pytest's exit 5 for a
rank count no test in that environment asks for.
The rank counts live in the workflow while
@pytest.mark.parallel(n)lives in the test, so a testasking for a count nobody launches would be deselected by every pass and never run — invisible in a
green pipeline.
etc/check_parallel_partition.shasserts the passes claim exactly as many tests asthe marker selects, and runs in all four jobs before their tests. It needs no MPI (the markers are
attached at collection time) and takes seconds. It fails as intended when a
parallel(6)test isadded while the workflow launches 2..5.
Verification that the tests actually run
Collection partition is exact, and so is execution. With a plugin asserting that every
parallel-marked test sees the world size it declares:No test is skipped anywhere, and none runs on a communicator smaller than it asked for.
The tests also still bite. Deleting a collective in
generic_implicit_MPIfails them:AllreduceuendBcastA real bug this surfaced: leaked mpi4py-fft communicators
A
PFFTallocates MPI communicators (MPI_Cart_createplus anMPI_Cart_subper axis) and neverreleases them. Creating and dropping
IMEX_Laplacian_MPIFFTfails at 1022 instances on MPICH:Pre-existing and not test-specific — any long-running driver that builds many problems hits the same
ceiling. It stayed invisible because every MPI test used to get its own short-lived
mpirun.Sharing one session reaches it: without the fix the serial pass fails 108 tests while each of those
files passes in isolation and master shows zero failures.
Fixed in
pySDC/helpers/fft_helper.py: aPFFTsubclass whose__del__callsdestroy(). Onedefinition, since the owners (three problem/transfer classes and
SpectralHelper's cache) share nobase.
destroy()is idempotent and arrays made withnewDistArraystay usable after it, bothchecked before tying release to garbage collection.
Not related to the exit-143 MPICH flakiness fixed in #671: that was a silent SIGTERM ~1 s into
collection, before any test body runs and therefore before any
PFFTexists, and it wasAzure-region correlated. This fails deep into a run with a loud MPI error stack and no region
dependence.
Three smaller defects found while porting
test_differentiation_matrix2Dnever accepted a communicator — it builtSpectralHelper(comm=None)unconditionally, sotest_differentiation_MPIwas running N redundantserial copies under
mpirun, paying the launch cost while testing nothing about thedistributed path.
Rectilinear.setupMPIis class-level state, so after the fieldsIO writes every later readreturned only the local block. The old model got the global read for free because the checks ran
in the parent process. Now reset explicitly, in a
finally.PRTE_MCA_rmaps_default_mapping_policy, which is the OpenMPI 5 spelling; that image shipsOpenMPI 4 and reads
OMPI_MCA_rmaps_base_oversubscribe. It only showed up now because the jobnever launched more than two ranks before.
The firedrake job has never measured coverage
Its coverage database carries ~365 file rows and one row of line data on master. The container
mounts the workspace at
/repositories, the job sat in./pySDC, and pySDC is installed editablefrom
/repositories/pySDC— the same directory under two path strings, so what its modules reportnever matched
source = ['pySDC']resolved against the working directory. Running from/repositories/pySDCmakes them agree:Now covering
firedrake_ensemble_communicator,pySDC_as_gusto_time_discretization,firedrake_mesh,GenericGusto,HeatFiredrake,TransferFiredrakeMeshand both step_7 tutorials.The tutorial
C_pySDC_with_PETSc.main()readsys.argvdirectly, so the only way to drive it was as a script.It now takes
num_procs_spaceandfname, with argv parsing moved into__main__— still runnableexactly as the tutorial text describes, and callable from the tests.
pytest-isolate-mpiremovedNothing uses it, so the dependency and the 0.3 pin are gone. The pin existed because 0.4 broke
module teardown (#691); 0.3 in turn refuses to load inside an
mpiexecsession, which madeincremental migration impossible without
-p no:pytest_isolate_mpi.Not ported, deliberately
test_crash.py— aborts a rank on purpose, which needs isolation.test_datatypes.py::test_PyTorch_dtype— runs in the pytorch job, which has no mpi-pytest; thatis why the file keeps its launcher and
__main__.Developer workflow
Unchanged: plain
pytest <file>andpytest <file>::<test>still run the MPI tests, viampi-pytest's forking mode. Slower, but you do not need to remember a rank count to run one test.
🤖 Generated with Claude Code