Repository navigation
Deal the slowest tests to CI shards by duration, not hash - #2637
Merged
Merged
Conversation
Over 95 runs on 24 October, shard 2/5 averaged 57 minutes (up to 70) against 49 for shard 1/5. Shards are chosen by a hash of each node id, which spreads short tests evenly but not long ones: a 22-minute property test and several of 4 to 7 minutes hashed to shard 2. shard_durations.toml lists the 295 tests that took at least a minute, measured from the logs of eight merge queue runs by .github/test_durations.py. conftest.py deals them out longest first, each to the least loaded shard; every other test keeps its hash. On the measured durations the shards come out at 53 to 55 minutes instead of 48 to 59. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Aggregate impact on certified data: passBase Checks: 0 broken by this change, 1 failing on both sides, 0 fixed. Moves flagged for review: 0 (thresholds in Notes
Moves flagged for reviewNone. Spending by benefit and service
Caseloads
Standard reform costings (change in gov_balance)
Standard reforms: change in each reform's target
Household net income by decileNo household's net income changes in any year. No change in any year: receipts by tax head; government balance, each component counted once; model totals; household income totals; population; poverty rates; people in poverty; inequality; mean household net income in each side's own deciles. Checks failing on both base and head (1)
Counted-once checkEach of 91 components was raised by £1 per unit in 2026-27, and every head of its totals was read household by household: its own head must rise by exactly £1, no other tax head may move, no other benefit may rise, and the model's own total must move by what its heads moved. A head that breaks a rule is measured again twice: with every other head held at its baseline value, and with every other head held at its value after the first £1 while the component rises again. It fails where either still shows the response. 0 failed; 0 were skipped.
Provenance
Dataset: Receipts by tax head: head levels
Spending by benefit and service: head levels
Caseloads: head levels
Government balance, each component counted once: head levels
Model totals: head levels
Household income totals: head levels
Population: head levels
Poverty rates: head levels
People in poverty: head levels
Inequality: head levels
Mean household net income in each side's own deciles: head levels
Standard reform costings (change in gov_balance): head levels
Standard reforms: change in each reform's target: head levels
|
The first run with the 60-second table (run 40095846881) took 53, 46, 55, 60 and 55 minutes: shard 2/5 dropped from the slowest to the fastest, but shard 4/5 carried 235 worker-minutes against 180 to 209 for the others. The 295 listed tests were only 59% of the suite's time, and the hash left the remainder uneven. The 567 tests of 20 seconds or more are 75% of it; on the measured durations of nine runs the shards then come out at 54 minutes each, give or take half a minute. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vahid-ahmadi
marked this pull request as ready for review
October 24, 2026 21:17
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#2040 (9d3a9be) replaced test_frs_shaped_units_keep_the_under_20_presumption, so after merging main the existence check in test_ci_test_shards.py failed on its table line. The test it became is placed by the hash until the table is regenerated. The table's header said a stale entry was harmless; it now says the check fails on one and what a renaming PR should do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Merged main (49a7590) into this branch and pushed, head now aa09c20. After the merge, The table header said a stale entry was harmless, which the existence check contradicts. It now says the check fails on a stale entry, and that a PR renaming or removing a listed test should edit its line here. That is a small cost this table puts on other PRs; the alternative is to drop the existence check and accept silent drift. Ran locally on the merged tree: Still needs a reviewer: none is assigned or requested. 🤖 Generated with Claude Code |
juaristi22
approved these changes
Oct 26, 2026
juaristi22
left a comment
juaristi22
left a comment
Collaborator
There was a problem hiding this comment.
Program review
Base repository: PolicyEngine/policyengine-uk
PR number: 2637
Reviewed head SHA: aa09c20
Merge base SHA: 49a7590
Mode: full
Scope: All six changed files: duration-based CI sharding, advisory table, offline generator, tests and changelog; affected build, workflow and coverage-checker contracts.
Source manifest: /private/tmp/policyengine-command-runs/d69b42c555e3/pr2637-external-review-20261026-review-sources.json
Review status: COMPLETE
Source Documents
No source documents registered; see scope and validation.
Critical
None.
Should Address
None.
Suggestions
S1 — source-1 — Match the PR prose count to the checked-in table (OPEN)
Location: captured PR body Summary, versus policyengine_uk/tests/shard_durations.toml's [seconds] mapping. The summary says567 listed tests; independently parsing the exact head gives566 unique entries. All566 have positive finite weights and refer to existing files. This is a minor descriptive count mismatch; it does not drop a collected test or undermine assignment. Update the prose count when editing the description. Evidence: pr2637-external-review-20261026-policy-static-proof.json.
Evidence Gaps
No material gaps reported.
Notes
- No blocking defects or material evidence gaps found. S1 is an optional correction to the PR description: the table contains 566 entries, while the description says 567. Routing and coverage are unaffected.
- Duration weights affect routing only. Stable hash fallback, test collection and the final exactly-once coverage audit remain intact. Source and wheel packaging were checked.
- The generator estimates durations from gaps in offline worker logs. Historical performance figures remain author-reported; this review does not independently measure CI speed or guarantee total wall time.
- The captured default branch is 16 commits ahead. Being behind main is informational. Both reviewers examined the exact aa09c20 snapshot and full merge-base diff.
- The disposable snapshot was removed normally after both reviewers finished. Four independent identity/status checks passed. No new external policy sources were needed.
- Timings cover capture through consolidation and normal cleanup. Role durations overlap and are not additive.
Validation Summary
Local tests and collection were NOT RUN; no country/Core imports, environment changes, private CI logs or artifacts were used. Matching-head CI for aa09c20 records 34 successful checks and one skipped Publish step. Source and release YAML tests, all ten Python shards, Test/Result and Lint succeeded. Publish is skipped because PR builds explicitly set publish=false. Static TOML/AST inspection checked all 566 table entries and 160 referenced files, with no invalid references. These source checks and matching-head CI support the review; no additional local execution gate is required.
Timing
elapsed seconds: 1130.92s; code role seconds: 275.21s; policy role seconds: 350.01s
Review Severity
APPROVE. Open findings: 0 critical, 0 should address, 1 suggestions.
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.
Summary
Shard 2/5 of "Test / Python" is the slowest shard in most runs. Shards are chosen by a hash of each test's node id (
--shardinconftest.py), not by file, so a file's tests are already spread over all five. A hash spreads thousands of short tests evenly but not a few long ones, and by chance the longest test in the suite (test_capital_share_components.py::test_capital_matches_the_original_lists, about 22 minutes on one worker) and several of 4 to 7 minutes hashed to shard 2.This PR deals the slowest tests out by duration instead:
policyengine_uk/tests/shard_durations.tomllists the 567 tests that took at least 20 seconds on CI (about three quarters of the suite's time), with their mean duration over nine runs.conftest.pydeals those tests out longest first, each to the least loaded shard (deterministic, ties by node id then shard). Every other test keeps its hash. A test missing from the table, or the table missing, falls back to the hash, so a stale table only costs balance, never coverage..github/test_durations.py(standard library) regenerates the table from saved shard logs: it times each test as the gap between consecutive results from the same pytest-xdist worker.test_ci_test_shards.pygains invariant 12: the dealing is order-independent and no shard's listed load exceeds another's by more than the longest test, shards still partition any collection that includes listed tests,shard_offollows the table, and every table entry names a test function that exists.The 60-minute limit itself is already 75 on main (#2626). Runs cancelled at 60 minutes after that, such as run 40050823323, came from branches cut before it. This PR adds headroom under that limit; it does not change it.
Evidence (before)
All 95 Code changes runs since 21 October with all five shards green (merge queue and pull requests), minutes in "Run the tests":
Shard 2/5 was the slowest in 54 of the 95. Summing the per-test durations from eight of those runs by shard reproduces this (48, 59, 55, 56, 53 minutes at four workers). With this table the same durations come out at about 54 minutes each.
Locally,
--collect-only --shard K/5for K = 1 to 5 partitions all 9,140 collected tests (1,782 / 1,873 / 1,828 / 1,829 / 1,828).After
Job durations from this PR's own runs, minutes:
Shard 2/5 is no longer the slowest. With the 60-second table shard 4/5 carried 235 worker-minutes against 180 to 209, because the hashed remainder (41% of the time) was uneven; the 20-second table fixes that. The slowest shard in each head run is 56 to 58 minutes against a 59-minute mean of the slowest shard over the 95 runs before. The remaining spread between shards is run-to-run noise in individual tests (one took 11 minutes in one run and 7.7 on average), which no static table removes. One run each is not much evidence: the gain is mainly that no shard is now systematically slow.
Notes
test_capital_matches_the_original_listsalone takes about 22 minutes (max_examples=25, up to 30 households and a fullSimulationper example). It sets a floor for whichever shard holds it.Tests
pytest policyengine_uk/tests/code_health/test_ci_test_shards.py: 51 passed.ruff checkandruff format --checkon the changed Python.Reproducer: n/a: CI test sharding only; model source and dependencies are unchanged
axiom: n/a: CI change, no policy content
🤖 Generated with Claude Code