test(rccl): add host microtests for src/cft_dev_runtime.cc by aduforest · Pull Request #13320 · ROCm/rocm-systems

aduforest · 2026-10-12T22:19:01Z

JIRA ID: AICOMRCCL-2825

What

Host-only microtests for src/cft_dev_runtime.cc: the CFT support probe, size helpers, bind/unbind helpers, the non-13030 stand-ins and the three ncclGet*DeviceLeInfo queries. Fourth PR in the AICOMRCCL-2825 stack.

How

New binary rccl-UnitTestsMicroCftDevRuntime (cft-dev-runtime-test.cc, suites CftDevRuntime*Microtest), with core.cc compiled for real because the LE-info queries call it. The four dev_runtime.cc symbols it uses come from a new fakes/cft_dev_runtime_fakes.cc. CUDA_VERSION is below 13030 here, so the CUDA logical-endpoint calls are compiled out. The tests cover everything else.

Registered like every other microtest binary: run_host_tests.sh, its own test_categories_micro_cft_dev_runtime.yaml, ci-precheckin.json, and the llvm-cov list in test_executor.py.

Coverage

123/123 lines and 10/10 testable functions. Regions are 315/340 not counting ncclParamCftEnable, which is only called from the compiled-out block. Most of the rest is NCCLCHECK's own macro branches.

Mutation Testing

13 mutations on the hipified source, all killed.

Test Plan

CftDevRuntime*: 41/41, and 41/41 × 20 shuffled.
rccl-UnitTestsMicro on develop: 1903/1903.
Test-only change, plus registering the binary in ci-precheckin.json and test_executor.py.

🤖 Generated with Claude Code

therock-pr-bot · 2026-10-12T22:20:06Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 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?

aduforest · 2026-10-12T23:10:06Z

@argus review

amd-argus

ARGUS

Bottom line: Worth settling before approval: the three LE-info query suites diverge, leaving ncclGetPeerDeviceLeInfo's null-argument guards and counted-slot indexing with no test at all.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

aduforest · 2026-10-13T05:44:17Z

@argus review

aduforest · 2026-10-13T07:28:58Z

@argus re-review

amd-argus

ARGUS

Findings

1 finding(s) that could not be posted as inline comments

projects/rccl/test/host/MICROTEST_README.md:464 (Low) New fakes file deviates from the README's bolded naming rule with no row or exception recorded MICROTEST_README.md:405 states in bold that a fakes file is named after the production TU that DEFINES the symbol, never after the target that first needed it, and all four symbols in cft_dev_runtime_fakes.cc are defined by src/dev_runtime.cc, as the file's own header says at cft_dev_runtime_fakes.h:7-11. The TU table at MICROTEST_README.md:419-464 records no row for it. The repo already has the shape for this: src/rccl_wrap.cc carries two rows at lines 450-451 for exactly this reason, and the two files there never define the same symbol, which holds here too. Could it get a second src/dev_runtime.cc row worded the same way?

Bottom line: Worth settling before approval: the three ncclGet* suites never observe ncclDevrWinRegEnabled's arguments, so swapping ncclDevrRegisterCft for ncclDevrRegisterAll at cft_dev_runtime.cc:302 stays green.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

aduforest · 2026-10-13T19:20:38Z

@argus review

amd-argus

ARGUS

Bottom line: Worth settling before approval: NcclGetDeviceLeInfoTestBase leaves cftSize at 0, so the peer suite's null-arg and no-CFT-support tests stay green with the production checks they pin deleted.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

aduforest · 2026-10-13T21:29:43Z

@argus review

amd-argus · 2026-10-13T21:30:35Z

@aduforest the current head (61da715f) already has a review from me above; nothing has changed since then for me to look at again. Push a change and I'll pick it up, or ask again once you have.

aduforest · 2026-10-13T21:31:23Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

