Skip to content

test(rccl): pin the NIC gate on the graph search even-channel preference - #14002

Draft
thomas-huber wants to merge 1 commit into
developfrom
users/thomas-huber/compare-graphs-even-channel-gate-test
Draft

thomas-huber wants to merge 1 commit into
developfrom
users/thomas-huber/compare-graphs-even-channel-gate-test

Conversation

@thomas-huber

Copy link
Copy Markdown
Contributor

Motivation

NCCL 2.32.3 made ncclTopoCompareGraphs keep an even-channel reference over an odd-channel graph when the reference has at least the same aggregate bandwidth, but only while the graph has fewer channels than the system has NICs (NVIDIA/nccl@24264d1499). RCCL picked it up with the 2.32.3 sync in #12321. CompareGraphs_OddCountTiesGoToTheEvenReference (#13605) covers the preference; nothing covered the NIC condition, so dropping or loosening it would go unnoticed.

Technical Details

  • Two new SearchMicrotest cases in rccl-UnitTestsMicroTopo tie an odd-channel graph that has fewer hops against an even-channel reference with the same aggregate bandwidth, where the NIC condition does not hold, and expect the hop count to decide, as it did before the sync:
    • CompareGraphs_WithoutNicsOddCountTiesGoToFewerHops: two GPUs and no NICs; 1 channel at 40 GB/s against 2 at 20 GB/s.
    • CompareGraphs_OddCountAtTheNicCountTiesGoToFewerHops: three GPUs with a NIC each; 3 channels at 40 GB/s against 4 at 30 GB/s.
  • Registration: "Search*" in test_categories_micro_topo.yaml already covers them.
  • Test-only; no source changes. Upstream: NVIDIA/nccl@24264d1499.

Issue Tracking

Test Plan

  • The Search* cases of rccl-UnitTestsMicroTopo from the standalone build (test/host/run_host_tests.sh build) and the in-tree build, on develop (98f6a72dcc49): the five CompareGraphs_* cases three runs each, the other 74 once.
  • The same with ncclTopoCompareGraphs changed three ways, one at a time: the even-channel check disabled, the NIC condition removed, and its < relaxed to <=.
  • MI355X (gfx950), ROCm 7.0.2, release build (install.sh -l -t).

Test Result

  • On develop: all 79 Search* cases pass in both builds, the CompareGraphs_* cases 3/3.
  • Even-channel check disabled: CompareGraphs_OddCountTiesGoToTheEvenReference fails 3/3; the two new cases pass.
  • NIC condition removed: both new cases fail 3/3 (the even reference wins the tie); CompareGraphs_OddCountTiesGoToTheEvenReference still passes.
  • <= instead of <: only CompareGraphs_OddCountAtTheNicCountTiesGoToFewerHops fails 3/3.
  • The other 74 Search* cases pass with every change, so none of the full searches on the test topologies depends on the preference.

Submission Checklist

Since the 2.32.3 sync, ncclTopoCompareGraphs keeps an even-channel
reference over an odd-channel graph with no more aggregate bandwidth,
but only while the graph has fewer channels than the system has NICs.
CompareGraphs_OddCountTiesGoToTheEvenReference covers the preference;
nothing covered the NIC gate.

Two SearchMicrotest cases tie an odd graph that has fewer hops against
an even reference, on a system without NICs and with as many channels
as NICs, and expect the hop count to decide.

Upstream: NVIDIA/nccl@24264d1499 ("graph/search: update compareGraphs to
favor even channels")
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rccl/test/host/search-test.cc; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant