Repository navigation
Conversation
The module fixture in test_aggregate_impact_certified.py builds its own simulations, and each shard that holds one of the file's tests pays for them. The duration table deals the file's long entries apart, so main spreads its nine tests over five shards. Deal and hash the file as one unit, so one shard pays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
juaristi22
left a comment
There was a problem hiding this comment.
Review at 94a43ce01ed17357b179fc657e1966fe0e853a98: COMPLETE, no findings or material evidence gaps.
The placement-unit change and its collection/deal/hash integration preserve the tested partition, shared-file co-location and load-bound contracts. All 55 focused sharding/CI-health tests passed in one campaign. No model calculations, certified aggregate fixtures or private data were run.
This validation covers shard placement and the associated collection behavior. Historical memory figures and expected memory savings remain attributed to the author's CI evidence; they were not independently measured here.
The PR remains Draft with CI pending, so this is a completed review comment, not an approval.
The certified aggregate tests on one shard of seven still lost that shard's runner at 15.6 GB on a large merge, and the latest-data smoke tests lost another. The tests that carry the microsimulation marker now run in an eighth shard with one worker, and the other seven run none of them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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; standard reform costings (change in gov_balance); standard reforms: change in each reform's target. Checks failing on both base and head (1)
Counted-once checkThe check raised each of 92 components by £1 per unit in 2026-27 and read every head of its totals 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 response that is not finite breaks every rule. The check measures a head that breaks a rule twice more: once holding every other head at its baseline value, and once holding every other head at its value after the first £1 while the component rises again. The head fails where either measurement still shows the response. The check cannot hold a head at a value that is not finite. In each unit where another head is not finite after the first £1, it holds that head at its baseline value, and the second measurement judges no household the unit belongs to. There a head fails wherever it first broke a rule, or wherever the check could not confirm whether its response contains the component. 0 failed and the check skipped 0.
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
|
juaristi22
left a comment
There was a problem hiding this comment.
Incremental review at 595fd3fdf55ba252ed9c6c995e27174e56df52c3: COMPLETE, no findings or material evidence gaps.
The revised design routes microsimulation-marked tests to the last of eight CI shards with one worker, while the other seven shards partition the remaining tests. The single-shard fallback, marker inheritance, partition/selection and outcome-log contracts, workflow worker settings, and duration-table changes were checked together.
146 focused tests passed, with no failures or skips, across CI-sharding, runner-variable and aggregate-workflow health checks. All 22 removed duration entries bind to actual marked tests. The earlier 55 passes describe the superseded design and are not counted as validation of this revision.
No private data, certified aggregate fixtures or model calculations were run. The validation establishes routing/worker contracts; memory figures and performance expectations remain attributed to the author's hosted evidence.
The PR remains Draft with CI pending, so this is a completed review comment, not an approval. The passing code review can be retained while readiness and exact-head CI are checked later.
The tests that carry the
microsimulationmarker now run in an eighth CI shard with one worker, so only one simulation of the whole dataset runs at a time. The other seven shards run none of them.These tests set the memory peaks on the 16 GB runners. In merge queue run 42212587066, the five shards that built the fixture in
test_aggregate_impact_certified.pypeaked at 14.2 to 15.3 GB, and the two without it at 10.9 and 12.6 GB.#2887, which merges 28 pull requests, leaves less room, since its runners hold about 11 GB in the first minute of tests. With data tests on all seven shards, it lost eight of its 14 shard jobs at 15.4 to 15.7 GB within minutes (Test, Release test). Shard 6/7 of the first then lost its runner at 15.6 GB 51 minutes in, with
test_latest_data_smoke.pyrunning.An earlier head of this PR moved only the certified file to one shard. On #2887's merge that shard still lost its runner at 15.6 GB, about seven minutes in (job 127936972309), so this head moves every data test.
The table lists 33 minutes for these tests, including four entries that each measured the same fixture, against the job's 90-minute limit. I removed their 22 entries from
shard_durations.toml, which re-deals the rest: 463 of the remaining 551 listed tests change shard, and the seven shards' listed loads stay within 20 seconds of each other. Unlisted tests keep their shard. The queue requiresTest / ResultandLint, which don't name shards, so the rename to K/8 needs no ruleset change.Invariants, tested in
test_ci_test_shards.py:shard_ofover one shard fewer places the rest, so every collected test runs in one shard and only one.check_test_shards.pypasses the run.Seven mutations of this rule each fail one of these tests.
Reproducer:
policyengine_uk/tests/code_health/test_ci_test_shards.py::test_the_last_shard_runs_exactly_the_tests_that_need_the_dataset🤖 Generated with Claude Code