Repository navigation
Conversation
Seams for scheduler.cc, p2p_resiliency_recovery.cc, gdr_peermem.cc and gdr_probe.cc, and transport_stubs gains net.cc's NET_OPTIONAL_RECV_COMPLETION param, for net_ib_cast microtest binaries that do not compile those TUs. Co-Authored-By: Claude <noreply@anthropic.com>
…seAinic Co-Authored-By: Claude <noreply@anthropic.com>
Byte-identical copies of the fakes the other net_ib_cast microtest PRs own (verbs, mlx5/ionic direct verbs, sockets, and the connect, p2p, p2p_resiliency and qp_sharing seams), and os_fakes' ncclOsStrSep as #13601 adds it. Co-Authored-By: Claude <noreply@anthropic.com>
rccl-UnitTestsMicroNetIbCastInit covers src/transport/net_ib_cast/{init,common,
capability,reg,gdr,multiplane,gin,net_ib_cast_gid_inspect}.cc over fake verbs
contexts, with the real xml.cc, utils.cc and net_telemetry.cc. Production bugs
are pinned as KnownBug_ tests (AICOMRCCL-3088..3096).
Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
✅ 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-ib-cast-multiplane-test.cc:191(Low) Load_PipWithoutInterface_LeavesTheInterfaceEmpty checks only INFO log text, not the stored pip interfaceprojects/rccl/test/host/net-ib-cast-init-test.cc:222(Low) GetRealPort test stores stack arrays in IbCastDevs[].pciPath; a failed ASSERT makes TearDown free() them and abort the binaryprojects/rccl/test/host/net-ib-cast-gin-test.cc:1131(Low) KnownBug death test expects KilledBySignal(SIGSEGV), which does not match under BUILD_ADDRESS_SANITIZER (ASan exits 1)projects/rccl/test/host/fakes/net_ib_cast_qp_sharing_fakes.cc:15(Low) Comment says CMakeLists.txt pins these RCCL_PARAM copies to qp_sharing.cc, but no rccl_assert_source_line covers themprojects/rccl/test/host/net-ib-cast-test-common.h:221(Low) Control-statement bodies on their own line without braces (style guide requires braces); 6 sites
This PR changes 6,549 lines across 41 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.
Pin the copied connect, qp_sharing, p2p, p2p_resiliency and socket fake defaults to production, check the stored PIP interface, let the slot-index crash pin accept ASan's exit, own the GetRealPort paths, and brace the single-statement bodies. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed the five non-inline findings in 4d3155b:
|
|
@argus re-review |
…init-host-microtests # Conflicts: # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
|
@argus re-review |
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,610 lines across 41 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: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,610 lines across 41 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: InitDevices_IbDisabled_FailsBeforeLoadingVerbs cannot fail as written, and the GIN error-path pins leak gin.cc allocations that LeakSanitizer reports in ASan builds.
Comment @argus review to re-run, or @argus re-review after pushing.
…_DISABLE test bite Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed both Medium findings in 5b15c49:
|
|
@argus re-review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,635 lines across 41 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: InitDevices_NoVerbsLibrary_Fails passes with the verbs check removed, and the VDevice fixtures run strcmp on uninitialized PCI roots.
Comment @argus review to re-run, or @argus re-review after pushing.
…l PCI roots Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed both Medium findings in a7e2c2e:
272/272 pass, |
|
@argus re-review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,648 lines across 41 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-2813-net-ib-cast-init-host-microtests
…-1] read (pre-merge hardening) - ZeroedPtr allocates with the comm structs' alignas() alignment instead of calloc's 16 bytes, which cleared the UBSan misaligned-access reports in rccl-UnitTestsMicroNetIbCastInit. - The unknown-event case uses IBV_EVENT_DEVICE_SPEED_CHANGE, a real event the loop has no case for, instead of casting 999 into the enum. - MakeVDevice's negative-index case puts -1 second, since production reads IbCastDevs[devs[0]] before its range check. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,617 lines across 47 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-2813-net-ib-cast-init-host-microtests
Define ncclOsStrSep right after ncclOsGetPid, where #13840 adds the same fake, so merging the two branches keeps one definition instead of silently compiling two. Co-authored-by: Cursor <cursoragent@cursor.com>
…unt (pre-merge hardening) #12749 made init.cc call wrap_ionicdv_ctx_get_udma_count, so the Init binary no longer linked. ionicdvwrap_fakes gains hooks for it and for wrap_ionicdv_qp_get_udma_idx, defaulting to what the real wrappers return without libionic. Tests cover the normalized per-device count and the mlx5-only data-direct checks in GetProperties and DMA-BUF registration. Co-Authored-By: Claude <noreply@anthropic.com>
|
The remaining fake moves overlap Dhana's PRs, we agreed to merge them into one follow-up once both land |
…comrccl-2813-net-ib-cast-init-host-microtests
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,682 lines across 47 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: InitDevices_TooManyPlanes_FailsAfterListing runs through a production double free of the device list (init.cc:591, then 740) without pinning it, and the telemetry and SendCommInit tests still pass with the code they check removed.
Comment @argus review to re-run, or @argus re-review after pushing.
Co-Authored-By: Claude <noreply@anthropic.com>
…comrccl-2813-net-ib-cast-init-host-microtests
|
@argus re-review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,721 lines across 47 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: Claude <noreply@anthropic.com>
|
@argus re-review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,721 lines across 47 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-2813-net-ib-cast-init-host-microtests
…view Co-Authored-By: Claude <noreply@anthropic.com>
Host CI runs these binaries through run_host_tests.sh, so the json entries are not needed. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
ARGUS
This PR changes 6,725 lines across 47 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.
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 6,708 lines across 46 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-13840-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-13840-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
JIRA ID: AICOMRCCL-2813
What
Host-only microtests for
src/transport/net_ib_cast/{init,common,capability,reg,gdr,multiplane,gin,net_ib_cast_gid_inspect}.cc, in a newrccl-UnitTestsMicroNetIbCastInitwith one test TU per unit.How
xml.cc,utils.ccandnet_telemetry.ccare the real ones. The GDR answers latched bystd::call_onceare tested in isolated processes.transport_stubs.ccgains net.cc'sNET_OPTIONAL_RECV_COMPLETIONparam, andos_fakes.ccgainsncclOsStrSep, the same hunk as test(rccl): add host microtests for src/bootstrap.cc #13601.KnownBug_tests:NCCL_IB_MERGE_NICS=1(AICOMRCCL-3088).IbCastMakeVDevicecompares and logs PCI roots it never set when a path has none (AICOMRCCL-3097).Coverage
Lines: init.cc 642/652, common.cc 185/185, common_cast.h 57/57, capability.cc 162/162, reg.cc 116/117, gdr.cc 73/73, multiplane.cc 186/189, gin.cc 491/491 (the GDAKI arm is compiled out, as in the shipping build), net_ib_cast_gid_inspect.cc 102/102. The misses are calloc failures, a capability probe that cannot fail, a telemetry slot that is never out of range, and the return after reg.cc's loop.
Mutation Testing
172 mutations on the hipified sources, all killed; 5 of them needed a sharper test first. 1 more against the review changes, also killed. 11 more for #12749's runtime uDMA count and its mlx5-only data-direct checks, all killed.
Test Plan
Standalone build:
rccl-UnitTestsMicroNetIbCastInit275/275 in about 0.4 s, and--gtest_shuffle --gtest_repeat=20is clean. Every other host binary was rebuilt against the changed fakes and run once. DdaColl and Rocmwrap fail as they do on develop (the break #13819 fixes) and link none of the changed files.