ci(gpu): gate GPU tests in-workflow so the required check always reports - #321
Merged
Merged
Conversation
`pytest (GPU, MI350)` is a required status check in the `main protection` ruleset, but gpu-tests.yml filtered its `pull_request` trigger by path. A workflow skipped by path filtering never reports -- its checks stay Pending -- so any PR touching none of those paths could never satisfy the requirement and was permanently unmergeable. Five of the seven currently open PRs are blocked this way (#307, #313, #314, #315, #317). Drop the trigger-level `paths:` filter and decide inside the workflow instead: a new `changes` job on ubuntu-latest lists the PR's files and sets an output, and the GPU jobs gate on it. A job skipped by a conditional reports Success, so non-GPU PRs now pass the required check without ever occupying the single MI350 runner. The pattern list is one-to-one with the `paths:` filter it replaces, so which PRs run GPU work is unchanged; verified against all seven open PRs. Job names are unchanged, so the ruleset needs no edit. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the GPU CI workflow so the required pytest (GPU, MI350) check always produces a conclusion on PRs by moving path-based gating from the workflow trigger into an in-workflow “changes” gate job.
Changes:
- Removes
pull_request.pathsfiltering so the workflow always runs and reports checks. - Adds a
changesjob that lists PR files viagh apiand outputs whether GPU-relevant paths changed. - Gates
pytest (GPU, MI350)(and thus its dependent regression job) on thechangesoutput while preserving the fork-PR safety guard.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The gate fetched the file list through process substitution, which discards the command's exit status -- `set -euo pipefail` never saw a failed `gh api`. A transient outage, rate limit or token-scope problem therefore produced an empty list and a confident `gpu=false`, skipping the GPU jobs and handing a GPU-touching PR a green required check without ever testing it. Capture the listing explicitly and default to `gpu=true` when it cannot be retrieved or comes back empty, so an indeterminate gate errs towards running the suite instead of silently waving a PR through. Co-authored-by: Cursor <cursoragent@cursor.com>
qianghan-amd
added a commit
that referenced
this pull request
Aug 4, 2026
Conflict in .github/workflows/gpu-tests.yml, resolved in favour of main's design. `pytest (GPU, MI350)` is a required status check, and main's #321 removed the `paths:` filter from the `pull_request` trigger because a workflow skipped by path filtering leaves its checks Pending forever instead of reporting, making any PR outside those paths permanently unmergeable. The filter now lives in the `changes` job as a bash `case` pattern array. This branch had added four entries to the `paths:` list that main deleted, so keeping either side alone would have lost something: main's side drops the env probe from GPU-relevant paths, and this branch's side reinstates the trigger filter that made the required check unmergeable. The four entries moved into the `patterns` array instead -- env_knobs.py, environment.py, audit_env_knobs.py and test_env_knob_audit.py -- so the probe still triggers the GPU suite under main's gate. The audit step merged cleanly and is unchanged. Verified rather than assumed: the array is bash, not YAML, so the explanatory comment sits inside `patterns=(...)`; expanding the real array shows 17 elements with no comment leaking in as one. Running main's own `case` loop against the merged patterns matches on each of the four files individually and still reports false for a docs-only change, so the gate neither lost coverage nor became unconditionally true. Full tests/ -n 16 in the ROCm 7.0.2 image, same command and hardware: merged 21 failed / 2911 passed / 93 skipped against origin/main's 21 failed / 2791 passed / 93 skipped, failure lists identical, so 0 new failures and +120 passing. Shared-file ruff findings are an exact multiset match with origin/main; all four workflows parse and the generated knob table still matches the registry. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
pytest (GPU, MI350)is a required status check in the activemain protectionruleset (strict_required_status_checks_policy: true). Butgpu-tests.ymlfiltered itspull_requesttrigger with apaths:list.Per GitHub's docs on required status checks, those two settings are incompatible:
So a PR touching none of the filtered paths never produces a
pytest (GPU, MI350)result at all, and the merge gate waits forever on a check that will never arrive. This is not theoretical — five of the seven currently open PRs are blocked by it:docs/**.github/workflows/{lock-requirements,refresh-baselines}.yml.github/workflows/*(setup-python bump)nightly-eval.yml,pages.yml,docs/src/aorta/cli/**,src/aorta/tools/**,pyproject.tomltests/instrumentation/test_environment.pyscripts/ci/gen_dashboard.pyNote the workflow's own header comment anticipated making this check required, but marking it so didn't account for the paths filter.
The fix
Move the path decision from the trigger into the workflow:
pull_request:no longer carriespaths:, so the workflow always starts and always reports.changesjob onubuntu-latestlists the PR's files viagh api .../pulls/N/filesand sets agpuoutput. Non-PR events (schedule,workflow_dispatch) always returntrue.gpu-testsgainsneeds: changesand gates onneeds.changes.outputs.gpu == 'true', keeping its existing fork-PR guard.gpu-regressionskips along with it, since a dependency that did not succeed skips the dependent job.A skipped job reports Success, so docs-only PRs satisfy the required check in seconds without ever touching the MI350 runner.
permissions:gainspull-requests: readfor the file listing.Job names are unchanged, so the ruleset needs no edit.
Behaviour is preserved
The pattern list is one-to-one with the
paths:filter it replaces. I replayed the gate logic against the actual file lists of all seven open PRs and it reproduces the old decisions exactly —gpu=truefor #318 and #320,gpu=falsefor the other five.Edge cases also checked: a brand-new file under
scripts/ci/, a deep subtree undersrc/aorta/workloads/,gpu-tests.ymlitself, an unrelated workflow, docs-only, and near-miss lookalikes (scripts/other/x.py,docker.md,src/aorta/racecar/x.py) all classify correctly.One implementation note worth flagging for review: the patterns are single-quoted in a bash array and iterated as
"${patterns[@]}". An earlier draft used an unquoted string list, which the shell filename-expanded against the working directory — that silently turnedscripts/ci/*into whatever happened to exist locally and would have missed newly added files. The array form keeps them literal so they reachcaseas patterns.Verification
This PR touches
.github/workflows/gpu-tests.yml, which is in the pattern list, so GPU jobs should run here and prove the positive path still works. The skip path is proven by updating #315 withmainafterwards and confirmingpytest (GPU, MI350)reports as skipped/Success.Made with Cursor