Skip to content

Run the UK test suite in CI and tier the suite by engine and country (#699) - #766

Merged
juaristi22 merged 3 commits into
mainfrom
uk-ci-lane-699
Aug 25, 2026
Merged

Run the UK test suite in CI and tier the suite by engine and country (#699)#766
juaristi22 merged 3 commits into
mainfrom
uk-ci-lane-699

Conversation

@juaristi22

@juaristi22 juaristi22 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #699 and cuts CI wall clock from ~2h25m to ~40m.

1 — the UK lane (#699). CI synced only --extra us, so every policyengine-uk-gated test skipped on every run and no UK engine path was exercised automatically. That is the gap that let the adapter's rejected fiscal_year kwarg reach a licensed build unseen (#690) — the failure mode @vahid-ahmadi named as already having happened once. requires_us/requires_uk are now registered markers backed by a root collection hook, and the UK gates are converted onto requires_uk, so 23 UK engine tests run on every PR (2 licensed-HF-cache tests still skip cleanly by design). Per the review note on the issue this is marker-registration, not path selection: group membership is a startswith("test_uk_") predicate, so new WS-E test files are absorbed automatically and --verify fails hard if the partition ever breaks. All engine jobs sync both extras, which is also what removes the uk-only-env metadata failures in test_us_multispine_pool_tool.py.

2 — engine/country tiering. CI was 2–5 min through Jul 31, ~35 min from Aug 1–2 (--extra us + the live pool-input guards), and ~145 min by Aug 24. The growth is overwhelmingly US and shared families — test_spec_engine_* ≈24m, test_puf_qrf_chain ≈18m, test_us_trade_* ≈16.5m, test_us_multispine_pool* ≈15m, test_us_stacked_spine ≈10m — against ≈15–20m for all UK families combined. So this PR does not touch a single US test file; it changes only when work runs:

lane runs contents
lint always partition verify, --locked sync, ruff
fast always full tracked inventory, no engine extras (engine tests skip through their existing guards), 3 groups
engine-shared always spec-engine + shared, both engines
engine-us / engine-uk main pushes, or when that country's / shared paths change country groups, both engines
wheels always unchanged charter gate

A UK PR pays the shared and UK groups and not the US sweep. Every uv sync gains --locked (the uk-extra lock entries had been drifting unverified since 1ad49d5).

Receipts

  • Collection 6,679 → 6,679 for build+frame — no test silently dropped.
  • -m requires_uk selects 25 tests: 25 passed with engines, 25 skipped, 0 errors in a purpose-built engine-free venv (the fast tier's environment).
  • uk group: 1,247 passed / 14 skipped (build proc) + 3 passed (frame proc) = 1,264, matching collection exactly. The 14 skips are licensed-artifact gates.
  • ruff check . clean; tools/ci_test_groups.py --verifyverification=ok; both partitions disjoint and exhaustive over 301 tracked files.
  • Engine-free collection rises by exactly 6: test_uk_efrs_weighted_totals.py's module-level gate became a pytestmark, so its tests are now collected-and-skipped rather than silently uncollected. More honest reporting, not a regression.

Design notes for review

  • Country conditioning uses three statically named jobs, not one conditional matrix. A job-level if cannot read the matrix context (only github, needs, vars, inputs), and a dynamic fromJSON matrix would drop check contexts entirely — which blocks a required check instead of satisfying it, as a skipped job does.
  • Memory. Each engine group is a subset of the build shard's proven partitions (_LANE-NOTES.md:361-374), and frame's engine-heavy files run as their own process. The simpler argument: every group is a strict subset of the single build-shard process that passes today, so none can be worse. The one new factor — the UK engine boot — lands on the smallest-baseline group. /usr/bin/time -v on every pytest invocation gives per-group residency receipts from the first run.
  • tools/ci_test_groups.py is the partition authority: stdlib-only so it runs before any sync, with --verify asserting disjoint + exhaustive and printing defaulted files so new tests cannot fall out of CI silently.
  • Divergence worth flagging: the review note asked for "uk extra + full suite". A UK PR here runs all UK tests, shared/spec, and the full engine-free inventory — but not the US engine-gated groups, which defer to main's push run. That is a deliberate scoping decision made after that comment; a country PR merging over a fresh change to the other country is certified by main's push run.

Follow-ups (not in this PR)

  • Branch protection: required contexts change from test (3.13|3.14) / wheels (…) to lint, fast (…), engine-shared (…), engine-us (…), engine-uk (…), wheels (…). Repo settings need updating when this merges.

🤖 Generated with Claude Code

cc @MaxGhenis

juaristi22 and others added 2 commits August 25, 2026 11:12
CI installed only the `us` extra, so every policyengine-uk-gated test
skipped on every run and no UK engine path was exercised automatically —
the gap that let the adapter's rejected `fiscal_year` kwarg reach a
licensed build unseen (#690). Register `requires_us`/`requires_uk` as real
pytest markers with a root collection hook, and convert the UK gates onto
`requires_uk` so the marker is a genuine selector rather than a per-file
alias. US files keep their existing `importorskip` guards untouched.

Restructure the workflow so the engine suite leaves the PR critical path.
`fast` runs the full tracked inventory without engine extras in three
groups; `engine-shared` always runs the spec/shared group with both
engines; `engine-us` and `engine-uk` are gated by a `changes` job that
classifies the diff, so a UK PR pays the UK and shared groups but not the
US sweep. `wheels` is unchanged — the charter's packaging gate still
builds real wheels and runs the suite against the installed constellation.
Every sync gains `--locked`.

Country conditioning uses statically named jobs whose `if` reads only
`needs` outputs: the matrix context is unavailable in a job-level `if`,
and a dynamic matrix would drop check contexts entirely, which blocks
required checks instead of satisfying them as a skipped job does.

`tools/ci_test_groups.py` is the partition authority — stdlib-only so it
runs before any sync, with `--verify` asserting the groups stay disjoint
and exhaustive and printing defaulted files so new tests cannot fall out
of CI silently. Groups are subsets of the build shard's proven memory
partitions, and frame's engine-heavy files run as their own process.

Receipts: collection unchanged at 6,679 for build+frame; `-m requires_uk`
selects 25 tests, all passing with engines and all skipping cleanly
without them; the uk group runs 1,247 passed / 14 skipped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The first CI run of the new lanes failed 11 tests in `fast (rest)`, on both
Pythons: 10 in test_us_multispine_pool_tool.py and 1 in
test_us_release_head_to_head_scorer.py, all on a missing policyengine-us.

They fail rather than skip because their guard and their dependency disagree.
Each gates on `importorskip("tables")` and then reaches an unguarded engine
import (build_us_fiscal_refresh_release._load_frame does
`from policyengine_us.data import USSingleYearDataset`). The wheels gate never
exposed this: it installs only wheels plus pytest, so `tables` is absent there
and the tests skip before reaching the import. The fast lane syncs the whole
workspace, where `tables` is present — an engine-free environment CI has never
had until now.

Name the two files in an explicit ENGINE_ONLY tuple that the fast tier skips.
They still run in full in the engine tier (us-am and us-qs), which installs
both extras, so no coverage is lost. --verify prints them under [engine-only]
and fails if either path stops existing, so the exclusion can never go silent,
and the engine tier still partitions the complete 301-file inventory.

Evidence: the fast `rest` group engine-free is 4,874 passed / 201 skipped / 0
failed after the change, so no other file carries the same shape.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@juaristi22

Copy link
Copy Markdown
Collaborator Author

First-run receipts (all 22 checks green)

Wall clock: 41.4 min, down from ~145 min — and this run exercised the full engine matrix, because a PR touching .github/, tools/, and conftest.py classifies as shared. A UK-only PR skips all eight engine-us legs.

The changes job classified this PR as {'shared': True, 'us': False, 'uk': True}us: False is a mechanical confirmation that no US test file was touched.

Durations and peak residency (/usr/bin/time -v, 3.13 leg)

job duration peak RSS
wheels 41.4 min
engine-us (us-am) 34.5 min 5.1 GiB
engine-shared 34.0 min 4.5 GiB
engine-us (us-p) 22.2 min 2.1 GiB
fast (spine-uk) 20.2 min
engine-uk 15.5 min 2.2 GiB
fast (rest) ~14 min 1.3 GiB
fast (trade) 3.0 min
lint 0.5 min

Every group is far under the runner's 16 GB — the largest is 5.1 GiB against an 18.5 GiB single-process figure for the old build shard. The memory argument in the description was conservative by roughly 3×.

Two estimates that were badly wrong, in the same direction

fast (trade) came in at 3.0 min against a ~17 min estimate, and engine-uk peaked at 2.2 GiB where the design budgeted against a 15 GiB ceiling. Both point the same way: much of the old suite's cost was contention inside one oversized process, not the tests themselves. Splitting bought more than linear time-division predicts.

UK lane confirmed working

policyengine-uk==2.89.0 installed, zero engine-absence skips anywhere in the log, and the group ran 1,244 passed / 17 skipped. The 17 are licensed-artifact gates; locally (where those artifacts are cached) the same group is 1,247 passed / 14 skipped — same 1,261 total. The UK engine tests execute in CI for the first time.

The defect this run caught (fixed in 91001c8)

11 tests failed rather than skipped in fast (rest) on both Pythons. They gate on importorskip("tables") — satisfied by the workspace sync — and then reach an unguarded policyengine_us import. The wheels gate never exposed this because tables is absent from its venv, so they skip there; the fast lane is the first engine-free environment CI has had with tables present. Fixed by naming the two files in an explicit ENGINE_ONLY tuple that the fast tier skips and --verify prints; they still run in full in the engine tier. Proof: the engine-free rest group is now 4,874 passed / 201 skipped / 0 failed, so no other file carries the same shape.

Follow-up this run identifies

engine-shared is the always-on pole at 34 min and will define the floor on quiet PRs. The rebalance knob named in the description does not help it — that moves test_us_fiscal_*, which lives in us-am. The real lever is test_spec_engine_* (26 files repeating load_bundle("us") across 15 module-scoped fixtures); a session-scoped fixture there would cut the always-on cost materially. Shared infrastructure, so it belongs to whoever owns the spec engine rather than this PR.

@juaristi22
juaristi22 requested review from hua7450 and vahid-ahmadi and removed request for vahid-ahmadi August 25, 2026 11:05

@hua7450 hua7450 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four findings from a pass over the workflow and partition tool: one blocking, one one-line fix, two notes. All claims below checked against the branch (classifier logic simulated on probe paths, imports grepped, pathspec behavior tested empirically).


1. Blocking — the change classifier never sees the calibrate/fit/data shards, so their engine-dependent consumers skip pre-merge

A PR touching only packages/microcosm-calibrate/src/** (same for -fit and -data) classifies as {shared: false, us: false, uk: false}: the shared filter includes only the build and frame src/** trees, and no us/uk pattern matches those shards either. Both engine-us and engine-uk then skip.

That's a hole in exactly the "cross-cutting" category the shared lane exists for:

  • microcosm-build depends on microcosm-frame, microcosm-fit, and microcosm-calibrate in its core dependencies (and on microcosm-data via the us extra, which every engine lane installs), so the three quiet shards sit in the same dependency position as frame — which is classified shared.
  • 19 test_us_*/test_uk_* files import microcosm.calibrate/microcosm.fit — e.g. test_uk_hmrc_calibration.pyuk_runtime/hmrc_calibration.pyfrom microcosm.calibrate import …. All of them live only in the conditional lanes.
  • The three shards' own test files have their engine-tier home in us-am (is_calibrate_data_fitus-am in tools/ci_test_groups.py), which is also skipped — so even a PR editing one of those test files doesn't run it with engines before merge.

The always-on lanes soften this (fast runs the shards' unit tests engine-free, engine-shared exercises calibrate transitively, wheels reruns everything engine-free), so what escapes is specifically breakage only observable with an engine installed. That failure mode currently defers to main's push run — but without the deliberate scoping rationale the PR gives for country PRs.

Suggested fix: generalize the src pattern (the existing country-runtime excludes keep country-scoped changes classifying narrowly), and route the three shards' tests dirs:

"shared": {
    "include": [
        ...
        "packages/*/src/**",
        "packages/microcosm-calibrate/tests/**",
        "packages/microcosm-data/tests/**",
        "packages/microcosm-fit/tests/**",
    ],
    "exclude": [
        "packages/microcosm-build/src/microcosm/build/us_runtime/**",
        "packages/microcosm-build/src/microcosm/build/uk_runtime/**",
        ...
    ],
},

(Three explicit tests lines because fnmatch has no brace expansion. Routing the tests dirs to us instead of shared would also work, since us-am is where they execute with engines.)


2. One-line fix — engine-shared shouldn't depend on changes

engine-shared declares needs: changes but has no if and never reads the classifier's outputs. If changes fails — its inline script makes live GitHub API calls with no retry, so a transient network error is enough — the dependency-skip takes the always-on engine lane down along with the conditional ones, and that run silently loses its unconditional engine coverage.

Dropping the needs: changes line decouples it; the job consumes nothing from classification.


3. Note for the branch-protection follow-up (no change requested in this PR)

The Follow-ups section proposes requiring engine-us (…) / engine-uk (…) as branch-protection contexts. That plan can't work as stated: jobs.<job_id>.if is evaluated before strategy.matrix is applied (workflow-syntax docs), so when a lane skips, GitHub reports a single skipped engine-us check and the per-leg contexts (engine-us (3.13, us-p), …) are never created — a UK-only PR would sit on "Expected — waiting for status" forever. When the lane runs, only per-leg names report. So no static required-context list covers both cases.

When protection is eventually enabled, the standard shape is a single non-matrix summary job as the only required context:

  ci-ok:
    needs: [changes, lint, fast, engine-shared, engine-us, engine-uk, wheels]
    if: always()
    runs-on: ubuntu-latest
    steps:
      - run: |
          test "${{ needs.changes.result }}" = "success"
          for r in "${{ needs.lint.result }}" "${{ needs.fast.result }}" \
                   "${{ needs['engine-shared'].result }}" "${{ needs.wheels.result }}"; do
            test "$r" = "success"
          done
          for r in "${{ needs['engine-us'].result }}" "${{ needs['engine-uk'].result }}"; do
            case "$r" in success|skipped) ;; *) exit 1 ;; esac
          done

This also closes a second seam: dependency-skipped jobs "may not block merging" (required-checks troubleshooting), so with per-lane contexts a failed changes would skip the engine lanes without any required check going red. The gate above accepts a lane skip only when classification itself succeeded.


4. Docs suggestion — state the test-placement invariant in CLAUDE.md

The CLAUDE.md edit documents the requires_* markers and names tools/ci_test_groups.py as the partition authority, but the placement/naming invariant the partition silently depends on isn't stated anywhere. The trap: the fast and engine lanes pass explicit file lists built from git ls-files -- 'packages/*/tests/test_*.py', while local uv run pytest discovers recursively (testpaths = ["packages"]) — and the natural place to misplace a file already exists: tests/fixtures/ and tests/golden/ (data-only today). A test dropped next to its fixtures runs with engines in today's CI (recursive per-shard discovery) but escapes the inventory under this PR — verified: a probe file in tests/fixtures/ doesn't match the pathspec, so --verify can never see it either. Only the engine-free wheels lane, which still discovers recursively, would collect it — and an engine-gated test skips there, so it would never execute with an engine anywhere in CI while showing green.

Suggested addition to the testing section:

Adding a test file: it must sit directly in packages/<shard>/tests/ (flat — no subdirectories; fixtures/ and golden/ hold data only) and be named test_*.py, or the fast and engine lanes will never run it even though local uv run pytest collects it (only the engine-free wheels lane still would, where engine-gated tests skip). Build tests that exercise a country engine must be named test_us_* or test_uk_* so they land in that country's CI lane; an engine-dependent file named anything else falls into the always-on shared-spec group and runs on every PR. Check tools/ci_test_groups.py --verify — your new file should appear in the group you expect, and never under [defaulted].

…context

Review findings from @hua7450, all four confirmed against the branch.

**Classifier hole (blocking).** A PR touching only microcosm-calibrate,
-fit, or -data classified as {shared: false, us: false, uk: false}, so both
country lanes skipped and their engine-dependent consumers never ran
pre-merge. Verified: those four probe paths all classified all-false, and
uk_runtime imports microcosm.calibrate in cgt_calibration, battery_bindings,
and diagnostics, with 19 country test files importing calibrate or fit. The
shared include now covers `packages/*/src/**` plus the three quiet shards'
tests directories, whose engine-tier home (us-am) was skipped too. The
country-runtime excludes still keep country changes narrow: UK runtime, US
runtime, and country test files classify exactly as before, so a UK PR still
skips the US sweep.

**engine-shared no longer needs `changes`.** It reads no classifier output,
so the dependency only created a way to lose the unconditional engine lane
when the classifier's un-retried API calls fail.

**One aggregate required context.** Per-leg matrix names cannot be required:
`jobs.<job_id>.if` is evaluated before the matrix expands, so a skipped lane
reports one bare `engine-us` check and per-leg contexts never appear — a
required `engine-us (3.13, us-p)` would wait forever on a UK-only PR. `ci-ok`
is the single context to require, and it also closes the seam where a
dependency-skip lets a failed `changes` take the engine lanes down with no
required check going red: it accepts a lane skip only when classification
succeeded. Logic exercised over five scenarios; the classifier-failed case
blocks.

**Stray nested tests now fail the partition.** The lanes run flat file lists
while local pytest and the wheels lane discover recursively, so a test parked
in tests/fixtures/ would run locally, stay green in CI, and never execute
against an engine. Confirmed empirically: such a probe is invisible to the
pathspec but collected recursively. `--verify` now fails on it by name, and
CLAUDE.md states the placement and naming invariant.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@juaristi22

Copy link
Copy Markdown
Collaborator Author

Thanks @hua7450 — all four confirmed against the branch and addressed in 404e457. I reproduced each claim rather than taking it on faith; notes below where I went further or diverged.

1. Classifier hole — fixed

Confirmed exactly as described: microcosm-calibrate/src, -fit/src, -data/src, and microcosm-calibrate/tests/test_solve.py all classified {shared: false, us: false, uk: false}. The dependency claim holds too — uk_runtime imports microcosm.calibrate in cgt_calibration.py:34, battery_bindings.py:95, and diagnostics.py:28, and 19 country test files import calibrate or fit. So a calibrate change could break UK engine tests with neither country lane running. Blocking was the right severity.

Took your suggested shape: packages/*/src/** plus the three tests directories. Re-simulated the classifier on probe paths to confirm the generalization doesn't over-trigger — the narrowness that makes this PR worthwhile survives:

path shared us uk lanes
microcosm-calibrate/src/… all three (was: shared only)
microcosm-calibrate/tests/… all three (was: shared only)
uk_runtime/spi.py shared + uk (unchanged)
us_runtime/puf.py shared + us (unchanged)
test_uk_spi_income.py shared + uk (unchanged)

2. needs: changes on engine-shared — dropped

Correct, and the failure mode is worse than cosmetic: the classifier's inline script makes un-retried API calls, so one transient error would have taken the unconditional engine lane down. Removed, with a comment recording why it must not come back.

3. Required contexts — implemented rather than deferred

Your analysis is right and my Follow-ups note was wrong: if is evaluated before matrix expansion, so no static per-leg list covers both the skipped and running cases. I've added ci-ok in this PR rather than leaving a known-broken plan in the description — it's a dozen lines and it makes enabling protection a settings-only change. Shout if you'd rather it landed separately.

I exercised the logic across five scenarios; note the third, which is the second seam you flagged:

scenario result
all success PASS
UK PR, engine-us skipped PASS
changes failed, lanes skipped BLOCK
engine-uk failed BLOCK
wheels failed BLOCK

4. Placement invariant — documented and enforced

Verified the trap is live: a probe at packages/microcosm-build/tests/fixtures/test_probe_ci.py is invisible to the pathspec (git ls-files → 0 matches) but collected by recursive discovery (→ 1), and fixtures//golden/ already exist in four shards.

I took your CLAUDE.md wording, but a documented invariant still relies on someone remembering it, so --verify now fails on such a file by name — it names the paths and says where to move them. Clean on the current tree; catches the probe when planted. That makes the doc an explanation rather than the only defence.

@juaristi22
juaristi22 merged commit 2263df3 into main Aug 25, 2026
23 checks passed
MaxGhenis added a commit that referenced this pull request Aug 26, 2026
…he union (third application)

Main moved the attested surfaces again (#743 first calibrated UK
candidate, #766 CI lane, #764 rename), so the merge re-pins the UK
spec_sha256, re-cuts the three gate-battery digests into the
microcosm-data contract and its test mirror, and regenerates the
release-input coverage manifest over the union - the d70ea39 pattern.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run the UK test suite and a smoke-rung build in CI

2 participants