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..8bee1a07b07 100644 --- a/Makefile +++ b/Makefile @@ -16,10 +16,53 @@ 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). Four runs on 2026-10-04, such as +# 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. +# 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. +# 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 (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 (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_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) +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 +228,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..a35bc6ee1c3 --- /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, 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 new file mode 100644 index 00000000000..629e5c2da85 --- /dev/null +++ b/policyengine_us/tests/code_health/test_rest_python_groups.py @@ -0,0 +1,409 @@ +"""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. 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 +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 make runs below, and the stub run would overwrite the +# CI groups' GNU time reports. +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] + + +@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()} + 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 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: + 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 + + +# 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"], + ["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" + 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.""" + 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)