Repository navigation
test(rccl): add host-only unit tests for gin.cc and wqe_lat_mon.{cc,h} - #13980
Conversation
JIRA ID: AICOMRCCL-2811 Adds deterministic host-only coverage for the net_ib GIN proxy backend and WQE post-to-poll latency monitor to rccl-UnitTestsMicroTransport. The suites cover GDR gating, ring collectives, connect/context/MR lifecycle, RMA batching and WR construction, completion/flush behavior, WQE tracking, thresholds, reporting, and P2/Welford statistics. Reuses the existing transport binary and shared fakes, with TU-local seams only where sibling real definitions require isolation. Production findings are pinned as disabled forward specifications under AICOMRCCL-2397 children AICOMRCCL-3094 and AICOMRCCL-3131. 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. |
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
There was a problem hiding this comment.
ARGUS
Findings
5 more finding(s), not posted inline
projects/rccl/test/host/transport/net_ib/gin-test.cc:390(Low) DevIsMlx5 out-of-range member check passes even without thephys >= ncclNIbDevsbound (ncclIbDevs[2] is zero, i.e. IB_PROVIDER_NONE)projects/rccl/test/host/transport/net_ib/wqe-lat-mon-test.cc:421(Low) Snapshot test pins p99/p99.9 == 300 (the median) at n=5 while max is 500, locking in the P2 warm-up discontinuity as correctprojects/rccl/test/host/transport/net_ib/wqe-lat-mon-test.cc:631(Low) Error-returning g_ncclSocketGetAddr stub in the gate-disabled test hides a gate bypass that the fail-loud default would catchprojects/rccl/test/host/transport/net_ib/gin-test.cc:106(Low) Default gin hook lambdas are written twice: at declaration (92-106, 128-146) and again in ResetGinHooks (179-205), which SetUp already callsprojects/rccl/test/host/transport/net_ib/wqe-lat-mon-test.cc:107(Low) tracking/trackedPostNs/inflight arming is hand-written at ~10 sites; an ArmTracking helper shared with RecordSample would remove ~20 lines
This PR changes 2,227 lines across 4 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 once the "n=1" check at wqe-lat-mon-test.cc:682 is tightened so it cannot match qpn=100.
Comment @argus review to re-run, or @argus re-review after pushing.
Anchor the latency summary check on its count field so qpn=100 cannot satisfy it, let the gate-disabled summary test reach the fail-loud socket fake, and check only percentile ordering at the five-sample P2 switch while a new test pins the exact path below it. Give DevIsMlx5's out-of-range member an mlx5 provider so only the device bound rejects it. Share one default for the gin hooks between their declarations and ResetGinHooks, and arm WQE tracking through one ArmTracking helper. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the ARGUS review in cd9cda2:
Six targeted mutants are killed, and the transport binary now has 456 tests. The branch also merges the updated #13973 head. |
There was a problem hiding this comment.
ARGUS
This PR changes 2,212 lines across 4 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.
Replace the GIN suite's local post_send/poll_cq recorder with fakes/ibv_ops_fakes, which the other net_ib suites in this binary now share. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
ARGUS
This PR changes 2,161 lines across 4 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.
…port binary out of CI precheckin
- AllGather checks the send and receive registrations each post uses. - Connect runs with GIN_IB_TC both unset and set. - CreateContext moves onto a shared scene: the single-rank test counts the listen and traffic-class calls and covers both batch-size clamps, and a six-rank, stride-2 case checks the handle each connect gets and the slot each accept fills. - IFlush counts the flush-QP creation. - GinDataScene gains PostPut and QueueCompletion for the put tests. - InitType reads its context as develop's ncclIbNetCtx.
…k seam - The slow, stall and summary tests count ncclSocketGetAddr calls, which every report makes through getPeerInfo, instead of matching log text. socket_fakes' GetAddr default returns an error rather than aborting, so the count is what shows a summary that slipped past a guard. - The stall scan runs on four QPs, so device 1's interleaved QPs (1 and 3) differ from a blocked layout's (2 and 3). - The ascending and descending snapshot tests are one loop over both orders. - libc_seam.h maps clock_gettime to a g_clockGettime hook in libc_fakes (default: the real clock). This test and net-ib-cast-scheduler-test.cc now share it instead of their TU-local seams.
There was a problem hiding this comment.
ARGUS
This PR changes 2,242 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.
…oute each side builds
…3755, #13835 and #14381, one verbs recorder #13835 landed the same clock_gettime seam (g_clockGettime, micro_clock_gettime) on develop, so the four libc hunks here go and the scheduler and wqe_lat_mon tests use develop's. #13918 folded fakes/ibv_ops_fakes into IbVerbsScene.h, so GinDataScene builds on a scene: its data and flush QPs, CQ and PD are the scene's, and the recorder checks read scene.verbs.
…atency checks - gin: hooks are ScopedHooks, or HookOf members of GinCreateContextScene; HookOf moves into ScopedHook.h from IbVerbsWorld.h and diag-p2p-test.cc. HostAllocLedger gains Malloc (ncclIbMalloc) and a fail-on-Nth-call knob, and replaces the file's own allocation tracker. The GDR, PeerMem and DMA-BUF probes rename onto NetIbSupportSeams.h, which gains PeerMem, and the fixture resets through NetIbResetGlobals(). - gin: DestroyContext closes every peer of every context; CreateContext runs a ring barrier per peer, treats stride 0 as every rank, connects with GIN_IB_TC over IB_TC and listens into its own handle slot; Connect publishes dev, allGather and allToAll; the all-gather sends and receives on the right comms; all-to-all gathers nranks * len; a disabled flush returns no request and an enabled one drains the batch first; the signal SGE carries the scratchpad lkey. Two duplicate halves go. - wqe_lat_mon: percentile targets are written out; Init follows IB_WQE_LATENCY_REPORT; P^2 estimates are pinned exactly; the summary pins its mean/std/percentile order; a zero-device stall scan reads no clock. - wqe_lat_mon: develop's NetIbWqeLatMonUnitTests.cpp goes, with its CMake line and runner entry; its exact pins, 10k-sample checks, pipelined walk and extra field checks move here.
Restores test/transport/NetIbMPI/NetIbWqeLatMonUnitTests.cpp, its line in test/CMakeLists.txt and the net_ib_wqe_lat_unit entry in tools/scripts/test_runner/configs/net_ib_transport.json exactly as they are on develop, and drops the README sentence that described their removal. This PR changes host tests only; the checks wqe-lat-mon-test.cc gained stay.
There was a problem hiding this comment.
ARGUS
This PR changes 2,357 lines across 10 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.
Co-authored-by: Dhanavikram Sekar <dhanavikram@users.noreply.github.com>
Co-authored-by: Dhanavikram Sekar <dhanavikram@users.noreply.github.com>
TheRock Submodule Bump ActivityNewest first
|
Motivation
src/transport/net_ib/gin.cc,wqe_lat_mon.cc, andwqe_lat_mon.himplement the host-side IB GIN proxy backend and per-QP WQE post-to-poll latency monitor. They had no host-only microtest coverage, so collective setup, RMA batching, completion behavior, latency state transitions, and failure paths required the larger runtime or RDMA hardware.This PR is stacked directly on PR 13973.
Technical Details
Extends the existing
rccl-UnitTestsMicroTransportbinary and mirrors both production units undertest/host/transport/net_ib/.NetIbGinMicrotestcovers:NetIbWqeLatMonMicrotestcovers:The tests reuse the existing transport binary and the generic NCCL and socket fakes. The GIN data path posts and polls on an
IbVerbsScene, the unit'sncclIbMalloc/ncclCalloc/freego throughHostAllocLedger.h(which gains a page-alignedMallocand a fail-on-Nth-call knob), and its GDR, PeerMem and DMA-BUF probes rename ontotransport/net_ib/NetIbSupportSeams.h. Hooks areScopedHooks;HookOf, the member form, moves intoScopedHook.h. TU-local seams isolate only dependencies whose real definitions are already owned by sibling transport tests.clock_gettimegoes through develop'sg_clockGettimeseam (#13835) infakes/libc_seam.h, whichnet-ib-cast-scheduler-test.ccnow uses in place of its own TU-local seam. Runner, category, and coverage-object registration come from PR #13858 because this uses the samerccl-UnitTestsMicroTransportbinary; like the other host-only microtests it stays out ofci-precheckin.jsonafter #14381.Production defects discovered by these tests are preserved as disabled forward specifications and tracked only under AICOMRCCL-2397, the designated host-only incidental-findings epic.
Issue Tracking
JIRA ID: AICOMRCCL-2811
Test Plan
run_host_tests.sh allon the PR 13973 head.rccl-UnitTestsMicroTransportbuilds.--gtest_shuffle --gtest_repeat=100.Test Result
gin.cc, 96.93% forwqe_lat_mon.cc, 98.23% combined.Submission Checklist