diff --git a/.github/workflows/build_uw3_and_test.yaml b/.github/workflows/build_uw3_and_test.yaml index f9520f557..fecdfc275 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: @@ -43,19 +47,50 @@ env: WORKERS: 4 jobs: - test: + # 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: + shard: ${{ fromJSON(needs.plan.outputs.shards) }} + + # 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 - # 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 +110,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 +131,53 @@ 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 }}/${{ 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 + # 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 + timeout-minutes: 5 + 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..c9b5fe9d9 100644 --- a/docs/developer/CHANGELOG.md +++ b/docs/developer/CHANGELOG.md @@ -4,6 +4,60 @@ 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. + +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 +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..c70b567bb 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,35 @@ 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 +PRINT_SHARDS=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 + ;; + --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 @@ -50,7 +77,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 +90,108 @@ 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 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 +# 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. +# +# 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 "$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" +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 +229,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 +281,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 +319,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 +334,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 +435,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)"