Skip to content

fix(rccl): drop the GDR flush case the NCCL sync never wired up - #12062

Merged
amd-jiali merged 6 commits into
developfrom
users/jialili/gdr-flush-mpi-compile
Sep 29, 2026
Merged

amd-jiali merged 6 commits into
developfrom
users/jialili/gdr-flush-mpi-compile

Conversation

@amd-jiali

Copy link
Copy Markdown
Contributor

Motivation

  • The NCCL v2.31.2-1 sync (b71e057a71c) added GdrFlushTest.ForcedScratchpadWrite_ReproducesFault under ENABLE_FAULT_INJECTION. That case calls gdrSupported() and RunRecvFlushBurst(..., /*forceWrite=*/true, &forced). There is no gdrSupported symbol, and RunRecvFlushBurst takes three arguments. FAULT_INJECTION defaults to ON, so rccl-UnitTestsMPI fails to compile. The nightly then reports return code 183 because OpenMPI cannot start the missing binary. The coco PR arm does not build this target, so the break only shows up in oci-rccl-unit-nightly.

  • #10450 wrapped the case in #if 0. The calls were still the merge-introduced ones, and the case cannot be restored without a forceWrite hook that was never added.

Technical Details

Remove ForcedScratchpadWrite_ReproducesFault from projects/rccl/test/transport/NetIbMPI/GdrFlushTests.cpp. It was not in the tree before the sync. The other GDR flush cases keep the three-argument RunRecvFlushBurst and gdrPtrSupport().

Issue Tracking

JIRA ID: AICOMRCCL-2403

Test Plan

  • On a compute node with ROCm 7.14.0, from a tree that contains b71e057a71c, rebuild rccl-UnitTestsMPI with ENABLE_MPI_TESTS=ON and FAULT_INJECTION=ON.

  • Confirm GdrFlushTests.cpp compiles, the binary is produced, and ForcedScratchpadWrite is not linked.

Test Result

The debug cache had FAULT_INJECTION=ON and ENABLE_MPI_TESTS=ON.

GdrFlushTests.cpp compiled and rccl-UnitTestsMPI linked.

Submission Checklist

amd-jiali and others added 2 commits September 22, 2026 19:59
ForcedScratchpadWrite_ReproducesFault calls gdrSupported and a
four-argument RunRecvFlushBurst, neither of which exists, so
rccl-UnitTestsMPI does not build when fault injection is on.

Co-authored-by: Cursor <cursoragent@cursor.com>
…k lands

ForcedScratchpadWrite_ReproducesFault was merge-introduced and never
callable; keep the intent next to the remaining flush tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@amd-jiali
amd-jiali requested review from a team September 22, 2026 20:32
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rccl/test/transport/NetIbMPI/GdrFlushTests.cpp; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM once the comment at GdrFlushTests.cpp:221 drops the "removed" framing for the scratchpad RDMA_WRITE. That one is a wording fix, so it does not need another pass from me.

Comment @argus review to re-run, or @argus re-review after pushing.

@weilewei weilewei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable to me

Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM once the comment at GdrFlushTests.cpp:220-225 is collapsed onto the suggested two lines: the round-1 one-liner was appended rather than substituted, so line 224 breaks off mid-sentence. That is a comment fix, so it does not need another pass from me.

Comment @argus review to re-run, or @argus re-review after pushing.

@amd-jiali
amd-jiali merged commit 21138a4 into develop Sep 29, 2026
85 of 87 checks passed
@amd-jiali
amd-jiali deleted the users/jialili/gdr-flush-mpi-compile branch September 29, 2026 18:12
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants