Run the tests across worker processes by default - #566
Conversation
…ick (#563) Two processes building the SAME mesh in one working directory raced. The generated file is named from the mesh PARAMETERS, so identical geometry is exactly the colliding case: one process opened `<name>.msh.h5` for reading while the other was still writing it, and PETSc raised error 76. A parameter sweep run as concurrent single-rank jobs hits this, and so does any parallel test run. The fix is atomicity. Both writes — gmsh's `.msh` and the `.msh.h5` PETSc converts it to — now land under a process-unique name and are renamed into place, so a reader sees a complete file or no file. `_scratch_name` keeps the extension, because gmsh chooses its output format from it. The rename makes an MPI barrier necessary before the read (the other ranks must not look before rank 0 has renamed) and sufficient, so that barrier is now explicit rather than implied by the write being slow. The directory is settable with `UW_MESH_CACHE_DIR`, replacing the `.meshes` string literal that was hardcoded at every site across the five meshing modules; tests/conftest.py gives each xdist worker its own. Measured: the atomicity is what fixes the race — with every worker forced to share one directory the run is equally green — so the per-worker directory is only there to stop four workers redoing the same gmsh work. Two generators were writing to the wrong place entirely: QuarterAnnulus and SegmentofAnnulus created `.meshes/` and then wrote their `.msh` into the working directory, because the prefix was missing from the name. That is where stray .msh files in run directories come from, and it made those two maximally exposed to the race. Separately, the quick tier was not quick. `./uw test` advertises ~2 minutes and took 9:45, because pytest MERGES marks: a module-level `pytestmark = pytest.mark.level_1` plus a per-test `@pytest.mark.level_2` leaves the test marked BOTH, so `-m level_1` selects it and the author's demotion does nothing. Nine files rely on that demotion, and the heaviest of their tests is a 96-second homotopy solve. A level now selects by excluding the levels above it, which needs no change to any test file. The recursion-prevention tests set an absolute `setrecursionlimit(50)`, which assumes the stack is nearly empty; under xdist the worker's own frames spend the budget before the test body starts. The limit is now measured from the current depth, so the tests assert what they mean. Measured, level_1 on a 16-core box: before 9:45 (xdist impossible: 6 failed, 3 errors) after, serial 7:26 all green after, -n 4 2:17 all green Level 2 also runs green under -n 4 (693 passed, 4:35). Fixes #563 Underworld development team with AI support from Claude Code
Now that the mesh-file race is fixed (#563), the suite can use the machine it is running on. Both runners distribute with --dist loadfile, which keeps every test in a file on one worker: tests within a file are written to follow each other, while global state (Model, units, PETSc contexts) does not cross between workers. That is the same reasoning the existing per-file isolation mode and the CI batching already rest on. Thread pools are pinned. Each worker is a full PETSc/BLAS process, and without OMP/OPENBLAS/MKL_NUM_THREADS=1 they each start their own pool and oversubscribe the machine — measured badly enough to run SLOWER than serial. Worker count defaults to min(cores, 8). Measured on 16 cores, level 1: serial 9:45 -n 4 2:17 -n 8 1:32 -n 16 1:31 and three point-locator tests FAIL Throughput saturates by 8, so the last doubling buys nothing, and it costs something: at one worker per core the file-to-worker grouping shifts and exposes test pollution. Deterministic across two runs and unaffected by UW_ENABLE_TELEMETRY, so it is state left by whatever shared the process, not numerics degrading under load. Filed separately rather than papered over. --isolation now means one worker rather than a separate mode, which is what it was always for: every run is per-file isolated already, and --isolation removes the concurrency too, for a test that passes alone and fails in a full run. --workers/-j overrides the default. MPI batches are untouched: they run their own mpirun invocation. End to end, `./uw test` goes from 9:45 to 1:22. Underworld development team with AI support from Claude Code
Adversarial review1. The CI number in the description is a PROJECTION, and we said so. 12-15 minutes on the 4-vCPU runner is extrapolated from a 16-core machine; the runner has different cores, different memory bandwidth, and a cold pixi cache. The first CI run on this branch measures it for real. We would rather publish the projection labelled as one than quietly let the local number stand in for it. 2. The 8-worker cap rests on a failure we did not root-cause. We chose 3. Thread pinning is the difference between a speedup and a regression. Worth calling out because it is invisible in the diff's intent: an earlier unpinned run of a comparable selection took 4h44m and 34 CPU-hours against 38 minutes serial. If anyone later "simplifies" the three exports away, the suite does not get slightly slower, it collapses. 4. What 5. Not verified. We ran level 1 (1175 tests) and level 2 (693) green under workers; we did NOT run level 3 that way, and the CI script's batches are exercised only by CI itself. If a level-3 physics suite carries cross-file coupling, this change will find it — on the first run rather than in review. Underworld development team with AI support from Claude Code |
There was a problem hiding this comment.
Pull request overview
This PR makes the default test execution mode run across multiple pytest-xdist worker processes (per-file isolation with --dist loadfile) while pinning thread pools to avoid oversubscription, and it hardens mesh-file writing so concurrent workers/runs don’t race on .msh / .h5 intermediates.
Changes:
- Enable multi-worker xdist (
--dist loadfile) by default inscripts/test.sh(CI) andscripts/test_levels.sh(dev), withOMP_NUM_THREADS=1(and BLAS equivalents) and a default worker cap of 8. - Route generated mesh outputs through a shared helper (
meshing/_mesh_files.py) and make gmsh + PETSc HDF5 writes atomic via write-to-scratch + rename. - Adjust recursion regression tests to set recursion limits relative to current stack depth; document correct marker selection semantics for test “levels”.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_0650_recursion_prevention_regression.py | Makes recursion-limit assertions robust under xdist by computing headroom from current stack depth. |
| tests/pytest.ini | Documents correct -m selection semantics given pytest’s mark-merging behavior. |
| tests/conftest.py | Sets per-xdist-worker UW_MESH_CACHE_DIR early to avoid redundant mesh builds and reduce cross-worker contention. |
| src/underworld3/meshing/_mesh_files.py | Introduces mesh output directory resolution and atomic gmsh write helper. |
| src/underworld3/meshing/cartesian.py | Uses mesh_file_dir() and atomic write_gmsh() for generated meshes; updates docstrings accordingly. |
| src/underworld3/meshing/annulus.py | Uses mesh_file_dir() and atomic write_gmsh(); fixes previously-missing .meshes/ prefix for some generators. |
| src/underworld3/meshing/spherical.py | Uses mesh_file_dir() and atomic write_gmsh() for generated meshes. |
| src/underworld3/meshing/segmented.py | Uses mesh_file_dir() and atomic write_gmsh() for generated meshes. |
| src/underworld3/meshing/geographic.py | Uses mesh_file_dir() and atomic write_gmsh() for generated meshes. |
| src/underworld3/discretisation/discretisation_mesh.py | Writes the PETSc-generated .h5 sidecar atomically and adds a barrier before non-root ranks read it. |
| scripts/test.sh | Runs serial test batches via xdist workers by default; pins thread pools; defaults workers to min(cores, 8). |
| scripts/test_levels.sh | Runs level-selected tests via xdist workers by default; adds --workers/-j; updates level marker expressions to exclude higher levels. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if [ -z "$WORKERS" ]; then | ||
| _cores=$( (command -v nproc >/dev/null && nproc) \ | ||
| || sysctl -n hw.ncpu 2>/dev/null || echo 2 ) | ||
| WORKERS=$(( _cores < 8 ? _cores : 8 )) | ||
| fi | ||
| echo "Serial batches: $WORKERS worker process(es)" | ||
| PYTEST="pytest --config-file=tests/pytest.ini --dist loadfile -n $WORKERS" |
| scratch = _scratch_name(filename) | ||
| gmsh.write(str(scratch)) | ||
| os.replace(scratch, filename) |
| if [ -z "$WORKERS" ]; then | ||
| _cores=$( (command -v nproc >/dev/null && nproc) \ | ||
| || sysctl -n hw.ncpu 2>/dev/null || echo 4 ) | ||
| WORKERS=$(( _cores < 8 ? _cores : 8 )) | ||
| fi | ||
|
|
||
| # --timeout=120: prevent tests from hanging indefinitely (2 min per test max) | ||
| # This prevents test pollution from global state (Model, units, PETSc) | ||
| # Show test configuration | ||
| echo "Configuration:" | ||
| if [ $RUN_ISOLATION -eq 1 ]; then | ||
| # One worker, still per-file: sequential AND isolated, for pinning down a | ||
| # pollution failure rather than for speed. | ||
| ISOLATION_OPTS="--dist loadfile -n 1" | ||
| echo " 🔒 Process isolation: ON" | ||
| echo " 🔒 Process isolation: ON (1 worker, one file at a time)" | ||
| else | ||
| ISOLATION_OPTS="" | ||
| echo " ⚡ Process isolation: OFF (fast mode)" | ||
| ISOLATION_OPTS="--dist loadfile -n $WORKERS" | ||
| echo " ⚡ Workers: $WORKERS (one file at a time per worker)" |
The first CI run of this change came back SLOWER — 52m37s against 49m43s — and the log says why: "Serial batches: 1 worker process(es)". Core detection returned 1 on the runner, so every batch paid for a worker spawn and a fresh underworld3 import and parallelised nothing. Fourteen batches of that is the three minutes. Two fixes, because either alone would leave the trap armed: - The workflow sets WORKERS=4 outright. GitHub's standard ubuntu-latest has 4 vCPU; guessing it from inside the job is what failed. - The script refuses to use xdist at one worker, falling back to the plain in-process run. A single worker is strictly worse than none. Detection is also simplified — plain `nproc`, then `sysctl`, then a default — and now reports the core count it found, so a wrong answer is visible in the log instead of silently costing three minutes. The over-clever `(command -v nproc && nproc)` form it replaces is what produced the 1. Underworld development team with AI support from Claude Code
The first CI run of this branch was SLOWER, and the projection in the description was wrongReporting this against my own change rather than letting the local numbers stand. 52m37s, against a 49m43s baseline. The log says why: Core detection returned 1 on the runner, so xdist ran with a single worker: a process spawn and a fresh The cause is the over-clever detection I wrote,
Detection is now plain The local numbers are unaffected — Underworld development team with AI support from Claude Code |
Status: the speedup is real, the job is RED, and it should stay redSecond CI run, with the worker count set explicitly:
So the parallelism does what it was supposed to — 1.6x on the real runner, not the 3-4x I projected from a 16-core box, and that projection remains the thing I got wrong. But the job fails, and the failure is not incidental to this PR. Three point-locator tests fail, and they are pointing at something real (#567): Every point, not a tolerance miss. Ruled out since the last comment: it is not the per-worker mesh directory (forcing a shared directory reproduces it exactly), not telemetry, not load (deterministic), and not any of the obvious neighbouring files run immediately before it. Why it appears now: on We are not skipping or xfailing these to get the PR green. A locator answering in the wrong cell for an entire query set is worth more than the merge is. This PR is blocked on #567, which now has a one-minute local reproduction. If the speedup is wanted before #567 is understood, the honest interim is to enable workers in Underworld development team with AI support from Claude Code |
Stacked on #565 — the mesh-file race had to die first, or this simply fails. Merge that one before this.
What changes
Both test runners (
scripts/test_levels.shfor developers,scripts/test.shfor CI) distribute across worker processes with--dist loadfile. That keeps every test in a file on one worker, which is the granularity this suite is safe at: tests within a file are written to follow each other, while global state (Model, units, PETSc contexts) does not cross between workers. It is the same reasoning the existing per-file isolation mode and the CI batching already rest on.Thread pools are pinned. Each worker is a full PETSc/BLAS process; without
OMP_NUM_THREADS=1(and the OpenBLAS/MKL equivalents) they each start their own pool and oversubscribe the machine. This is not a micro-optimisation — measured, an unpinned parallel run is slower than serial.MPI batches are untouched — they run their own
mpiruninvocation.Why 8 workers and not
-n autoMeasured on a 16-core box, level 1:
Throughput saturates by 8, so the last doubling buys nothing — and it costs something. At one worker per core, three point-locator tests fail. That is not numerics degrading under load: it reproduces identically across two runs and is unaffected by
UW_ENABLE_TELEMETRY. With--dist loadfile, changing the worker count changes which files share a process, so this is state left behind by whatever shared the worker. Filed as its own issue rather than papered over with a retry.Default is therefore
min(cores, 8), overridable with--workers N/-j N, orWORKERSin the environment for CI.--isolationnow means what it was forIt drops to one worker rather than selecting a different mode. Every run is per-file isolated already;
--isolationadditionally removes the concurrency, which is what you actually want when a test passes alone and fails in a full run.Result
./uw testend to end: 9:45 → 1:22, 1175 passed.On CI's 4-vCPU runner this should take the 44 minutes of pytest to roughly 12-15. I have not measured that — the runner is not this machine — so treat the CI number as a projection until the first run on this branch reports it. The projection is the point of the change, but it is a projection.
Underworld development team with AI support from Claude Code