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
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>
MaxGhenis
changed the title
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
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.
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Review status: COMPLETE — one nonblocking suggestion
Reviewed head: 595fd3fdf55ba252ed9c6c995e27174e56df52c3
The newly cited Test and Release test runs both passed at this PR head. All 16 shard logs support the approximate 61–72-minute non-data and 19-minute data test durations. The earlier implementation review and 146 passing tests remain valid; no tests or model calculations were repeated.
S1 — Use memory units consistent with free -m. The description's 10.1–12.2 and 9.5 numerals reproduce the logged values divided by 1,000, but the producer reports MiB. The non-data samples are 10,125–12,203 MiB, about 9.9–11.9 GiB; the data samples are 9,520–9,540 MiB, about 9.3 GiB. In decimal GB those ranges round to 10.6–12.8 GB and 10.0 GB. Labeling the figures consistently would make the new paragraph precise.
These are whole-runner used-memory samples taken once per minute. The successful runs and implementation assessment are supported; this suggestion concerns the new reporting units. Current observed CI is clear, and the PR is Ready.
The Test job's memory log printed free -m's figures, which count MiB, under the label MB, and the workflow's comments called its peaks GB, some of them in thousands of MiB. The log now labels its figures MiB, .github/test_durations.py reads lines with either label, and the comments give each peak in GiB: - merge queue run 42212587066: 14.2 to 15.3 GiB on the five shards that built the certified fixture, 10.9 and 12.6 GiB on the two without it; - runs 42218280596 and 42218280615: 15.1 to 15.4 GiB on the eight shard jobs they lost within minutes, and 15.2 GiB on the shard the first lost 51 minutes in; - run 42231098419: 15.2 GiB on the certified shard it lost; - run 38156967136: 15.3 GiB of the 15.6 GiB the runner reports as used and available. On two shard logs from 595fd3f, the parser gives the same durations as before, and the same again when each memory line says MiB. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. I've restated every memory figure in the description and the workflow's comments in GiB, and the job now labels its memory log MiB, the unit |
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Review status: COMPLETE — S1 resolved, approval awaiting current-head CI
Reviewed head: ca2ea8da10120b7db84febd0b805523928fe4e6b.
S1 — RESOLVED. The description and workflow comments now use the correct MiB-to-GiB conversions, the producer labels its free -m readings MiB, and the duration parser accepts both MB and MiB. I checked the restated historical figures against the cited job logs and retained the previously verified 595fd3fdf telemetry. These are minute-sampled whole-runner memory readings; the shutdown observations retain their original evidential scope.
New validation passed 55 parser/producer diagnostic assertions and three actual CLI comparisons. All 16 cached logs produced nonempty duration mappings with both labels; the CLI outputs matched with 240 TOML entries covering 3,546 tests. The unchanged sharding implementation retains its prior 146 passing repository tests. Those historical passes were not rerun or relabelled as new tests at this head. No model calculations or population runs were performed.
There are no open findings or material evidence gaps. This is a comment because the Test/Release test matrices and aggregate-impact checks are still running at the new head. Approval can follow once applicable current-head checks pass, without repeating the completed code review.
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Review status: COMPLETE — APPROVE
Approved head: ca2ea8da10120b7db84febd0b805523928fe4e6b.
The completed follow-up review remains valid at this unchanged head and description. S1 is resolved, with no open findings or material evidence gaps.
Current-head CI is now clear: 40 checks passed and Publish was skipped, including passing applicable required checks. The PR is ready for review. Current head, description, review activity, CI, required-check rules and duplicate approvals are guarded again immediately before publication.
The retained evidence includes 146 historical sharding-test passes, 55 parser/producer assertions and three CLI comparisons from the completed follow-up. All 130 technical evidence members were hash-revalidated. No tests, model calculations or population runs were repeated for this CI-only approval.
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 runners where
free -mcounts 15.6 GiB used and available. In merge queue run 42212587066, the five shards that built the fixture intest_aggregate_impact_certified.pypeaked at 14.2 to 15.3 GiB, and the two without it at 10.9 and 12.6 GiB.#2887, a draft that combined 28 pull requests, left less room, since its runners held about 11.4 GiB in the first minute of tests. With data tests on all seven shards, it lost eight of its 14 shard jobs at 15.1 to 15.4 GiB within minutes (Test, Release test). Shard 6/7 of the first then lost its runner at 15.2 GiB 51 minutes in, with
test_latest_data_smoke.pyrunning.An earlier head of this PR moved only the certified file to one shard. When #2887 merged that head, the certified shard still lost its runner at 15.2 GiB, 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.In the runs at
595fd3fdf(Test, Release test), the seven shards without data tests peaked at 9.9 to 11.9 GiB and ran their tests in 61 to 72 minutes. The data shard ran in 19 minutes and peaked at 9.3 GiB. The memory log labels its figures MiB, the unitfree -mcounts, and.github/test_durations.pyreads lines with either label.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