Repository navigation
ci: skip TheRock CI matrix on draft pull requests - #10752
Merged
Merged
Conversation
Add ready_for_review to the pull_request trigger types, skip the setup job while a PR is a draft, and make the downstream Linux/Windows jobs require needs.setup.result == 'success' so a skipped setup correctly cascades instead of running with empty inputs. Fixes #10751 Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
github actions
project: none
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the TheRock CI GitHub Actions workflow to avoid running the full Linux/Windows build+test matrix for draft pull requests, while still running it automatically once the PR is marked “Ready for review”, reducing unnecessary CI runner usage during WIP iteration.
Changes:
- Add
ready_for_reviewto thepull_requesttrigger types so CI fires immediately when a PR leaves draft state. - Skip the
setupjob on draft PRs (while keeping non-PR triggers unaffected). - Ensure the Linux/Windows matrix jobs explicitly require
needs.setup.result == 'success'so they correctly cascade whensetupis skipped.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Aug 13, 2026
geomin12
reviewed
Aug 13, 2026
Address review feedback: relying on GitHub Actions' if: always() + needs.<job>.result skip-cascade semantics for a required check is fragile. Instead, always run setup to completion; the Python config script now returns an empty project list for draft PRs before doing any path-diffing work, so the pre-existing needs.setup.outputs.*_projects != '[]' gate on the build/test jobs skips them without any special-casing. Co-authored-by: Cursor <cursoragent@cursor.com>
tony-davis
marked this pull request as draft
August 13, 2026 22:28
Co-authored-by: Cursor <cursoragent@cursor.com>
This reverts commit 1f885f13788ec69ac979b4f4c15b6f0e5f2f7c05. (No content changes to revert since the original commit was empty; this is a forward no-op commit purely to remove the test marker from history.) Co-authored-by: Cursor <cursoragent@cursor.com>
tony-davis
marked this pull request as ready for review
August 13, 2026 22:32
tony-davis
added a commit
that referenced
this pull request
Aug 13, 2026
## Summary Extends the existing auto-retry watcher (added in #9174, #9658) to also cover the per-PR `TheRock CI` workflow. Unlike the nightlies (4 total attempts), it gets only 1 auto-retry (2 total attempts) so a genuine regression still surfaces quickly and flaky infra doesn't multiply runner usage on every PR push. ## Risk Assessment Risk 2. One-file change to an isolated CI orchestration workflow; no product code, reusable-workflow interface, or build/test lane is touched. It only acts on runs that already concluded `failure`, and the per-workflow attempt cap bounds worst-case runner usage. This is the same mechanism already proven on the nightlies (#9174, #9658), just applied to a second, lower-cap workflow name. ## Related - Work tracking: #10762 (Closes) - Related PRs: #9174, #9658 (nightly predecessors), #10752 (restricts `TheRock CI` to non-draft PRs -- complementary, reduces overall run volume) ## Device / Architecture Coverage Architecture-independent. This is CI orchestration only (it calls the GitHub API to re-run jobs); it runs no device code and needs no per-arch verification. ## Testing Summary - YAML parses cleanly (`python -c "import yaml; yaml.safe_load(open('.github/workflows/auto-retry-failed.yml'))"`). - `pre-commit run --files .github/workflows/auto-retry-failed.yml` passed locally (trailing-whitespace, end-of-file-fixer, check-yaml). - Functional behavior can only be exercised on the default branch, because `workflow_run` always uses the copy of this file on `develop`. Plan: watch the first `TheRock CI` failure after merge and confirm it gets exactly one auto-retry. ## Testing Checklist - [x] YAML parse - `python -c "import yaml; yaml.safe_load(...)"` - Status: Passed - [x] pre-commit (formatting/yaml) - `pre-commit run --files .github/workflows/auto-retry-failed.yml` - Status: Passed - [ ] PR CI - GitHub PR checks - Status: Pending - [ ] Post-merge observation - confirm a failed `TheRock CI` run gets exactly 1 auto-retry - Status: Pending ## Flags / Guardrails Per-workflow attempt cap via `run_attempt`: `TheRock CI` gets `run_attempt < 2` (1 auto-retry, 2 total attempts); the two nightly workflows keep the existing `run_attempt < 4` (unchanged). Only fires on `conclusion == 'failure'`; `cancelled` (concurrency/superseded runs, or a run with individually cancelled jobs -- `rerun-failed-jobs` never touches those) and `startup_failure` are excluded. No new permissions; reuses the existing `actions: write` grant. ## Adjacent Tests Considered No product or test code changes. Checked that `TheRock CI`'s `cancel-in-progress: true` concurrency interacts safely with a queued retry: a new push cancels a stale retry, which is the desired behavior (no point retrying stale code). ## Risk Acceptance / Waivers `W-BUILD`: CI orchestration only, no product behavior change; mirrors the already-proven nightly pattern from #9174/#9658. ## Technical Changes - Add `"TheRock CI"` to the `workflow_run.workflows` list in `.github/workflows/auto-retry-failed.yml`. - Change the attempt-cap `if:` condition to branch by workflow name: `run_attempt < 2` for `TheRock CI`, `run_attempt < 4` for everything else (nightlies, unchanged). - Update header/job comments to document the per-workflow cap and to note that `rerun-failed-jobs` already excludes individually cancelled jobs. Co-authored-by: Cursor <cursoragent@cursor.com>
geomin12
reviewed
Aug 13, 2026
geomin12
left a comment
geomin12
left a comment
Collaborator
There was a problem hiding this comment.
lgtm but can you open a test draft PR and make sure this works as expected?
Contributor
Author
Thanks @geomin12 ! That evidence was in the description, and it's a good idea. Already executed. |
geomin12
approved these changes
Aug 14, 2026
2 tasks
tony-davis
enabled auto-merge (squash)
August 14, 2026 21:43
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.
Summary
TheRock CIcurrently launches the full Linux/Windows build+test matrix on every push to a PR, including while it's still a draft. This skips that matrix for draft PRs and instead fires it once when a PR is marked "Ready for review", so work-in-progress pushes stop burning shared build-runner capacity.Risk Assessment
Risk 2/5. CI-trigger-only change, no product or test behavior change. The draft-skip logic lives entirely in
therock_configure_ci.py: thesetupjob always runs to completion (it is never itself skipped), and for a draft PR the script returns an empty project list before doing any path-diffing work. That means the pre-existing, unmodifiedneeds.setup.outputs.linux_projects != '[]'gate on the build/test jobs handles the skip, with noif: always()/ skip-cascade logic for the requiredTheRock CI Summarycheck to get wrong (see review discussion below for the earlier, more fragile version of this approach).Related
Device / Architecture Coverage
No product/device code changes; this is CI wiring only. Passing PR CI is sufficient to merge.
Testing Summary
python -m pytest .github/scripts/tests/therock_configure_ci_test.py-- 36 passed (3 new: draft-PR skips CI without diffing paths, ready-for-review runs CI normally,is_draftignored on non-pull_requestevents).pre-commit run --files .github/workflows/therock-ci.yml .github/scripts/therock_configure_ci.py .github/scripts/tests/therock_configure_ci_test.pypassed.Setuplogs showis_draft: True->"Pull request is a draft, skipping CI"->linux_projects: '[]',windows_projects: '[]';Linux/Windowsmatrix jobsskipped; requiredTheRock CI Summarychecksuccess.ready_for_reviewtrigger,Setupsucceeded, and the full Linux/Windows build matrix launched normally (~25 build jobs across all projects/GPU families).concurrencygroup correctly cancel the stale draft-state run the moment the ready-for-review run started, avoiding a redundant in-flight run.if:on thesetupjob instead of the Python-side skip; that revision was also live-verified the same way (run 31725884279 draft-skip, run 31726513075 ready-for-review) before being replaced per review feedback (see Technical Changes).Testing Checklist
pytest .github/scripts/tests/therock_configure_ci_test.py- Status: Passed (36/36)pre-commit run --files ...- Status: PassedSetupsucceeds with empty project lists,Linux/Windowsskipped,TheRock CI Summarysuccess- Status: Passedready_for_reviewtrigger verification (final revision) - marking ready (no push) fired a newTheRock CIrun and launched the full build matrix - Status: PassedFlags / Guardrails
None. Behavior is unconditional (no opt-out flag) since it only affects draft-state PRs, which are never merge-ready by definition.
Adjacent Tests Considered
Checked the other
pull_request_target-triggered bot (libraries-pr-bot.yml) -- it already has ablock_draftpolicy hook (currently disabled) and is unaffected by this change since it's a separate workflow. No other caller referencestherock-ci.ymlas a reusable workflow (it's only invoked via its ownon:triggers), so no interface/caller updates are needed.Risk Acceptance / Waivers
None needed.
Technical Changes
ready_for_reviewto thepull_requesttrigger types, so CI fires immediately when a draft PR is marked ready instead of waiting for the next push.IS_DRAFTenv var (fromgithub.event.pull_request.draft) to the "Determine Linux/Windows projects to run" steps.therock_configure_ci.py,retrieve_projects()now returns an empty project list immediately for a draft pull request, before diffing modified paths. Thesetupjob itself is never skipped; the pre-existingneeds.setup.outputs.*_projects != '[]'gate on the build/test jobs handles the rest unchanged.therock_configure_ci_test.py.Review history: an earlier revision implemented the skip via a job-level
if:onsetupplusneeds.setup.result == 'success'on the downstream jobs. @geomin12 flagged that relying on Actions' skip-cascade semantics for a required check is fragile; this revision addresses that by keepingsetupalways-green and pushing the skip decision into testable Python instead.