Repository navigation
test(rccl): cover enqueue.cc rearch, validation, topology and NVLS paths in microtests - #14529
Merged
Merged
Conversation
…ths in microtests Add rccl-UnitTestsMicroEnqueue tests for rawTaskAppend and the rearch CE init job, the rmaTaskAppend and WaitSignal argument checks, user red-op datatype and mixed-capture stream rejection, unpaired self send/recv, the pivot-ring, tree-defined and no-tuner arch overrides, the PAT size windows, tuner chunk size clamping, and the CollNet/NVLS channel tuning and net-registration proxy setup compiled into ROCm. ncclMgmtTaskEnqueue and ncclLaunchOneRank become seams in the shared stubs, the enqueue binary links the existing allocator and RMA fakes, and the CE dispatch symbols taskAppend references get fail-loud stubs.
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
sdarisala
approved these changes
Oct 29, 2026
mch
reviewed
Oct 29, 2026
mch
approved these changes
Oct 29, 2026
mch
left a comment
mch
left a comment
Contributor
There was a problem hiding this comment.
Would be best to move that fake to the right place
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
2 more finding(s), not posted inline
projects/rccl/test/host/enqueue-test.cc:4819(Low) ThreePivotBiRings seeds TREE as cheapest, so deletinginfo->algorithm = NCCL_ALGO_TREEat enqueue.cc:3200 still passes; seed the algorithm the override must replace.projects/rccl/test/host/enqueue-test.cc:4509(Low) The FP8 rejection test only uses ncclFloat8e4m3, so dropping ncclFloat8e5m2 from the check at enqueue.cc:4577 still passes; loop over both types.
Bottom line: Worth settling before approval: the RearchCeInitScene enqueue hook keeps the job on failure, which hides a latent ceInitJob double free, and the RMA argument table has no passing baseline case.
Comment @argus review to re-run, or @argus re-review after pushing.
The seam now lives in fakes/mgmt_task_enq_fakes.{h,cc}, named after its
owning production TU, with a signature assertion against the production
declaration. nccl_stubs.cc no longer defines the symbol, so the
RCCL_STUBS_OMIT_ncclMgmtTaskEnqueue guard is dropped and
rccl-UnitTestsMicroMgmtTaskEnq links the real file without an omission.
…ides RmaArgsScene now owns per-context RMA queues, and two new tests run it unbroken: a PutSignal that queues one task with the computed window offsets, and a WaitSignal whose descriptors are grouped into one task per context. The three-pivot test seeds the opposite algorithm and protocol as cheapest so the override has to replace both, and the FP8 rejection test covers both FP8 types.
rmaTaskAppend and rawTaskAppend each carried a copy of the WaitSignal argument checks, and the copies differed on the count check: a negative nDesc was rejected on the rearch path but reached the default path, which queued nothing and returned success. validateWaitSignalDescs now owns the checks for both paths, so a negative nDesc is rejected with ncclInvalidArgument on each, and the shared test table gains that row.
ncclMgmtTaskEnqueue frees the job when it fails, and ceCollTaskAppend then freed it again on its fail path. The job pointer is now cleared before the result is checked, matching ncclDevCommCreate. The test hook for ncclMgmtTaskEnqueue frees the job on failure as production does, so the enqueue failure test now exercises that ownership hand-off.
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,139 lines across 13 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
1 task done
1 task
alex-breslow-amd
added a commit
that referenced
this pull request
Oct 30, 2026
…14633) ## Motivation `develop` fails to build the rccl host microtests: the comm-libs stage of TheRock Multi-Arch CI has failed on every rccl merge since 6c4f559 (for example [this run on 723d405](https://github.com/ROCm/rocm-systems/actions/runs/40959195378)): ``` test/host/fakes/rccl_wrap_fakes.cc:438:6: error: redefinition of 'rcclCeAllReduceEnabled' test/host/fakes/rccl_wrap_fakes.cc:441:6: error: redefinition of 'rcclForceCeAllReduceEnabled' ``` #14529 added fail-loud stubs for both functions, and #13098, which merged about ten hours later, added faked-value definitions of the same two functions without removing the stubs. Each PR built on its own. ## Technical Details * Removes the fail-loud stubs and keeps the faked-value definitions from #13098. `ce-coll-test.cc` sets `g_rcclParamCeAllReduce` and relies on `rcclCeAllReduceEnabled()` honoring it, so the fail-loud versions would abort that test. * These were the only duplicate definitions in the CI log, and no other file under `test/` defines either function. ## Issue Tracking ## Test Plan * TheRock Multi-Arch CI comm-libs stage on this PR (builds `rccl-UnitTestsMicroCollectives`, `rccl-UnitTestsMicroWarpSpeed` and `rccl-UnitTestsMicroInit`, the three targets that failed). ## Test Result Pending CI. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. Co-authored-by: Cursor <cursoragent@cursor.com>
1 task
alex-breslow-amd
added a commit
that referenced
this pull request
Oct 30, 2026
## Motivation After #14633 fixed the `rccl_wrap_fakes.cc` redefinition, the "RCCL host unit tests" job still fails on `develop`, this time at link time for `rccl-UnitTestsMicroEnqueue` and `rccl-UnitTestsMicroEnqueue-devlinker` ([run](https://github.com/ROCm/rocm-systems/actions/runs/40979204833)): ``` ld.lld: error: undefined symbol: rcclCeArNonRegCap(ncclComm const*) >>> referenced by enqueue.cc:4742 ... (taskAppend(ncclComm*, ncclInfo*)) ``` #13098 switched `taskAppend()`'s unregistered CE AllReduce check from `rcclCeAr2ShotMax()` to `rcclCeArNonRegCap()`. The enqueue microtests added in #14529 link `enqueue.cc` against `rccl_wrap_fakes.cc`, which only fakes the old function. ## Technical Details * Adds a fail-loud `rcclCeArNonRegCap()` fake next to `rcclCeAr2ShotMin()`. The enqueue microtests never reach that branch: in #14529 it sat behind a fail-loud `rcclCeAllReduceEnabled()` and those tests passed. * No other file under `test/` defines `rcclCeArNonRegCap`, so this can't reintroduce a redefinition. ## Issue Tracking ## Test Plan * "RCCL host unit tests" and TheRock Multi-Arch CI on this PR. ## Test Result Pending CI. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
TheRock Submodule Bump ActivityNewest first
|
PJAvinash
added a commit
that referenced
this pull request
Nov 5, 2026
## Motivation Repeated creation and destruction of 8-rank, one-process-per-GPU communicators can hang during DDA IPC initialization. The blocked ranks remain inside: `hipIpcOpenMemHandle -> hsa_amd_ipc_memory_attach -> IPCRecvHandle` This was reproduced when one collective test completed and a subsequent test created another MP-8 communicator. ## Technical Details DDA IPC teardown could free a rank's exported scratch or barrier allocation while peer ranks still retained imported IPC mappings to that allocation. `commReclaim` runs `commFree` serially per local rank with no synchronization between ranks, so one rank's free could precede a peer's mapping close. The IPC handler also only closed mappings after a completely successful exchange. If `hipIpcOpenMemHandle` failed partway through an exchange, mappings opened before the failure were not closed. This left the HIP/ROCr IPC lifecycle improperly ordered and could cause a later IPC import to hang. ## Changes * Close all imported DDA scratch and barrier mappings before any rank frees its exported allocations: * Destroy and abort: in `commDestroySync`, which runs concurrently for every local rank, before its existing intranode barrier. * Revoke: in `commReclaim`'s revoked-communicator stage, for every local rank, before the serial `commCleanup` loop. Revoke stays non-collective. * Before unmapping, wait on `comm->doneEvent` so DDA kernels still running on the user stream cannot touch an unmapped peer address. A failed wait logs a warning and cleanup continues. * No new synchronization between ranks: `commFree` / `ncclDdaIpcCommFini` stay non-collective, so destroying a communicator whose ranks share a process cannot deadlock. * `ncclDdaIpcCommFini` is best-effort, closes any remaining mappings itself (init-failure and other direct `commFree` paths), and always completes the remaining cleanup. * Make peer-mapping cleanup idempotent. * Close mappings created by partially completed IPC exchanges. * Add the DDA IPC lifecycle microtest binary (`rccl-UnitTestsMicroDdaIpcLifecycle`) to `run_host_tests.sh`. Known limit: with one process per GPU, revoke has no cross-process synchronization by design, so the revoke ordering covers ranks within a process. [internal-env][no-behavior-change] ## Issue Tracking JIRA ID : AICOMRCCL-3064 ## Test Plan Added host-only DDA IPC lifecycle tests (`rccl-UnitTestsMicroDdaIpcLifecycle`) covering: * Eight-rank normal destroy: mappings closed before serial `commFree`, no allocation freed while still mapped. * Closing mappings waits on each rank's `doneEvent` before the first unmap, and a failed wait does not stop cleanup. * `ncclDdaIpcCommFini` without a prior close (init-failure path) stays local and releases every mapping and allocation. * Peer-close failure is best-effort and cleanup completes. * Partial `hipIpcOpenMemHandle` failure cleanup and idempotent close. * `RCCL_DDA_ENABLE=0` still allocates DDA scratch (required by CE-scratch AllGather/AlltoAll). Added init tests covering: * `commDestroySync` ordering (close mappings -> intranode barrier -> proxy stop), including aborted comms. * `commReclaim` closing all local ranks' mappings before serial cleanup, for normal and revoked communicators. ## Test Result * Host microtest binaries (every `rccl-UnitTestsMicro*` target): 80 run, 79 pass. The only failing test, `EnqueueMicrotest.TopoGetAlgoInfo_ThreePivotBiRings_PicksTreeLlUpToTheCutoffThenRingSimple`, also fails on develop (from #14529). Develop's new `rccl-UnitTestsMicroHostDispatch` (#14121) does not compile with ROCm 7.2.1 (`nlohmann/json.hpp` conversion error); this PR does not touch it. * DDA IPC lifecycle: 7/7. Init microtests: 745/745. DDA IPC: 36/36. * MP-8 DDA-enabled AllReduce and Reduce, `--gtest_repeat=5`: 5/5 iterations passed (no hang or fault). * MP-8 with `RCCL_DDA_ENABLE=0`, AllGather and AlltoAll: passed; `ddaScratch` allocated on all ranks. * MP-2 AllReduce, `--gtest_repeat=3`: 3/3 passed. * The new revoke and `doneEvent` tests were each confirmed to fail with their fix removed. ## Submission Checklist * Look over the contributing guidelines at <https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md>. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Host microtests never reach the enqueue.cc rearch path (rawTaskAppend and the async CE init job), the RMA and WaitSignal argument checks, the no-tuner arch overrides, or the NVLS and CollNet arms compiled into ROCm, so regressions there only surface on GPU runs, if at all.
Fix: add rccl-UnitTestsMicroEnqueue tests that call those helpers directly and assert the queued task fields, the exact warning and return code of each rejected argument, and the algorithm, protocol, channel and proxy-op values each override selects, with production code unchanged. ncclMgmtTaskEnqueue and ncclLaunchOneRank become seams in the shared stubs, and the binary links the existing allocator and RMA fakes.
Test plan: full run_host_tests.sh run passes on a CPU-only host; enqueue.cc line coverage from the host binaries goes from 1322 to 1769 lines; deliberately broken copies of each covered condition make the new tests fail (see table below).