Repository navigation
test(rccl): follow #12386's LL128 gating in the DDA launcher microtests - #13819
Merged
Merged
Conversation
#12386 dropped the RCCL_DDA_LL check from the fabric LL128 eligibility functions (LL128 is gated by RCCL_DDA_LL128 upstream). #13585's microtests, merged a few hours earlier, still expected RCCL_DDA_LL=0 to reject LL128, so rccl-UnitTestsMicroDdaColl fails 5 tests on develop. Assert that RCCL_DDA_LL=0 leaves LL128 eligible. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
✅ 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}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
3 more finding(s), not posted inline
projects/rccl/test/DdaFabricEligibilityTests.cpp:41(Low) DdaFabricEligibilityTests.cpp:33-42 still says LL128 is gated by rcclParamDdaLL() and skips LL128 tests when RCCL_DDA_LL=0DdaFabricLL128EligibilityTestinDdaFabricEligibilityTests.cpp:33-42still says LL128 is gated byrcclParamDdaLL()and callsGTEST_SKIPwhenRCCL_DDA_LL=0. That file was not touched here. Its LL128 tests call the current eligibility functions, which no longer read that param, so the skip drops coverage for no reason. Could we drop the skip and fix the comment there as well, to finish the #12386 alignment?projects/rccl/test/host/algorithms/dda/all_gather/dda-all-gather-fabric-ll128-test.cc:93(Low) No unit test asserts RCCL_DDA_LL128=0 disables LL128; pre-existing gap, worth a follow-upprojects/rccl/test/host/algorithms/dda/alltoall/dda-alltoall-fabric-ll128-test.cc:92(Nit) "gated by RCCL_DDA_LL128 alone" comment (also all_gather:90, reduce_scatter:106) omits that the gate is in the selector and RCCL_DDA_LL128_THRESHOLD overrides it
Bottom line: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
Develop now warns "cuMem support requires ROCm >= 7.12 or the 7.0.2.x backport"; RocmwrapMicrotest still looked for the old HIP_VERSION text. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@argus review |
1 similar comment
Member
Author
|
@argus review |
pvallem
approved these changes
Oct 20, 2026
mch
approved these changes
Oct 20, 2026
mch
left a comment
mch
left a comment
Contributor
There was a problem hiding this comment.
Nice, thanks for fixing these.
This was referenced Oct 21, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
pvallem
pushed a commit
that referenced
this pull request
Oct 25, 2026
…GIN (#13840) 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 new `rccl-UnitTestsMicroNetIbCastInit` with one test TU per unit. ## How - The verbs, mlx5/ionic, socket and neighbouring net_ib_cast seams are shared fakes. The ones the other net_ib_cast PRs also use are byte-identical copies; this PR owns the scheduler, p2p_resiliency_recovery, gdr_peermem and gdr_probe fakes. Fake ibv contexts route post_send/poll_cq back to the test. `xml.cc`, `utils.cc` and `net_telemetry.cc` are the real ones. The GDR answers latched by `std::call_once` are tested in isolated processes. - `transport_stubs.cc` gains net.cc's `NET_OPTIONAL_RECV_COMPLETION` param, and `os_fakes.cc` gains `ncclOsStrSep`, the same hunk as #13601. - Pins 10 production bugs as `KnownBug_` tests: - AINIC fuses NICs by default, although the code means to require `NCCL_IB_MERGE_NICS=1` (AICOMRCCL-3088). - A GDR probe that could not run is latched as "no GDR" (AICOMRCCL-3089). - A data-direct lookup error skips the cleanup (AICOMRCCL-3090). - A failed device init turns the next one into a silent no-op (AICOMRCCL-3091). - Deregistering unsorts the MR cache, so a cached range gets registered twice (AICOMRCCL-3092). - A regMr failure on a later device keeps the earlier devices' MRs (AICOMRCCL-3093). - GIN connect, context and registration failures leak comms and MRs (AICOMRCCL-3094). - The RMA proxy's error report indexes devices by completion slot, and can crash (AICOMRCCL-3095). - IPutSignal with data but no destination posts an empty WR instead of the signal (AICOMRCCL-3096). - A plane-index failure frees the ibv device list twice (AICOMRCCL-3143). - Not pinned, as the outcome depends on uninitialized memory: `IbCastMakeVDevice` compares 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-UnitTestsMicroNetIbCastInit` 275/275 in about 0.4 s, and `--gtest_shuffle --gtest_repeat=20` is 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. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA ID: AICOMRCCL-2826
What
Fixes the host-test failures on develop: 5 tests in
rccl-UnitTestsMicroDdaColland 1 inrccl-UnitTestsMicroRocmwrap.How
#12386 removed the
RCCL_DDA_LLcheck from the fabric LL128 eligibility functions (LL128 is gated byRCCL_DDA_LL128upstream). #13585, merged a few hours earlier, still expectedRCCL_DDA_LL=0to reject LL128. The tests now assertRCCL_DDA_LL=0leaves LL128 eligible.Develop also reworded the cuMem driver-version warning ("requires ROCm >= 7.12 or the 7.0.2.x backport");
RocmwrapMicrotestnow looks for that text.Test Plan
rccl-UnitTestsMicroDdaColl271/271 andrccl-UnitTestsMicroRocmwrap35/35, each 20x shuffled clean (standalone build). Re-adding the oldRCCL_DDA_LLgate fails the new test.