Repository navigation
test(rccl): cover dense all-to-all ncclPutSignal on the CE path - #12906
Merged
amd-arozanov merged 6 commits intoOct 13, 2026
Merged
Conversation
Every rank puts twice to every other rank in one group and waits for all of them, once with uniform sizes and once with sizes around the CE chunk size. With four or more ranks the uneven profile is the only case that makes the copy engine split a batch into round-robin chunk waves, which no test ran before. The CI batch config runs the test in the 2-rank Host API suites and in a new single-node 8-GPU suite, since chunking needs more than one remote peer per round. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Destination-independent source patterns leave some misplaced chunks undetectable, weakening the stated coverage.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds CI coverage for dense all-to-all ncclPutSignal, including uneven, chunked copy-engine batches.
Changes:
- Adds uniform and uneven dense all-to-all validation.
- Adds 2-rank and dedicated 8-GPU CI coverage.
| File | Description |
|---|---|
projects/rccl/test/HostApiMPITests.cpp |
Implements the new host API test. |
projects/rccl/tools/scripts/test_runner/configs/mi300x_mellanox_ib.json |
Registers the test in CI suites. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Move the 8-rank DenseAllToAllPutSignal run into mpi_8rank_single_node, which already hosts MPI tests that need eight ranks on one node, and drop the dedicated host_api_dense_a2a_8rank config and its suite. Co-authored-by: Cursor <cursoragent@cursor.com>
The puts read a byte pattern that repeated every 256 bytes and did not depend on the destination, so a chunk delivered to the wrong peer or at the wrong chunk offset left the data unchanged. Give each put its own source slot and fill it with 64-bit words that encode the sender, the receiver, the put index and the word index. Chunking already starts at three ranks, so say 3+ in the test header. Co-authored-by: Cursor <cursoragent@cursor.com>
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the 8-rank CI entry pins no env, so the only run that can reach CE chunking goes green just the same when the batch path is unavailable.
Comment @argus review to re-run, or @argus re-review after pushing.
Check that a rank's own receive slots stay untouched, since no put targets them, so a chunk landing there no longer goes unnoticed. Derive the size index and the slot bound from the size array, and assert that every put is a whole number of 64-bit words, which the fill and verify helpers rely on. Name the size profile in failure output. Pin NCCL_CUMEM_ENABLE=1 and RCCL_CE_BATCH_ASYNC_ENABLE=1 on the 8-rank CI entry so it runs on a symmetric window and the batch path it is meant to cover, and stop its description from promising chunking. Co-authored-by: Cursor <cursoragent@cursor.com>
The check after each put read only the 4 KiB guard, so a chunk written to the wrong offset further into the 13 MiB slot went unnoticed. Scan the rest of the slot instead, as the own-rank slot check already does. Co-authored-by: Cursor <cursoragent@cursor.com>
i-kosarev
reviewed
Oct 7, 2026
… fails ASSERT_EQ inside a void helper returns only from the helper, so a failed hipMemcpy let the test put an unfilled buffer and report a data mismatch later. Return the result instead and assert on it once after the fill loop; the collective check makes every rank stop together. Co-authored-by: Cursor <cursoragent@cursor.com>
pvallem
approved these changes
Oct 9, 2026
1 task done
amd-arozanov
deleted the
users/arozanov/aicomrccl-1964-rma-ce-batch-test
branch
October 13, 2026 15:38
Contributor
TheRock Submodule Bump ActivityNewest first
|
ScottTodd
pushed a commit
that referenced
this pull request
Oct 14, 2026
## Summary The Multi-Arch CI summary step passes `toJSON(needs)` inside single quotes. A `ci:*` label is copied into `linux_test_labels` as a Python list such as `['ci: code-coverage']`. The quote inside that value ends the shell string, and the space in the label splits the argument, so `workflow_summary.py` exits with `unrecognized arguments: code-coverage]`. Seen on #13027 and #12906 (`Multi-Arch CI Summary`). Pass that JSON through the `NEEDS_JSON` environment variable and expand `"$NEEDS_JSON"`, so the shell does not tokenize it. The same call is updated in the ASAN and nightly workflows. Issue ID: ROCm/TheRock#8995 ## Test plan - [x] Add the `ci: code-coverage` label and confirm `Multi-Arch CI Summary` gets past argument parsing. This workflow-only change also selects a full multi-arch build. **(AI Asist)** --------- 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.
Motivation
This PR verifies the NCCL 2.30.7 sync feature "batched CE operations in the RMA CE put/wait path", which reached develop through #9924 (Merge rccl/drop_2026-07 to develop). It also covers the round-robin chunking of uneven CE batches that the NCCL 2.32.3 sync (#12321) added on top of it. The PR adds a test and a CI entry only and changes no library code.
Existing host API tests cover a ring put and single-peer groups only, and the host unit tests fake ncclCeLaunchBatchOps, so the chunked batch path in ncclCeLaunchChunkedMemcpyBatchAsync was never executed by any test.
Technical Details
HostApiTest.DenseAllToAllPutSignalintest/HostApiMPITests.cpp. Every rank issues two ncclPutSignal to every other rank inside one group and waits for all of them. It runs a uniform profile (4 MiB per put) and an uneven profile with sizes around the defaultNCCL_CE_CHUNK_SIZE(3, 9 and 12 MiB plus odd tails). Each put has its own source pattern and receive slot followed by a sentinel guard, so a dropped, truncated or misplaced chunk fails the check.mi300x_mellanox_ib.jsonadds the test to the existing 2-rank Host API suites and to the existing 8-rank single-node suiteMPI Tests (8-rank, single-node)(mpi_8rank_single_node).Issue Tracking
JIRA ID: AICOMRCCL-1964
Test Plan
Single node, 8x MI350X (gfx950), ROCm 10.1 nightly (HIP 7.16), debug build with host API tests:
DenseAllToAllPutSignalat 8 and 2 ranks with the batch path on and off (RCCL_CE_BATCH_ASYNC_ENABLE=1/=0) and with chunking disabled or reduced (NCCL_CE_CHUNK_SIZE=0/1048576).HostApiTest.*at 2 and 8 ranks.Test Result
DenseAllToAllPutSignalpasses in all configurations, and the mutation makes it fail.HostApiTest.*passes exceptPutSignalEnqueueIsAsync, which fails on HIP 7.16 with or without this PR (HIP runtime defect, tracked in AICOMRCCL-2279; passes withRCCL_CE_BATCH_ASYNC_ENABLE=0).Submission Checklist