From 51f96aae2d8ee18a850e530a26c49791d6ab42e0 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Thu, 15 Oct 2026 20:26:36 +0000 Subject: [PATCH] build: prepare PR07 with reviewed workflow simplifications Signed-off-by: Aaron Gonzales --- .github/CODEOWNERS | 1 + .github/workflows/ci.yml | 33 ++++++++++++++- CONTRIBUTING.md | 7 ++++ Makefile | 8 +++- docs/engineering/ci.md | 77 ++++++++++++++++++++++++++++++++++ scripts/check_ci_results.py | 46 ++++++++++++++++++++ tests/test_check_ci_results.py | 55 ++++++++++++++++++++++++ tests/test_ci_workflows.py | 39 +++++++++++++++++ 8 files changed, 263 insertions(+), 3 deletions(-) create mode 100644 scripts/check_ci_results.py create mode 100644 tests/test_check_ci_results.py diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b8c7c82..62ac945 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -31,3 +31,4 @@ /.dockerignore @NVIDIA-NeMo/user_sim_maintainers /tests/test_container_runner.py @NVIDIA-NeMo/user_sim_maintainers /tests/test_coverage_gate.py @NVIDIA-NeMo/user_sim_maintainers +/tests/test_check_ci_results.py @NVIDIA-NeMo/user_sim_maintainers diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0e4823e..f577ce1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,10 +24,15 @@ concurrency: group: ci-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} +env: + PYTEST_WORKERS: "2" + NEMO_TELEMETRY_ENABLED: "false" + jobs: static: name: Lint, format, docs and licences runs-on: ubuntu-latest + timeout-minutes: 30 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -49,6 +54,7 @@ jobs: test: name: Tests (Python ${{ matrix.python-version }}) runs-on: ubuntu-latest + timeout-minutes: 30 strategy: fail-fast: false matrix: @@ -69,11 +75,11 @@ jobs: # Durations are reported because a slow test is invisible until # someone profiles the suite, and the runner is the only place that # measures the hardware CI actually runs on. - run: make test PYTEST_ARGS="--durations=25" + run: make test PYTEST_ARGS="--durations=25 --junitxml=junit.xml" - name: Test suite with coverage if: matrix.python-version == '3.12' - run: make coverage PYTEST_ARGS="--durations=25" + run: make coverage PYTEST_ARGS="--durations=25 --junitxml=junit.xml" - name: Coverage report if: always() && matrix.python-version == '3.12' @@ -84,6 +90,15 @@ jobs: if-no-files-found: error retention-days: 14 + - name: Test reports on failure + if: failure() + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: test-failure-python-${{ matrix.python-version }} + path: junit.xml + if-no-files-found: warn + retention-days: 14 + - name: Offline self-check run: make smoke @@ -95,6 +110,7 @@ jobs: packaging: name: Wheel and extension seams runs-on: ubuntu-latest + timeout-minutes: 30 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -152,3 +168,16 @@ jobs: path: test-results/container/ include-hidden-files: true if-no-files-found: warn + retention-days: 14 + + status: + name: CI Status + if: always() + needs: [static, test, packaging, e2e, container] + runs-on: ubuntu-latest + timeout-minutes: 5 + env: + CI_JOB_RESULTS: ${{ toJSON(needs) }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - run: make check-ci-status diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e6c6ba4..39d02a3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -263,6 +263,13 @@ Your PR title must follow [Conventional Commits](https://www.conventionalcommits ## What CI runs +`CI Status` aggregates static checks, all supported Python versions, packaging, +installed CLI e2e tests, and CPU container tests. Missing, skipped, cancelled, or +failed mandatory jobs fail that status. Run the same Make targets locally; see +[local and CI validation](docs/engineering/ci.md) for commands, diagnostics, and +the maintainer checklist for changing required checks. DCO and PR-title checks +remain separate required policies. + Every job in `.github/workflows/ci.yml` calls a `make` target, so anything CI rejects reproduces locally with the same command. Tests and the dependency licence check run on Python 3.11 to 3.14. Pull-request checks run for every diff --git a/Makefile b/Makefile index 5eda56e..1f7ae98 100644 --- a/Makefile +++ b/Makefile @@ -61,7 +61,7 @@ COV_FAIL_UNDER ?= 84 .DEFAULT_GOAL := help .PHONY: help bootstrap check-tools check-lock install-dev install-ci install-kernel health-check check-notebooks-clean lint lint-fix format format-check check-all \ - check-hooks check-all-fix validate-native validate-ci test test-fast test-e2e coverage smoke dist check-dist check-release-tag check-extensions check-doc-links clean-notebooks check-license-headers update-license-headers check-dependency-licenses clean + check-ci-status check-hooks check-all-fix validate-native validate-ci test test-fast test-e2e coverage smoke dist check-dist check-release-tag check-extensions check-doc-links clean-notebooks check-license-headers update-license-headers check-dependency-licenses clean help: ## Show the available targets @@ -308,3 +308,9 @@ test-container: container-build-test ## Build fresh and run offline CPU coverag container-audit: ## Audit the local Docker context before image build python3 scripts/run_container_tests.py audit $(CONTAINER_ARGS) + + +# The aggregate uses only the runner's standard library; setup failures in other +# jobs must not prevent their results from being checked. +check-ci-status: ## Reject missing or unsuccessful jobs in CI_JOB_RESULTS JSON + python3 scripts/check_ci_results.py diff --git a/docs/engineering/ci.md b/docs/engineering/ci.md index 8a0c9c7..c0ce013 100644 --- a/docs/engineering/ci.md +++ b/docs/engineering/ci.md @@ -51,3 +51,80 @@ CI uploads `coverage.xml` as `coverage-python-3.12` for 14 days, even when the coverage command fails. Missing XML fails the upload step, and uploading a report does not suppress a preceding test or coverage failure. If collection or setup stops before XML is produced, inspect the failed step's logs. + +## Mandatory jobs and aggregate status + +`CI Status` waits for static checks, the complete Python test matrix, packaging, +installed CLI e2e, and CPU container tests. Every job must report `success`. +Failures, cancellation, missing results, malformed results, and skipped jobs fail +the aggregate; there are no intentional job skips. Conditional coverage steps +inside the test matrix do not skip their enclosing job. + +The aggregate runs with `always()` and invokes `make check-ci-status` with +`CI_JOB_RESULTS` set to GitHub's `toJSON(needs)` object. Its result checker uses +only Python's standard library so dependency installation failures in other jobs +can still be reported. When adding a mandatory job, update both the workflow's +`needs` list and the explicit registry in `scripts/check_ci_results.py`; workflow +tests reject an omitted dependency or unregistered job. + +CI bounds native pytest workers to two and sets job timeouts. Failed native +suites upload JUnit XML; e2e failures upload their available artifacts; the container job always uploads +its available diagnostics, including successful reports. If setup fails before an artifact exists, the failed step's GitHub +logs remain the diagnostic record. Offline test jobs disable telemetry and use +no provider credentials. The container job runs tests without external network +access; its dependency download occurs during image construction. + +A local native bundle runs in one Python environment; GitHub additionally +orchestrates the supported-version matrix and the CPU container. Run +`make test-container` separately to reproduce the container gate. + +## Reruns, cancellation, and stacked PRs + +For a transient infrastructure failure on the current revision, open its `CI` +run in GitHub Actions and choose **Re-run all jobs**. **Re-run failed jobs** is +also suitable when the successful jobs remain valid, but confirm that the failed +mandatory jobs and `CI Status` execute again and finish successfully. Rerunning +only `CI Status` cannot repair a failed dependency. If a partial rerun leaves the +aggregate stale or fails to rerun a dependency, rerun all jobs. See GitHub's +[rerun instructions](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs). + +PR runs share a concurrency group by PR number. A newer PR run cancels an older +in-progress run; use the newest run for the current revision and leave the +superseded one cancelled. If the current run is cancelled without a replacement, +rerun all its jobs after confirming no newer run is pending. Cancellation may +prevent even the `always()` aggregate from completing; a cancelled or absent +aggregate is not passing evidence. + +Each stacked PR targets its immediate parent branch. When a parent changes or +merges, check the child's base and incremental diff, update its branch as needed, +and rerun affected local Make targets. Recheck the base after GitHub retargets a +child automatically. A base edit alone is not a trigger for this workflow's +default `pull_request` events. If no fresh PR run appears after retargeting, +close and reopen the unmerged PR to trigger one. Verify that the resulting checks +cover the current head and base before merging. An old run's +[rerun uses its original SHA and ref](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs); +it does not validate a new head or merge base. A manual branch dispatch also +does not replace validation of the PR's test merge commit. + +Recheck required policies and approvals after every stack update. Approval +dismissal and approval of the latest push depend on the repository's active +[review protection settings](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches). +Request renewed review when approvals are dismissed or the reviewed diff changes; +do not assume that passing CI preserves an earlier approval. This guide does not +verify or change those settings. + +## Required-check migration for maintainers + +This change does not edit repository rules. First verify that all mandatory jobs +and `CI Status` complete successfully on representative PRs. Keep current +required checks until that evidence exists. Then prepare a ruleset change that +replaces only the status contexts produced by `ci.yml` with the exact observed +`CI Status` context. Check the actual check-run name in GitHub before saving the +rule; do not guess a workflow/name prefix. + +Preserve the separately required `dco` and +`semantic-pull-request / semantic-pull-request` policy contexts. Trailer +validation introduced by the contribution-policy change must also remain +required according to that change's checklist. These independent policies are +not members of this workflow's aggregate. Retest a successful PR and a PR with +a deliberately failing mandatory check after the authorized ruleset update. diff --git a/scripts/check_ci_results.py b/scripts/check_ci_results.py new file mode 100644 index 0000000..95d6419 --- /dev/null +++ b/scripts/check_ci_results.py @@ -0,0 +1,46 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Reject incomplete or unsuccessful mandatory GitHub Actions job results.""" + +from __future__ import annotations + +import json +import os +import sys + +REQUIRED_JOBS = frozenset({"static", "test", "packaging", "e2e", "container"}) + + +def validate_results(raw: str) -> list[str]: + """Return failures for the exact required job set; no skips are permitted.""" + try: + results = json.loads(raw) + except (json.JSONDecodeError, TypeError): + return ["CI_JOB_RESULTS must contain the JSON object produced by toJSON(needs)"] + if not isinstance(results, dict): + return ["CI_JOB_RESULTS must be a job-result object"] + failures = [] + for job in sorted(REQUIRED_JOBS - results.keys()): + failures.append(f"{job}: missing required job") + for job in sorted(results.keys() - REQUIRED_JOBS): + failures.append(f"{job}: unexpected job; update the explicit required job registry") + for job in sorted(REQUIRED_JOBS & results.keys()): + entry = results[job] + result = entry.get("result") if isinstance(entry, dict) else None + if result != "success": + failures.append(f"{job}: expected success, received {result!r}") + return failures + + +def main() -> int: + failures = validate_results(os.environ.get("CI_JOB_RESULTS", "")) + if failures: + print("Mandatory CI jobs did not all succeed:", *failures, sep="\n ", file=sys.stderr) + return 1 + print("Every mandatory CI job succeeded.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_check_ci_results.py b/tests/test_check_ci_results.py new file mode 100644 index 0000000..54d0f4a --- /dev/null +++ b/tests/test_check_ci_results.py @@ -0,0 +1,55 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""The aggregate cannot report success when required jobs did not succeed.""" + +import importlib.util +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts/check_ci_results.py" +spec = importlib.util.spec_from_file_location("check_ci_results", SCRIPT) +checker = importlib.util.module_from_spec(spec) +spec.loader.exec_module(checker) + + +def successful_results(): + return {job: {"result": "success"} for job in checker.REQUIRED_JOBS} + + +def test_all_required_jobs_successful(): + assert checker.validate_results(json.dumps(successful_results())) == [] + + +@pytest.mark.parametrize("result", ["failure", "cancelled", "skipped", "pending", None]) +@pytest.mark.parametrize("job", sorted(checker.REQUIRED_JOBS)) +def test_each_unsuccessful_job_rejects_aggregate(job, result): + results = successful_results() + results[job] = {"result": result} + assert checker.validate_results(json.dumps(results)) + + +def test_missing_and_unexpected_jobs_are_rejected(): + results = successful_results() + del results["e2e"] + results["replacement"] = {"result": "success"} + failures = checker.validate_results(json.dumps(results)) + assert any("e2e: missing" in failure for failure in failures) + assert any("replacement: unexpected" in failure for failure in failures) + + +@pytest.mark.parametrize("raw", ["", "null", "[]", "{broken", '{"static": "success"}']) +def test_invalid_results_are_rejected(raw): + assert checker.validate_results(raw) + + +def test_script_exit_status_matches_result_contract(): + for results, expected in [(successful_results(), 0), ({}, 1)]: + env = dict(os.environ, CI_JOB_RESULTS=json.dumps(results)) + result = subprocess.run([sys.executable, str(SCRIPT)], env=env, capture_output=True) + assert result.returncode == expected diff --git a/tests/test_ci_workflows.py b/tests/test_ci_workflows.py index 59e5f96..14a756d 100644 --- a/tests/test_ci_workflows.py +++ b/tests/test_ci_workflows.py @@ -338,3 +338,42 @@ def test_native_matrix_matches_package_python_support() -> None: expected = [f"3.{minor}" for minor in range(30) if f"3.{minor}" in supported] matrix = _load(_WORKFLOWS / "ci.yml")["jobs"]["test"]["strategy"]["matrix"] assert matrix["python-version"] == expected + + +class TestMandatoryCiAggregate: + def test_aggregate_waits_for_every_job_and_checks_explicit_results(self) -> None: + import importlib.util + + script = _WORKFLOWS.parents[1] / "scripts" / "check_ci_results.py" + spec = importlib.util.spec_from_file_location("check_ci_results", script) + checker = importlib.util.module_from_spec(spec) + spec.loader.exec_module(checker) + jobs = _load(_WORKFLOWS / "ci.yml")["jobs"] + status = jobs["status"] + assert set(jobs) - {"status"} == checker.REQUIRED_JOBS + assert set(status["needs"]) == checker.REQUIRED_JOBS + assert status["if"] == "always()" + assert status["name"] == "CI Status" + assert status["env"]["CI_JOB_RESULTS"] == "${{ toJSON(needs) }}" + assert any(step.get("run") == "make check-ci-status" for step in status["steps"]) + + def test_mandatory_jobs_cannot_be_optional_or_ignore_failures(self) -> None: + jobs = _load(_WORKFLOWS / "ci.yml")["jobs"] + for name, job in jobs.items(): + assert job.get("timeout-minutes", 0) > 0, name + assert not job.get("continue-on-error"), name + if name != "status": + assert "if" not in job, name + for step in job["steps"]: + assert not step.get("continue-on-error"), (name, step) + + def test_native_matrix_workers_and_diagnostics_are_bounded(self) -> None: + doc = _load(_WORKFLOWS / "ci.yml") + assert doc["env"]["PYTEST_WORKERS"] == "2" + assert doc["env"]["NEMO_TELEMETRY_ENABLED"] == "false" + for name in ["test", "e2e", "container"]: + assert any( + step.get("if") in {"failure()", "always()"} + and step.get("uses", "").startswith("actions/upload-artifact@") + for step in doc["jobs"][name]["steps"] + ), name