Repository navigation
test(rccl): host microtests for SMI-indexed topology GPU devs - #13138
Merged
Merged
Conversation
…one GPU ncclTopoGetXmlFromGpu wrote the process-local HIP ordinal as the <gpu dev> attribute. With one GPU per process (HIP_VISIBLE_DEVICES=<rank>) every rank wrote dev 0, so the Rome model mapped every ring slot to rank 0 and ncclTopoPreset failed with "Local rank N not found in intra-node graph". The same collision hit CPX with one partition per process, and made ncclTopoGetLinkType answer every query for the first GPU, which could set RCCL_TOPO_XGMI_ALL on a node that is not all XGMI. Write the SMI index as dev again: it is machine-wide, distinct per CPX/DPX partition, and what comm->nvmlDev already holds. Keep the HIP ordinal for the HIP calls only. Key ncclTopoGetLinkType by comm rank, since its callers passed HIP ordinals that are not comparable across processes. Add rccl-UnitTestsMicroGraph, which runs the real graph TUs host-only, with tests for SPX and CPX one-GPU-per-process layouts, the Rome mapping, a topology file GPU, XGMI detection by rank and the all-XGMI flag. JIRA ID: ROCM-32444
✅ 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
1 finding(s) that could not be posted as inline comments
projects/rccl/test/host/MICROTEST_README.md:223(Low) rccl-UnitTestsMicroGraph is missing from all three MICROTEST_README.md lists where every sibling micro binary appearsrccl-UnitTestsMicroGraphis missing from MICROTEST_README.md, where every sibling micro binary appears in all three lists: the per-binary description block ending at line 223, the standalone-build enumeration at lines 726 to 730, and the run-command block at lines 759 to 769. The other two registration sites were both updated (run_host_tests.sh:167 and the new test_categories_micro_graph.yaml), so this looks like the one missed spot. Could we add it next torccl-UnitTestsMicroDiagnostics?
Bottom line: Worth settling before approval: rome_models.cc:4664 and 4850 pass slot indices where every other call site passes the dev-valued rdm, so the new machine-wide dev makes comm init fail with ncclInternalError on a non-prefix GPU subset.
Comment @argus review to re-run, or @argus re-review after pushing.
wenkaidu
approved these changes
Oct 14, 2026
parseA2a8P passed the slot-index permutation g8 to parseGraph for treeRail, and parseRome4P2H passed g to parseGraphLight for treeBase, while every other call site passes the dev-valued mapping rdm. Both only worked when the node's GPU devs were exactly 0..n-1, so a communicator on a non-prefix set of SMI indices (for example the upper half of a 16-GCD node) failed tree setup with ncclInternalError. parseGraphLight also wrote the GPU dev into treeBase, which ncclTreeBasePostset compares against comm->rank; it now writes the rank, as parseGraph does. Add host tests that build the same rail-tree and treeBase nodes with devs 0..7 and 8..15 and expect identical channels and trees, and list rccl-UnitTestsMicroGraph in MICROTEST_README.md.
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: user NCCL_RINGS/RCCL_TREES strings now match gpu.dev as the SMI index, so with a non-prefix HIP_VISIBLE_DEVICES set a previously valid ring string fails init with ncclInternalError (xml.cc:1000).
Comment @argus review to re-run, or @argus re-review after pushing.
mch
reviewed
Oct 15, 2026
… fake signatures ncclTopoGetLinkType now takes ranks, so the fake and its hook name them rank1/rank2. amdsmi_fakes.cc anchors each std::function hook to its amdsmi_wrap.h prototype with ASSERT_HOOK_MATCHES_PROD, as hip_fakes.cc does.
Align the tests with develop, which keys topology GPUs and ncclTopoGetLinkType by SMI index. The link-type tests now query by SMI index and nvmlDev, and the link-type fake names its parameters dev1/dev2 as the prototype does. Drop the two Rome tree tests for ranks on the upper half of a node, which cover behaviour outside this change.
pvallem
changed the title
mch
approved these changes
Oct 16, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
This was referenced Oct 21, 2026
1 task done
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.
The topology GPU dev fix in #13536 has no host-only test coverage.
Fix: add rccl-UnitTestsMicroGraph, which runs the real graph code with faked HIP and amd-smi to check SMI-indexed devs per process, Rome ring mapping, CPX partitions and XGMI detection, and assert the amd-smi fake signatures.
JIRA ID: ROCM-32444