Repository navigation
test(rccl): add host-only unit tests for src/transport/net.cc - #13548
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.
ARGUS
Findings
5 more finding(s), not posted inline
projects/rccl/test/host/net-test.cc:203(Low) No test uses a known vendor with a non-matching device id, so the device-prefix check at net.cc:300 is unpinnedprojects/rccl/test/test_categories_micro_net.yaml:3(Low) test_categories_micro_net.yaml header claims setup/connect/proxy-progress/buffer-pool coverage the suites do not haveprojects/rccl/test/host/net-test.cc:148(Low) ReadIbNicRate_SeveralPorts puts the max rate in the last port, so a first- or last-parsable regression can pass depending on readdir orderprojects/rccl/test/host/net-test.cc:217(Low) Detect_MissingPciIds only covers both id files missing, so a||to&&change at net.cc:294 still passesprojects/rccl/test/host/net-test.cc:21(Nit) net-test.cc uses std::function, std::vector, std::pair and std::call_once without including <functional>, <vector>, <utility>, <mutex>
Bottom line: Looks approval ready.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
ARGUS
This PR changes 5,560 lines across 22 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.
8f05b1b to
8614108
Compare
There was a problem hiding this comment.
ARGUS
This PR changes 5,569 lines across 23 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.
8614108 to
80878a9
Compare
There was a problem hiding this comment.
ARGUS
This PR changes 5,569 lines across 23 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.
80878a9 to
158bcc8
Compare
There was a problem hiding this comment.
ARGUS
This PR changes 5,569 lines across 23 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.
158bcc8 to
a5da1f9
Compare
There was a problem hiding this comment.
ARGUS
This PR changes 5,569 lines across 23 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.
a5da1f9 to
2cb1366
Compare
Co-authored-by: Dhanavikram Sekar <dhanavikram@users.noreply.github.com>
Drop this PR's copy of the hipMemGetHandleForAddressRange fake; develop now carries the same hook. Fake rcclNetRegSegmentCount, rcclNetGraphRegSegmentCount and rcclNetRegCommit in rccl_wrap_fakes, which net.cc's register paths call since #10885, and cover them. Build the Net target with _MICRO_DMABUF_EXPORT_DEF so standalone runs the cuMem DMA-BUF arms too.
c71fdc2 to
35d5f53
Compare
There was a problem hiding this comment.
ARGUS
This PR changes 5,658 lines across 23 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.
pr-rocm-systems-13548-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-13548-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13548-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
pr-rocm-systems-13548-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-13548-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-13548-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
pr-rocm-systems-13548-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
… of transport_net_ib_fakes Follow #13995 in net.cc: fake ncclIbSetListenGdrFlushFlag, ncclIbGetGpuFlushFlag, ncclIbGetGpuFlushFlagSeq and ncclIbSetGpuFlushFlagHint, give the fixture a p2p LL128 size, and cover the listen flag, the per-step sequence allocation and its gates, its publication instead of a flush, and its free. Move net.cc's other IB peers to their owners' fakes: netIbCast and rcclCastNetP2pPolicy to net_ib_cast_common/connect_fakes, and rcclNetP2pPolicy to transport_net_ib_p2p_policy_fakes, which targets that compile the real net_ib/connect.cc leave out. transport_net_ib_fakes.h forward-declares the IB types: init_fakes.h includes it ahead of init-test.cc's free() redirect.
…#14299) ## Motivation #13863 skips the Ruby and OCI coco smoke runs for PRs to develop that only touch `projects/rccl/test/host/*`. But a PR that adds or extends a micro host binary also has to register it outside `test/host/`, so it still books both clusters: - `projects/rccl/test/test_categories_micro*.yaml`: the binary's CTest categories, read by the in-tree `test/host` CMake. - `projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json` and `lib/test_executor.py`: math-ci precheckin and coverage registration. Open examples that run coco only because of these files: #13548, #13755, #13835, #13857, #13858, #13642, #13116. ## Technical Details `rccl_change_sets.py` `COVERAGE_RULES`, develop only: - **"host-only unit tests"** also covers `projects/rccl/test/test_categories_micro.yaml` and `test_categories_micro_*.yaml`. The GPU suites' `test_categories.yaml`, `_fixtures`, `_fixtures_debug`, `_mpi`, `_amdsmi` and `_altrsmi` do not match and still trigger coco, as would a future `test_categories_microbench.yaml`. - **New "test runner" rule**: `projects/rccl/tools/scripts/test_runner/*`. This skips coco for any change confined to `test_runner/`, not only registrations. `rccl-host-unit-tests.yml` runs the test_runner unit tests on every PR to develop touching `projects/rccl/`. `ci-precheckin.json` itself is exercised only by math-ci precheckin. It is a separate rule from host-only tests so each keeps its own examples and justification. Why coco loses nothing here: - Its PR jobs in ROCm/tRCCL (`ruby64-`/`oci-rccl-pr-build`, then `-pr-smoke`) run harness-owned `smoke-test.yaml` and `rccl-perf-regression.yaml`, plus `pr-device-api.yaml` on Ruby: GPU `rccl-UnitTests`/`Fixtures`/`Gdr` gtests and rccl-tests. - Their `install.sh -t` build does configure `test/host` in-tree and parse the micro category files. A parse error there is only a CMake warning, and TheRock Multi-Arch CI (a required check, no path filter) runs the same in-tree configure. - No PR job runs the micro binaries, CTest labels, or anything under `tools/scripts`. No committed tRCCL job names a test_runner JSON, and the harness never executes `test_executor.py`. Also corrects #13863's comment that the in-tree `test/host` build is compiled only by TheRock (coco's build compiles it too), and updates the `rccl-host-unit-tests.yml` header for the second rule. Unchanged: TheRock Multi-Arch CI, math-ci precheckin, and the label-gated AINIC TW / dispatch-only MI350P workflows that use the test runner. ## Issue Tracking JIRA ID: N/A. Follow-up to #13863. ## Test Plan - `python3 -m pytest .github/scripts/tests/*rccl*_test.py` (what `rccl-cluster-runner-allowlist.yml` runs). - New cases: registration paths skip on develop; GPU category files still match; a full new-micro-binary file list emits `rccl=false`; look-alikes (`test_categories.yaml`, `test_categories_micro.yml`, `test_categories_microbench.yaml`, `test_runner_x/`, `tools/test_runner/`) stay uncovered. - Mutation check on a scratch copy: reverting, dropping or widening each pattern, or pointing a rule at another workflow, fails the tests. - Ran `coco_paths` on the changed files of the 7 PRs above and on every `test/test_categories*.yaml` on develop. ## Test Result - 120 passed, 149 subtests passed. - All 7 PRs above now skip coco; 42 micro category files are covered and the 6 GPU category files still trigger coco. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
Move rcclNetP2pPolicy and the four GDR flush-flag accessors net.cc calls into transport_net_ib_connect_net_fakes, which targets that compile the real net_ib/connect.cc leave out. Cover the flag path's empty-step and no-flush gates and the reset after a failed sequence allocation, and name what each net_ib fakes file holds in the README.
# Conflicts: # projects/rccl/test/host/fakes/rccl_wrap_fakes.h
There was a problem hiding this comment.
ARGUS
This PR changes 6,053 lines across 28 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.
- fakes/transport_net_ib_fakes.{h,cc}: develop (#13995) now carries this
file for the Init and Diagnostics binaries; keep its GDR flush-flag
seams and add this branch's connect.cc fakes. The header takes the same
forward-declaration form as #13548, so init_fakes.h keeps working.
- ncclNetIb moves to the new fakes/transport_net_ib_common_fakes.cc, owned
by common.cc. Init links it; Transport compiles the real common.cc and
leaves it out.
- #13995 points the ncclNetIb vtable at ncclIbNetListen (connect.cc), so
the connect.cc fakes gain a fail-loud ncclIbNetListen.
The RCCL host unit tests workflow already runs it; host-only microtests are leaving ci-precheckin.json.
There was a problem hiding this comment.
ARGUS
This PR changes 6,036 lines across 27 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.
## Motivation `src/transport/coll_net.cc` (the CollNet transport: setup, connect, proxy progress, buffer registration and the comm-level CollNet setup) had no host-only tests, so its control paths were only exercised by runs on CollNet hardware. ## Technical Details Stacked on #13548 (`net.cc` host tests), which adds the topology and DMA-BUF seams this PR reuses; it will be rebased onto `develop` once that lands. New test TU `coll-net-test.cc` in the existing `rccl-UnitTestsMicro` binary. It `#include`s the hipified `coll_net.cc` and drives it through a fake `ncclCollNet_t`. Fakes: - `RCCL_STUBS_OMIT_COLL_NET` fences the CollNet stubs in `transport_stubs.cc` (`ncclCollNetSetup`) and `collective_stubs.cc` (the chain/direct buffer setups), since `rccl-UnitTestsMicro` now compiles the real ones. - `transport_stubs.cc` gains the GDRCopy params (pinned to their `net.cc` defaults) and seams for `ncclTransportP2pConnect`, the CollNet setup/check/free, `initCollnetChannel` and `ncclTransportInitRankMap` (default unchanged). - `rccl-UnitTestsMicro` now also links `rocmwrap_fakes.cc` and `topo_stubs.cc`. Covered: - `canConnect`, the DMA-BUF mapping flags, `collNetDumpMap`, the `collNetTransport` table - `send/recvProxySetup` and the shared collective comms and buffer pool (`sharedListen`, `sharedConnect`, `sharedFree`, `sharedBuffersInit/Destroy`) - `sendSetup`/`recvSetup`, `sendConnect`/`recvConnect`, `sendFree`/`recvFree` - `send/recvProxyConnect` (host or device pool, GDRCOPY, HSA and cuMem DMA-BUF with their fallbacks) and `send/recvProxyFree` - buffer registration: `ncclCollnetLocalRegisterBuffer`, `ncclCollnetGraphRegisterBuffer`, `ncclCollnetDeregBuffer`, `send/recvProxyRegBuffer`, `send/recvProxyDeregBuffer` - `calcAlgoOffset`, `calcRegionOffset` and the six collective issue helpers - `sendProxyProgress`, `collNetRecvFlush` and `recvProxyProgress` - `ncclCollNetSetup`, `ncclCollNetChainBufferSetup`, `ncclCollNetDirectBufferSetup`, `isCollNetChainHeadsSubset` The new suites are registered with one `CollNet*` pattern in `test_categories_micro.yaml`, and `MICROTEST_README.md` lists the unit. ## Issue Tracking JIRA ID: AICOMRCCL-2802 ## Test Plan - `run_host_tests.sh` (all phases). - `rccl-UnitTestsMicro --gtest_filter='CollNet*'` with `--gtest_shuffle --gtest_repeat=3`. - Also built in-tree (`BUILD_TESTS=ON`), which compiles the uncached-memory and cuMem DMA-BUF arms the standalone build leaves out. - Each commit builds and passes `rccl-UnitTestsMicro` on its own. - Mutation testing: changed the hipified `coll_net.cc` in about 1,140 places per build mode (standalone, in-tree and an ASan build) to check the tests catch each change. ## Test Result - All host binaries pass standalone and in-tree; `rccl-UnitTestsMicro` runs 2163 tests standalone (206 CollNet) and 2180 in-tree (214 CollNet). - 99.3% line coverage of `coll_net.cc`; the 10 lines not run are unreachable (e.g. the `fail:` fd closes, where the fd is always already -1). - Every surviving code change is equivalent (log text, a memory fence, a redundant bound), compiled out of that build, unreachable, or an out-of-bounds access only ASan sees. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Dhanavikram Sekar <dhanavikram@users.noreply.github.com>
## Motivation `src/transport/net.cc` (the NET transport: setup, connect, proxy progress and shared buffer pools) had no host-only tests, so its control paths were only exercised by multi-node runs. ## Technical Details New `rccl-UnitTestsMicroNet` binary. Its test TU `#include`s the hipified `net.cc` and drives it through a fake `ncclNet_t`. It needs its own binary because `transport_stubs.cc`, which most targets link, defines `rcclUseAinic` and `netTransport`, and `net.cc` defines both. `net.cc`'s other externals become seams or fail-loud floors in the fakes file that owns each one (`topo_stubs`, `proxy_fakes`, `os_fakes`, `rocmwrap_fakes`, `transport_net_ib_fakes`, `transport_p2p_fakes`, `nccl_stubs`, `hip_fakes`). `hipMemGetHandleForAddressRange` is faked only under `RCCL_CUMEM_DMABUF_EXPORT_SUPPORTED`, which in-RCCL builds inherit from the `rccl` target. Covered: - IB NIC detection from a temporary sysfs tree, the network attribute hints, `canConnect`, flush-type names, `ncclNetGetDeviceHandle` - the shared p2p buffer pool (`sharedNetBuffersInit/Get/Destroy`, `proxySharedInit`) - `sendSetup`/`recvSetup`, `send/recvProxySetup`, `sendConnect`/`recvConnect`, `sendFree`/`recvFree` - `send/recvProxyConnect` (dedicated and shared buffers and comms, GDR, GDRCOPY, HSA and cuMem DMA-BUF, device offload) and `send/recvProxyFree` - buffer registration: `ncclNetLocalRegisterBuffer`, `ncclNetGraphRegisterBuffer`, `ncclNetDeregBuffer`, `send/recvProxyRegBuffer`, `send/recvProxyDeregBuffer` - `sendProxyProgress` and `recvProxyProgress` `rocmwrap_fakes.cc` now owns the `hsa_status_string` fake, so `rccl-UnitTestsMicroRmaProxy` (which already links it) drops its local copy. The binary is registered in `CMakeLists.txt`, `run_host_tests.sh`, `test_categories_micro_net.yaml` and `test_executor.py` (not `ci-precheckin.json`: host-only microtests run in the RCCL host unit tests workflow instead), and `MICROTEST_README.md` lists it. ## Issue Tracking JIRA ID: AICOMRCCL-2801 ## Test Plan - `run_host_tests.sh` (all phases) on develop f94e399. - `rccl-UnitTestsMicroNet` with `--gtest_shuffle --gtest_repeat=3`. - Also built in-tree (`BUILD_TESTS=ON`), which compiles the cuMem DMA-BUF arms the standalone build leaves out. - Mutation testing: changed the hipified `net.cc` in 524 places to check the tests catch each change. ## Test Result - All 20 host binaries pass standalone (Net and RmaProxy also checked in-tree before this rebase); Net: 270 tests standalone, 281 in-tree. - 516 of 524 code changes caught. The 8 left change nothing observable (e.g. dropping `setXferNetAttrs`'s null-plugin guard, which `setNetAttrs` repeats). - ~98% line coverage of `net.cc`. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Dhanavikram Sekar <dhanavikram@users.noreply.github.com>
transport_net_ib_fakes.cc: develop's #13548 copy of the flush-flag and GidInfoQuery fakes matches this branch, so only the fail-loud listen, connect, accept and close stubs are this branch's; ncclNetIb stays in transport_net_ib_common_fakes.cc, which rccl-UnitTestsMicroNet (net-test.cc reads and assigns ncclNetIb) now links. MICROTEST_README.md: one row per net_ib fake file, with #13548's connect_net_fakes row.
…and #14381 hsa_status_string: #13548 put a copy in rocmwrap_fakes.cc. As promised in review, hsa_fakes.cc stays the one owner; rccl-UnitTestsMicro (coll-net-test's rocmwrap.h HSACHECK) and rccl-UnitTestsMicroNet (net.cc), which used the rocmwrap copy, link hsa_fakes.cc instead. ncclParamDmaBufEnable: rocmwrap.cc owns it, so rocmwrap_fakes.cc keeps the fake and the copy develop added to dev_runtime_micro_fakes.cc goes; rccl-UnitTestsMicro links both files. rcclParamForceEnableDMABUF is develop's copy and pin. The DmaBufEnable pin is a _rccl_cast_pins row. connect-test.cc: ncclSocketClose comes from os_fakes after #13755.
TheRock Submodule Bump ActivityNewest first
|
Motivation
src/transport/net.cc(the NET transport: setup, connect, proxy progress and shared buffer pools) had no host-only tests, so its control paths were only exercised by multi-node runs.Technical Details
New
rccl-UnitTestsMicroNetbinary. Its test TU#includes the hipifiednet.ccand drives it through a fakencclNet_t. It needs its own binary becausetransport_stubs.cc, which most targets link, definesrcclUseAinicandnetTransport, andnet.ccdefines both.net.cc's other externals become seams or fail-loud floors in the fakes file that owns each one (topo_stubs,proxy_fakes,os_fakes,rocmwrap_fakes,transport_net_ib_fakes,transport_p2p_fakes,nccl_stubs,hip_fakes).hipMemGetHandleForAddressRangeis faked only underRCCL_CUMEM_DMABUF_EXPORT_SUPPORTED, which in-RCCL builds inherit from therccltarget.Covered:
canConnect, flush-type names,ncclNetGetDeviceHandlesharedNetBuffersInit/Get/Destroy,proxySharedInit)sendSetup/recvSetup,send/recvProxySetup,sendConnect/recvConnect,sendFree/recvFreesend/recvProxyConnect(dedicated and shared buffers and comms, GDR, GDRCOPY, HSA and cuMem DMA-BUF, device offload) andsend/recvProxyFreencclNetLocalRegisterBuffer,ncclNetGraphRegisterBuffer,ncclNetDeregBuffer,send/recvProxyRegBuffer,send/recvProxyDeregBuffersendProxyProgressandrecvProxyProgressrocmwrap_fakes.ccnow owns thehsa_status_stringfake, sorccl-UnitTestsMicroRmaProxy(which already links it) drops its local copy.The binary is registered in
CMakeLists.txt,run_host_tests.sh,test_categories_micro_net.yamlandtest_executor.py(notci-precheckin.json: host-only microtests run in the RCCL host unit tests workflow instead), andMICROTEST_README.mdlists it.Issue Tracking
JIRA ID: AICOMRCCL-2801
Test Plan
run_host_tests.sh(all phases) on develop f94e399.rccl-UnitTestsMicroNetwith--gtest_shuffle --gtest_repeat=3.BUILD_TESTS=ON), which compiles the cuMem DMA-BUF arms the standalone build leaves out.net.ccin 524 places to check the tests catch each change.Test Result
setXferNetAttrs's null-plugin guard, whichsetNetAttrsrepeats).net.cc.Submission Checklist