From a7e1ee2043c05ac557d2e381eb5cf98f109c2c5e Mon Sep 17 00:00:00 2001 From: lmoresi Date: Wed, 28 Oct 2026 16:28:48 +1100 Subject: [PATCH 1/3] Shard the CI test batches across parallel jobs (#730) The test workflow was one job whose wall-clock tracked the whole suite. On 2026-10-28 that measured 91 minutes of tests across 16 serial batches plus the MPI phase, giving 96-minute jobs against a 120-minute cap, and #689 was cancelled at 2h00m52s with no failures recorded. The cap had started deciding outcomes rather than bounding runaway runs, which is the trigger #730 was filed against. The serial batches now run as a four-way shard matrix. Each batch is assigned to a shard by hand from the measured times, recorded in scripts/test.sh beside the batch definitions, because the batches differ by two orders of magnitude in cost and round-robin would group the three expensive ones together. The split is 33 / 19 / 18 / 22 minutes, so wall-clock becomes about 38 minutes including install and build. `--shard K/N` selects one; omitting it runs everything, which is what a local full run wants. Two parts had to move rather than be copied. The tier C report costs about 42 minutes and is now its own job: as a step on a matrix job it would run once per shard to produce one report. The MPI phase belongs to shard 4 alone, for the same reason and because it is the only part holding several ranks at once. An aggregate check named `test` reports the combined result, so one conclusion still answers whether the suite passed, and `development` carries no required status check that the rename could strand. Each shard rebuilds rather than fetching a shared artefact. The shards run concurrently, so the repeated build costs wall-clock once, and the repository is public, so the runner minutes are free. #730 named artefact sharing as the part needing design; measured against what it would save, it is not worth a transfer round trip and a second definition of "built". check_test_coverage.py reads the batch globs out of test.sh to prove every test file is reachable, and now recognises the sharded call site too. Left as it was, it reported all 355 files unreachable -- loud, which is the right direction, but it has to be kept in step. Verified by stubbing pytest and running each shard: 28 + 53 + 81 + 194 = 356 files, matching the unsharded run exactly, with no file in two shards and none in neither. Left open on #730: the test_106*/test_107* band is 33 minutes and sets the floor. It is a tail, not a volume -- 48% of its 149 tests finish in the first two minutes. Since the distribution is --dist loadfile a file is atomic on a worker, so the band cannot finish before its slowest file, and splitting it further wants the per-file numbers rather than a guess. Underworld development team with AI support from Claude Code https://claude.ai/code/session_01E87Q7KrpapxeQiLD1RiNXv --- .github/workflows/build_uw3_and_test.yaml | 103 +++++++++++++--- docs/developer/CHANGELOG.md | 46 +++++++ scripts/check_test_coverage.py | 20 ++- scripts/test.sh | 141 +++++++++++++++++++--- 4 files changed, 267 insertions(+), 43 deletions(-) diff --git a/.github/workflows/build_uw3_and_test.yaml b/.github/workflows/build_uw3_and_test.yaml index f9520f557..04287d278 100644 --- a/.github/workflows/build_uw3_and_test.yaml +++ b/.github/workflows/build_uw3_and_test.yaml @@ -7,6 +7,10 @@ name: test_uw3 # 60+ minutes on conda-forge solves and timing the runner out before tests # could start. The pixi.lock matches what local development uses, so CI now # runs against the same dependency state developers see. +# +# The serial batches run SHARDED across parallel jobs (#730). Wall-clock no +# longer tracks total test time: it tracks the slowest shard. See +# scripts/test.sh for the shard assignment and the measurements behind it. on: push: @@ -42,20 +46,35 @@ env: # their own mpirun. WORKERS: 4 + # How many shards the serial batches are split across. Changing this ONE + # number is not enough: scripts/test.sh assigns each batch to a shard by + # hand, from measurements, because the batches differ by two orders of + # magnitude in cost and round-robin would put the three expensive ones + # together. Change both, together, and say what you measured. + SHARDS: 4 + jobs: - test: + shards: + strategy: + # Every shard reports. Without this, the first shard to fail cancels the + # others and the log shows one failure where there may be four — which is + # the opposite of what a sharded run is for. + fail-fast: false + matrix: + # Must agree with SHARDS above. A matrix cannot read the env context, + # so this list is written out; scripts/test.sh refuses a shard number + # outside its own assignment, which is what catches the two drifting. + shard: [1, 2, 3, 4] + + name: shard ${{ matrix.shard }}/4 runs-on: ubuntu-latest - # Raised past 60 minutes on a 2026-08-15 measurement (serial phase ~55 min). - # xdist has since cut that: the five successful runs before 2026-09-12 spent - # 34, 35, 43, 44 and 48 minutes in "Run tests", well inside this budget. The - # headroom is why the previously-deferred bands could simply be switched on - # (#721 follow-up) rather than traded against something else. - # - # Sharding the batches across parallel jobs remains the durable fix if this - # tightens again; it was referenced here as "the CI-runtime issue" but never - # actually filed, so the numbers above are recorded in the issue that this - # change opens. - timeout-minutes: 120 + + # Per SHARD, not per suite. The unsharded job ran 96 minutes against a + # 120-minute cap and #689 was cancelled at 2h00m52s with zero failures, so + # the cap was deciding outcomes. The slowest shard is ~33 min of tests plus + # ~5 min of install and build; 60 leaves room for a slow runner while still + # being a real signal when a shard doubles. + timeout-minutes: 60 steps: - uses: actions/checkout@v4 @@ -75,6 +94,13 @@ jobs: # `pixi run -e dev build` invokes the build task from pixi.toml, # which is `pip install . --no-build-isolation` (non-editable — # editable installs are project policy violation, see CLAUDE.md). + # + # Each shard pays this (~2.5 min) rather than downloading a shared + # build artefact. That is deliberate: the shards run concurrently, so + # the repeated build costs wall-clock ONCE, and the repo is public, so + # the extra runner minutes are free. Sharing the artefact would add a + # pack/upload/download round trip and a second definition of what + # "built" means, to save nothing on the clock. run: pixi run -e dev build # Import smoke test: a hard crash during `import underworld3` (segfault @@ -89,13 +115,52 @@ jobs: run: pixi run -e dev python -u -X faulthandler -c "import faulthandler; faulthandler.enable(); import underworld3; print('import OK', underworld3.__version__)" - name: Run tests - run: pixi run -e dev ./scripts/test.sh --p 2 + run: pixi run -e dev ./scripts/test.sh --p 2 --shard ${{ matrix.shard }}/${{ env.SHARDS }} + + # One check that says whether the suite passed, so a reader (and anything + # querying statusCheckRollup for "test") gets a single answer rather than + # having to AND four shard results together. It is named `test` because that + # is what the check was called before sharding. + test: + name: test + needs: shards + if: always() + runs-on: ubuntu-latest + steps: + - name: Report the shard results + run: | + echo "shards: ${{ needs.shards.result }}" + if [ "${{ needs.shards.result }}" != "success" ]; then + echo "At least one shard did not pass — open the shard jobs above." + exit 1 + fi + + # The tier C characterisations are a REPORT, not a gate, so they run on a + # push to development rather than on every pull request. They include the + # slow level_3 cases and add ~42 min, which a PR should not pay to learn + # something that cannot block it. Failures here are expected to be read + # and answered, not reverted. + # + # Its own job, not a step on the sharded one: as a step it would run once per + # shard and cost its ~42 minutes four times over for one report. + tier-c: + name: Tier C characterisations (report only) + if: github.event_name == 'push' + runs-on: ubuntu-latest + timeout-minutes: 90 + + steps: + - uses: actions/checkout@v4 + + - name: Install pixi from lockfile + uses: prefix-dev/setup-pixi@v0.9.4 + with: + environments: dev + frozen: true + cache: true + + - name: Build UW3 + run: pixi run -e dev build - # The tier C characterisations are a REPORT, not a gate, so they run on a - # push to development rather than on every pull request. They include the - # slow level_3 cases and add ~42 min, which a PR should not pay to learn - # something that cannot block it. Failures here are expected to be read - # and answered, not reverted. - name: Tier C characterisations (report only) - if: github.event_name == 'push' run: pixi run -e dev ./scripts/test.sh --tier-c-report diff --git a/docs/developer/CHANGELOG.md b/docs/developer/CHANGELOG.md index 4aad91431..3ab3e6cba 100644 --- a/docs/developer/CHANGELOG.md +++ b/docs/developer/CHANGELOG.md @@ -4,6 +4,52 @@ This log tracks significant development work at a conceptual level, suitable for --- +## 2026 Q4 (October – December) + +### CI test batches sharded across parallel jobs (October 2026, #730) + +The test workflow ran as a single job whose wall-clock tracked the total cost +of the suite. Measured on 2026-10-28, that was 91 minutes of tests in 16 serial +batches plus the MPI phase, giving 96-minute jobs against a 120-minute cap, and +PR #689 was cancelled at 2h00m52s having recorded no failures. The cap was +deciding outcomes rather than bounding runaway runs. + +The serial batches now run as a four-way shard matrix. Each batch is assigned +to a shard by hand from the measured per-batch times, because the batches +differ by two orders of magnitude in cost and a round-robin assignment would +group the three expensive ones together. The assignment and its measurements +live in `scripts/test.sh` beside the batch definitions, and `--shard K/N` +selects one; omitting it runs everything, which is what a local full run wants. +The slowest shard carries about 33 minutes of tests, so the suite's wall-clock +becomes roughly 38 minutes including install and build. + +Two parts of the workflow needed moving rather than copying. The tier C +characterisation report, which costs about 42 minutes, became its own job: as a +step on a matrix job it would have run once per shard to produce one report. +The MPI phase belongs to a single shard for the same reason, and because it is +the only part of the suite holding several ranks at once. A new aggregate check +named `test` reports the combined result, so a single conclusion still answers +whether the suite passed. + +Each shard rebuilds Underworld3 rather than downloading a shared artefact. The +shards run concurrently, so the repeated build costs wall-clock once, and the +repository is public, so the extra runner minutes are free. Sharing the +artefact would add a transfer round trip and a second definition of what +"built" means. + +`scripts/check_test_coverage.py` reads the batch globs out of `scripts/test.sh` +to prove every test file is reachable, and it now recognises the sharded call +site as well. Without that it reported all 355 files as unreachable, which is +the right direction for the check to fail in. + +The remaining cost is concentrated rather than spread. The `test_106*` / +`test_107*` band takes 33 minutes, and 48% of its 149 tests finish in the first +two. Because the distribution is `--dist loadfile`, a file is atomic on a +worker and the band cannot finish before its slowest file does, so splitting it +further needs the per-file measurements and is left open on #730. + +--- + ## 2026 Q3 (July – September) ### Legacy Diffusion Restart Consistency (September 2026, #708) diff --git a/scripts/check_test_coverage.py b/scripts/check_test_coverage.py index 514f89f39..d7f47ca3b 100644 --- a/scripts/check_test_coverage.py +++ b/scripts/check_test_coverage.py @@ -29,14 +29,24 @@ def globs_run_by(script): - """Every tests/... pattern passed to $PYTEST on a line that is not commented.""" + """Every tests/... pattern the script hands to pytest, on an uncommented line. + + Three spellings reach pytest, and missing one of them makes this check + report the whole suite dark — loudly, which is the right direction, but it + still has to be kept in step with the script: + + * ``batch tests/...`` — the sharded call site (#730). The shard + number decides WHICH CI job runs it, not whether it runs, so coverage + does not care which one it is. + * ``"${PYTEST[@]}" tests/...`` — the runner is a bash array. + * ``$PYTEST tests/...`` — in case a plain one is left behind. + """ text = script.read_text().replace("\\\n", " ") patterns = set() for line in text.splitlines(): - # Both spellings: the runner is a bash ARRAY, so call sites read - # "${PYTEST[@]}", but a plain $PYTEST is still worth matching in case - # one is left behind or reintroduced. - if re.search(r"\$\{?PYTEST", line) and not line.lstrip().startswith("#"): + if line.lstrip().startswith("#"): + continue + if re.search(r"\$\{?PYTEST", line) or re.match(r"\s*batch\s+\d+\s", line): patterns.update(re.findall(r"tests/[A-Za-z0-9_\[\]*.\-]+", line)) return patterns diff --git a/scripts/test.sh b/scripts/test.sh index b6caaedda..fd135274c 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -6,6 +6,9 @@ # --p N Run parallel tests with N MPI ranks (default: skip parallel tests) # --full-parallel Add a second parallel pass at 4 ranks (see below; slow on CI) # --parallel-only Run ONLY parallel tests (skip all serial tests) +# --shard K/N Run only the batches assigned to shard K of N (see +# "Shard assignment" below). Omit it and everything runs, +# which is what a local full run wants. # --tier-c-report Run ONLY the tier C characterisations and report them. # Never gates: it always exits 0. CI asks for this on a # push to development, not on every pull request. @@ -15,6 +18,7 @@ # ./test.sh --p 2 # All serial + parallel (2 ranks) # ./test.sh --p 2 --full-parallel # ... and again at 4 ranks # ./test.sh --parallel-only --p 2 # Only parallel tests (debugging) +# ./test.sh --p 2 --shard 1/4 # One CI shard's worth # # We do not run one monolithic pytest because tests produce a large number of # PETSc objects which we cannot always guarantee that PETSc / petsc4py will free @@ -30,12 +34,24 @@ TIER_C_REPORT=0 # np=4 is oversubscribed and the pass costs far more than the ~3 minutes it # takes on a workstation — enough to exceed the 120-minute job cap (#573). FULL_PARALLEL=0 +# 0/0 means "no sharding": every batch runs. CI passes --shard K/N. +SHARD=0 +SHARD_COUNT=0 while [[ $# -gt 0 ]]; do case $1 in --p) PARALLEL_RANKS="$2" shift 2 ;; + --shard) + if [[ ! "$2" =~ ^[0-9]+/[0-9]+$ ]]; then + echo "Error: --shard wants K/N, got '$2'" + exit 1 + fi + SHARD="${2%/*}" + SHARD_COUNT="${2#*/}" + shift 2 + ;; --full-parallel) FULL_PARALLEL=1 shift @@ -50,7 +66,7 @@ while [[ $# -gt 0 ]]; do ;; *) echo "Unknown option: $1" - echo "Usage: $0 [--p N] [--full-parallel] [--parallel-only] [--tier-c-report]" + echo "Usage: $0 [--p N] [--full-parallel] [--parallel-only] [--shard K/N] [--tier-c-report]" exit 1 ;; esac @@ -63,6 +79,78 @@ if [ $PARALLEL_ONLY -eq 1 ] && [ $PARALLEL_RANKS -eq 0 ]; then exit 1 fi +# --------------------------------------------------------------------------- +# Shard assignment +# +# The batches below differ by two orders of magnitude in cost, so they are +# assigned to shards BY HAND from measurements. Round-robin would be worse than +# useless here: it would put test_106*/test_107* (33 min), test_105* (12 min) +# and test_110* (10 min) in whatever order the list happens to be in. +# +# Measured on run 40623403571 (development, 2026-10-28, SUCCESS, 4 xdist +# workers), per serial batch: +# +# test_106*,test_107* 33:00 <- sets the floor; see the note below +# test_105* 11:42 +# test_110* 10:23 +# test_08* 7:14 +# test_101*,test_102* 7:05 +# test_00[0-4]* 5:14 +# test_05*,test_06*,test_07* 3:44 +# test_005[1-9],test_00[6-7]* 3:29 +# test_02*,test_03* 1:13 +# test_01* 0:56 +# test_100[0-9]*,test_103* 0:54 +# docstring/jit_cache/... 0:48 +# test_1110*,test_1120* 0:25 +# parallel/test_1069 0:14 +# test_0050* 0:08 +# test_1450* 0:07 +# the --p N mpirun phase 4:38 +# ---------------------------------- +# total 91:19 (job wall-clock was 96 min with build) +# +# Giving: 33 / 19 / 18 / 22 minutes across the four shards, so the suite's +# wall-clock becomes ~33 min of tests plus ~5 min of install and build. +# +# The floor is test_106*/test_107*, and it is a TAIL, not a volume: 48% of its +# 149 tests finish in the first 2 minutes, then the remainder takes 31. Since +# the distribution is --dist loadfile, a file is atomic on a worker, so the +# batch cannot finish before its slowest FILE does. Splitting that band further +# is the next step and wants the per-file numbers, not a guess. +# +# Keep the count in agreement with SHARDS in build_uw3_and_test.yaml. +if [ "$SHARD_COUNT" -gt 0 ]; then + if [ "$SHARD" -lt 1 ] || [ "$SHARD" -gt "$SHARD_COUNT" ]; then + echo "Error: shard $SHARD is outside 1..$SHARD_COUNT" + exit 1 + fi + if [ "$SHARD_COUNT" -ne 4 ]; then + echo "Error: the batches below are assigned to 4 shards, not $SHARD_COUNT." + echo " Re-measure and re-assign them before changing the count." + exit 1 + fi + echo "Shard $SHARD of $SHARD_COUNT" +fi + +# batch — run a batch if this shard owns it. +# +# Call sites pass globs unquoted, so bash expands them before we see them and +# "$@" carries the file list intact. +batch() { + local owner=$1 + shift + if [ "$SHARD_COUNT" -gt 0 ] && [ "$owner" -ne "$SHARD" ]; then + return 0 + fi + "${PYTEST[@]}" "$@" || status=1 +} + +# shard_owns — for the phases that are not a single pytest call. +shard_owns() { + [ "$SHARD_COUNT" -eq 0 ] || [ "$1" -eq "$SHARD" ] +} + export UW_NO_USAGE_METRICS=0 # A hard crash must print a Python stack, not just "Segmentation fault". export PYTHONFAULTHANDLER=1 @@ -100,6 +188,10 @@ PYTEST=(pytest --config-file=tests/pytest.ini) # the job from ~45 to ~100 minutes against a 120-minute cap. Dropping the slow # ones would re-hide test_1064, whose two-month absence concealed #734, so the # COST moves off the pull-request path rather than the COVERAGE being cut. +# +# Since #730 it also runs in its own CI job rather than as a step on the test +# job: the test job is now a shard matrix, and a step on it would pay those +# ~42 minutes once per shard to produce one report. if [ $TIER_C_REPORT -eq 1 ]; then echo "==========================================" echo "Tier C characterisations (reported, not gating)" @@ -148,37 +240,37 @@ if [ $PARALLEL_ONLY -eq 0 ]; then python3 "$(dirname "$0")/check_test_coverage.py" || status=1 # Run simple tests (0000-0299: basic functionality, imports, simple operations) - "${PYTEST[@]}" tests/test_00[0-4]*py || status=1 - "${PYTEST[@]}" tests/test_0050*py || status=1 + batch 4 tests/test_00[0-4]*py + batch 4 tests/test_0050*py # test_006[2-9] and test_007x matched NO batch glob and so never ran in CI: # the whole integration-point suite (0064-0067), swarm repopulation, # mid-time velocity and the VE stress history. They are not covered by the # disabled test_06*py line below either - that one is 0600-0699. The band is # taken whole (00[6-7]) rather than enumerated, so a test added next to its # siblings is not dark again. Verified passing (103 tests) before wiring in. - "${PYTEST[@]}" tests/test_005[1-9]*py tests/test_00[6-7]*py || status=1 - "${PYTEST[@]}" tests/test_01*py || status=1 - "${PYTEST[@]}" tests/test_02*py tests/test_03*py || status=1 + batch 4 tests/test_005[1-9]*py tests/test_00[6-7]*py + batch 4 tests/test_01*py + batch 4 tests/test_02*py tests/test_03*py # Intermediate tests (0500-0799: data structures, transformations, enhanced interfaces) # test_06*py was disabled as "potentially problematic" and stayed that way. The # whole band was measured 2026-09-12 and passes; a band is not a unit of trust, # and holding one back for a suspicion nobody recorded meant its level_1 files # never ran at all. - "${PYTEST[@]}" tests/test_05*py tests/test_06*py tests/test_07*py || status=1 + batch 4 tests/test_05*py tests/test_06*py tests/test_07*py # Units system tests (0800-0899: unit-aware functions, arrays, and conversions) - "${PYTEST[@]}" tests/test_08*py || status=1 + batch 3 tests/test_08*py # Poisson solvers (including Darcy flow) - "${PYTEST[@]}" tests/test_100[0-9]*py tests/test_103*py || status=1 + batch 4 tests/test_100[0-9]*py tests/test_103*py # Solver / system tests (advanced solver problems) # test_101* / test_102* include the rotated free-slip suite (test_1018, # issue #504) and the MG / boundary-flux suites, which previously matched # no batch glob and never ran in CI. - "${PYTEST[@]}" tests/test_101*py tests/test_102*py || status=1 - "${PYTEST[@]}" tests/test_105*py || status=1 + batch 2 tests/test_101*py tests/test_102*py + batch 2 tests/test_105*py # The boundary-normal guard lives under tests/parallel/ but carries NO # mpi(min_size=2) mark, because the defect it guards is present in SERIAL in @@ -186,13 +278,13 @@ if [ $PARALLEL_ONLY -eq 0 ]; then # spherical shell at np=1). Run it here so the serial job covers that path — # every other serial test of the default boundary normal is on a box, where # flat walls make the question vacuous. It also runs in the --p N batch below. - "${PYTEST[@]}" tests/parallel/test_1069_boundary_normal_parallel.py || status=1 + batch 4 tests/parallel/test_1069_boundary_normal_parallel.py # test_106*/test_107* were held back pending "a triage/deselect decision". That # decision is made: the band is not the unit of exclusion. Of the 243 tests in # the deferred bands only 26 are level_3 or tier_c; 55 are level_1 AND tier_a — # hardened, fast, and exactly what CI exists to run. test_1064 is the slow one # and it is excluded by WHAT IT IS (tier_c) below, not by its number. - "${PYTEST[@]}" tests/test_106*py tests/test_107*py || status=1 + batch 1 tests/test_106*py tests/test_107*py # # test_1072 used to be pulled forward out of test_107* by name, because that # band was not batched and it is the ONLY guard on the 3-D free-surface sign @@ -201,23 +293,28 @@ if [ $PARALLEL_ONLY -eq 0 ]; then # running it twice. # Diffusion / Advection tests (test_110*: SUPG/SLCN/swarm adv-diff benchmarks) - "${PYTEST[@]}" tests/test_110*py || status=1 - "${PYTEST[@]}" tests/test_1110*py tests/test_1120*py || status=1 # Annulus + vector SL - "${PYTEST[@]}" tests/test_1450*py || status=1 + batch 3 tests/test_110*py + batch 4 tests/test_1110*py tests/test_1120*py # Annulus + vector SL + batch 4 tests/test_1450*py # Named (un-numbered) test files - JIT, docstrings, projections - "${PYTEST[@]}" tests/test_docstring_utils.py tests/test_jit_cache.py \ + batch 4 tests/test_docstring_utils.py tests/test_jit_cache.py \ tests/test_jit_deterministic_ordering.py \ tests/test_multicomponent_projection.py \ tests/test_snes_vector_asymmetric_jacobian.py \ - tests/test_symbol_disambiguation_prototype.py || status=1 + tests/test_symbol_disambiguation_prototype.py else echo "Skipping serial tests (--parallel-only specified)" echo "" fi # Parallel tests (run if --p N specified) -if [ $PARALLEL_RANKS -gt 0 ]; then +# +# The whole mpirun phase belongs to ONE shard (4), not to each of them. It +# costs ~4:38 and is the shard's smallest component, but running it four times +# would quadruple the only part of the suite that holds several MPI ranks at +# once, which is also the part most likely to contend on a shared runner. +if [ $PARALLEL_RANKS -gt 0 ] && shard_owns 4; then if command -v mpirun &> /dev/null; then echo "" echo "==========================================" @@ -297,6 +394,12 @@ if [ $PARALLEL_RANKS -gt 0 ]; then echo "⚠️ Warning: --p $PARALLEL_RANKS specified but mpirun not available" echo "⚠️ Skipping parallel tests" fi +elif [ $PARALLEL_RANKS -gt 0 ]; then + # --p N was given, but this shard does not own the mpirun phase. Say which + # one does, so a reader of one shard's log does not conclude the parallel + # tests were skipped for the whole run. + echo "" + echo "Parallel tests belong to shard 4; this is shard $SHARD." else echo "" echo "⚠️ Skipping parallel tests (use --p N to enable)" From d0b9056dcd8e7810367d14195a5b2078e8e1a488 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Wed, 28 Oct 2026 16:31:44 +1100 Subject: [PATCH 2/3] Review of the sharding: the shard count gets one definition The matrix listed [1, 2, 3, 4] and an env var said SHARDS: 4, while scripts/test.sh assigned batches to shards 1-4 independently. Three copies of one number, and the failure mode is not a nuisance: a matrix of [1, 2, 3] against an assignment that uses 4 runs GREEN while shard 4's batches -- 194 files, the largest group -- never run. Nothing in the logs says so. That is #721 exactly, files going dark behind a pattern that stopped covering them. So the number is derived rather than repeated. `--print-shards` reads the largest shard in the `batch` call sites out of the script itself and prints a JSON array; a `plan` job emits that, the matrix is built from it with fromJSON, and the shard argument uses strategy.job-total. Adding a `batch 5 ...` line now grows CI a fifth shard on the next run with nothing else to edit. Where a count still has to be compared, it is checked rather than assumed: --shard K/N refuses N that disagrees with the assignment, and refuses K outside 1..N, instead of running a partial suite quietly. Verified against the stub again after the refactor: 28 + 53 + 81 + 194 = 356 files, equal to the unsharded run, no file in two shards and none in neither. Also checked that the guards exit 1 rather than warning (mismatch and out-of-range both do), and that a failing batch fails its own shard and only its own shard. Underworld development team with AI support from Claude Code https://claude.ai/code/session_01E87Q7KrpapxeQiLD1RiNXv --- .github/workflows/build_uw3_and_test.yaml | 43 +++++++++++++++------- scripts/test.sh | 44 ++++++++++++++++++++--- 2 files changed, 70 insertions(+), 17 deletions(-) diff --git a/.github/workflows/build_uw3_and_test.yaml b/.github/workflows/build_uw3_and_test.yaml index 04287d278..fecdfc275 100644 --- a/.github/workflows/build_uw3_and_test.yaml +++ b/.github/workflows/build_uw3_and_test.yaml @@ -46,27 +46,43 @@ env: # their own mpirun. WORKERS: 4 - # How many shards the serial batches are split across. Changing this ONE - # number is not enough: scripts/test.sh assigns each batch to a shard by - # hand, from measurements, because the batches differ by two orders of - # magnitude in cost and round-robin would put the three expensive ones - # together. Change both, together, and say what you measured. - SHARDS: 4 - jobs: + # How many shards to run is NOT stated here. scripts/test.sh assigns each + # batch to a shard by hand, from measurements, and --print-shards reads the + # count back out of those assignments. The matrix is built from that, so the + # number has exactly one definition. + # + # A second copy would be a silent-coverage bug, not a nuisance: a matrix of + # [1,2,3] against an assignment using 4 runs green while shard 4's batches — + # the largest group, 194 files — never run. That is #721, files going dark + # with nothing to say so. + plan: + runs-on: ubuntu-latest + timeout-minutes: 5 + outputs: + shards: ${{ steps.shards.outputs.list }} + steps: + - uses: actions/checkout@v4 + - name: Read the shard count out of the batch assignment + id: shards + run: | + list=$(./scripts/test.sh --print-shards) + echo "shards: $list" + echo "list=$list" >> "$GITHUB_OUTPUT" + shards: + needs: plan strategy: # Every shard reports. Without this, the first shard to fail cancels the # others and the log shows one failure where there may be four — which is # the opposite of what a sharded run is for. fail-fast: false matrix: - # Must agree with SHARDS above. A matrix cannot read the env context, - # so this list is written out; scripts/test.sh refuses a shard number - # outside its own assignment, which is what catches the two drifting. - shard: [1, 2, 3, 4] + shard: ${{ fromJSON(needs.plan.outputs.shards) }} - name: shard ${{ matrix.shard }}/4 + # strategy.job-total is the matrix length, so the shard count is never + # written down a second time here either. + name: shard ${{ matrix.shard }}/${{ strategy.job-total }} runs-on: ubuntu-latest # Per SHARD, not per suite. The unsharded job ran 96 minutes against a @@ -115,7 +131,7 @@ jobs: run: pixi run -e dev python -u -X faulthandler -c "import faulthandler; faulthandler.enable(); import underworld3; print('import OK', underworld3.__version__)" - name: Run tests - run: pixi run -e dev ./scripts/test.sh --p 2 --shard ${{ matrix.shard }}/${{ env.SHARDS }} + run: pixi run -e dev ./scripts/test.sh --p 2 --shard ${{ matrix.shard }}/${{ strategy.job-total }} # One check that says whether the suite passed, so a reader (and anything # querying statusCheckRollup for "test") gets a single answer rather than @@ -126,6 +142,7 @@ jobs: needs: shards if: always() runs-on: ubuntu-latest + timeout-minutes: 5 steps: - name: Report the shard results run: | diff --git a/scripts/test.sh b/scripts/test.sh index fd135274c..6abc2117e 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -37,6 +37,7 @@ FULL_PARALLEL=0 # 0/0 means "no sharding": every batch runs. CI passes --shard K/N. SHARD=0 SHARD_COUNT=0 +PRINT_SHARDS=0 while [[ $# -gt 0 ]]; do case $1 in --p) @@ -52,6 +53,16 @@ while [[ $# -gt 0 ]]; do SHARD_COUNT="${2#*/}" shift 2 ;; + --print-shards) + # The shard count, read out of the batch assignment below, as a JSON + # array. The CI matrix is built from THIS rather than from a second + # copy of the number: a matrix of [1,2,3] against an assignment that + # uses 4 would run green while shard 4's batches -- the largest group + # -- never ran at all. That is the #721 failure, files going dark + # with nothing to say so, and the only safe fix is one definition. + PRINT_SHARDS=1 + shift + ;; --full-parallel) FULL_PARALLEL=1 shift @@ -119,15 +130,40 @@ fi # batch cannot finish before its slowest FILE does. Splitting that band further # is the next step and wants the per-file numbers, not a guess. # -# Keep the count in agreement with SHARDS in build_uw3_and_test.yaml. +# How many shards the assignment below actually uses, read from the `batch` +# call sites in this file. Nothing else states the number, so nothing else can +# disagree with it: add a `batch 5 ...` line and CI grows a fifth shard on the +# next run. The CI matrix comes from --print-shards. +SHARDS_ASSIGNED=$( + grep -oE '^[[:space:]]*batch[[:space:]]+[0-9]+' "$0" \ + | grep -oE '[0-9]+' | sort -n | tail -1 +) +if [ -z "$SHARDS_ASSIGNED" ]; then + echo "Error: no 'batch ...' call sites found in $0." + echo " Either the assignment was removed or the grep no longer matches it." + exit 1 +fi + +if [ "$PRINT_SHARDS" -eq 1 ]; then + # A JSON array for the CI matrix: 4 -> [1,2,3,4] + printf '[' + for ((i = 1; i <= SHARDS_ASSIGNED; i++)); do + [ "$i" -gt 1 ] && printf ',' + printf '%d' "$i" + done + printf ']\n' + exit 0 +fi + if [ "$SHARD_COUNT" -gt 0 ]; then if [ "$SHARD" -lt 1 ] || [ "$SHARD" -gt "$SHARD_COUNT" ]; then echo "Error: shard $SHARD is outside 1..$SHARD_COUNT" exit 1 fi - if [ "$SHARD_COUNT" -ne 4 ]; then - echo "Error: the batches below are assigned to 4 shards, not $SHARD_COUNT." - echo " Re-measure and re-assign them before changing the count." + if [ "$SHARD_COUNT" -ne "$SHARDS_ASSIGNED" ]; then + echo "Error: asked for shard $SHARD of $SHARD_COUNT, but the batches below" + echo " are assigned to $SHARDS_ASSIGNED shards. Running anyway would" + echo " leave whole batches unrun with nothing to say so." exit 1 fi echo "Shard $SHARD of $SHARD_COUNT" From 11cd3e8798b6ba96016ec250016f53b2fc7b1845 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Wed, 28 Oct 2026 17:02:19 +1100 Subject: [PATCH 3/3] Replace the sharding estimate with what the first run measured The commit that landed the matrix predicted 33 / 19 / 18 / 22 minutes from the per-batch times and a ~38 minute wall-clock. Run 40638600008 measured 23.6 / 28.3 / 17.3 / 22.6 including install and build, for 28.7 against 96 before. Better than claimed, and ranked differently: shard 1 carries the 33-minute test_106*/test_107* band and finished in 23.6, while shard 2 was estimated at 19 minutes of tests and is the long pole at 28.3. A batch costs less in a job that runs only its own batches than it did in sequence, by more than the assignment assumed, so the per-batch times are not a linear predictor of a shard. The numbers are recorded rather than acted on. Re-assigning off one sample would be fitting runner noise: the imbalance left (28.3 against 17.3) is of the same order as the variance on a shared runner, so it wants several runs first. Underworld development team with AI support from Claude Code https://claude.ai/code/session_01E87Q7KrpapxeQiLD1RiNXv --- docs/developer/CHANGELOG.md | 12 ++++++++++-- scripts/test.sh | 9 +++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/docs/developer/CHANGELOG.md b/docs/developer/CHANGELOG.md index 3ab3e6cba..c9b5fe9d9 100644 --- a/docs/developer/CHANGELOG.md +++ b/docs/developer/CHANGELOG.md @@ -20,8 +20,16 @@ differ by two orders of magnitude in cost and a round-robin assignment would group the three expensive ones together. The assignment and its measurements live in `scripts/test.sh` beside the batch definitions, and `--shard K/N` selects one; omitting it runs everything, which is what a local full run wants. -The slowest shard carries about 33 minutes of tests, so the suite's wall-clock -becomes roughly 38 minutes including install and build. + +Measured on the first sharded run (40638600008): shards of 23.6, 28.3, 17.3 and +22.6 minutes including install and build, for a wall-clock of 28.7 minutes +against 96 before. The estimates from the per-batch times did not transfer +linearly -- shard 1 carried the 33-minute band and finished in 23.6, while +shard 2 was estimated at 19 minutes of tests and became the long pole at 28.3. +A batch costs less in a job that runs only its own batches than it did in +sequence, by more than the assignment assumed. The split is worth revisiting, +but on more than one sample: runner variance on a shared runner is of the same +order as the imbalance that remains. Two parts of the workflow needed moving rather than copying. The tier C characterisation report, which costs about 42 minutes, became its own job: as a diff --git a/scripts/test.sh b/scripts/test.sh index 6abc2117e..c70b567bb 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -121,8 +121,13 @@ fi # ---------------------------------- # total 91:19 (job wall-clock was 96 min with build) # -# Giving: 33 / 19 / 18 / 22 minutes across the four shards, so the suite's -# wall-clock becomes ~33 min of tests plus ~5 min of install and build. +# Giving an ESTIMATED 33 / 19 / 18 / 22 minutes across the four shards. +# MEASURED on the first sharded run (40638600008), including install and build: +# 23.6 / 28.3 / 17.3 / 22.6, wall-clock 28.7 min against 96 before. The +# estimates are conservative and do not rank the same way -- a batch costs less +# in a job running only its own batches. Shard 2 is the long pole, not shard 1. +# Re-measure over several runs before re-assigning: runner variance here is of +# the same order as the imbalance left. # # The floor is test_106*/test_107*, and it is a TAIL, not a volume: 48% of its # 149 tests finish in the first 2 minutes, then the remainder takes 31. Since