Repository navigation
test(rccl): Validate GIN proxy poll batching and the RMA aggregation hint - #13221
rahulvaidya20 merged 6 commits into
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
🟢 Approval recommended
The tests, example instrumentation, build wiring, and documentation consistently validate the intended behavior.
0 open findings
What changed in this PR
Adds end-to-end validation and documentation for GIN proxy poll batching and RMA aggregation hints.
Changes:
- Adds host microtests and an MPI burst test covering batching and hint propagation.
- Enhances the RMA example plugin with GIN proxy support and operation counters.
- Adds integration coverage and documents the related environment variable and hint semantics.
| File | Description |
|---|---|
projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json |
Updates microtest description. |
projects/rccl/test/transport/GinDeviceMPITests.cpp |
Adds the local-counter put burst test. |
projects/rccl/test/test_categories_micro_gin_host.yaml |
Expands microtest category documentation. |
projects/rccl/test/host/gin-host-proxy-test.cc |
Tests batching and aggregation hints. |
projects/rccl/test/host/CMakeLists.txt |
Builds the new host microtests. |
projects/rccl/test/ext-plugins/tests/ext-rma/test_rma_plugin.py |
Adds end-to-end RMA batching tests. |
projects/rccl/plugins/rma/example/plugin.c |
Adds GIN proxy reporting and counters. |
projects/rccl/plugins/rma/example/nccl/net_device.h |
Defines network-device types for the example. |
projects/rccl/plugins/rma/example/nccl/common.h |
Aligns logging levels with RCCL. |
projects/rccl/docs/how-to/using-rccl-rma-plugin-api.rst |
Documents aggregation-hint semantics. |
projects/rccl/docs/api-reference/env-variables.rst |
Documents the poll-batch setting. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json:257(Low) Two descriptions of rccl-UnitTestsMicroGinHost still name only gin_host.cc after the binary gained a second unit The per-test description atconfigs/ci-precheckin.json:257, the one the runner prints, still reads "Host-only gin/gin_host.cc microtests" and names onlyGIN_PROXY_NTHREADS, while:318was updated.test/host/MICROTEST_README.md:251is stale the same way: it is the standing record of units under test and still lists this binary as coveringgin/gin_host.ccalone, where every other entry there enumerates all its units. Could both pick upgin_host_proxy.ccandGIN_PROXY_POLL_BATCH?
Bottom line: Worth settling before approval: _unit_tests_mpi probes only <RCCL_INSTALL_DIR>/build/..., which the convention the repo's own CI exports never matches, so the new ext-rma test always skips.
Comment @argus review to re-run, or @argus re-review after pushing.
|
Addressed the Argus review in 4 commits:
Kept the lower bound of one hinted op in the ext-rma test. It is the only end-to-end check that the hint reaches the plugin; the host microtests already pin the exact behavior deterministically. A comment now notes it assumes the burst outpaces the proxy, which a full CTA against the 32-slot queue does: 20 of 20 default-batch runs gave 248 hinted ops. Re-validated on MI350X (gfx950, RoCE, ROCm 10.1): host microtests 26/26, ext-rma 6/6 (hinted ops 0, 192 and 248 for batches 1, 4 and 32), the burst test with |
|
Second Argus round:
Not changed:
|
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/docs/how-to/using-rccl-rma-plugin-api.rst:81(Low) The page's description of plugins/rma/example was not updated for the two behaviours this PR gives it At using-rccl-rma-plugin-api.rst:81-83, outside this PR's hunks, the example is still described as only demonstrating "the lifecycle, registration and data-operation callback signatures". It now also reportsNCCL_NET_DEVICE_GIN_PROXYsoNCCL_GIN_TYPE=2selects it (plugin.c:89), and counts hinted data ops and logs them at finalize (plugin.c:167-176), which is the line the new_RMA_EXAMPLE_COUNTSregex depends on (test_rma_plugin.py:293). Since this PR already edits this page, worth a sentence there?
Bottom line: Worth settling before approval: with NCCL_GIN_PLUGIN also set, the RMA example's new NCCL_NET_DEVICE_GIN_PROXY type makes gin.cc skip the built-in proxy, so test_rma_example_backs_the_gin_proxy no longer proves its claim.
Comment @argus review to re-run, or @argus re-review after pushing.
|
Third Argus round:
Not changed:
Re-validated on MI350X (gfx950, RoCE, ROCm 10.1): host microtests 26/26, ext-rma 6/6 (hinted ops 0, 192 and 248 for batches 1, 4 and 32), 20 of 20 default-batch runs at 248, and |
dlamd1dai
left a comment
There was a problem hiding this comment.
The host microtests provide good coverage of batching, hint propagation, flush termination, peer isolation, and slot release. I found no additional correctness or contract defects.
Requesting changes because the ext-RMA test still asserts a timing-dependent positive hint count. Once the existing thread is resolved, this looks ready from a code-review perspective. CI is not currently clean: host tests pass, while MCI extra and TheRock are failing and RCCL precheckin is pending.
dlamd1dai
left a comment
There was a problem hiding this comment.
The ext-RMA test no longer requires a positive hinted-operation count. A batch of one still requires zero hinted operations because its upper bound is zero, and larger batches now assert only the safe upper bound. The host microtests continue to lock the exact hint pattern.
No remaining findings. Approving from a code-review perspective.
45b0bb6 to
989789c
Compare
JIRA ID: AICOMRCCL-2807
## What
Host-only microtests for `src/gin/gin_host_proxy.cc` and the versioned
GPU-context writers in `src/gin/proxy_gpucontext/`
(`gpucontext{,_v1,_v2}.cc`), in a new `rccl-UnitTestsMicroGinHostProxy`
binary (65 tests where rocmwrap.h's DMA-BUF export gate is on, 61 where
it is off).
Overlaps rahulvaidya20's #13221 (same file name, same unit); will
reconcile with it.
## How
- `gin-host-proxy-test.cc` `#include`s the four hipified units and seams
the RMA plugin and the device allocators in the TU;
`allocMemCPUAccessible` is the real `gdrwrap.h` helper, steered by
`ncclGdrCopy`. `hipHostMalloc`, `hipMemset` and the DMA-BUF export go
through `fakes/hip_fakes.cc` hooks (the export hook is new, built where
CMake finds `hipMemGetHandleForAddressRange`), and logging and params
come from `fakes/nccl_fakes.cc`. GFDs are posted straight into the
host-side queues.
- Pins 3 production bugs as observed, not fixed (AICOMRCCL-2991..2993):
- `ncclGinProxyRegMrSym`: when both DMA-BUF registrations fail, it
returns before closing the second exported fd.
- `ncclGinProxyCreateContext`: accepts `backendVersion` 0, fails in the
GPU-context writer, and releases nothing it created.
- `ncclGinProxyDestroyContext` never frees the signal-offsets buffer.
## Coverage
Lines: `gin_host_proxy.cc` 476/476 with the export gate on (456/467 with
it off: the 11 missed lines are the DMA-BUF registration, unreachable
there), `gpucontext.cc` 33/33, `gpucontext_v1.cc` 10/10,
`gpucontext_v2.cc` 13/13.
## Mutation Testing
57/57 killed: 53 in `gin_host_proxy.cc`, 4 in the GPU-context writers.
Review follow-up: 15 more (allocator pairing, `forceHost` in
`gdrwrap.h`, plugin properties, queue-size branches, the per-qword
wait), all killed; dropping the plugin's DMA-BUF flag is caught only
with the gate on, since with it off the flag changes nothing.
## Test Plan
Standalone build (HIP 7.1, gate off): 61/61; with the gate forced on
locally: 65/65. `--gtest_shuffle --gtest_repeat=50` is clean in both.
---------
Co-authored-by: Claude <noreply@anthropic.com>
…oxy host microtests
989789c to
cc4da33
Compare
PR Pundit review(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. Please reply to each point, whether it applies or not: we harvest these replies, and both kinds are enormously helpful for tuning the agent. Reach out to @peymanr for suggestions.)
1. 🟠 Should fix: PutBurst_LocalCounter passes despite corrupted or misdirected remote writes: this PR verifies only local completion
Who is affected: CI runs of Checked against the code by an independent pass. Related past reviews: #10388 on Shared from a pilot of PR Pundit, an agentic code reviewer that builds and tests the change and reads the project's past reviews. Reviewed at |
pr-rocm-systems-13221-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
pr-rocm-systems-13221-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. Performance regressions
|
TheRock Submodule Bump ActivityNewest first
|
Motivation
The GIN host proxy drains up to
NCCL_GIN_PROXY_POLL_BATCHGFDs per peer on each progress tick and passesncclRmaOptFlagsAggregateRequeststo the RMA backend when that peer's next operation is already queued. Neither was documented, the RMA example plugin could not serve as a GIN proxy backend, and nothing showed the hint reaching a backend end to end.Technical Details
rccl-UnitTestsMicroGinHostProxy) with two cases: the hint is set on put-with-signal, get and VA-signal ops, not only plain puts, and puts queued ahead of aniflushare hinted while the flush ends the run.NCCL_NET_DEVICE_GIN_PROXYsoNCCL_GIN_TYPE=2keeps the proxy over it, count data operations and hinted ones and log them at finalize, and pin its log levels to RCCL's numbering at compile time.GinMPIDeviceTests.PutBurst_LocalCounter: 32 threads issue 256 puts to one peer and wait only on a local counter, so it also runs over a backend that moves no data. It skips on non-proxy GIN backends.iput,iputSignalandigeteach count a hinted op.gin_device_get_flushgroups of the MI300X and MI355X functional test-runner configs.NCCL_GIN_PROXY_POLL_BATCHand when the GIN proxy sets the aggregation hint.Issue Tracking
JIRA ID: AICOMRCCL-2018
Test Plan
run_host_tests.shconfigure, build, run).PutBurst_LocalCounterandWaitCounter_Localover the built-in IB GIN proxy backend at poll batches 1 and 32, and check they skip underNCCL_GIN_TYPE=7.-DBUILD_PLUGIN_EXAMPLES=ON.Test Result
PutBurst_LocalCounterandWaitCounter_Localpassed over the IB backend at both batches on MI300X (gfx942, InfiniBand, ROCm 10.0) and MI350X (gfx950, RoCE, ROCm 10.1), and skip underNCCL_GIN_TYPE=7.Submission Checklist