- Refactor the three ncclGet*DeviceLeInfo fixture classes to share a common base (NcclGetDeviceLeInfoTestBase) with a one-line BigOffset() override each, removing ~15 duplicated lines while keeping all four gtest suite names and the Nccl* CTest pattern intact. - Add a MakeDevrTeam() helper that heap-allocates ncclDevrTeam sized for its flexible-array worldRankList (same idiom as dev_runtime.cc's own symTeamObtain), replacing an automatic ncclDevrTeam whose zero-element worldRankList the production code indexed out of bounds at team.nRanks. - ncclGetMultimemDeviceLeInfo's success test: OR NCCL_WIN_COLL_SYMMETRIC into the counted window's winFlags, so is tested as a real bit-mask, not just equality (every other winFlags in this file was exactly NCCL_WIN_CFT_COUNTED or exactly 0). - Add a dedicated NullArgs test to NcclGetPeerDeviceLeInfoTest (previously missing entirely), and extend NcclGetCftDeviceLeInfoTest's existing one to cover leId/leOffset, not just window; all three now pin the exact ncclInvalidArgument code instead of just EXPECT_NE(..., ncclSuccess). - Add a CountedWindow_UsesCountedBaseLeIdSlot case to NcclGetPeerDeviceLeInfoTest, mirroring the Cft sibling's existing one. - PeerOutsideFlatTeamBoundaries: drop the dead/backwards-commented le[0].baseId=1 setup (the boundary check runs before baseLeId is ever read) and pin both exact edges (peerWorld=12 rejecting, peerWorld=8 succeeding), which the previous 20/7 probes left both uncaught. - SymUnbindTeamLe_AlwaysSucceeds...: also assert the capability argument (mirrors the already-correct SymBindTeamLe twin). - Fix two comments: the fixture's bigSize=1 is load-bearing only for computeCftSize/computeCftMcSize, not ncclTeamCft/ncclTeamCftMultimem (ncclDevrInitOnce here is an always-succeeds fake that never reads devrState); and cft_dev_runtime_fakes.h's wrong function name. - Drop the nccl_device EXTRA_INCLUDES entry: verified empirically that this binary builds and all tests pass without it, same as PR3's core.cc finding. - Fix MICROTEST_README.md's dangling reference to rccl-UnitTestsMicroNcclDeviceCore, which doesn't exist on this branch. Re-measured coverage on cft_dev_runtime.cc after these fixes: regions 72.06%->82.35%, branches 57.89%->71.05%, lines still 100%. Remaining gaps are the same two documented categories from the PR description (ncclParamCftEnable, NCCLCHECK's own macro-internal branches).

…c-only and 13029 probes, dedupe prologue tests Co-Authored-By: Claude <noreply@anthropic.com>

…e reached, drop a vacuous flag Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-14T18:27:30Z

@argus review

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: nccl-cft-dev-runtime-test.cc:503. Worth clearing before approval.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

…akes table Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-14T20:38:53Z

Pushed 8dedee9 for the round-1 finding that couldn't be posted inline (MICROTEST_README.md fakes table): fakes/cft_dev_runtime_fakes.cc now has its own src/dev_runtime.cc row, worded like the two rccl_wrap.cc rows, plus a note that the two dev_runtime.cc fakes files never define the same symbol.

Your last summary says nccl-cft-dev-runtime-test.cc:503 is still open from earlier, but every inline thread on this PR shows as resolved by you, and line 503 on the current head is NcclGetCftDeviceLeInfoTest.CountedWindow_UsesCountedBaseLeIdSlot. If something there still needs changing, could you post it as an inline finding?

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

…tests Shorten the MICROTEST_README entry, CMake comments and test-file comments to the length the other host microtests use. Comment and doc changes only. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-14T23:43:19Z

@argus review

Give the binary its own test_categories_micro_<stem>.yaml and register it in ci-precheckin.json and test_executor.py's llvm-cov list, as MICROTEST_README's naming section requires, so the CI test runner and coverage pick it up. Rename to cft-dev-runtime-test.cc / rccl-UnitTestsMicroCftDevRuntime / CftDevRuntime*Microtest per the rules. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-14T23:59:53Z

@argus review

aduforest · 2026-10-15T01:11:12Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

aduforest · 2026-10-15T19:27:32Z

Folded into #13312 (one PR per Jira ticket, so AICOMRCCL-2825 runs CI once). Same commits, unchanged.

github-actions Bot added the project: rccl label Oct 12, 2026

systems-assistant Bot added the organization: ROCm label Oct 12, 2026

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

aduforest mentioned this pull request Oct 13, 2026

test(rccl): add host microtests for the ROCm runtime and SMI wrappers #13335

Merged

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

aduforest force-pushed the users/aduforest/aicomrccl-2825-cft-dev-runtime-host-microtests branch from d31a870 to 61da715 Compare October 13, 2026 19:20

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

Comment thread projects/rccl/test/host/cft-dev-runtime-test.cc Resolved

Comment thread projects/rccl/test/host/nccl-cft-dev-runtime-test.cc Resolved

Comment thread projects/rccl/test/host/nccl-cft-dev-runtime-test.cc Resolved

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

aduforest marked this pull request as ready for review October 14, 2026 02:55

aduforest requested review from a team October 14, 2026 02:55

aduforest and others added 4 commits October 14, 2026 11:09

test(rccl): add host microtests for src/cft_dev_runtime.cc

f648bbd

test(rccl): pin CFT registration args in all LE-info suites, symmetri…

50a8f37

…c-only and 13029 probes, dedupe prologue tests Co-Authored-By: Claude <noreply@anthropic.com>

test(rccl): seed a non-degenerate CFT team so peer prologue checks ar…

60262dd

…e reached, drop a vacuous flag Co-Authored-By: Claude <noreply@anthropic.com>

aduforest force-pushed the users/aduforest/aicomrccl-2825-cft-dev-runtime-host-microtests branch from da3c403 to 60262dd Compare October 14, 2026 18:27

amd-argus Bot reviewed Oct 14, 2026

View reviewed changes

docs(rccl): record cft_dev_runtime_fakes.cc in the MICROTEST_README f…

8dedee9

…akes table Co-Authored-By: Claude <noreply@anthropic.com>

amd-argus Bot reviewed Oct 14, 2026

View reviewed changes

test(rccl): trim microtest docs and comments to match the other host …

f8d922e

…tests Shorten the MICROTEST_README entry, CMake comments and test-file comments to the length the other host microtests use. Comment and doc changes only. Co-Authored-By: Claude <noreply@anthropic.com>

dhanavikram approved these changes Oct 14, 2026

View reviewed changes

amd-argus Bot reviewed Oct 15, 2026

View reviewed changes

aduforest mentioned this pull request Oct 15, 2026

test(rccl): add host microtests for the device API host side #13312

Merged

aduforest closed this Oct 15, 2026

Conversation

aduforest commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Coverage

Mutation Testing

Test Plan

Uh oh!

therock-pr-bot Bot commented Oct 12, 2026

✅ All Policy Checks Passed

Uh oh!

aduforest commented Oct 12, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduforest commented Oct 13, 2026

Uh oh!

aduforest commented Oct 13, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduforest commented Oct 13, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduforest commented Oct 13, 2026

Uh oh!

amd-argus Bot commented Oct 13, 2026

Uh oh!

aduforest commented Oct 13, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 14, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 14, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 14, 2026

Uh oh!

aduforest commented Oct 14, 2026

Uh oh!

aduforest commented Oct 15, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 15, 2026

Uh oh!

Reviewers

Uh oh!

aduforest commented Oct 12, 2026 •

edited

Loading