From 08e98c663a5fff264d003a527c9221225a727c72 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 13 Oct 2026 02:32:00 -0400 Subject: [PATCH 1/3] Split Rest CI into three Python shards and variables YAML --- .github/workflows/push.yaml | 105 +++++- .github/workflows/python-tests.yaml | 100 +++++- Makefile | 21 +- .../ci-rest-parallel-headroom.fixed.md | 1 + .../code_health/test_rest_python_groups.py | 314 +++++++++++++++--- 5 files changed, 458 insertions(+), 83 deletions(-) create mode 100644 changelog.d/ci-rest-parallel-headroom.fixed.md diff --git a/.github/workflows/push.yaml b/.github/workflows/push.yaml index a05fc24f19b..c24e83c21b8 100644 --- a/.github/workflows/push.yaml +++ b/.github/workflows/push.yaml @@ -310,20 +310,33 @@ jobs: - name: Run Contrib Python tests if: ${{ matrix.group == 'other-shard-1' }} run: uv run --no-sync make test-policy-contrib-python - Rest: + RestPython: needs: ReleaseLock - name: Full Suite - Rest (Python + variables) + name: Full Suite - Rest (Python, ${{ matrix.group }}) runs-on: ubuntu-latest - # Matches the PR Rest job (python-tests.yaml, raised to 90 in #9785). - # Oct 6-8 on push: p50 51 min, max success 58.8 min, and one run hit the - # old 60-minute limit (37719726248), which skipped that version's Publish. - timeout-minutes: 90 + # Existing group timings predict 20-32 minutes per shard plus setup. + timeout-minutes: 60 if: | (github.repository == 'PolicyEngine/policyengine-us') && (github.event.head_commit.message == 'Update PolicyEngine US') env: PYTHONUNBUFFERED: 1 - PYTEST_ADDOPTS: "--durations=25 -p no:unraisableexception -p no:threadexception" + OMP_NUM_THREADS: 4 + OPENBLAS_NUM_THREADS: 4 + MKL_NUM_THREADS: 4 + VECLIB_MAXIMUM_THREADS: 4 + NUMEXPR_NUM_THREADS: 4 + OMP_WAIT_POLICY: PASSIVE + strategy: + fail-fast: false + matrix: + include: + - group: shard-1 + shard: 1 + - group: shard-2 + shard: 2 + - group: shard-3 + shard: 3 steps: - name: Checkout repo uses: actions/checkout@v6 @@ -342,30 +355,90 @@ jobs: - name: Turn off default branching shell: bash run: bash ./update_itemization.sh - # Each Rest group runs in a separate process to release its heap. - # SPM simulation/API contracts run in the Microsimulation job instead. + # Each group remains a separate, sequential process on its runner. + # SPM simulation/API contracts stay in the Microsimulation job. - name: Run remaining Python tests - # Continue after failures, but stop when the workflow is cancelled. if: ${{ !cancelled() }} env: PYTEST_ADDOPTS: "--durations=0 -v -p no:unraisableexception -p no:threadexception" - run: uv run --no-sync make test-other-python-rest REST_REPORT_DIR=. + run: uv run --no-sync make test-other-python-rest REST_PYTHON_SHARD=${{ matrix.shard }} REST_REPORT_DIR=. + - name: Upload Rest Python timing and memory reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: rest-python-test-timings-${{ matrix.group }} + path: | + rest-python-*.xml + rest-python-*-resources.txt + RestVariables: + needs: ReleaseLock + name: Full Suite - Rest (variables YAML) + runs-on: ubuntu-latest + # Recent runs took 47-66 seconds, so this suite needs no further shards. + timeout-minutes: 30 + if: | + (github.repository == 'PolicyEngine/policyengine-us') + && (github.event.head_commit.message == 'Update PolicyEngine US') + env: + PYTHONUNBUFFERED: 1 + OMP_NUM_THREADS: 4 + OPENBLAS_NUM_THREADS: 4 + MKL_NUM_THREADS: 4 + VECLIB_MAXIMUM_THREADS: 4 + NUMEXPR_NUM_THREADS: 4 + OMP_WAIT_POLICY: PASSIVE + steps: + - name: Checkout repo + uses: actions/checkout@v6 + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: 3.14 + - name: Install uv + uses: astral-sh/setup-uv@v8.1.0 + with: + version: "0.12.13" + - name: Resolve latest compatible PolicyEngine Core + run: uv lock --upgrade-package policyengine-core + - name: Install dependencies + run: uv sync --locked --extra dev + - name: Turn off default branching + shell: bash + run: bash ./update_itemization.sh - name: Run tests/variables YAML tests - # Continue after failures, but stop when the workflow is cancelled. if: ${{ !cancelled() }} env: PYTEST_ADDOPTS: "--durations=0 --junitxml=rest-variables.xml -p no:unraisableexception -p no:threadexception" run: /usr/bin/time -v -o rest-variables-resources.txt uv run --no-sync make test-yaml-variables - - name: Upload Rest timing and memory reports + - name: Upload Rest variables timing and memory reports if: always() uses: actions/upload-artifact@v4 with: - name: rest-test-timings + name: rest-variables-test-timings path: | - rest-python-*.xml rest-variables.xml - rest-python-*-resources.txt rest-variables-resources.txt + Rest: + # Retain the old check name and the release Publish dependency. + name: Full Suite - Rest (Python + variables) + needs: [RestPython, RestVariables] + if: | + always() + && (github.repository == 'PolicyEngine/policyengine-us') + && (github.event.head_commit.message == 'Update PolicyEngine US') + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Check all Rest jobs succeeded + shell: bash + env: + PYTHON_RESULT: ${{ needs.RestPython.result }} + VARIABLES_RESULT: ${{ needs.RestVariables.result }} + run: | + if [[ "$PYTHON_RESULT" != "success" || "$VARIABLES_RESULT" != "success" ]]; then + echo "::error::Rest jobs did not all succeed: Python=$PYTHON_RESULT, variables=$VARIABLES_RESULT" + exit 1 + fi Microsimulation: needs: ReleaseLock name: Full Suite - Microsimulation diff --git a/.github/workflows/python-tests.yaml b/.github/workflows/python-tests.yaml index 034aa44b16d..b13200db8c9 100644 --- a/.github/workflows/python-tests.yaml +++ b/.github/workflows/python-tests.yaml @@ -1,19 +1,32 @@ name: Python tests on: workflow_call: -# Group these jobs in the Actions view while retaining two independent runners. +# Keep each Python shard and the variables suite on independent runners. # Only the guarded PR workflow calls this file; it has no automatic triggers. jobs: - Rest: - name: Full Suite - Rest (Python + variables) + RestPython: + name: Full Suite - Rest (Python, ${{ matrix.group }}) runs-on: ubuntu-latest - # Passing runs on 2026-10-04 took 42-55 minutes (#9653, #9766, #9769, - # #9777, #9784), so a 60-minute limit cancels PRs that add a few - # minutes of tests (#9695 was cancelled at 60 minutes twice). - timeout-minutes: 90 + # Existing group timings predict 20-32 minutes per shard plus setup. + timeout-minutes: 60 env: PYTHONUNBUFFERED: 1 - PYTEST_ADDOPTS: "--durations=25 -p no:unraisableexception -p no:threadexception" + OMP_NUM_THREADS: 4 + OPENBLAS_NUM_THREADS: 4 + MKL_NUM_THREADS: 4 + VECLIB_MAXIMUM_THREADS: 4 + NUMEXPR_NUM_THREADS: 4 + OMP_WAIT_POLICY: PASSIVE + strategy: + fail-fast: false + matrix: + include: + - group: shard-1 + shard: 1 + - group: shard-2 + shard: 2 + - group: shard-3 + shard: 3 steps: - name: Checkout repo uses: actions/checkout@v6 @@ -32,30 +45,83 @@ jobs: - name: Turn off default branching shell: bash run: bash ./update_itemization.sh - # Each Rest group runs in a separate process to release its heap. - # SPM simulation/API contracts run in the Microsimulation job instead. + # Each group remains a separate, sequential process on its runner. + # SPM simulation/API contracts stay in the Microsimulation job. - name: Run remaining Python tests - # Continue after failures, but stop when the workflow is cancelled. if: ${{ !cancelled() }} env: PYTEST_ADDOPTS: "--durations=0 -v -p no:unraisableexception -p no:threadexception" - run: uv run --no-sync make test-other-python-rest REST_REPORT_DIR=. + run: uv run --no-sync make test-other-python-rest REST_PYTHON_SHARD=${{ matrix.shard }} REST_REPORT_DIR=. + - name: Upload Rest Python timing and memory reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: rest-python-test-timings-${{ matrix.group }} + path: | + rest-python-*.xml + rest-python-*-resources.txt + RestVariables: + name: Full Suite - Rest (variables YAML) + runs-on: ubuntu-latest + # Recent runs took 47-66 seconds, so this suite needs no further shards. + timeout-minutes: 30 + env: + PYTHONUNBUFFERED: 1 + OMP_NUM_THREADS: 4 + OPENBLAS_NUM_THREADS: 4 + MKL_NUM_THREADS: 4 + VECLIB_MAXIMUM_THREADS: 4 + NUMEXPR_NUM_THREADS: 4 + OMP_WAIT_POLICY: PASSIVE + steps: + - name: Checkout repo + uses: actions/checkout@v6 + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: 3.14 + - name: Install uv + uses: astral-sh/setup-uv@v8.1.0 + with: + version: "0.12.13" + - name: Resolve latest compatible PolicyEngine Core + run: uv lock --upgrade-package policyengine-core + - name: Install dependencies + run: uv sync --locked --extra dev + - name: Turn off default branching + shell: bash + run: bash ./update_itemization.sh - name: Run tests/variables YAML tests - # Continue after failures, but stop when the workflow is cancelled. if: ${{ !cancelled() }} env: PYTEST_ADDOPTS: "--durations=0 --junitxml=rest-variables.xml -p no:unraisableexception -p no:threadexception" run: /usr/bin/time -v -o rest-variables-resources.txt uv run --no-sync make test-yaml-variables - - name: Upload Rest timing and memory reports + - name: Upload Rest variables timing and memory reports if: always() uses: actions/upload-artifact@v4 with: - name: rest-test-timings + name: rest-variables-test-timings path: | - rest-python-*.xml rest-variables.xml - rest-python-*-resources.txt rest-variables-resources.txt + Rest: + # Retain the old check name and the release Publish dependency. + name: Full Suite - Rest (Python + variables) + needs: [RestPython, RestVariables] + if: ${{ always() }} + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Check all Rest jobs succeeded + shell: bash + env: + PYTHON_RESULT: ${{ needs.RestPython.result }} + VARIABLES_RESULT: ${{ needs.RestVariables.result }} + run: | + if [[ "$PYTHON_RESULT" != "success" || "$VARIABLES_RESULT" != "success" ]]; then + echo "::error::Rest jobs did not all succeed: Python=$PYTHON_RESULT, variables=$VARIABLES_RESULT" + exit 1 + fi Microsimulation: name: Full Suite - Microsimulation runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 481a6cdb1ff..c19c5415bc2 100644 --- a/Makefile +++ b/Makefile @@ -43,8 +43,25 @@ REST_SPM_TESTS := $(TESTS)/core/test_spm_policy_family.py \ # in its grid fixture. Core reached 13.3 GB in CI run 38141803955 after this # and three smaller modules arrived; isolate SSI rather than filling heavy-a # (8.1 GB including the pending capital-gains MTR change). These groups remain -# sequential subprocesses on the existing Rest runner. +# sequential subprocesses within each Rest Python shard. REST_PYTHON_GROUPS := cached-arrays heavy-a heavy-b core-heavy core policy-parameters policy remaining-heavy remaining +# CI runs three shards of these existing isolated processes. Across seven +# recent runs (including 38326922385 and 38289790999), their summed group +# times were 23-32, 20-30 and 22-29 minutes; the old serial job hit 90 minutes. +# No shard runs more than one Python group at a time. Locally, leaving +# REST_PYTHON_SHARD unset still runs every group in the original order. +REST_PYTHON_SHARD_1_GROUPS := cached-arrays core-heavy core +REST_PYTHON_SHARD_2_GROUPS := heavy-a policy-parameters remaining +REST_PYTHON_SHARD_3_GROUPS := heavy-b policy remaining-heavy +REST_PYTHON_SHARD ?= +ifeq ($(REST_PYTHON_SHARD),) +REST_PYTHON_SELECTED_GROUPS := $(REST_PYTHON_GROUPS) +else +REST_PYTHON_SELECTED_GROUPS := $(REST_PYTHON_SHARD_$(REST_PYTHON_SHARD)_GROUPS) +ifeq ($(strip $(REST_PYTHON_SELECTED_GROUPS)),) +$(error REST_PYTHON_SHARD must be 1, 2 or 3) +endif +endif 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 \ @@ -281,7 +298,7 @@ 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: - @failed=""; for group in $(REST_PYTHON_GROUPS); do \ + @failed=""; for group in $(REST_PYTHON_SELECTED_GROUPS); do \ echo "=== Rest Python group: $$group ==="; \ $(MAKE) --no-print-directory test-other-python-rest-$$group || failed="$$failed $$group"; \ done; \ diff --git a/changelog.d/ci-rest-parallel-headroom.fixed.md b/changelog.d/ci-rest-parallel-headroom.fixed.md new file mode 100644 index 00000000000..740326c17df --- /dev/null +++ b/changelog.d/ci-rest-parallel-headroom.fixed.md @@ -0,0 +1 @@ +Split the Rest CI suite into three isolated Python shards and a variables YAML job to avoid false failures at the 90-minute limit, retaining the aggregate check and timing reports. 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 da2a2045a57..28e2c46a1b2 100644 --- a/policyengine_us/tests/code_health/test_rest_python_groups.py +++ b/policyengine_us/tests/code_health/test_rest_python_groups.py @@ -1,18 +1,21 @@ -"""Each remaining Python test file runs in exactly one Rest group. +"""Each remaining Python test file runs in exactly one Rest group and shard. 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. +pytest process per group in REST_PYTHON_GROUPS. CI assigns those groups to +parallel Python shards, with the variables YAML in a separate job. 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. +still passes. So this guard expands the workflows' matrix commands through +`make -n`, asks pytest which files each group collects, and checks that each +file of the old single command runs 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 or shards leaves one out. Running +the target with a stub pytest checks that every group runs and that any +failure fails the target. The original Rest check aggregates all shard +results, and must fail on failed, cancelled or skipped jobs. pytest lists the files here through its own discovery (paths, --ignore, python_files), but nothing is imported: running this file as a script swaps @@ -42,14 +45,20 @@ WORKFLOWS = [ REPO / ".github" / "workflows" / name for name in ("python-tests.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): +# CI runs this file under a shard of `make test-other-python-rest`, and that +# make exports these. Passed on, they would restrict the guard's reference to +# one shard, or carry its report directory into the stub run and overwrite +# the CI groups' GNU time reports. +PARENT_MAKE_VARIABLES = ( + "MAKEFLAGS", + "MFLAGS", + "MAKELEVEL", + "REST_REPORT_DIR", + "REST_PYTHON_SHARD", +) + + +def dry_run(target, *overrides, prefixes=("pytest ", "/usr/bin/time ")): """The commands `make ` runs, each split into its arguments.""" if shutil.which("make") is None: pytest.skip("make is not installed") @@ -65,7 +74,7 @@ def dry_run(target, *overrides): return [ shlex.split(line) for line in result.stdout.splitlines() - if line.startswith(("pytest ", "/usr/bin/time ")) + if line.startswith(prefixes) ] @@ -113,41 +122,89 @@ def partition_problems(reference, groups): return problems -def rest_steps(workflow): - return yaml.safe_load(workflow.read_text())["jobs"]["Rest"]["steps"] +def workflow_jobs(workflow): + return yaml.safe_load(workflow.read_text())["jobs"] -def ci_report_dir(workflow): - """The REST_REPORT_DIR a workflow's Rest job passes to the groups.""" +def rest_steps(workflow, job="RestPython"): + return workflow_jobs(workflow)[job]["steps"] + + +def expanded_matrix(text, entry): + """Resolve the matrix fields GitHub substitutes before running a step.""" + return re.sub( + r"\$\{\{\s*matrix\.(\w+)\s*\}\}", + lambda match: str(entry[match.group(1)]), + text, + ) + + +def ci_python_shards(workflow): + """The real Make overrides and artifact configuration of every shard.""" + jobs = workflow_jobs(workflow) + calls = [ + name + for name, config in jobs.items() + for step in config.get("steps", []) + if "make test-other-python-rest" in step.get("run", "") + ] + assert calls == ["RestPython"], (workflow.name, calls) + job = jobs["RestPython"] + matrix = job["strategy"]["matrix"] + assert set(matrix) == {"include"}, matrix + assert job["strategy"]["fail-fast"] is False runs = [ step["run"] - for step in rest_steps(workflow) + for step in job["steps"] 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 = [ + (upload,) = [ step - for step in rest_steps(workflow) + for step in job["steps"] 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() + shards = {} + for entry in matrix["include"]: + name = entry["group"] + assert name not in shards, f"{workflow.name}: {name} runs twice" + argv = shlex.split(expanded_matrix(runs[0], entry)) + assert argv[:5] == ["uv", "run", "--no-sync", "make", "test-other-python-rest"] + assert any(arg.startswith("REST_PYTHON_SHARD=") for arg in argv[5:]), argv + assert all(arg.startswith("REST_") and "=" in arg for arg in argv[5:]), argv + shards[name] = { + "overrides": argv[5:], + "artifact": expanded_matrix(upload["with"]["name"], entry), + "patterns": upload["with"]["path"].split(), + } + assert shards, f"{workflow.name} has no Python shards" + return shards + +def ci_report_dir(workflow): + """The REST_REPORT_DIR each workflow shard passes to the groups.""" + dirs = [] + for shard in ci_python_shards(workflow).values(): + values = [ + arg.removeprefix("REST_REPORT_DIR=") + for arg in shard["overrides"] + if arg.startswith("REST_REPORT_DIR=") + ] + assert len(values) == 1, f"{workflow.name} runs without one REST_REPORT_DIR" + dirs.extend(values) + assert len(set(dirs)) == 1, dirs + return dirs[0] -def reported_groups(report_dir): + +def reported_groups(report_dir, *overrides): """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}"): + for argv in dry_run( + "test-other-python-rest", f"REST_REPORT_DIR={report_dir}", *overrides + ): 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=")] @@ -195,35 +252,173 @@ def reference(old_command): return collected_files(old_command) -def test_groups_collect_each_file_of_the_old_single_process_once(groups, reference): +@pytest.fixture(scope="module") +def group_files(groups): + return {group: collected_files(spec["args"]) for group, spec in groups.items()} + + +def test_groups_collect_each_file_of_the_old_single_process_once( + group_files, 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) + problems = partition_problems(reference, group_files) 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)] +@pytest.mark.parametrize("workflow", WORKFLOWS, ids=lambda path: path.name) +def test_ci_shards_run_each_group_and_each_python_file_once( + workflow, groups, group_files, reference, report_dir +): + selected = {} + for shard, config in ci_python_shards(workflow).items(): + shard_groups = reported_groups(report_dir, *config["overrides"]) + assert shard_groups, f"{workflow.name}: {shard} runs no groups" + for group, spec in shard_groups.items(): + assert group in groups, f"{workflow.name}: unknown group {group}" + assert spec == groups[group], (workflow.name, shard, group, spec) + selected[f"{shard}/{group}"] = group_files[group] + problems = partition_problems(reference, selected) + assert not problems, f"{workflow.name}:\n" + "\n".join(problems) + + +@pytest.mark.parametrize("workflow", WORKFLOWS, ids=lambda path: path.name) +def test_each_group_writes_reports_its_ci_shard_uploads(workflow, report_dir): + artifacts = [] + reports = [] + for config in ci_python_shards(workflow).values(): + artifacts.append(config["artifact"]) + shard_reports = [ + report + for spec in reported_groups(report_dir, *config["overrides"]).values() + for report in spec["reports"] + ] + reports.extend(shard_reports) + patterns = [os.path.normpath(p) for p in config["patterns"]] missing = [ report - for report in reports + for report in shard_reports if not any(fnmatch(os.path.normpath(report), p) for p in patterns) ] assert not missing, f"{workflow.name} does not upload {missing}" - steps = yaml.safe_load(workflow.read_text())["jobs"]["Rest"]["steps"] - for name in ("Run remaining Python tests", "Run tests/variables YAML tests"): - (step,) = [step for step in steps if step.get("name") == name] - assert step["if"] == "${{ !cancelled() }}" + assert len(set(reports)) == len(reports), reports + variable_uploads = [ + step + for step in rest_steps(workflow, "RestVariables") + if step.get("uses", "").startswith("actions/upload-artifact") + ] + assert len(variable_uploads) == 1 + artifacts.append(variable_uploads[0]["with"]["name"]) + assert len(set(artifacts)) == len(artifacts), artifacts + for job in ("RestPython", "RestVariables"): + steps = rest_steps(workflow, job) + target = { + "RestPython": "test-other-python-rest", + "RestVariables": "test-yaml-variables", + }[job] + (run,) = [step for step in steps if f"make {target}" in step.get("run", "")] + assert run["if"] == "${{ !cancelled() }}" (upload,) = [ step for step in steps if step.get("uses", "").startswith("actions/upload-artifact") ] assert upload["if"] == "always()" + assert not any(step.get("continue-on-error", False) for step in steps) + + +@pytest.mark.parametrize("workflow", WORKFLOWS, ids=lambda path: path.name) +def test_variable_yaml_selection_runs_once_in_its_own_job(workflow): + jobs = workflow_jobs(workflow) + calls = [ + (job, step) + for job, config in jobs.items() + for step in config.get("steps", []) + if "make test-yaml-variables" in step.get("run", "") + ] + assert len(calls) == 1, (workflow.name, calls) + job, step = calls[0] + assert job == "RestVariables" + assert "strategy" not in jobs[job], "variables YAML must run exactly once" + assert shlex.split(step["run"]) == [ + "/usr/bin/time", + "-v", + "-o", + "rest-variables-resources.txt", + "uv", + "run", + "--no-sync", + "make", + "test-yaml-variables", + ] + (command,) = dry_run("test-yaml-variables", prefixes=("python ",)) + assert command == [ + "python", + "policyengine_us/tests/test_batched.py", + "policyengine_us/tests/variables", + "--batches", + "1", + ] + assert not any( + "make test-other-python" in candidate.get("run", "") + for candidate in jobs[job]["steps"] + ) + assert not any( + "make test-yaml-variables" in candidate.get("run", "") + for candidate in rest_steps(workflow) + ) + (upload,) = [ + candidate + for candidate in jobs[job]["steps"] + if candidate.get("uses", "").startswith("actions/upload-artifact") + ] + junit = [ + arg.removeprefix("--junitxml=") + for arg in shlex.split(step["env"]["PYTEST_ADDOPTS"]) + if arg.startswith("--junitxml=") + ] + assert junit == ["rest-variables.xml"] + patterns = upload["with"]["path"].split() + assert all( + any(fnmatch(report, pattern) for pattern in patterns) + for report in [junit[0], "rest-variables-resources.txt"] + ) + + +@pytest.mark.parametrize("workflow", WORKFLOWS, ids=lambda path: path.name) +def test_original_rest_check_aggregates_all_shards_without_hiding_failure(workflow): + jobs = workflow_jobs(workflow) + summary = jobs["Rest"] + assert summary["name"] == "Full Suite - Rest (Python + variables)" + assert set(summary["needs"]) == {"RestPython", "RestVariables"} + assert "always()" in summary["if"] + assert "strategy" not in summary + assert not summary.get("continue-on-error", False) + if workflow.name == "push.yaml": + assert "Rest" in jobs["Publish"]["needs"] + assert len(summary["steps"]) == 1 + (step,) = summary["steps"] + assert step["env"] == { + "PYTHON_RESULT": "${{ needs.RestPython.result }}", + "VARIABLES_RESULT": "${{ needs.RestVariables.result }}", + } + assert not step.get("continue-on-error", False) + for python in ("success", "failure", "cancelled", "skipped"): + for variables in ("success", "failure", "cancelled", "skipped"): + env = dict(os.environ, PYTHON_RESULT=python, VARIABLES_RESULT=variables) + result = subprocess.run( + ["bash", "-e", "-c", step["run"]], + cwd=REPO, + env=env, + capture_output=True, + text=True, + ) + assert (result.returncode == 0) == (python == variables == "success"), ( + workflow.name, + python, + variables, + result.stdout + result.stderr, + ) @pytest.mark.parametrize("workflow", WORKFLOWS, ids=lambda path: path.name) @@ -278,6 +473,29 @@ 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_inherited_ci_shard_does_not_restrict_the_reference(monkeypatch, groups): + monkeypatch.setenv("REST_PYTHON_SHARD", "2") + monkeypatch.setenv("REST_REPORT_DIR", "inherited-reports") + plain = dry_run("test-other-python-rest") + assert [argv[1:] for argv in plain] == [spec["args"] for spec in groups.values()] + + +@pytest.mark.parametrize("shard", ["0", "4", "unknown"]) +def test_invalid_shard_cannot_pass_without_running_any_groups(shard): + 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", "test-other-python-rest", f"REST_PYTHON_SHARD={shard}"], + cwd=REPO, + env=env, + capture_output=True, + text=True, + ) + assert result.returncode != 0 + assert "REST_PYTHON_SHARD must be 1, 2 or 3" in result.stderr + + 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.""" From 82c148df8b1d420c6bd09b3fc381b5b3fd275f8e Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 13 Oct 2026 12:29:39 -0400 Subject: [PATCH 2/3] Keep the Rest CI split within a 27-runner PR budget --- .github/workflows/push.yaml | 82 +++++----- .github/workflows/python-tests.yaml | 77 +++++----- Makefile | 12 +- .../ci-rest-parallel-headroom.fixed.md | 2 +- .../code_health/test_pr_runner_budget.py | 21 ++- .../code_health/test_rest_python_groups.py | 144 ++++++++++-------- 6 files changed, 185 insertions(+), 153 deletions(-) diff --git a/.github/workflows/push.yaml b/.github/workflows/push.yaml index c24e83c21b8..f576cecb456 100644 --- a/.github/workflows/push.yaml +++ b/.github/workflows/push.yaml @@ -312,9 +312,9 @@ jobs: run: uv run --no-sync make test-policy-contrib-python RestPython: needs: ReleaseLock - name: Full Suite - Rest (Python, ${{ matrix.group }}) + name: Full Suite - Rest (Python, shard-1 + variables) runs-on: ubuntu-latest - # Existing group timings predict 20-32 minutes per shard plus setup. + # Historical group times, including variables: 33-46 minutes plus setup. timeout-minutes: 60 if: | (github.repository == 'PolicyEngine/policyengine-us') @@ -327,16 +327,6 @@ jobs: VECLIB_MAXIMUM_THREADS: 4 NUMEXPR_NUM_THREADS: 4 OMP_WAIT_POLICY: PASSIVE - strategy: - fail-fast: false - matrix: - include: - - group: shard-1 - shard: 1 - - group: shard-2 - shard: 2 - - group: shard-3 - shard: 3 steps: - name: Checkout repo uses: actions/checkout@v6 @@ -361,23 +351,38 @@ jobs: if: ${{ !cancelled() }} env: PYTEST_ADDOPTS: "--durations=0 -v -p no:unraisableexception -p no:threadexception" - run: uv run --no-sync make test-other-python-rest REST_PYTHON_SHARD=${{ matrix.shard }} REST_REPORT_DIR=. + run: uv run --no-sync make test-other-python-rest REST_PYTHON_SHARD=1 REST_REPORT_DIR=. - name: Upload Rest Python timing and memory reports if: always() uses: actions/upload-artifact@v4 with: - name: rest-python-test-timings-${{ matrix.group }} + name: rest-python-test-timings-shard-1 path: | rest-python-*.xml rest-python-*-resources.txt - RestVariables: - needs: ReleaseLock - name: Full Suite - Rest (variables YAML) + - name: Run tests/variables YAML tests + if: ${{ !cancelled() }} + env: + PYTEST_ADDOPTS: "--durations=0 --junitxml=rest-variables.xml -p no:unraisableexception -p no:threadexception" + run: /usr/bin/time -v -o rest-variables-resources.txt uv run --no-sync make test-yaml-variables + - name: Upload Rest variables timing and memory reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: rest-variables-test-timings + path: | + rest-variables.xml + rest-variables-resources.txt + Rest: + needs: [RestPython] + name: Full Suite - Rest (Python + variables) runs-on: ubuntu-latest - # Recent runs took 47-66 seconds, so this suite needs no further shards. - timeout-minutes: 30 + # Historical group times: 32-46 minutes plus setup. + # This runner also retains the legacy summary check, keeping PRs at 27 runners. + timeout-minutes: 60 if: | - (github.repository == 'PolicyEngine/policyengine-us') + always() + && (github.repository == 'PolicyEngine/policyengine-us') && (github.event.head_commit.message == 'Update PolicyEngine US') env: PYTHONUNBUFFERED: 1 @@ -405,38 +410,31 @@ jobs: - name: Turn off default branching shell: bash run: bash ./update_itemization.sh - - name: Run tests/variables YAML tests + # Each group remains a separate, sequential process on its runner. + # SPM simulation/API contracts stay in the Microsimulation job. + - name: Run remaining Python tests + id: python_tests if: ${{ !cancelled() }} env: - PYTEST_ADDOPTS: "--durations=0 --junitxml=rest-variables.xml -p no:unraisableexception -p no:threadexception" - run: /usr/bin/time -v -o rest-variables-resources.txt uv run --no-sync make test-yaml-variables - - name: Upload Rest variables timing and memory reports + PYTEST_ADDOPTS: "--durations=0 -v -p no:unraisableexception -p no:threadexception" + run: uv run --no-sync make test-other-python-rest REST_PYTHON_SHARD=2 REST_REPORT_DIR=. + - name: Upload Rest Python timing and memory reports if: always() uses: actions/upload-artifact@v4 with: - name: rest-variables-test-timings + name: rest-python-test-timings-shard-2 path: | - rest-variables.xml - rest-variables-resources.txt - Rest: - # Retain the old check name and the release Publish dependency. - name: Full Suite - Rest (Python + variables) - needs: [RestPython, RestVariables] - if: | - always() - && (github.repository == 'PolicyEngine/policyengine-us') - && (github.event.head_commit.message == 'Update PolicyEngine US') - runs-on: ubuntu-latest - timeout-minutes: 5 - steps: + rest-python-*.xml + rest-python-*-resources.txt - name: Check all Rest jobs succeeded + if: always() shell: bash env: PYTHON_RESULT: ${{ needs.RestPython.result }} - VARIABLES_RESULT: ${{ needs.RestVariables.result }} + SHARD_RESULT: ${{ steps.python_tests.outcome }} run: | - if [[ "$PYTHON_RESULT" != "success" || "$VARIABLES_RESULT" != "success" ]]; then - echo "::error::Rest jobs did not all succeed: Python=$PYTHON_RESULT, variables=$VARIABLES_RESULT" + if [[ "$PYTHON_RESULT" != "success" || "$SHARD_RESULT" != "success" ]]; then + echo "::error::Rest shards did not all succeed: shard-1=$PYTHON_RESULT, shard-2=$SHARD_RESULT" exit 1 fi Microsimulation: @@ -496,7 +494,7 @@ jobs: if: | (github.repository == 'PolicyEngine/policyengine-us') && (github.event.head_commit.message == 'Update PolicyEngine US') - needs: [ReleaseLock, ReleaseLockTests, Baseline, HouseholdAPIPartners, Contrib, Rest, Microsimulation] + needs: [ReleaseLock, ReleaseLockTests, Baseline, HouseholdAPIPartners, Contrib, RestPython, Rest, Microsimulation] # PyPI trusted publishing: PyPI accepts uploads only from this workflow in # the pypi environment, which is limited to main. No stored PyPI token. environment: pypi diff --git a/.github/workflows/python-tests.yaml b/.github/workflows/python-tests.yaml index b13200db8c9..ab34743c7fc 100644 --- a/.github/workflows/python-tests.yaml +++ b/.github/workflows/python-tests.yaml @@ -1,13 +1,14 @@ name: Python tests on: workflow_call: -# Keep each Python shard and the variables suite on independent runners. +# Two sequential Rest runners keep each shard below its timeout. +# Shard 1 includes variables; shard 2 also supplies the legacy summary check. # Only the guarded PR workflow calls this file; it has no automatic triggers. jobs: RestPython: - name: Full Suite - Rest (Python, ${{ matrix.group }}) + name: Full Suite - Rest (Python, shard-1 + variables) runs-on: ubuntu-latest - # Existing group timings predict 20-32 minutes per shard plus setup. + # Historical group times, including variables: 33-46 minutes plus setup. timeout-minutes: 60 env: PYTHONUNBUFFERED: 1 @@ -17,16 +18,6 @@ jobs: VECLIB_MAXIMUM_THREADS: 4 NUMEXPR_NUM_THREADS: 4 OMP_WAIT_POLICY: PASSIVE - strategy: - fail-fast: false - matrix: - include: - - group: shard-1 - shard: 1 - - group: shard-2 - shard: 2 - - group: shard-3 - shard: 3 steps: - name: Checkout repo uses: actions/checkout@v6 @@ -51,20 +42,36 @@ jobs: if: ${{ !cancelled() }} env: PYTEST_ADDOPTS: "--durations=0 -v -p no:unraisableexception -p no:threadexception" - run: uv run --no-sync make test-other-python-rest REST_PYTHON_SHARD=${{ matrix.shard }} REST_REPORT_DIR=. + run: uv run --no-sync make test-other-python-rest REST_PYTHON_SHARD=1 REST_REPORT_DIR=. - name: Upload Rest Python timing and memory reports if: always() uses: actions/upload-artifact@v4 with: - name: rest-python-test-timings-${{ matrix.group }} + name: rest-python-test-timings-shard-1 path: | rest-python-*.xml rest-python-*-resources.txt - RestVariables: - name: Full Suite - Rest (variables YAML) + - name: Run tests/variables YAML tests + if: ${{ !cancelled() }} + env: + PYTEST_ADDOPTS: "--durations=0 --junitxml=rest-variables.xml -p no:unraisableexception -p no:threadexception" + run: /usr/bin/time -v -o rest-variables-resources.txt uv run --no-sync make test-yaml-variables + - name: Upload Rest variables timing and memory reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: rest-variables-test-timings + path: | + rest-variables.xml + rest-variables-resources.txt + Rest: + needs: [RestPython] + if: ${{ always() }} + name: Full Suite - Rest (Python + variables) runs-on: ubuntu-latest - # Recent runs took 47-66 seconds, so this suite needs no further shards. - timeout-minutes: 30 + # Historical group times: 32-46 minutes plus setup. + # This runner also retains the legacy summary check, keeping PRs at 27 runners. + timeout-minutes: 60 env: PYTHONUNBUFFERED: 1 OMP_NUM_THREADS: 4 @@ -91,35 +98,31 @@ jobs: - name: Turn off default branching shell: bash run: bash ./update_itemization.sh - - name: Run tests/variables YAML tests + # Each group remains a separate, sequential process on its runner. + # SPM simulation/API contracts stay in the Microsimulation job. + - name: Run remaining Python tests + id: python_tests if: ${{ !cancelled() }} env: - PYTEST_ADDOPTS: "--durations=0 --junitxml=rest-variables.xml -p no:unraisableexception -p no:threadexception" - run: /usr/bin/time -v -o rest-variables-resources.txt uv run --no-sync make test-yaml-variables - - name: Upload Rest variables timing and memory reports + PYTEST_ADDOPTS: "--durations=0 -v -p no:unraisableexception -p no:threadexception" + run: uv run --no-sync make test-other-python-rest REST_PYTHON_SHARD=2 REST_REPORT_DIR=. + - name: Upload Rest Python timing and memory reports if: always() uses: actions/upload-artifact@v4 with: - name: rest-variables-test-timings + name: rest-python-test-timings-shard-2 path: | - rest-variables.xml - rest-variables-resources.txt - Rest: - # Retain the old check name and the release Publish dependency. - name: Full Suite - Rest (Python + variables) - needs: [RestPython, RestVariables] - if: ${{ always() }} - runs-on: ubuntu-latest - timeout-minutes: 5 - steps: + rest-python-*.xml + rest-python-*-resources.txt - name: Check all Rest jobs succeeded + if: always() shell: bash env: PYTHON_RESULT: ${{ needs.RestPython.result }} - VARIABLES_RESULT: ${{ needs.RestVariables.result }} + SHARD_RESULT: ${{ steps.python_tests.outcome }} run: | - if [[ "$PYTHON_RESULT" != "success" || "$VARIABLES_RESULT" != "success" ]]; then - echo "::error::Rest jobs did not all succeed: Python=$PYTHON_RESULT, variables=$VARIABLES_RESULT" + if [[ "$PYTHON_RESULT" != "success" || "$SHARD_RESULT" != "success" ]]; then + echo "::error::Rest shards did not all succeed: shard-1=$PYTHON_RESULT, shard-2=$SHARD_RESULT" exit 1 fi Microsimulation: diff --git a/Makefile b/Makefile index c19c5415bc2..842b3d70fb2 100644 --- a/Makefile +++ b/Makefile @@ -45,21 +45,21 @@ REST_SPM_TESTS := $(TESTS)/core/test_spm_policy_family.py \ # (8.1 GB including the pending capital-gains MTR change). These groups remain # sequential subprocesses within each Rest Python shard. REST_PYTHON_GROUPS := cached-arrays heavy-a heavy-b core-heavy core policy-parameters policy remaining-heavy remaining -# CI runs three shards of these existing isolated processes. Across seven +# CI runs two shards of these existing isolated processes. Across seven # recent runs (including 38326922385 and 38289790999), their summed group -# times were 23-32, 20-30 and 22-29 minutes; the old serial job hit 90 minutes. +# times were 32-45 and 32-46 minutes; variables adds about a minute to shard 1. +# Both stay near 45 minutes, while the old serial job hit its 90-minute limit. # No shard runs more than one Python group at a time. Locally, leaving # REST_PYTHON_SHARD unset still runs every group in the original order. -REST_PYTHON_SHARD_1_GROUPS := cached-arrays core-heavy core -REST_PYTHON_SHARD_2_GROUPS := heavy-a policy-parameters remaining -REST_PYTHON_SHARD_3_GROUPS := heavy-b policy remaining-heavy +REST_PYTHON_SHARD_1_GROUPS := cached-arrays core-heavy policy-parameters remaining-heavy remaining +REST_PYTHON_SHARD_2_GROUPS := heavy-a heavy-b core policy REST_PYTHON_SHARD ?= ifeq ($(REST_PYTHON_SHARD),) REST_PYTHON_SELECTED_GROUPS := $(REST_PYTHON_GROUPS) else REST_PYTHON_SELECTED_GROUPS := $(REST_PYTHON_SHARD_$(REST_PYTHON_SHARD)_GROUPS) ifeq ($(strip $(REST_PYTHON_SELECTED_GROUPS)),) -$(error REST_PYTHON_SHARD must be 1, 2 or 3) +$(error REST_PYTHON_SHARD must be 1 or 2) endif endif REST_CACHED_ARRAY_TESTS := $(TESTS)/test_formulas_do_not_write_into_cached_arrays.py diff --git a/changelog.d/ci-rest-parallel-headroom.fixed.md b/changelog.d/ci-rest-parallel-headroom.fixed.md index 740326c17df..b57fee959de 100644 --- a/changelog.d/ci-rest-parallel-headroom.fixed.md +++ b/changelog.d/ci-rest-parallel-headroom.fixed.md @@ -1 +1 @@ -Split the Rest CI suite into three isolated Python shards and a variables YAML job to avoid false failures at the 90-minute limit, retaining the aggregate check and timing reports. +Split the Rest CI suite into two sequential Python shards, folding variables YAML and the legacy aggregate check into those runners to avoid the 90-minute timeout while increasing the PR runner budget from 26 to 27. Retain isolated test processes and timing reports. diff --git a/policyengine_us/tests/code_health/test_pr_runner_budget.py b/policyengine_us/tests/code_health/test_pr_runner_budget.py index ef6f6197167..5ded3067e45 100644 --- a/policyengine_us/tests/code_health/test_pr_runner_budget.py +++ b/policyengine_us/tests/code_health/test_pr_runner_budget.py @@ -43,13 +43,17 @@ def test_pr_runner_budget_keeps_the_heavy_suites_separate(): counts[name] = len(matrix["include"]) else: counts[name] = math.prod(len(values) for values in matrix.values()) - assert sum(counts.values()) == 26 + # Rest hit the 90-minute timeout on #10024, #9913, #10139 and #10029 + # in October 2026. One extra runner buys about 45 minutes of headroom: + # two tested shards share variables and the legacy summary check. + assert sum(counts.values()) == 27 assert { name: counts[name] - for name in ("Baseline", "Contrib", "Rest", "Microsimulation") + for name in ("Baseline", "Contrib", "RestPython", "Rest", "Microsimulation") } == { "Baseline": 11, "Contrib": 9, + "RestPython": 1, "Rest": 1, "Microsimulation": 1, } @@ -71,16 +75,21 @@ def test_suite_groups_have_no_extra_runners_or_triggers(): # PyYAML parses the bare `on` key as True. No push/PR trigger here: # the draft guard must control the only route to these runners. assert GROUP_WORKFLOWS[name][True] == {"workflow_call": None} - assert set(PYTHON_JOBS) == {"Rest", "Microsimulation"} + assert set(PYTHON_JOBS) == {"RestPython", "Rest", "Microsimulation"} assert {name: job["timeout-minutes"] for name, job in PYTHON_JOBS.items()} == { - "Rest": 90, + "RestPython": 60, + "Rest": 60, "Microsimulation": 60, } - for job in PYTHON_JOBS.values(): + for name, job in PYTHON_JOBS.items(): assert job["runs-on"] == "ubuntu-latest" - assert not job.get("needs") + assert job.get("needs", []) == (["RestPython"] if name == "Rest" else []) assert not job.get("strategy") assert not job.get("continue-on-error") + # The second tested runner also checks the first runner's result. There + # is no third runner just for the summary, or a variables-only runner. + assert PYTHON_JOBS["Rest"]["if"] == "${{ always() }}" + assert PYTHON_JOBS["Rest"]["name"] == "Full Suite - Rest (Python + variables)" def test_sequential_variants_preserve_failures_and_allow_later_results(): 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 28e2c46a1b2..b8607a556f6 100644 --- a/policyengine_us/tests/code_health/test_rest_python_groups.py +++ b/policyengine_us/tests/code_health/test_rest_python_groups.py @@ -5,10 +5,12 @@ 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. CI assigns those groups to -parallel Python shards, with the variables YAML in a separate job. +two Python shards on separate runners, with the variables YAML in the first +shard. The second shard retains the original Rest check and waits for the +first shard so it can fail if either shard fails. Paths and --ignore flags can drop a file, or run it twice, while every test -still passes. So this guard expands the workflows' matrix commands through +still passes. So this guard expands the workflows' shard commands through `make -n`, asks pytest which files each group collects, and checks that each file of the old single command runs exactly once. A new test file lands in core or policy when it sits under those folders and in remaining otherwise; @@ -130,15 +132,6 @@ def rest_steps(workflow, job="RestPython"): return workflow_jobs(workflow)[job]["steps"] -def expanded_matrix(text, entry): - """Resolve the matrix fields GitHub substitutes before running a step.""" - return re.sub( - r"\$\{\{\s*matrix\.(\w+)\s*\}\}", - lambda match: str(entry[match.group(1)]), - text, - ) - - def ci_python_shards(workflow): """The real Make overrides and artifact configuration of every shard.""" jobs = workflow_jobs(workflow) @@ -148,36 +141,36 @@ def ci_python_shards(workflow): for step in config.get("steps", []) if "make test-other-python-rest" in step.get("run", "") ] - assert calls == ["RestPython"], (workflow.name, calls) - job = jobs["RestPython"] - matrix = job["strategy"]["matrix"] - assert set(matrix) == {"include"}, matrix - assert job["strategy"]["fail-fast"] is False - runs = [ - step["run"] - for step in job["steps"] - if "make test-other-python-rest" in step.get("run", "") - ] - assert len(runs) == 1, f"{workflow.name}: {len(runs)} steps run the groups" - (upload,) = [ - step - for step in job["steps"] - if step.get("uses", "").startswith("actions/upload-artifact") - ] + assert sorted(calls) == ["Rest", "RestPython"], (workflow.name, calls) shards = {} - for entry in matrix["include"]: - name = entry["group"] - assert name not in shards, f"{workflow.name}: {name} runs twice" - argv = shlex.split(expanded_matrix(runs[0], entry)) + for job_name, selector in (("RestPython", "1"), ("Rest", "2")): + job = jobs[job_name] + assert "strategy" not in job, f"{workflow.name}: extra {job_name} runners" + (run,) = [ + step + for step in job["steps"] + if "make test-other-python-rest" in step.get("run", "") + ] + (upload,) = [ + step + for step in job["steps"] + if step.get("uses", "").startswith("actions/upload-artifact") + and "rest-python-*.xml" in step["with"]["path"].split() + ] + argv = shlex.split(run["run"]) assert argv[:5] == ["uv", "run", "--no-sync", "make", "test-other-python-rest"] - assert any(arg.startswith("REST_PYTHON_SHARD=") for arg in argv[5:]), argv + selectors = [ + arg.removeprefix("REST_PYTHON_SHARD=") + for arg in argv[5:] + if arg.startswith("REST_PYTHON_SHARD=") + ] + assert selectors == [selector], (workflow.name, job_name, argv) assert all(arg.startswith("REST_") and "=" in arg for arg in argv[5:]), argv - shards[name] = { + shards[f"shard-{selector}"] = { "overrides": argv[5:], - "artifact": expanded_matrix(upload["with"]["name"], entry), + "artifact": upload["with"]["name"], "patterns": upload["with"]["path"].split(), } - assert shards, f"{workflow.name} has no Python shards" return shards @@ -304,31 +297,33 @@ def test_each_group_writes_reports_its_ci_shard_uploads(workflow, report_dir): assert len(set(reports)) == len(reports), reports variable_uploads = [ step - for step in rest_steps(workflow, "RestVariables") + for step in rest_steps(workflow, "RestPython") if step.get("uses", "").startswith("actions/upload-artifact") + and "rest-variables.xml" in step["with"]["path"].split() ] assert len(variable_uploads) == 1 artifacts.append(variable_uploads[0]["with"]["name"]) assert len(set(artifacts)) == len(artifacts), artifacts - for job in ("RestPython", "RestVariables"): + for job in ("RestPython", "Rest"): steps = rest_steps(workflow, job) - target = { - "RestPython": "test-other-python-rest", - "RestVariables": "test-yaml-variables", - }[job] - (run,) = [step for step in steps if f"make {target}" in step.get("run", "")] + (run,) = [ + step + for step in steps + if "make test-other-python-rest" in step.get("run", "") + ] assert run["if"] == "${{ !cancelled() }}" (upload,) = [ step for step in steps if step.get("uses", "").startswith("actions/upload-artifact") + and "rest-python-*.xml" in step["with"]["path"].split() ] assert upload["if"] == "always()" assert not any(step.get("continue-on-error", False) for step in steps) @pytest.mark.parametrize("workflow", WORKFLOWS, ids=lambda path: path.name) -def test_variable_yaml_selection_runs_once_in_its_own_job(workflow): +def test_variable_yaml_selection_runs_once_in_the_first_python_shard(workflow): jobs = workflow_jobs(workflow) calls = [ (job, step) @@ -338,8 +333,9 @@ def test_variable_yaml_selection_runs_once_in_its_own_job(workflow): ] assert len(calls) == 1, (workflow.name, calls) job, step = calls[0] - assert job == "RestVariables" + assert job == "RestPython" assert "strategy" not in jobs[job], "variables YAML must run exactly once" + assert step["if"] == "${{ !cancelled() }}" assert shlex.split(step["run"]) == [ "/usr/bin/time", "-v", @@ -359,19 +355,17 @@ def test_variable_yaml_selection_runs_once_in_its_own_job(workflow): "--batches", "1", ] - assert not any( - "make test-other-python" in candidate.get("run", "") - for candidate in jobs[job]["steps"] - ) assert not any( "make test-yaml-variables" in candidate.get("run", "") - for candidate in rest_steps(workflow) + for candidate in rest_steps(workflow, "Rest") ) (upload,) = [ candidate for candidate in jobs[job]["steps"] if candidate.get("uses", "").startswith("actions/upload-artifact") + and "rest-variables.xml" in candidate["with"]["path"].split() ] + assert upload["if"] == "always()" junit = [ arg.removeprefix("--junitxml=") for arg in shlex.split(step["env"]["PYTEST_ADDOPTS"]) @@ -390,22 +384,50 @@ def test_original_rest_check_aggregates_all_shards_without_hiding_failure(workfl jobs = workflow_jobs(workflow) summary = jobs["Rest"] assert summary["name"] == "Full Suite - Rest (Python + variables)" - assert set(summary["needs"]) == {"RestPython", "RestVariables"} + assert summary["needs"] == ["RestPython"] assert "always()" in summary["if"] assert "strategy" not in summary + assert summary["timeout-minutes"] == 60 assert not summary.get("continue-on-error", False) + assert not jobs["RestPython"].get("continue-on-error", False) if workflow.name == "push.yaml": - assert "Rest" in jobs["Publish"]["needs"] - assert len(summary["steps"]) == 1 - (step,) = summary["steps"] + assert " ".join(summary["if"].split()) == ( + "always() && (github.repository == 'PolicyEngine/policyengine-us') " + "&& (github.event.head_commit.message == 'Update PolicyEngine US')" + ) + assert {"RestPython", "Rest"} <= set(jobs["Publish"]["needs"]) + else: + assert summary["if"] in ("always()", "${{ always() }}") + (step,) = [ + candidate + for candidate in summary["steps"] + if "PYTHON_RESULT" in candidate.get("env", {}) + ] + (python_step,) = [ + candidate + for candidate in summary["steps"] + if candidate.get("id") == "python_tests" + ] + assert "make test-other-python-rest" in python_step["run"] + assert "REST_PYTHON_SHARD=2" in shlex.split(python_step["run"]) + assert python_step["if"] == "${{ !cancelled() }}" + # This runner executes shard 2 before checking shard 1. Its own failures + # must remain failures, and cancellation skipping its Python step must + # fail the check even if shard 1 succeeded. + assert summary["steps"][-1] == step + assert step["if"] in ("always()", "${{ always() }}") + assert step["shell"] == "bash" assert step["env"] == { "PYTHON_RESULT": "${{ needs.RestPython.result }}", - "VARIABLES_RESULT": "${{ needs.RestVariables.result }}", + "SHARD_RESULT": "${{ steps.python_tests.outcome }}", } - assert not step.get("continue-on-error", False) + assert not any( + candidate.get("continue-on-error", False) + for candidate in summary["steps"] + jobs["RestPython"]["steps"] + ) for python in ("success", "failure", "cancelled", "skipped"): - for variables in ("success", "failure", "cancelled", "skipped"): - env = dict(os.environ, PYTHON_RESULT=python, VARIABLES_RESULT=variables) + for shard in ("success", "failure", "cancelled", "skipped"): + env = dict(os.environ, PYTHON_RESULT=python, SHARD_RESULT=shard) result = subprocess.run( ["bash", "-e", "-c", step["run"]], cwd=REPO, @@ -413,10 +435,10 @@ def test_original_rest_check_aggregates_all_shards_without_hiding_failure(workfl capture_output=True, text=True, ) - assert (result.returncode == 0) == (python == variables == "success"), ( + assert (result.returncode == 0) == (python == shard == "success"), ( workflow.name, python, - variables, + shard, result.stdout + result.stderr, ) @@ -480,7 +502,7 @@ def test_inherited_ci_shard_does_not_restrict_the_reference(monkeypatch, groups) assert [argv[1:] for argv in plain] == [spec["args"] for spec in groups.values()] -@pytest.mark.parametrize("shard", ["0", "4", "unknown"]) +@pytest.mark.parametrize("shard", ["0", "3", "unknown"]) def test_invalid_shard_cannot_pass_without_running_any_groups(shard): if shutil.which("make") is None: pytest.skip("make is not installed") @@ -493,7 +515,7 @@ def test_invalid_shard_cannot_pass_without_running_any_groups(shard): text=True, ) assert result.returncode != 0 - assert "REST_PYTHON_SHARD must be 1, 2 or 3" in result.stderr + assert "REST_PYTHON_SHARD must be 1 or 2" in result.stderr def import_name(file): From 4bd2b03757421237302ff6a2db66a55bff7d21b5 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 13 Oct 2026 14:26:10 -0400 Subject: [PATCH 3/3] Raise Rest Python shard timeouts to 90 minutes --- .github/workflows/push.yaml | 4 ++-- .github/workflows/python-tests.yaml | 4 ++-- policyengine_us/tests/code_health/test_pr_runner_budget.py | 4 ++-- policyengine_us/tests/code_health/test_rest_python_groups.py | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/push.yaml b/.github/workflows/push.yaml index f576cecb456..4917f0024b3 100644 --- a/.github/workflows/push.yaml +++ b/.github/workflows/push.yaml @@ -315,7 +315,7 @@ jobs: name: Full Suite - Rest (Python, shard-1 + variables) runs-on: ubuntu-latest # Historical group times, including variables: 33-46 minutes plus setup. - timeout-minutes: 60 + timeout-minutes: 90 if: | (github.repository == 'PolicyEngine/policyengine-us') && (github.event.head_commit.message == 'Update PolicyEngine US') @@ -379,7 +379,7 @@ jobs: runs-on: ubuntu-latest # Historical group times: 32-46 minutes plus setup. # This runner also retains the legacy summary check, keeping PRs at 27 runners. - timeout-minutes: 60 + timeout-minutes: 90 if: | always() && (github.repository == 'PolicyEngine/policyengine-us') diff --git a/.github/workflows/python-tests.yaml b/.github/workflows/python-tests.yaml index ab34743c7fc..7d22abd7b6d 100644 --- a/.github/workflows/python-tests.yaml +++ b/.github/workflows/python-tests.yaml @@ -9,7 +9,7 @@ jobs: name: Full Suite - Rest (Python, shard-1 + variables) runs-on: ubuntu-latest # Historical group times, including variables: 33-46 minutes plus setup. - timeout-minutes: 60 + timeout-minutes: 90 env: PYTHONUNBUFFERED: 1 OMP_NUM_THREADS: 4 @@ -71,7 +71,7 @@ jobs: runs-on: ubuntu-latest # Historical group times: 32-46 minutes plus setup. # This runner also retains the legacy summary check, keeping PRs at 27 runners. - timeout-minutes: 60 + timeout-minutes: 90 env: PYTHONUNBUFFERED: 1 OMP_NUM_THREADS: 4 diff --git a/policyengine_us/tests/code_health/test_pr_runner_budget.py b/policyengine_us/tests/code_health/test_pr_runner_budget.py index 5ded3067e45..48fcb490cbb 100644 --- a/policyengine_us/tests/code_health/test_pr_runner_budget.py +++ b/policyengine_us/tests/code_health/test_pr_runner_budget.py @@ -77,8 +77,8 @@ def test_suite_groups_have_no_extra_runners_or_triggers(): assert GROUP_WORKFLOWS[name][True] == {"workflow_call": None} assert set(PYTHON_JOBS) == {"RestPython", "Rest", "Microsimulation"} assert {name: job["timeout-minutes"] for name, job in PYTHON_JOBS.items()} == { - "RestPython": 60, - "Rest": 60, + "RestPython": 90, + "Rest": 90, "Microsimulation": 60, } for name, job in PYTHON_JOBS.items(): 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 b8607a556f6..f76b4e2818b 100644 --- a/policyengine_us/tests/code_health/test_rest_python_groups.py +++ b/policyengine_us/tests/code_health/test_rest_python_groups.py @@ -387,7 +387,7 @@ def test_original_rest_check_aggregates_all_shards_without_hiding_failure(workfl assert summary["needs"] == ["RestPython"] assert "always()" in summary["if"] assert "strategy" not in summary - assert summary["timeout-minutes"] == 60 + assert summary["timeout-minutes"] == 90 assert not summary.get("continue-on-error", False) assert not jobs["RestPython"].get("continue-on-error", False) if workflow.name == "push.yaml":