Repository navigation
ci(rccl): build the coco PR baseline at the PR's merge-base - #14105
Merged
alex-breslow-amd merged 2 commits intoOct 29, 2026
Merged
Conversation
github-actions
Bot
added
the
github actions
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
cmcknigh
reviewed
Oct 27, 2026
cmcknigh
left a comment
cmcknigh
left a comment
Contributor
There was a problem hiding this comment.
Review of the change to resolve the PR baseline at the merge-base. The mechanism is correct: I checked the compare call against the real API (this PR's head resolves to merge-base c7047ffc473d, matching git merge-base; a fork-PR head also resolves on the base repo), and the CI command python3 -m pytest .github/scripts/tests/*rccl*_test.py passes at this head (105 passed, 2 skipped; black 26.5.1 clean).
Seven inline findings: three P3 (tests defined after the __main__ guard; the silent fallback in the harness probe; GitHub API failures surfacing as a bare red gate) and four P4.
fix-then-ship: the tests after the main guard and the silent fallback in the harness probe are worth fixing before merge; the rest can follow.
speriaswamy-amd
force-pushed
the
users/speriaswamy/rccl-coco-merge-base
branch
from
October 27, 2026 20:52
62d2e2a to
588cedf
Compare
Contributor
Author
|
@cmcknigh thanks. All seven are addressed in 588cedf, replies inline. Summary:
Rebased onto current Checks: |
cmcknigh
reviewed
Oct 27, 2026
cmcknigh
left a comment
cmcknigh
left a comment
Contributor
There was a problem hiding this comment.
P4 (nitpick): the Test Plan still says the unit tests pass at a count of 21, and the description never mentions require_baseline_sha. At this head the file has 42 tests, and that input is what turns a harness without --baseline-sha from a warning into a setup failure.
Someone preparing the 1.4.x bump from this description would not see that they have to set require_baseline_sha: true. That step is in #14389.
Evidence: the PR body; .github/actions/rccl-resolve-coco-run/action.yml:36-42.
Could you update the Test Plan count, and note that the bump sets require_baseline_sha: true?
ship — the earlier review's issues are resolved. What is left is that the pin stays inactive until the follow-up bumps coco_ref to 1.4.x and sets require_baseline_sha.
Contributor
pr-rocm-systems-14105-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14105-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-14105-ruby64-rccl-pr-smokeBuilds
Failures
200 tests blocked and 15 tests skipped. |
speriaswamy-amd
force-pushed
the
users/speriaswamy/rccl-coco-merge-base
branch
from
October 28, 2026 17:40
588cedf to
d916980
Compare
Contributor
Author
|
@cmcknigh thanks. In d916980 (rebased onto
One thing to flag on your "ship" note, that the The red "RCCL PR gate" on the previous head wasn't from this change: the |
cmcknigh
reviewed
Oct 28, 2026
cmcknigh
left a comment
cmcknigh
left a comment
Contributor
There was a problem hiding this comment.
fix-then-ship — the connection-reset gap from the last round is fixed. What is left is a test that fails behind an HTTP proxy, and a deterministic InvalidURL that is retried as transient.
speriaswamy-amd
force-pushed
the
users/speriaswamy/rccl-coco-merge-base
branch
from
October 28, 2026 21:11
d916980 to
d1411d0
Compare
Contributor
Author
|
@cmcknigh thanks. Both inline findings are fixed and answered there. In On the
This PR's own gate run should be the first end-to-end check of the pin: the |
Contributor
pr-rocm-systems-14105-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14105-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
speriaswamy-amd
force-pushed
the
users/speriaswamy/rccl-coco-merge-base
branch
from
October 29, 2026 02:10
d1411d0 to
7afd498
Compare
Contributor
Author
|
Heads-up: the "Check RCCL cluster runner allowlist" check was red on
I also rebased onto Evidence from the previous head |
Contributor
pr-rocm-systems-14105-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14105-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-14105-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-14105-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14105-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-14105-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-14105-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
cmcknigh
approved these changes
Oct 29, 2026
The coco PR gate built its develop baseline at develop tip on every run, so changes merged after a PR branched read as the PR's own. Resolve the PR's merge-base with its base branch and pass it as --baseline-sha when the coco harness supports the flag. Otherwise warn, or fail the setup when the run requires the pin. Signed-off-by: speriaswamy-amd <surya.periaswamy@amd.com>
The coco PR gate now runs the 1.4.x harness, which declares --baseline-sha. Require the pin, so a harness that loses the flag fails setup instead of measuring against develop tip behind a warning. Rolling coco_ref back to 1.3.x must turn the requirement off as well. Signed-off-by: speriaswamy-amd <surya.periaswamy@amd.com>
speriaswamy-amd
force-pushed
the
users/speriaswamy/rccl-coco-merge-base
branch
from
October 29, 2026 14:57
7afd498 to
683f8ca
Compare
Contributor
Author
|
Thanks for the approval. I pushed a rebase ( Why it was needed: the required Multi-Arch CI Summary was red on the old head. A PR run builds against I'll merge once the required checks (RCCL PR gate, Multi-Arch CI Summary, HIP NVIDIA CI Summary) are green on this head. If a TheRock job unrelated to this change fails again, I will say so here and not work around it. |
Contributor
pr-rocm-systems-14105-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14105-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-14105-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
| Fail a PR run whose coco harness cannot build the baseline at the PR's | ||
| merge-base (coco pr --baseline-sha), instead of warning and measuring | ||
| against the baseline's configured ref. | ||
| required: false |
Contributor
There was a problem hiding this comment.
Shouldn't this be true?
Contributor
TheRock Submodule Bump ActivityNewest first
|
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.
Motivation
The coco PR gate built its develop baseline at develop tip on every run, because each run uses a fresh output directory. A PR was therefore measured against code it did not contain. In 18 replayed ruby64 runs, seven unrelated PRs showed another commit's 1-node reduce_scatter change (#12928) as their own.
Technical Details
rccl_resolve_coco_run.pynow, for PR runs:GET /pulls/{n}) and its merge-base with the head (GET /compare/{base}...{head}), using the job'sGITHUB_TOKEN;--baseline-sha <merge-base>tococo_args, which already flows into both the build-only and the testcoco prcalls;When the pin applies. It is passed only when the checked-out coco harness declares
--baseline-sha(the probe searchescoco/cli/). A harness without the flag gets a workflow warning and the baseline stays on its configured ref. The newrequire_baseline_shaaction input (defaultfalse) turns that warning into a setup failure.The PR gate requires the pin. #14389 moved the default harness to
1.4.x, which declares--baseline-sha, so the second commit passesrequire_baseline_sha: 'true'inrccl-coco-pr.yml. A harness that loses the flag now fails setup instead of silently measuring against develop tip. The scheduled workflow never runs in PR mode, so it is unchanged. Rollingcoco_refback to1.3.xhas to turn the input off as well, and a comment in the workflow says so.The wiring test reads the action's
action.yml, which the RCCL script-test job (rccl-cluster-runner-allowlist.yml) did not check out. That workflow's sparse checkout now includes.github/actions/rccl-resolve-coco-run, and itspathsfilter includes that directory, so an edit to the action's inputs re-runs the test that guards them.The action also passes the harness location explicitly (
HARNESS_DIR) instead of relying on the step's working directory. Periodic runs are unchanged.Issue Tracking
JIRA ID: AICOMRCCL-2204
Test Plan
.github/scripts/tests/rccl_resolve_coco_run_test.py: 50 tests pass when run directly, with or without an HTTP proxy in the environment.python3 -m pytest .github/scripts/tests/*rccl*_test.pygives 157 passed, 2 skipped. They cover:--baseline-stack;main()end to end: missingHARNESS_DIR, a periodic run, a required pin on an old harness, and an API failure;require_baseline_sha(default off) and the PR workflow passes it, since GitHub only warns about an undeclared input.InvalidURLclause and a misspelt workflow input each makes at least one test fail.c7047ffc473d(matchinggit merge-base), an uppercase head resolves the same, and a nonexistent PR fails in 0.2 s with the request in the message.Test Result
All pass. This PR's own gate run is the first end-to-end check: with the
1.4.xdefault harness, thecoco prcommand lines should show--baseline-shaand the baseline stack's RCCL version line should be the PR's merge-base. Without this change the gate runs the1.4.xharness but still builds its baseline at develop tip.Submission Checklist