[Workflow] run modal GPU workflows only from the merge queue - #8412
Conversation
The modal workflows spend Modal GPU quota on every PR event (review_requested / ready_for_review / synchronize), while the merge queue entry - the commit that actually gates the merge - never runs them, because the workflows have no merge_group trigger. Flip that: - Add a merge_group (checks_requested) trigger to both modal workflows so required checks are reported on queued entries. - Skip the modal deploy jobs on pull_request_target runs. The cheap no-secret collect/filter jobs keep running on PR events so the Required checks still report (a skipped dependent job counts as success), but no Modal Sandbox is created until the PR enters the merge queue. - modal-torch-latest: fetch the merge-group commit as the candidate tree and select tests against the queue's base SHA, so queue runs keep subset selection instead of falling back to the full suite. - dorny/paths-filter v4.0.1+ supports merge_group natively, so modal-accelerate's path filtering needs no changes. - Failure propagation is preserved: a failing collect-tests still fails the deploy check explicitly on every event. - Document the new gating and the merge_group trust context in TEST_SELECTION.md and CONTRIBUTING.md. Net effect: one Modal run per queued (batched) merge instead of one per PR event; pushes to master and manual runs are unchanged. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d227f903ce
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # pull_request_target runs skip the Sandbox entirely (a skipped dependent | ||
| # job still satisfies the Required check), so the tests gate the merge queue | ||
| # entry instead of every PR update. | ||
| if: ${{ !cancelled() && (needs.collect-tests.result != 'success' || (needs.collect-tests.outputs.mode != 'none' && github.event_name != 'pull_request_target')) }} |
There was a problem hiding this comment.
When a queued PR modifies this workflow or ci/torch_latest.py, this condition admits the merge_group run, whose checkout resolves to the PR-controlled github.sha; the job then executes that controller while injecting MODAL_TOKEN_ID and MODAL_TOKEN_SECRET. The candidate can read or exfiltrate those credentials before failing the queue check, so the merge-group path must not expose repository secrets to code from the merged candidate tree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 586a049: both the selector and the controller now check out the trusted base revision on merge queue runs (github.event.merge_group.base_sha), so ci/torch_latest.py no longer comes from the merge-group commit. The candidate is only ever fetched by exact SHA inside the no-secret Modal Sandbox, restoring the workflow's documented invariant. The residual exposure is a queued PR rewriting the workflow YAML itself — inherent to GitHub's merge queue, documented in TEST_SELECTION.md.
| # Plain PR events keep this job skipped so they never spend Modal quota (a | ||
| # skipped dependent job still satisfies the Required status); the merge | ||
| # queue re-runs it on the merged tree. | ||
| if: needs.collect-tests.outputs.deepspeed == 'true' && github.event_name != 'pull_request_target' |
There was a problem hiding this comment.
For a merge-group entry containing changes to ci/accelerate.py or this workflow, the default checkout selects the merge-group SHA and modal run -m ci.accelerate imports that candidate-controlled module directly on the GitHub runner. Because the job environment contains both Modal credentials and HF_TOKEN, such a PR can exfiltrate them without ever merging; keep the secret-bearing launcher on a trusted revision or otherwise remove secrets from this context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 586a049: the deploy checkout resolves to github.event.merge_group.base_sha on queue runs, so modal run -m ci.accelerate imports the trusted master launcher. The merged candidate is fetched separately as validated git data (ci/torch_latest.py checkout-candidate), and ci/accelerate.py uploads the tree under test from DS_CI_CANDIDATE_ROOT when set. Remotely the env var is unset, so the in-image module at /root keeps resolving to its own location exactly as before.
| files your PR changes, builds an import graph over `deepspeed/` and the `unit` test | ||
| helpers, and runs only the tests that could be affected. This keeps CI fast without | ||
| losing coverage (`push` to `master` always runs everything). The full design — and | ||
| run their modal tests on the merge queue entry instead of on every PR push, to |
There was a problem hiding this comment.
The reviewed commit is a non-merge commit, but its commit message has no Signed-off-by trailer, so it violates the repository's mandatory commit requirement and will be rejected by the DCO check. Add the author signoff before submitting it.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The commit does carry the trailer: Signed-off-by: Ma, Guokai <guokai.ma@intel.com> is in the commit body of d227f90 (see git log -1 --format=%B or the commits API messageBody), and the DCO check on the PR passes. It looks like only the subject line was inspected.
Address Codex review: on merge_group runs the candidate-controlled merge-group commit must not be the code that runs next to the Modal/HF secrets. - modal-torch-latest: both jobs now check out the trusted base revision (pull_request_target base SHA, or merge_group.base_sha for queue entries) instead of the merge-group commit. The candidate is still fetched by exact SHA inside the no-secret Sandbox, restoring the documented invariant for queue runs. - modal-accelerate: the deploy checkout resolves to the same trusted revision, and the merged candidate is fetched as validated git data (ci/torch_latest.py checkout-candidate) into a separate root; ci/accelerate.py uploads the tree under test from that root when DS_CI_CANDIDATE_ROOT is set (merge queue) and from its own checkout otherwise. Remotely the env var is unset, so the in-image module at /root keeps resolving to its own location as before. - Docs updated accordingly. Bootstrap note: this PR's own queue entry runs the trusted master launcher, so the accelerate path still follows the pre-change flow once; the new launcher/candidate split is live from the next merge on. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
tohtana
left a comment
There was a problem hiding this comment.
Hi @delock,
This looks good to me. Thank you for configuring these workflows.
We need to add the modal worlflows to branch protection after this PR is merged. Currently modal workflows don't have merge_group, so merging will be blocked if we add them to branch protection now.
Motivation
Both modal workflows spend Modal GPU quota on every PR event (
review_requested/ready_for_review/synchronize), while the merge queue entry — the commit that actually gates the merge — never runs them, because the workflows have nomerge_grouptrigger. This PR moves the GPU spend to the merge queue only.Behavior change
pull_request_target(review_requested / ready_for_review / synchronize)deployreports skipped, which still satisfies Required checksmerge_group(checks_requested)pushto master /workflow_dispatchChanges
.github/workflows/modal-torch-latest.ymlmerge_group(checks_requested) trigger — required checks must be reported on queued entries or the merge fails;merge_group(the merge-group commit diffed againstmerge_group.base_sha), so queue runs keep subset selection instead of falling back to the full suite;deploygainsgithub.event_name != 'pull_request_target'; the explicit failure-propagation path (a failingcollect-testsstill fails the check on every event) is preserved..github/workflows/modal-accelerate.yml: same trigger and the samedeploygating.dorny/paths-filter@v4(≥ v4.0.1) supportsmerge_groupnatively, so path filtering is unchanged.TEST_SELECTION.md(job flow, deploy gating, amerge_grouptrust-context note in the security model) andCONTRIBUTING.md.Net effect: one Modal run per queued (batched) merge instead of one per PR event. The repo's merge queue is active (recent merges went through it), so this takes effect immediately once merged.
Verification
merge_groupthe candidate resolves to the base-repo merge-group SHA and the queue's base SHA.mode=all); selector failures still fail the check on every event; queue entries run forall/subsetand skip fornone.bash -non the modified inline script;python ci/test_tests_fetcher.py16/16;pre-commit run --filespasses on all four files.Notes for reviewers
merge_groupruns use the workflow/controller from the merged commit with access to secrets — inherent to GitHub's merge queue, documented in the security-model section ofTEST_SELECTION.md; PRs touchingci/*should be reviewed with that in mind.skippedcounts as success). No settings change is needed for this PR itself.Security hardening (follow-up commit)
Follow-up
586a049a4(from Codex review): onmerge_groupruns, every GitHub-side checkout now resolves to the trusted base revision (github.event.merge_group.base_sha) instead of the merge-group commit, so queued-PR code never runs next to the Modal/HF tokens:modal-torch-latest: the selector and the controller come from trusted master; the merged candidate is still only ever fetched by exact SHA inside the no-secret Modal Sandbox.modal-accelerate: themodal runlauncher comes from trusted master; the merged candidate is fetched as validated git data (ci/torch_latest.py checkout-candidate) into$RUNNER_TEMP/deepspeed-candidate, andci/accelerate.pyuploads the tree under test fromDS_CI_CANDIDATE_ROOTwhen set. Remotely the env var is unset and the module keeps resolving inside the image at/rootas before.The residual exposure is a queued PR rewriting the workflow YAML itself — inherent to GitHub's merge queue and documented in
TEST_SELECTION.md. Bootstrap note: this PR's own queue entry runs the pre-change trusted launcher once; the new launcher/candidate split is live from the next merge on.