Repository navigation
Conversation
Cover src/gin/gin_host_proxy.cc (the GIN proxy backend over an RMA plugin: listen/connect, symmetric MR registration with the DMA-BUF retry, context create/destroy, GFD polling, submission and completion) and the versioned GPU-context writers in src/gin/proxy_gpucontext/, in a new rccl-UnitTestsMicroGinHostProxy binary. Pins as observed: the second DMA-BUF fd leaks when both registrations are rejected, backendVersion 0 fails only after every buffer and the RMA context are created (and leaks them), a non-power-of-two maxRecvs gives a non-power-of-two queue, and destroy never frees the signal-offsets buffer. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
![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
5 more finding(s), not posted inline
projects/rccl/test/host/gin-host-proxy-test.cc:858(Low) ginCtx_ left dangling after mid-test destroyContext; a failing Create then makes TearDown destroy it twiceprojects/rccl/test/host/gin-host-proxy-test.cc:49(Low) Outer for loops without braces around a nested if or a wrapped body (lines 49, 1114, 1295)projects/rccl/test/host/gin-host-proxy-test.cc:906(Low) Create_LogsWhenGdrCopyIsUnavailable proves its no-GDR half only through log textprojects/rccl/test/host/gin-host-proxy-test.cc:808(Nit) WriteCombined assertion pins a host_flags argument that gdrwrap.h allocMemCPUAccessible ignoresprojects/rccl/test/host/gin-host-proxy-test.cc:989(Nit) Pinned-bug tests use '// Observed:' instead of the repo's 'LATENT BUG (file:line)' convention
This PR changes 1,552 lines across 7 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: Looks approval ready.
Comment @argus review to re-run, or @argus re-review after pushing.
Null ginCtx_ after every mid-test destroy, brace the nested loops, check the counters pointer the GPU gets with and without GDRCopy, drop the host_flags assertions allocMemCPUAccessible ignores on HIP, and use the LATENT BUG (file:line) convention for the pinned bugs. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
Addressed in 53517f8 (all five, rebuilt, 64/64 and clean × 20 shuffled):
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,558 lines across 7 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.
aduforest
marked this pull request as ready for review
October 15, 2026 22:28
…comrccl-2807-gin-host-proxy-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/host/run_host_tests.sh # projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,558 lines across 7 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.
…comrccl-2807-gin-host-proxy-host-microtests # Conflicts: # projects/rccl/test/host/CMakeLists.txt # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,586 lines across 7 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: Worth settling before approval: gin-host-proxy-test.cc:200 forces NCCL_CUMEM_DMABUF_EXPORT_GATE on and fakes hipMemGetHandleForAddressRange unconditionally, which can stop the host-test build on HIP < 7.0.
Comment @argus review to re-run, or @argus re-review after pushing.
hipMemGetHandleForAddressRange and hipMemRangeHandleType only exist from HIP 7.0, so forcing NCCL_CUMEM_DMABUF_EXPORT_GATE on and faking the call unconditionally broke the host-test build on older HIP. Guard them the way rma_proxy.cc does. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,595 lines across 7 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.
…roxy binary - HIP comes from hip_fakes.cc (hipMemset hook, and a new g_hipMemGetHandleForAddressRange hook for the DMA-BUF export), logging and params from nccl_fakes.cc; the tests install the hooks with ScopedHook and assert on logs with LogCapture. - Pin comments name their Jira ticket instead of production line numbers. - test_executor.py lists only this PR's binary. Co-Authored-By: Claude <noreply@anthropic.com>
…comrccl-2807-gin-host-proxy-host-microtests
Member
Author
|
@argus review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,572 lines across 9 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.
…ms in hip_fakes.cc Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,572 lines across 9 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.
dhanavikram
approved these changes
Oct 20, 2026
mch
previously requested changes
Oct 20, 2026
mch
left a comment
mch
left a comment
Contributor
There was a problem hiding this comment.
Biggest one is the sleep. Adding sleeps to tests generally leads to flakey test. It's better to explicitly signal between threads.
aduforest
dismissed
mch’s stale review
October 24, 2026 01:26
Blocking merge, comments already resolved
…comrccl-2807-gin-host-proxy-host-microtests
The GitHub host CI runs this binary through run_host_tests.sh. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,681 lines across 9 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.
Contributor
pr-rocm-systems-13596-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
pvallem
approved these changes
Oct 25, 2026
…comrccl-2807-gin-host-proxy-host-microtests Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,678 lines across 8 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.
Contributor
pr-rocm-systems-13596-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-13596-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
aduforest
deleted the
users/aduforest/aicomrccl-2807-gin-host-proxy-host-microtests
branch
October 26, 2026 10:45
dhanavikram
added a commit
that referenced
this pull request
Oct 26, 2026
- fakes/ibvwrap_fakes.{h,cc}: take develop's version, which already has the
CQ/QP, GID, ECE and device-name seams this branch added, and keep only
what develop lacks: the g_ibvQueryPkey seam and a plain ibvWrOpcodeStr
next to develop's other string helpers.
- fakes/mlx5dvwrap_fakes.{h,cc}: take develop's version; it carries the
same g_mlx5dvCreateQp seam.
- fakes/hip_fakes.{h,cc}: drop this branch's copy of the
hipMemGetHandleForAddressRange seam; develop's copy (#13596) has the same
name, gate and default.
- MICROTEST_README.md: keep both sides' rows.
Contributor
TheRock Submodule Bump ActivityNewest first
|
1 task done
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.
JIRA ID: AICOMRCCL-2807
What
Host-only microtests for
src/gin/gin_host_proxy.ccand the versioned GPU-context writers insrc/gin/proxy_gpucontext/(gpucontext{,_v1,_v2}.cc), in a newrccl-UnitTestsMicroGinHostProxybinary (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#includes the four hipified units and seams the RMA plugin and the device allocators in the TU;allocMemCPUAccessibleis the realgdrwrap.hhelper, steered byncclGdrCopy.hipHostMalloc,hipMemsetand the DMA-BUF export go throughfakes/hip_fakes.cchooks (the export hook is new, built where CMake findshipMemGetHandleForAddressRange), and logging and params come fromfakes/nccl_fakes.cc. GFDs are posted straight into the host-side queues.ncclGinProxyRegMrSym: when both DMA-BUF registrations fail, it returns before closing the second exported fd.ncclGinProxyCreateContext: acceptsbackendVersion0, fails in the GPU-context writer, and releases nothing it created.ncclGinProxyDestroyContextnever frees the signal-offsets buffer.Coverage
Lines:
gin_host_proxy.cc476/476 with the export gate on (456/467 with it off: the 11 missed lines are the DMA-BUF registration, unreachable there),gpucontext.cc33/33,gpucontext_v1.cc10/10,gpucontext_v2.cc13/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,forceHostingdrwrap.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=50is clean in both.