Skip to content

Make the performance tests independent of CI runner speed - #1441

Draft
MaxGhenis wants to merge 3 commits into
mainfrom
perf-tests-runner-independent
Draft

MaxGhenis wants to merge 3 commits into
mainfrom
perf-tests-runner-independent

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1429

Problem

test (macos-latest, 3.11) failed on wall-clock limits in tests/test_performance.py, on main and on unrelated PRs, with nothing slower in the code. Each failure listed in #1429 was the only one in its job. On main @ 2da4a45 (run 38101317049), TestBenchmark::test_benchmark_cps_like took 266.3s against its 240s limit (1 failed, 501 passed, 8 skipped, 3 xfailed). versioning needs test, so versioning and publish were skipped, and that merge's release waited for a later push (3.2.0, be55c81).

Two tests compared elapsed time with a fixed number of seconds:

  • TestExtractResultsStructure::test_extract_builds_dataframe_without_row_loop: extract_time < 60.0 for 200 records. Its docstring says it guards against a return to row-by-row extraction with redundant calculate calls (the pre-Vectorize PolicyEngineRunner for ~100x speedup #692 code).
  • TestBenchmark (test_benchmark_500_records < 120s, test_benchmark_cps_like < 240s). The class was already marked @pytest.mark.slow with the docstring "Run with: pytest -m slow", but nothing deselected slow tests. On main, pyproject.toml has no markers and addopts is -ra -q, and there is no tests/conftest.py. CI runs pytest tests/, so both benchmarks ran in every job.

Change (test-only; no file under policyengine_taxsim/ changes)

  1. The extract test counts calls instead of timing them. It runs PolicyEngineRunner on 51 households, one in each state, and on the same households repeated 4 times (204 records). It counts the Microsimulation.calculate calls made while _extract_vectorized_results runs, and requires the two counts to be equal and nonzero. Only top-level calls count: a PolicyEngine formula reads its inputs through calculate too, and how many of those happen depends on the households. The counter covers the main simulation, the rebate-free twin and the marginal-rate branch. The households alternate idtl 2 and 5. idtl 2 alone never requests staxbc or sctc, and idtl 5 alone never requests addmed or v44; together they request all 43 implemented outputs.
  2. slow tests are skipped unless asked for. The marker is registered in pyproject.toml. A tests/conftest.py hook skips slow tests unless a -m expression is given, so pytest tests/ (CI) skips the two benchmarks with a reason, and pytest -m slow runs them. With any -m expression, pytest's own selection decides. Skipping is more visible than -m "not slow" in addopts: a skip is reported with its reason, while a deselected test is silently not run, even when named by node id.
  3. The CPS-like record construction moved into _make_cps_like_records, shared by the benchmark and the new test. It returns a frame identical to the old inline code for n=2000 (pd.testing.assert_frame_equal, checked locally).
  4. A changelog fragment, and a note under "Running tests" in CLAUDE.md.

TestGeneratePhaseEfficiency::test_generate_does_not_scale_linearly keeps its timing ratio with a floored denominator (#696, #1073). It is not in #1429's failure evidence, so it is left alone.

The invariant the extract test checks: for a fixed set of households, the number of top-level calculate calls in the extract phase does not depend on how many records there are.

Evidence

The new check still catches per-record work

Each mutant is a throwaway copy of policyengine_taxsim from origin/main (git archive origin/main policyengine_taxsim | tar -x) with one edit. The test was run against each copy with PYTHONPATH=<copy> pytest tests/test_performance.py -k test_extract_builds_dataframe_without_row_loop. A plugin printed which copy was imported, to confirm the edit was the code under test.

Copy Edit Result Calls (51 / 204 records)
clean none pass 166 / 166
M1 the pre-#692 pattern in the single-variable branch: one calculate per record fail 1266 / 4632
M2 one calculate per record in the Delaware staxbc path (_apply_de_staxbc_elected_path, reached only at idtl 5) fail 216 / 369
M3 one extra calculate per Delaware record only fail 167 / 170
M4 one calculate per record on the marginal-rate branch simulation fail 216 / 369
M5 one calculate per record on the rebate-free twin (v40) fail 266 / 572

The first version of this PR used idtl 2 only. M2 and M3 passed against it, because staxbc is requested only at idtl 5. That is why the households now alternate idtl 2 and 5.

Equal counts are exact, not approximate. A probe on main @ 72fdc04 counted the same number of top-level calls for 20 and 200 records at idtl 0 (11 and 11), and for 51 and 255 records at idtl 2 (112 and 112). In the clean run, the 166 calls split as 160 on the main simulation, 4 on the rebate-free twin and 2 on the marginal-rate branch.

CI on this branch (ba9498b)

All 12 checks pass. The extract test passed in all six test jobs, which cover policyengine-us 1.783.0 (Python 3.10) and 2.38.19 (Python 3.11) on macOS, Ubuntu and Windows. Each job reports SKIPPED [2] tests/test_performance.py: wall-clock benchmark, skipped by default; run with pytest -m slow. On macOS 3.11 the skip list differs from main @ 2da4a45's only by that line (8 skipped there, 10 here). macOS 3.11 summary: 700 passed, 10 skipped, 3 xfailed in 4230.18s (1:10:30).

Locally (pytest 8.4.2, Python 3.11):

  • pytest tests/test_performance.py -k TestBenchmark: 2 skipped, with the reason.
  • pytest tests/test_performance.py::TestBenchmark::test_benchmark_cps_like: 1 skipped. A named benchmark is reported as skipped, not silently deselected.
  • pytest tests/ --collect-only -m slow: 2 tests, both in TestBenchmark.
  • pytest tests/test_performance.py --collect-only -m "not slow": 11 of 13.

Cost

The new test runs the runner twice (51 and 204 records); the default run no longer runs the two benchmarks. CPU time per runner call, measured in one Python 3.11 process on the same machine (time.process_time(), which a loaded host disturbs much less than wall time):

Default run Workload CPU
before old extract test (200 records, idtl 0) 30.1s
before test_benchmark_500_records 26.7s
before test_benchmark_cps_like (2,000 records, idtl 2) 87.7s
before total 144.5s
after new extract test: 51 records 50.9s
after new extract test: 204 records 44.6s
after total 95.5s

Wall time on this host was not representative (load averages of 150 to 390 on 18 cores).

Not in this PR

axiom: n/a: test-only change

🤖 Generated with Claude Code

@vercel

vercel Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
policyengine-taxsim Ready Ready Preview Oct 12, 2026 4:15pm UTC

Request Review

MaxGhenis and others added 2 commits October 11, 2026 21:03
The extract-phase test compared elapsed time with a 60-second limit and
failed on macOS CI runners with nothing slower in the code (#1429). It now
counts the calculate calls the extract phase makes for 51 and 204 records in
the same states and requires the same number.

TestBenchmark was already marked slow ("Run with: pytest -m slow"), but the
marker was unregistered and nothing skipped it, so its absolute limits ran in
every CI job. Register the marker and skip slow tests unless a -m expression
is given.

Fixes #1429

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
idtl 2 alone never requests staxbc or sctc, so a per-record loop in the
Delaware staxbc path went unnoticed (two mutants passed). Alternate idtl 2
and 5, which together request every implemented output.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of #1441: the old timed test ran standard output (idtl 0) and the
call-count test ran only idtl 2 and 5, so a per-record path taken only
when every record asks for standard output would go unnoticed (for
example, computing marginal rates one household at a time). Run the count
check in both output modes.

Also: the CPS-like states are TAXSIM SOI numbers, not FIPS codes; the
check is a fixed number of top-level calls, not one per output variable;
and any -m expression, not only -m slow, turns off the slow skip.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch was successfully deployed

1 active deployment
Preview — 0b294fb6 Deployed Oct 12, 2026 by vercel[bot]
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.

macOS 3.11 CI fails on wall-clock limits in test_performance.py (main and unrelated PRs)

1 participant