Repository navigation
Conversation
|
@argus review |
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
Precheckin's "Failed in Compile stage" was a real link error my standalone build couldn't reproduce. In-tree builds copy the @argus review |
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: rccl-UnitTestsMicroRoctxNvml never gets ROCTX_ENABLE in the standalone arm run_host_tests.sh builds, so the push/pop assertions added this round compile out in CI.
Comment @argus review to re-run, or @argus re-review after pushing.
dbbfd41 to
8440af2
Compare
|
@argus review |
|
@argus review |
Covers the kernel-version probe, the cuMem VMM functional probe and its cleanup, the cuMem capability gates, the out-of-window NCCL_CUMEM_ENABLE / cuMem-host arms, and rocmLibraryInit's ROCr version and DMA-BUF setup, in a new rccl-UnitTestsMicroRocmwrap binary. Co-Authored-By: Claude <noreply@anthropic.com>
…dress ARGUS findings on rocmwrap microtests Co-Authored-By: Claude <noreply@anthropic.com>
…tub.cc New rccl-UnitTestsMicroRoctxNvml binary compiling both files for real (comm_fakes.cc stubs roctx_scoped_range_in for every other target). The test TU supplies its own ncclDebugLog and g_loadParam, so no shared fakes are linked; param_redirect.h makes RCCL_LOG_ROCTX controllable per test, and a malloc seam scoped to roctx.cc reaches both roctxAlloc failure paths. stringify() overflows its 1024-byte buffer once a message is truncated (confirmed under ASan); filed as AICOMRCCL-2914 and not pinned here. Co-Authored-By: Claude <noreply@anthropic.com>
In-tree builds copy the rccl target's COMPILE_DEFINITIONS (PRIVATE ones included) into every test target, so ROCTX_ENABLE reaches this binary whenever roctracer is found. roctx.cc then calls roctxRangePushA/Pop, which nothing on the micro-test link line provides; that was the precheckin "Failed in Compile stage". The standalone build never defines it, so this did not reproduce locally. Define both entry points in the test TU to record calls, and make the scoped-range tests hold in either build: push/pop events and the exact message pushed with ROCTX_ENABLE, the WARN fallback without it. Co-Authored-By: Claude <noreply@anthropic.com>
…pop checks, address ARGUS round 2 Co-Authored-By: Claude <noreply@anthropic.com>
…l README entry Co-Authored-By: Claude <noreply@anthropic.com>
68e6d90 to
090feb0
Compare
|
@argus review |
…rt gate stays unarmed when ROCr lacks DMA-BUF Co-Authored-By: Claude <noreply@anthropic.com>
Covers init/shutdown caching, GPU count, PCI bus-id lookups both ways, link info, firmware version, fabric discovery (including the amd_smi fabric ABI probe and its sysfs fallbacks) and telemetry, across the WSL2, amd_smi_lib and ARSMI backends, in a new rccl-UnitTestsMicroAmdsmiWrap binary. Co-Authored-By: Claude <noreply@anthropic.com>
Seed a nonzero firmware version in the WSL2 test so only the early return can produce 0, drive install/clear/dlsym from one symbol table and check every pfn slot after dlopen, count calls to prove getProcessorHandle's early returns, name the typed-fabric-layout condition once, and drop a dead store in the too-many-devices test. Co-Authored-By: Claude <noreply@anthropic.com>
Give the lib PCIe case a nonzero bandwidth pair and add an ARSMI PCIe link with a direct-link weight, so both XGMI type checks are needed. Co-Authored-By: Claude <noreply@anthropic.com>
Add a hip-id-1 CPU after GPU1 so the link lookup needs its GPU type check, and populate the out-of-range fabric slot so the bounds checks are needed. Co-Authored-By: Claude <noreply@anthropic.com>
…vml-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
…vml-host-microtests # Conflicts: # projects/rccl/test/host/CMakeLists.txt # 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
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,075 lines across 12 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-2822-roctx-nvml-host-microtests
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,074 lines across 12 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.
Also reset g_legacyTelemName in Reset() and drop the Jira key from the roctx-nvml-stub-test.cc header. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,074 lines across 12 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 re-review |
|
@aduforest the current head ( |
|
@argus review |
|
@aduforest the current head ( |
# Conflicts: # projects/rccl/test/host/MICROTEST_README.md
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,102 lines across 12 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.
# Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,092 lines across 12 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.
TheRock Submodule Bump ActivityNewest first
|
JIRA ID: AICOMRCCL-2822
What
Host-only microtests for
src/misc/roctx.cc,src/misc/nvmlwrap_stub.cc,src/misc/rocmwrap.ccandsrc/misc/amdsmi_wrap.cc.Complementary to
test/AmdSmiWrapTests.cpp.How
rccl-UnitTestsMicroRoctxNvml(plus a standalone-only-roctxvariant that builds theROCTX_ENABLEarm against a stand-inroctracer/roctx.h): roctx.cc and nvmlwrap_stub.cc. Amallocseam scoped toroctx.ccreachesroctxAlloc's failure paths.rccl-UnitTestsMicroRocmwrap: the kernel-version probe, the cuMem probe and capability gates, androcmLibraryInit's ROCr and DMA-BUF setup.kernel_configis added torccl_mark_oracle_tus_generatedso an in-tree configure works before hipify runs.rccl-UnitTestsMicroAmdsmiWrap: init, GPU count, PCI bus-id lookups, link info, firmware version, and fabric discovery and telemetry, on the WSL2, amd_smi_lib and ARSMI backends, through a scriptedFakeSmiin the file's ownpfn_*table.rocmLibraryInitleaves the DMA-BUF export gate armed when the kernel-config check fails.rocmLibraryInitstill enables DMA-BUF when/proc/kallsymscan't be read.amd_smi_getNumDevicecounts CPU processors as GPUs.amd_smi_getDeviceIndexByPciBusIdkeeps only the device number's low hex digit (AICOMRCCL-2935).amd_smi_getLinkInfologs an unknown index, then queries a null handle and returns success.stringify()overflows its buffer pastMAX_MESSAGE_LENGTH(AICOMRCCL-2914). Not covered: the in-window arms ofncclCuMemEnable/ncclCuMemHostEnable, which only compile on ROCm 7.12+.Coverage
Every line of roctx.cc, nvmlwrap_stub.cc and amdsmi_wrap.cc; rocmwrap.cc 209/212 lines (ROCm 7.1).
Mutation Testing
76 mutations, 75 killed; the survivor is equivalent (dropping amdsmi_wrap's per-thread fast path only skips a lock).
Test Plan
Standalone build, each × 20 shuffled:
rccl-UnitTestsMicroRoctxNvmland-roctx31/31 each,rccl-UnitTestsMicroRocmwrap35/35,rccl-UnitTestsMicroAmdsmiWrap69/69.CI runs 68 AmdsmiWrap tests:
ProcessorHandle_MissingSymbolFailsQuietlyonly compiles without the amd_smi headers.