Repository navigation
Conversation
Covers NCCL_DEBUG / NCCL_DEBUG_LEVELS / NCCL_DEBUG_SUBSYS / timestamp parsing, the log line format per level, NCCL_DEBUG_FILE expansion, reset and thread naming in a new rccl-UnitTestsMicroDebug binary. Co-Authored-By: Claude <noreply@anthropic.com>
Covers the rcclApiFuncTable layout and every public entry point's dispatch through it, the deprecated ncclAllToAll(v) spellings and the MSCCL no-op stubs, in a new rccl-UnitTestsMicroApiTrace binary. Co-Authored-By: Claude <noreply@anthropic.com>
Covers RCCL_REPLAY_FILE naming, the JSON and binary formats of every record() overload, skip(), graph-capture deferral through host nodes and parseJsonEntry, in a new rccl-UnitTestsMicroRecorder binary. Co-Authored-By: Claude <noreply@anthropic.com>
Covers CollTrace's worker thread, ring buffer, intra-node aggregation and reporting, CudaWaitEvent polling, EventQueue, the collTrace* entry points and CollTraceUtils, in a new rccl-UnitTestsMicroCollTrace binary. Co-Authored-By: Claude <noreply@anthropic.com>
Adds CommDumpMicrotest (ncclCommDump's null guards and proxy-trace dump) and TimerMicrotest (ENABLE_TIMER calibration and TIME_* accumulators) to rccl-UnitTestsMicro. Co-Authored-By: Claude <noreply@anthropic.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
@argus review |
aduforest
changed the title
![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/debug-test.cc:710(Low) Env_WithoutTheEnvPluginReadsTheProcessEnvironment depends on and clobbers inherited NCCL_DEBUG* env varsprojects/rccl/test/host/api-trace-test.cc:59(Low) ncclDebugLog capture fake and debug globals are copied into three new test files with small driftprojects/rccl/test/host/debug-test.cc:723(Nit) Multi-line/nested loop bodies without braces (also debug-test.cc:751-753, coll-trace-test.cc:137-139, 164-165, 172-173)projects/rccl/test/host/coll-trace-test.cc:409(Nit) Raw new CollTrace leaks its worker thread past the fixture's comm if the ASSERT fails (also line 509-513)projects/rccl/test/host/api-trace-test.cc:228(Nit) EXPECT_FORWARDED hand-aligns line continuations, misaligned on line 227 (.clang-format uses DontAlign)
This PR changes 3,460 lines across 16 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: the recorder Parse_* round-trip tests fail in the README's standalone Release build, where NDEBUG compiles out the sscanf calls parseJsonEntry wraps in assert().
Comment @argus review to re-run, or @argus re-review after pushing.
…comments parseJsonEntry runs its sscanf calls inside assert(), so the two round-trip tests cannot pass in a Release (NDEBUG) build. Drop comments that restate a test's name. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,450 lines across 16 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.
Adds fakes/debug_fakes.{h,cc}, the line-recording ncclDebugLog that the
api_trace, recorder and latency_profiler microtests each had a copy of.
Also: the env-reading debug test now saves and restores inherited NCCL_DEBUG*
variables, CollTrace tests own their tracer through unique_ptr, and multi-line
loop bodies get braces.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Also addressed the five non-inline findings in 3fe1868: the env test saves/restores inherited NCCL_DEBUG* vars; the three copied ncclDebugLog capture fakes are now one fakes/debug_fakes.{h,cc}; multi-line loop bodies have braces; CollTrace tests hold the tracer in unique_ptr; EXPECT_FORWARDED no longer hand-aligns its continuations. @argus re-review |
|
@argus re-review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,501 lines across 18 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.
The in-build clang does not emit the pncclResetDebugInit alias, so drop that call. ApiTrace inherits RCCL_ROCPROFILER_REGISTER=1 in-build and then needs librocprofiler-register; remove it so both builds test the same config. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,501 lines across 18 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-2823-debug-trace-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
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,501 lines across 18 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.
- The fake bootstrapIntraNodeAllGather fills only the ranks it is given; the happy-path CollTrace tests now run with eight local ranks, and a new Trace_KnownBug_FewerThanEightLocalRanksReportNothing pins the aggregation failure with fewer. - Rename the _Observed pins (and the two unsuffixed ones) to <Fn>_KnownBug_<What>. - recorder-test: use CaptureStdout, drop the no-op parseBinLog() call, give the `#define private public` its reason. debug-test: add missing std includes. - README: "Where a fake belongs" row for fakes/debug_fakes.cc. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,519 lines across 18 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-2823-debug-trace-host-microtests
…(pre-merge hardening) - coll-trace-test.cc defines g_loadParam, which collectives_fakes.cc references. x86 --gc-sections drops the reference; ppc64le does not. - api-trace-test.cc expects no ROCTX range when NVTX_NO_IMPL compiles NVTX3_FUNC_WITH_PARAMS out (in-tree builds without ROCTX). - recorder-test.cc computes the fake allocation base with integer math, skips the memcpy for an empty binary log, and sets datatype/op on the raw rcclApiCalls it records. UBSan reported all three. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,736 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.
…comrccl-2823-debug-trace-host-microtests
…per review - Sockets: the socketpair fakes are now installed on socket_fakes.cc's hooks (copied from #13858, plus a block for ncclSocketSetFd, ncclSocketTryRecv and the other socket.cc entry points it does not cover), so ncclSocketClose is defined once. The service teardown test now also checks the listen socket is closed through that hook. - Logging: use the ncclDebugLog recorder in debug_fakes.cc (from #13584), which now also counts ncclSetThreadName. - Params: use ParamOverride next to g_loadParam in nccl_fakes.h (from #13642). - The AICOMRCCL-2966 pin, now Won't Do, becomes a plain current-behaviour test. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,734 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.
…comrccl-2823-debug-trace-host-microtests
Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,733 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.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,665 lines across 26 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-13584-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-13584-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
…comrccl-2823-debug-trace-host-microtests Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus re-review |
There was a problem hiding this comment.
ARGUS
This PR changes 3,659 lines across 25 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-13584-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-13584-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
TheRock Submodule Bump ActivityNewest first
|
JIRA ID: AICOMRCCL-2823
What
Host-only microtests for
debug.cc,misc/api_trace.cc,misc/recorder.cc,misc/latency_profiler/*,commDump.ccandinclude/timer.h, in four new binaries (rccl-UnitTestsMicro{Debug,ApiTrace,Recorder,CollTrace});commDump.cc/timer.h(x86-64 only) joinrccl-UnitTestsMicro.Complements
test/latency_profiler/LatencyProfilerUnitTest.cpp(CollTraceUtils/EventQueue only) and the GPU-basedtest/_RecorderTests.cpp.How
#includes the hipified unit; the CollTrace worker thread runs for real. Shared fakes supply the recorder, env, datatype names, and two new recording fakes:fakes/roctx_fakes.cc(replaces theroctx_scoped_range_instubs incomm_fakes.ccandmem_manager_src_wrapper.cc) andfakes/profiler_fakes.cc(ncclProfilerProxyTraceDumpIfAny, moved fromnccl_stubs.cc). The remaining edges (clock, host/pid/tid, HIP events, bootstrap all-gather, params) are seamed in the test.recorder.hgets an include guard, so the recorder test opens only that header up.KnownBug_tests, not fixed:NCCL_DEBUG_SUBSYS: the runtime parser ignoresALLOC_HOSTand the published option list lacksVERBS(AICOMRCCL-2950).%inNCCL_DEBUG_FILEreads past the NUL (AICOMRCCL-2953).RCCL_REPLAY_FILEsplits at the first.in the whole path (AICOMRCCL-2955).cudaDev == 0, not rank 0 (AICOMRCCL-2958), and reports nothing with fewer than 8 local ranks (AICOMRCCL-2968).getSizeMbrejects the namesncclDatatypeToStringgives FP8 and Uint8 (AICOMRCCL-2959).hipEventCreateis only logged, and the event is handed out with null HIP events (no ticket yet).parseJsonEntry's non-collective formats have more conversions than pointers (AICOMRCCL-2957).Coverage
Lines: debug.cc 403/403, api_trace.cc 268/268 (built without
RCCL_ROCPROFILER_REGISTER, so its rocprofiler-register block is not in that figure), recorder.cc 521/533, latency_profiler 319/327, commDump.cc 14/14, timer.h 15/15; the misses are braces afterreturn, null checks aftermake_uniqueand the emptyparseBinLogstub.Mutation Testing
148 targeted mutants, 146 killed; the 2 survivors are equivalent. 6 more against the review changes, all killed.
Test Plan
Standalone build (Debug): the four new binaries (Debug 66, ApiTrace 13, Recorder 35, CollTrace 40) pass
--gtest_shuffle --gtest_repeat=20; every binary that links the moved fakes (rccl-UnitTestsMicro, Init x3, Enqueue x2, WarpSpeed, Ce, SymKernels, TaskPrep,rccl-HostUnitTests) passes once. Release (-DNDEBUG): two recorder round-trip tests skip becauseparseJsonEntry's sscanf runs insideassert().