From 4d6664a7cf6627bc4bbb5c1160b186eb4b24f64d Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 4 Oct 2026 11:38:22 -0400 Subject: [PATCH 1/3] Run the Rest job's remaining Python tests in six pytest processes As one pytest process, the remaining Python tests peaked at 15.7 GB RSS on the 16 GB runner (run 37198184683), with 1.48M major page faults, and since #9782 the step has been cancelled partway through on several runs, with no log retained. test-other-python-rest now runs six groups in sequence, one pytest process each. cached-arrays, heavy-a and heavy-b hold the seven slowest modules; then come core/, policy/, and remaining, which takes every other file, including new ones. The target runs every group even after one fails, and fails if any did. With REST_REPORT_DIR set, as both workflows set it, each group writes its own JUnit XML and GNU time report, and the Rest job uploads them all. The SPM step, the --ignores and the test set are unchanged. code_health/test_rest_python_groups.py takes the old single command and each group's command from `make -n`, lists the files pytest collects for each without importing them, and checks that the groups collect every file of the old command exactly once. It also checks that every group's reports are uploaded by both workflows. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr.yaml | 14 +- .github/workflows/push.yaml | 14 +- Makefile | 57 ++- changelog.d/ci-split-rest-job.changed.md | 1 + .../code_health/test_rest_python_groups.py | 333 ++++++++++++++++++ 5 files changed, 403 insertions(+), 16 deletions(-) create mode 100644 changelog.d/ci-split-rest-job.changed.md create mode 100644 policyengine_us/tests/code_health/test_rest_python_groups.py diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index c5a8a15dfcf..a2bc60258d8 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -483,7 +483,9 @@ jobs: shell: bash run: bash ./update_itemization.sh # Run sequentially on this runner; process exit releases the SPM heap - # before collecting the remaining Python files. + # before collecting the remaining Python files. Those run as one pytest + # process per group (REST_PYTHON_GROUPS in the Makefile), and each group + # writes rest-python-.xml and rest-python--resources.txt. - name: Run SPM Python tests env: PYTEST_ADDOPTS: "--durations=0 -v --junitxml=rest-python-spm.xml -p no:unraisableexception -p no:threadexception" @@ -491,8 +493,8 @@ jobs: - name: Run remaining Python tests if: always() env: - PYTEST_ADDOPTS: "--durations=0 -v --junitxml=rest-python-other.xml -p no:unraisableexception -p no:threadexception" - run: /usr/bin/time -v -o rest-python-other-resources.txt uv run --no-sync make test-other-python-rest + PYTEST_ADDOPTS: "--durations=0 -v -p no:unraisableexception -p no:threadexception" + run: uv run --no-sync make test-other-python-rest REST_REPORT_DIR=. - name: Run tests/variables YAML tests if: always() env: @@ -504,11 +506,9 @@ jobs: with: name: rest-test-timings path: | - rest-python-spm.xml - rest-python-other.xml + rest-python-*.xml rest-variables.xml - rest-python-spm-resources.txt - rest-python-other-resources.txt + rest-python-*-resources.txt rest-variables-resources.txt Microsimulation: needs: ReleaseLock diff --git a/.github/workflows/push.yaml b/.github/workflows/push.yaml index 955e891bd90..b0301210dda 100644 --- a/.github/workflows/push.yaml +++ b/.github/workflows/push.yaml @@ -299,7 +299,9 @@ jobs: shell: bash run: bash ./update_itemization.sh # Run sequentially on this runner; process exit releases the SPM heap - # before collecting the remaining Python files. + # before collecting the remaining Python files. Those run as one pytest + # process per group (REST_PYTHON_GROUPS in the Makefile), and each group + # writes rest-python-.xml and rest-python--resources.txt. - name: Run SPM Python tests env: PYTEST_ADDOPTS: "--durations=0 -v --junitxml=rest-python-spm.xml -p no:unraisableexception -p no:threadexception" @@ -307,8 +309,8 @@ jobs: - name: Run remaining Python tests if: always() env: - PYTEST_ADDOPTS: "--durations=0 -v --junitxml=rest-python-other.xml -p no:unraisableexception -p no:threadexception" - run: /usr/bin/time -v -o rest-python-other-resources.txt uv run --no-sync make test-other-python-rest + PYTEST_ADDOPTS: "--durations=0 -v -p no:unraisableexception -p no:threadexception" + run: uv run --no-sync make test-other-python-rest REST_REPORT_DIR=. - name: Run tests/variables YAML tests if: always() env: @@ -320,11 +322,9 @@ jobs: with: name: rest-test-timings path: | - rest-python-spm.xml - rest-python-other.xml + rest-python-*.xml rest-variables.xml - rest-python-spm-resources.txt - rest-python-other-resources.txt + rest-python-*-resources.txt rest-variables-resources.txt Microsimulation: needs: ReleaseLock diff --git a/Makefile b/Makefile index 1dd73838558..85633803df9 100644 --- a/Makefile +++ b/Makefile @@ -16,10 +16,46 @@ BATCH := python policyengine_us/tests/test_batched.py TESTS := policyengine_us/tests # Run the expensive SPM construction/isolation tests in a separate process # before the remaining files on the same CI runner, releasing their heap. -# The remaining group discovers every other Python test, including future files. +# The remaining Python tests then run in REST_PYTHON_GROUPS below. REST_SPM_TESTS := $(TESTS)/core/test_spm_policy_family.py \ $(TESTS)/core/test_spm_simulation_isolation.py \ $(TESTS)/core/test_spm_system.py +# The remaining Python tests run as one pytest process per group, one after +# another, so each exit releases its heap before the next group starts. As a +# single process they peaked at 15.7 GB RSS on the 16 GB runner, with 1.5M +# major page faults (CI run 37198184683), and later runs were cancelled +# mid-step with no log, as runners out of memory are (see test-yaml-reform). +# Each file runs in the first group whose paths hold it: the listed heavy +# modules, then core/, then policy/, and remaining takes every other file, +# including new ones. code_health/test_rest_python_groups.py checks that the +# groups together collect each file of the old single process exactly once. +# Test time per group in that run, 1,453 s in all: +# cached-arrays (403 s): test_formulas_do_not_write_into_cached_arrays. +# heavy-a (286 s): md_poverty_line_credit_invariants (134 s), +# ald_determinism (94 s), ssi_state_supplement_medicaid_dependency (59 s). +# heavy-b (221 s): multi_year_simulation (88 s), +# behavioral_response_measurements (84 s), +# dependent_losses_agi_invariants (49 s). +# core (120 s), policy (185 s), remaining (239 s). core has since gained +# three modules, which that run did not measure. +REST_PYTHON_GROUPS := cached-arrays heavy-a heavy-b core policy remaining +REST_CACHED_ARRAY_TESTS := $(TESTS)/test_formulas_do_not_write_into_cached_arrays.py +REST_HEAVY_A_TESTS := $(TESTS)/test_md_poverty_line_credit_invariants.py \ + $(TESTS)/core/test_ald_determinism.py \ + $(TESTS)/core/test_ssi_state_supplement_medicaid_dependency.py +REST_HEAVY_B_TESTS := $(TESTS)/core/test_multi_year_simulation.py \ + $(TESTS)/core/test_behavioral_response_measurements.py \ + $(TESTS)/core/test_dependent_losses_agi_invariants.py +REST_LISTED_TESTS := $(REST_CACHED_ARRAY_TESTS) $(REST_HEAVY_A_TESTS) $(REST_HEAVY_B_TESTS) +REST_IGNORES := $(addprefix --ignore=,$(TESTS)/policy/contrib $(TESTS)/microsimulation $(REST_SPM_TESTS)) +# $(call rest_pytest,,,) is the pytest +# command for one group. CI sets REST_REPORT_DIR, and each group then writes +# its own JUnit XML and GNU time report there: rest-python-.xml and +# rest-python--resources.txt. Unset, it is plain pytest, since macOS +# /usr/bin/time has no -v. +rest_pytest = $(strip $(if $(REST_REPORT_DIR),/usr/bin/time -v -o $(REST_REPORT_DIR)/rest-python-$(1)-resources.txt) \ + pytest $(2) --maxfail=0 $(REST_IGNORES) $(addprefix --ignore=,$(3)) \ + $(if $(REST_REPORT_DIR),--junitxml=$(REST_REPORT_DIR)/rest-python-$(1).xml)) all: build format: @@ -185,8 +221,25 @@ test-other-python: pytest policyengine_us/tests/ --maxfail=0 --ignore=$(TESTS)/policy/contrib --ignore=$(TESTS)/microsimulation test-other-python-spm: pytest $(REST_SPM_TESTS) --maxfail=0 +# Every group runs even after one fails; the target fails if any group did. test-other-python-rest: - pytest $(TESTS)/ --maxfail=0 --ignore=$(TESTS)/policy/contrib --ignore=$(TESTS)/microsimulation $(addprefix --ignore=,$(REST_SPM_TESTS)) + @failed=""; for group in $(REST_PYTHON_GROUPS); do \ + echo "=== Rest Python group: $$group ==="; \ + $(MAKE) --no-print-directory test-other-python-rest-$$group || failed="$$failed $$group"; \ + done; \ + if [ -n "$$failed" ]; then echo "Failed Rest Python groups:$$failed"; exit 1; fi +test-other-python-rest-cached-arrays: + $(call rest_pytest,cached-arrays,$(REST_CACHED_ARRAY_TESTS)) +test-other-python-rest-heavy-a: + $(call rest_pytest,heavy-a,$(REST_HEAVY_A_TESTS)) +test-other-python-rest-heavy-b: + $(call rest_pytest,heavy-b,$(REST_HEAVY_B_TESTS)) +test-other-python-rest-core: + $(call rest_pytest,core,$(TESTS)/core,$(REST_LISTED_TESTS)) +test-other-python-rest-policy: + $(call rest_pytest,policy,$(TESTS)/policy,$(REST_LISTED_TESTS)) +test-other-python-rest-remaining: + $(call rest_pytest,remaining,$(TESTS)/,$(REST_LISTED_TESTS) $(TESTS)/core $(TESTS)/policy) test-microsimulation: pytest $(TESTS)/microsimulation --maxfail=0 # Local convenience: run both halves back to back. diff --git a/changelog.d/ci-split-rest-job.changed.md b/changelog.d/ci-split-rest-job.changed.md new file mode 100644 index 00000000000..6e87973a254 --- /dev/null +++ b/changelog.d/ci-split-rest-job.changed.md @@ -0,0 +1 @@ +Run the Full Suite - Rest job's remaining Python tests as six sequential pytest processes instead of one, each with its own JUnit and memory report, so no single process approaches the 16 GB runner limit; a code-health test checks that the groups run every test file of the old process exactly once. diff --git a/policyengine_us/tests/code_health/test_rest_python_groups.py b/policyengine_us/tests/code_health/test_rest_python_groups.py new file mode 100644 index 00000000000..700fa4cd746 --- /dev/null +++ b/policyengine_us/tests/code_health/test_rest_python_groups.py @@ -0,0 +1,333 @@ +"""Each remaining Python test file runs in exactly one Rest group. + +The Rest CI job used to run every Python test outside contrib, microsimulation +and the SPM files in one pytest process. That process peaked at 15.7 GB RSS on +the 16 GB runner (CI run 37198184683), and later runs lost the runner partway +through the step. `make test-other-python-rest` now runs the same files as one +pytest process per group in REST_PYTHON_GROUPS. + +Paths and --ignore flags can drop a file, or run it twice, while every test +still passes. So this guard takes each command from `make -n`, asks pytest +which files it collects, and checks that the groups together collect each file +of the old single command exactly once. The remaining group takes every file +no earlier group holds, so a new test file lands there; the check fails if a +change to the groups leaves one out. + +pytest lists the files here through its own discovery (paths, --ignore, +python_files), but nothing is imported: running this file as a script swaps +each test module for a placeholder. It also skips conftest.py files, because +importing policyengine_us/conftest.py imports the whole model. +test_conftest_files_cannot_change_which_files_are_collected keeps that safe. +""" + +import ast +import json +import os +import re +import shlex +import shutil +import subprocess +import sys +import tempfile +from collections import defaultdict +from fnmatch import fnmatch +from pathlib import Path + +import pytest +import yaml + +REPO = Path(__file__).resolve().parents[3] +TESTS = REPO / "policyengine_us" / "tests" +WORKFLOWS = [REPO / ".github" / "workflows" / name for name in ("pr.yaml", "push.yaml")] +# CI runs this file under `make test-other-python-rest REST_REPORT_DIR=.`, and +# that make exports these. Passed on, they would carry its flags and report +# directory into the dry runs below. +PARENT_MAKE_VARIABLES = ("MAKEFLAGS", "MFLAGS", "MAKELEVEL", "REST_REPORT_DIR") + + +def dry_run(target, *overrides): + """The commands `make ` runs, each split into its arguments.""" + if shutil.which("make") is None: + pytest.skip("make is not installed") + env = {k: v for k, v in os.environ.items() if k not in PARENT_MAKE_VARIABLES} + result = subprocess.run( + ["make", "-n", "--no-print-directory", target, *overrides], + cwd=REPO, + env=env, + capture_output=True, + text=True, + ) + assert result.returncode == 0, result.stdout + result.stderr + return [ + shlex.split(line) + for line in result.stdout.splitlines() + if line.startswith(("pytest ", "/usr/bin/time ")) + ] + + +def collected_files(args, cwd=REPO): + """The test files pytest collects for these arguments, relative to cwd. + + A command that collects nothing (pytest's exit 5) lists no files, which + partition_problems reports by group. + """ + env = {k: v for k, v in os.environ.items() if k != "PYTEST_ADDOPTS"} + with tempfile.TemporaryDirectory() as tmp: + listing = Path(tmp) / "files.json" + result = subprocess.run( + [sys.executable, __file__, str(listing), *args], + cwd=cwd, + env=env, + capture_output=True, + text=True, + ) + assert result.returncode in (0, 5), (args, result.stdout + result.stderr) + return json.loads(listing.read_text()) + + +def partition_problems(reference, groups): + """How the groups' files fail to cover each reference file exactly once.""" + owners = defaultdict(list) + for group, files in groups.items(): + for file in files: + owners[file].append(group) + problems = [ + f"{group} collects no files" for group, files in groups.items() if not files + ] + problems += [ + f"{file} is collected by no group" + for file in sorted(set(reference) - set(owners)) + ] + for file, file_groups in sorted(owners.items()): + if len(file_groups) > 1: + problems.append(f"{file} is collected by {', '.join(file_groups)}") + if file not in reference: + problems.append( + f"{file} is collected by {', '.join(file_groups)} but was not " + "in the old single process" + ) + return problems + + +def rest_steps(workflow): + return yaml.safe_load(workflow.read_text())["jobs"]["Rest"]["steps"] + + +def ci_report_dir(workflow): + """The REST_REPORT_DIR a workflow's Rest job passes to the groups.""" + runs = [ + step["run"] + for step in rest_steps(workflow) + if "make test-other-python-rest" in step.get("run", "") + ] + assert len(runs) == 1, f"{workflow.name}: {len(runs)} steps run the groups" + match = re.search(r"REST_REPORT_DIR=(\S+)", runs[0]) + assert match, f"{workflow.name} runs the groups without REST_REPORT_DIR" + return match.group(1) + + +def ci_uploaded_patterns(workflow): + """The artifact paths a workflow's Rest job uploads.""" + uploads = [ + step + for step in rest_steps(workflow) + if step.get("uses", "").startswith("actions/upload-artifact") + ] + assert len(uploads) == 1, f"{workflow.name}: {len(uploads)} upload steps" + return uploads[0]["with"]["path"].split() + + +def reported_groups(report_dir): + """Each group's pytest arguments and report files, in run order. + + The group name comes from the JUnit file, rest-python-.xml. + """ + groups = {} + for argv in dry_run("test-other-python-rest", f"REST_REPORT_DIR={report_dir}"): + assert argv[:3] == ["/usr/bin/time", "-v", "-o"], argv + assert argv[4] == "pytest", argv + junit = [arg for arg in argv[5:] if arg.startswith("--junitxml=")] + assert len(junit) == 1, argv + xml = junit[0].removeprefix("--junitxml=") + match = re.fullmatch(rf"{re.escape(report_dir)}/rest-python-(.+)\.xml", xml) + assert match, xml + group = match.group(1) + assert group not in groups, f"{group} runs twice" + groups[group] = { + "args": [arg for arg in argv[5:] if arg != junit[0]], + "reports": [xml, argv[3]], + } + return groups + + +@pytest.fixture(scope="module") +def report_dir(): + dirs = {ci_report_dir(workflow) for workflow in WORKFLOWS} + assert len(dirs) == 1, f"The workflows disagree on REST_REPORT_DIR: {dirs}" + return dirs.pop() + + +@pytest.fixture(scope="module") +def groups(report_dir): + return reported_groups(report_dir) + + +@pytest.fixture(scope="module") +def old_command(): + """The pytest arguments of the single process the groups replace. + + It was test-other-python, the Python tests outside contrib and + microsimulation, with the SPM files left to their own step. + """ + (other,) = dry_run("test-other-python") + (spm,) = dry_run("test-other-python-spm") + assert other[0] == spm[0] == "pytest", (other, spm) + spm_files = [arg for arg in spm[1:] if not arg.startswith("-")] + return other[1:] + [f"--ignore={file}" for file in spm_files] + + +def test_groups_collect_each_file_of_the_old_single_process_once(groups, old_command): + reference = collected_files(old_command) + # The reference is a real collection: it holds this file. + assert Path(__file__).relative_to(REPO).as_posix() in reference + collected = {group: collected_files(spec["args"]) for group, spec in groups.items()} + problems = partition_problems(reference, collected) + assert not problems, "\n".join(problems) + + +def test_each_group_writes_reports_the_rest_job_uploads(groups, report_dir): + reports = [report for spec in groups.values() for report in spec["reports"]] + assert len(set(reports)) == len(reports), reports + for workflow in WORKFLOWS: + patterns = [os.path.normpath(p) for p in ci_uploaded_patterns(workflow)] + missing = [ + report + for report in reports + if not any(fnmatch(os.path.normpath(report), p) for p in patterns) + ] + assert not missing, f"{workflow.name} does not upload {missing}" + + +def test_groups_run_as_plain_pytest_without_a_report_dir(groups): + """Locally the same groups run without GNU time, which macOS lacks.""" + plain = dry_run("test-other-python-rest") + assert [argv[0] for argv in plain] == ["pytest"] * len(groups) + assert [argv[1:] for argv in plain] == [spec["args"] for spec in groups.values()] + + +def test_conftest_files_cannot_change_which_files_are_collected(old_command): + """The listing skips conftest.py; that is exact only while no conftest.py + the Rest groups load defines collection hooks, collect_ignore or plugins.""" + ignored = [ + REPO / arg.removeprefix("--ignore=") + for arg in old_command + if arg.startswith("--ignore=") + ] + conftests = [REPO / "conftest.py", REPO / "policyengine_us" / "conftest.py"] + conftests += sorted(TESTS.rglob("conftest.py")) + found = [] + for conftest in conftests: + if not conftest.exists() or any(conftest.is_relative_to(p) for p in ignored): + continue + for node in ast.parse(conftest.read_text()).body: + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + names = [node.name] + elif isinstance(node, ast.Assign): + names = [t.id for t in node.targets if isinstance(t, ast.Name)] + elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): + names = [node.target.id] + elif isinstance(node, (ast.Import, ast.ImportFrom)): + names = [alias.asname or alias.name for alias in node.names] + else: + names = [] + found += [ + f"{conftest.relative_to(REPO)}: {name}" + for name in names + if name.startswith(("pytest_", "collect_ignore")) + ] + assert not found, ( + "These can change which files pytest collects, which the listing in " + f"this guard does not see; make it load conftest files: {found}" + ) + + +def write_test_files(root, names): + for name in names: + path = root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("def test_nothing():\n pass\n") + + +def test_the_check_reports_a_file_outside_every_group(tmp_path): + write_test_files(tmp_path, ["a/test_one.py", "b/test_two.py", "new/test_new.py"]) + reference = collected_files(["."], cwd=tmp_path) + collected = { + "a": collected_files(["a"], cwd=tmp_path), + "b": collected_files(["b"], cwd=tmp_path), + } + assert partition_problems(reference, collected) == [ + "new/test_new.py is collected by no group" + ] + + +def test_the_check_reports_a_file_in_two_groups(tmp_path): + write_test_files(tmp_path, ["a/test_one.py", "b/test_two.py"]) + reference = collected_files([".", "--ignore=b"], cwd=tmp_path) + collected = { + "a": collected_files(["a"], cwd=tmp_path), + "everything": collected_files(["."], cwd=tmp_path), + } + assert partition_problems(reference, collected) == [ + "a/test_one.py is collected by a, everything", + "b/test_two.py is collected by everything but was not in the old " + "single process", + ] + + +# --- Run as a script: list the files pytest collects, importing none --------- + + +class _Unimported(pytest.File): + """A test file pytest would import, standing in as one placeholder item.""" + + def collect(self): + yield _Placeholder.from_parent(self, name=self.path.name) + + +class _Placeholder(pytest.Item): + def runtest(self): + raise AssertionError("listed for collection only") + + +class _FileListing: + def __init__(self): + self.files = [] + + @pytest.hookimpl(tryfirst=True) + def pytest_pycollect_makemodule(self, module_path, parent): + return _Unimported.from_parent(parent, path=module_path) + + def pytest_collection_finish(self, session): + # Read the kept items, not the modules built: for a file argument + # pytest builds a module for each file beside it, then keeps one. + self.files = [ + Path(os.path.relpath(item.path)).as_posix() for item in session.items + ] + + +if __name__ == "__main__": + output, *pytest_args = sys.argv[1:] + listing = _FileListing() + status = pytest.main( + [ + *pytest_args, + "--collect-only", + "-q", + "--noconftest", + "-p", + "no:cacheprovider", + ], + plugins=[listing], + ) + Path(output).write_text(json.dumps(listing.files)) + sys.exit(status) From 210efe11bc2c082f8fd55db666808b03f63c72c9 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 4 Oct 2026 12:52:05 -0400 Subject: [PATCH 2/3] Move the newer heavy core modules into the heavy Rest groups Review of the split found that two core modules added after run 37198184683 are heavy too. Scaled to that run's speed, the timings from runs 37198179476 and 37196338557 put test_marginal_tax_rate_coverage at about 124 s and test_dependent_net_investment_income_invariants at about 64 s. The core group would have reached about 330 s. Move the first into heavy-b and the second into heavy-a. That gives 403 / 350 / 345 / 142 / 185 / 239 s across the six groups. Correction to the previous commit's message: the failed runs did keep logs. Four runs on 2026-10-04 (37201566995, 37201944922, 37202679009, 37205594473) end with "The runner has received a shutdown signal" and make's "test-other-python-rest] Killed". That came after 1,530-1,560 passed tests, inside test_formulas_do_not_write_into_cached_arrays or at the start of test_md_poverty_line_credit_invariants. The Makefile comment now cites them. The guard gains two checks: - It runs the real target with a stub pytest, failing one group and SIGKILLing another. Every group must still run, with the dry run's arguments, and the target must fail, naming both groups. - Test file names must stay unique outside packages. The old single process failed on a clash, and separate groups would not. The changelog now states the memory bound as the aim, since the first CI run will be the first to measure each group. Co-Authored-By: Claude Opus 5.5 --- Makefile | 33 ++++---- changelog.d/ci-split-rest-job.changed.md | 2 +- .../code_health/test_rest_python_groups.py | 75 +++++++++++++++++-- 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/Makefile b/Makefile index 85633803df9..5ce945522c6 100644 --- a/Makefile +++ b/Makefile @@ -23,27 +23,34 @@ REST_SPM_TESTS := $(TESTS)/core/test_spm_policy_family.py \ # The remaining Python tests run as one pytest process per group, one after # another, so each exit releases its heap before the next group starts. As a # single process they peaked at 15.7 GB RSS on the 16 GB runner, with 1.5M -# major page faults (CI run 37198184683), and later runs were cancelled -# mid-step with no log, as runners out of memory are (see test-yaml-reform). +# major page faults (CI run 37198184683). Four runs on 2026-10-04, such as +# 37202679009, then lost the runner 1,530-1,560 tests in, inside +# test_formulas_do_not_write_into_cached_arrays or at the start of +# test_md_poverty_line_credit_invariants: "The runner has received a shutdown +# signal", as test-yaml-reform records for a process past 16 GB. # Each file runs in the first group whose paths hold it: the listed heavy -# modules, then core/, then policy/, and remaining takes every other file, -# including new ones. code_health/test_rest_python_groups.py checks that the -# groups together collect each file of the old single process exactly once. -# Test time per group in that run, 1,453 s in all: +# modules, then core/, then policy/, and remaining takes every other file. +# code_health/test_rest_python_groups.py checks that the groups together +# collect each file of the old single process exactly once. +# Test time per group, from run 37198184683 (1,453 s in all) plus the three +# core modules added since, timed in runs 37171744596, 37196338557 and +# 37198179476 and scaled to the speed of 37198184683: # cached-arrays (403 s): test_formulas_do_not_write_into_cached_arrays. -# heavy-a (286 s): md_poverty_line_credit_invariants (134 s), -# ald_determinism (94 s), ssi_state_supplement_medicaid_dependency (59 s). -# heavy-b (221 s): multi_year_simulation (88 s), -# behavioral_response_measurements (84 s), +# heavy-a (350 s): md_poverty_line_credit_invariants (134 s), +# ald_determinism (94 s), dependent_net_investment_income_invariants +# (64 s), ssi_state_supplement_medicaid_dependency (59 s). +# heavy-b (345 s): marginal_tax_rate_coverage (124 s), +# multi_year_simulation (88 s), behavioral_response_measurements (84 s), # dependent_losses_agi_invariants (49 s). -# core (120 s), policy (185 s), remaining (239 s). core has since gained -# three modules, which that run did not measure. +# core (142 s), policy (185 s), remaining (239 s). REST_PYTHON_GROUPS := cached-arrays heavy-a heavy-b core policy remaining REST_CACHED_ARRAY_TESTS := $(TESTS)/test_formulas_do_not_write_into_cached_arrays.py REST_HEAVY_A_TESTS := $(TESTS)/test_md_poverty_line_credit_invariants.py \ $(TESTS)/core/test_ald_determinism.py \ + $(TESTS)/core/test_dependent_net_investment_income_invariants.py \ $(TESTS)/core/test_ssi_state_supplement_medicaid_dependency.py -REST_HEAVY_B_TESTS := $(TESTS)/core/test_multi_year_simulation.py \ +REST_HEAVY_B_TESTS := $(TESTS)/core/test_marginal_tax_rate_coverage.py \ + $(TESTS)/core/test_multi_year_simulation.py \ $(TESTS)/core/test_behavioral_response_measurements.py \ $(TESTS)/core/test_dependent_losses_agi_invariants.py REST_LISTED_TESTS := $(REST_CACHED_ARRAY_TESTS) $(REST_HEAVY_A_TESTS) $(REST_HEAVY_B_TESTS) diff --git a/changelog.d/ci-split-rest-job.changed.md b/changelog.d/ci-split-rest-job.changed.md index 6e87973a254..a35bc6ee1c3 100644 --- a/changelog.d/ci-split-rest-job.changed.md +++ b/changelog.d/ci-split-rest-job.changed.md @@ -1 +1 @@ -Run the Full Suite - Rest job's remaining Python tests as six sequential pytest processes instead of one, each with its own JUnit and memory report, so no single process approaches the 16 GB runner limit; a code-health test checks that the groups run every test file of the old process exactly once. +Run the Full Suite - Rest job's remaining Python tests as six sequential pytest processes instead of one, each with its own JUnit and memory report, to keep each process well below the 16 GB runner's memory; a code-health test checks that the groups run every test file of the old process exactly once. diff --git a/policyengine_us/tests/code_health/test_rest_python_groups.py b/policyengine_us/tests/code_health/test_rest_python_groups.py index 700fa4cd746..d2203048da3 100644 --- a/policyengine_us/tests/code_health/test_rest_python_groups.py +++ b/policyengine_us/tests/code_health/test_rest_python_groups.py @@ -9,9 +9,10 @@ Paths and --ignore flags can drop a file, or run it twice, while every test still passes. So this guard takes each command from `make -n`, asks pytest which files it collects, and checks that the groups together collect each file -of the old single command exactly once. The remaining group takes every file -no earlier group holds, so a new test file lands there; the check fails if a -change to the groups leaves one out. +of the old single command exactly once. A new test file lands in core or +policy when it sits under those folders and in remaining otherwise; the check +fails if a change to the groups leaves one out. Running the target with a stub +pytest checks that every group runs and that any failure fails the target. pytest lists the files here through its own discovery (paths, --ignore, python_files), but nothing is imported: running this file as a script swaps @@ -41,7 +42,8 @@ WORKFLOWS = [REPO / ".github" / "workflows" / name for name in ("pr.yaml", "push.yaml")] # CI runs this file under `make test-other-python-rest REST_REPORT_DIR=.`, and # that make exports these. Passed on, they would carry its flags and report -# directory into the dry runs below. +# directory into the make runs below, and the stub run would overwrite the +# CI groups' GNU time reports. PARENT_MAKE_VARIABLES = ("MAKEFLAGS", "MFLAGS", "MAKELEVEL", "REST_REPORT_DIR") @@ -186,8 +188,12 @@ def old_command(): return other[1:] + [f"--ignore={file}" for file in spm_files] -def test_groups_collect_each_file_of_the_old_single_process_once(groups, old_command): - reference = collected_files(old_command) +@pytest.fixture(scope="module") +def reference(old_command): + return collected_files(old_command) + + +def test_groups_collect_each_file_of_the_old_single_process_once(groups, reference): # The reference is a real collection: it holds this file. assert Path(__file__).relative_to(REPO).as_posix() in reference collected = {group: collected_files(spec["args"]) for group, spec in groups.items()} @@ -215,6 +221,63 @@ def test_groups_run_as_plain_pytest_without_a_report_dir(groups): assert [argv[1:] for argv in plain] == [spec["args"] for spec in groups.values()] +def test_test_file_names_stay_unique(reference): + """pytest imports a test file outside a package by its file name, so the + old single process failed on two such files with one name ("import file + mismatch"). Split across groups, both would pass; make test-other-python + would still fail.""" + by_name = defaultdict(list) + for file in reference: + if not (REPO / file).parent.joinpath("__init__.py").exists(): + by_name[Path(file).name].append(file) + clashes = {name: files for name, files in by_name.items() if len(files) > 1} + assert not clashes, clashes + + +# Stands in for pytest on PATH: records its arguments, then exits with the +# next outcome in STUB_OUTCOMES, or kills itself with SIGKILL, as the kernel's +# out-of-memory killer does, for "killed". +STUB_PYTEST = """#!/bin/sh +echo "$*" >> "$STUB_CALLS" +n=$(wc -l < "$STUB_CALLS" | tr -d ' ') +outcome=$(echo "$STUB_OUTCOMES" | cut -d ' ' -f "$n") +if [ "$outcome" = killed ]; then kill -9 $$; fi +exit "$outcome" +""" + + +@pytest.mark.parametrize( + "outcomes", + [["0", "1", "0", "killed", "0", "0"], ["0"] * 6], + ids=["one-fails-one-killed", "all-pass"], +) +def test_every_group_runs_and_any_failure_fails_the_target(tmp_path, groups, outcomes): + assert len(groups) == len(outcomes), "give each group one outcome" + stub = tmp_path / "bin" / "pytest" + stub.parent.mkdir() + stub.write_text(STUB_PYTEST) + stub.chmod(0o755) + calls = tmp_path / "calls.txt" + env = {k: v for k, v in os.environ.items() if k not in PARENT_MAKE_VARIABLES} + env["PATH"] = f"{stub.parent}{os.pathsep}{env['PATH']}" + env["STUB_CALLS"] = str(calls) + env["STUB_OUTCOMES"] = " ".join(outcomes) + result = subprocess.run( + ["make", "--no-print-directory", "test-other-python-rest"], + cwd=REPO, + env=env, + capture_output=True, + text=True, + ) + # Every group ran, with the arguments the dry runs above check. + ran = [" ".join(spec["args"]) for spec in groups.values()] + assert calls.read_text().splitlines() == ran + failed = [group for group, outcome in zip(groups, outcomes) if outcome != "0"] + assert (result.returncode != 0) == bool(failed), result.stdout + result.stderr + if failed: + assert f"Failed Rest Python groups: {' '.join(failed)}" in result.stdout + + def test_conftest_files_cannot_change_which_files_are_collected(old_command): """The listing skips conftest.py; that is exact only while no conftest.py the Rest groups load defines collection hooks, collect_ignore or plugins.""" From 9f77f575eb0b7405d0d173e5cf3bb53327f85949 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 4 Oct 2026 13:15:59 -0400 Subject: [PATCH 3/3] Check every Rest group's failure path and pytest's real import names Second-round review of the split: - The stub-pytest test failed only heavy-a and core. A `-` prefix on any other group's recipe would have hidden that group's failures. An all-fail case now exercises every recipe. A mutation check on cached-arrays, heavy-b, policy and remaining failed it, as it should. - The duplicate-name check now compares the module name that pytest's default import mode gives each file: its dotted path inside a package, or its stem outside one. - Correction to the previous commit's message and the Makefile comment: the four runner losses came 1,424-1,454 tests into the test-other-python-rest step. The 1,530-1,560 figure counted the SPM step's 106 tests as well. Co-Authored-By: Claude Opus 5.5 --- Makefile | 2 +- .../code_health/test_rest_python_groups.py | 31 +++++++++++++------ 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index 5ce945522c6..8bee1a07b07 100644 --- a/Makefile +++ b/Makefile @@ -24,7 +24,7 @@ REST_SPM_TESTS := $(TESTS)/core/test_spm_policy_family.py \ # another, so each exit releases its heap before the next group starts. As a # single process they peaked at 15.7 GB RSS on the 16 GB runner, with 1.5M # major page faults (CI run 37198184683). Four runs on 2026-10-04, such as -# 37202679009, then lost the runner 1,530-1,560 tests in, inside +# 37202679009, then lost the runner 1,424-1,454 tests into the step, inside # test_formulas_do_not_write_into_cached_arrays or at the start of # test_md_poverty_line_credit_invariants: "The runner has received a shutdown # signal", as test-yaml-reform records for a process past 16 GB. diff --git a/policyengine_us/tests/code_health/test_rest_python_groups.py b/policyengine_us/tests/code_health/test_rest_python_groups.py index d2203048da3..629e5c2da85 100644 --- a/policyengine_us/tests/code_health/test_rest_python_groups.py +++ b/policyengine_us/tests/code_health/test_rest_python_groups.py @@ -221,15 +221,24 @@ def test_groups_run_as_plain_pytest_without_a_report_dir(groups): assert [argv[1:] for argv in plain] == [spec["args"] for spec in groups.values()] -def test_test_file_names_stay_unique(reference): - """pytest imports a test file outside a package by its file name, so the - old single process failed on two such files with one name ("import file - mismatch"). Split across groups, both would pass; make test-other-python - would still fail.""" +def import_name(file): + """The module name pytest's default import mode gives a test file: its + dotted path within its outermost package, or its stem outside one.""" + path = REPO / file + parts = [path.stem] + while (path.parent / "__init__.py").exists(): + path = path.parent + parts.insert(0, path.name) + return ".".join(parts) + + +def test_test_files_keep_distinct_import_names(reference): + """The old single process failed on two test files with one import name + ("import file mismatch"). Split across groups, both would pass; make + test-other-python would still fail.""" by_name = defaultdict(list) for file in reference: - if not (REPO / file).parent.joinpath("__init__.py").exists(): - by_name[Path(file).name].append(file) + by_name[import_name(file)].append(file) clashes = {name: files for name, files in by_name.items() if len(files) > 1} assert not clashes, clashes @@ -248,8 +257,12 @@ def test_test_file_names_stay_unique(reference): @pytest.mark.parametrize( "outcomes", - [["0", "1", "0", "killed", "0", "0"], ["0"] * 6], - ids=["one-fails-one-killed", "all-pass"], + [ + ["0", "1", "0", "killed", "0", "0"], + ["killed", "1", "1", "1", "1", "1"], + ["0"] * 6, + ], + ids=["one-fails-one-killed", "all-fail", "all-pass"], ) def test_every_group_runs_and_any_failure_fails_the_target(tmp_path, groups, outcomes): assert len(groups) == len(outcomes), "give each group one outcome"