Repository navigation
Conversation
Manual dispatch runs on master are green, so enable the pull_request and merge_group triggers per the rollout plan. Diff-based selection in collect-tests keeps unrelated PRs at mode=none; merge_group runs the full v1 scope because it has no PR base to diff against. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84d0005c4
ℹ️ 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".
| required: false | ||
| default: 'main' | ||
| type: string | ||
| pull_request: |
There was a problem hiding this comment.
Once this pull-request job is made required, a failure in check-paths or collect-tests (checkout, selector, validation, artifact upload, or runner failure) leaves needs.collect-tests.result != 'success', so the condition at .github/workflows/cpu-torch-latest-v1-multirank.yml:122 skips unit-tests; skipped dependent jobs satisfy a required check, as the Modal workflow notes and guards against at .github/workflows/modal-torch-latest.yml:271-280, and the PR can merge without tests. Make the required job enter an explicit failing guard whenever collection was expected but failed, while retaining intentional path and mode=none skips.
Useful? React with 👍 / 👎.
| required: false | ||
| default: 'main' | ||
| type: string | ||
| pull_request: |
There was a problem hiding this comment.
For a PR that changes ci/tests_fetcher.py, the checkout at line 85 leaves the candidate's CI code in place and line 98 executes it, so that version can emit a valid mode=none result and skip the required test job even though the trusted selector's ci/** rule would force a full run. This is the exact required-check bypass that .github/workflows/TEST_SELECTION_STATE.md:21-24 says the base-branch selector flow prevents; execute trusted CI code over the candidate tree instead.
Useful? React with 👍 / 👎.
| required: false | ||
| default: 'main' | ||
| type: string | ||
| pull_request: |
There was a problem hiding this comment.
For a PR whose selected subset contains no retained multi-rank item—for example, a direct edit to tests/unit/v1/zero/test_overlap_comm_record_stream.py, whose tests are not DistributedTests—the selector returns subset and starts this job, but DS_MULTIRANK_ONLY at line 292 makes tests/conftest.py:77-93 deselect every item and pytest exits 5 for no tests. Manual dispatch always selects the full scope, so it does not exercise this case, and the new required check will block otherwise valid PRs; detect an empty applicable subset before invoking pytest or handle that no-test outcome explicitly.
Useful? React with 👍 / 👎.
| required: false | ||
| default: 'main' | ||
| type: string | ||
| pull_request: |
There was a problem hiding this comment.
When a PR's target branch has advanced, github.event.pull_request.base.sha used at line 96 can be days older than the base in the checked-out merge commit, so the selector's merge-base diff includes intervening master changes and may choose mode=all for an unrelated PR (for example after a ci/** or core-runtime change), spending the roughly one-hour suite this trigger is intended to avoid. The repository documents this payload behavior at .github/workflows/TEST_SELECTION.md:87-93; use the merge commit's exact first parent as check-paths.yml does.
Useful? React with 👍 / 👎.
Follow-up to #8926 per its rollout plan.
The
cpu-torch-latest-v1-multiranksuite has been green on repeated manual dispatch runs against master, so:pull_request:is added back to theon:block; diff-based selection (collect-tests+ci/tests_fetcher.py) keeps unrelated PRs atmode=none, so the ~1h suite only runs for PRs that touch thetests/unit/v1dependency graph.merge_group:is included so the merge queue can satisfy the check once it is registered as required in branch protection (a merge_group event has no PR base, so it runs the full v1 scope).Required-check registration in branch protection is a repo-settings step for maintainers and is not part of this diff.
AI assistance: prepared with AI assistance (DeepSeek Harness agent) under human review; the submitter has read every changed line.
Signed-off-by: Ma, Guokai guokai.ma@intel.com