test(rccl): add host microtests for the param subsystem by aduforest · Pull Request #13211 · ROCm/rocm-systems

aduforest · 2026-10-09T19:12:03Z

JIRA ID: AICOMRCCL-2820

What

Host-only microtests for the param subsystem: the parser headers (src/include/param/parser_{default,enum,bitset,list}.h), src/param/param_registry.cc, ncclParam<T> (src/include/param/param.h), src/param/param.cc, src/param/c_api.cc and the env loading in src/misc/param.cc.
Host-only counterparts of test/ParameterApiTests.cpp / ParamTests.cpp.

How

param-parsers-test.cc (header-only) and param-registry-test.cc go in rccl-UnitTestsMicro. A small FakeParam stands in for ncclParam<T>, and locking is checked by holding the registry mutex while a concurrent add/find/remove blocks.
The rest is in a new rccl-UnitTestsMicroParam, because misc/param.cc defines ncclGetEnv, initEnv and ncclLoadParam, which the shared fakes also define. The env plugin, ncclOsSetEnv, fopen and logging are seamed in the test TU; initEnv() and NCCL_NO_CACHE latch behind std::call_once, so those cases run in isolated processes.
Pins 4 production bugs as KnownBug_ tests: a negative number parses as a huge unsigned value, a narrower integer type truncates silently, and setEnvFile passes an empty name to setenv and cuts names and values to 1023 chars (AICOMRCCL-2962).

Coverage

Every function and line of each unit except one parser line, an EINVAL arm glibc never raises for base 10.

Mutation Testing

80 mutations on the hipified sources, all killed.

Test Plan

Standalone build: ParamParsersMicrotest.* 52/52 and ParamRegistryMicrotest.* 11/11 in rccl-UnitTestsMicro, rccl-UnitTestsMicroParam 54/54, each × 20 shuffled.

aduforest · 2026-10-10T03:03:42Z

@argus review

amd-argus

ARGUS

Findings

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

projects/rccl/src/include/param/parser_bitset.h:109 (Low) ncclParamBitsetOf's desc hardcodes a comma while honouring an arbitrary delimiter (follow-up) Follow-up rather than something to fix here, since it predates this PR: ncclParamBitsetOf hardcodes "Comma-separated list of:" at parser_bitset.h:109 while honouring the delimiter argument at parser_bitset.h:104, so a non-comma bitset param advertises the wrong separator in its help text. parser_list.h:64 already has the pattern, it interpolates the delimiter. Worth knowing before pinning that string in the bitset desc test.

Bottom line: Looks approval ready. The inline notes are small coverage gaps, mostly one added assertion each.

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

aduforest · 2026-10-10T19:49:47Z

Pushed fixes for all five findings above. @argus re-review

aduforest · 2026-10-11T02:24:43Z

@argus re-review

amd-argus

ARGUS

Bottom line: Looks approval ready; all seven findings from the last round are fixed. The rest are optional coverage gaps, the main one being that the ';' delimited ListOf parser never reaches resolve().

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

aduforest · 2026-10-12T18:18:17Z

@argus re-review

amd-argus

ARGUS

Bottom line: Looks approval ready; both findings from the last round are fixed. The two inline notes are optional coverage gaps, one added assertion each.

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

aduforest · 2026-10-12T18:45:50Z

@argus review

amd-argus · 2026-10-12T18:51:08Z

@aduforest the current head (58f9f5e6) 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-12T19:11:27Z

@argus review

aduforest · 2026-10-12T19:30:42Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM once Integer_RejectsNullEmptyAndTrailingGarbage repeats its two garbage cases for uint32_t: the unsigned arm's guards at parser_default.h:102 are never taken today.

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

aduforest · 2026-10-12T19:57:47Z

@argus review

aduforest · 2026-10-12T20:08:58Z

@argus review

aduforest · 2026-10-12T20:45:18Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

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

aduforest · 2026-10-13T17:57:34Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

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

aduforest · 2026-10-13T18:33:53Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

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

New rccl-UnitTestsMicroParam covers ncclParam<T> (include/param/param.h), src/param/param.cc, src/param/c_api.cc and the legacy env loading in src/misc/param.cc. It is its own binary because misc/param.cc defines ncclGetEnv, initEnv and ncclLoadParam, which the shared fakes in rccl-UnitTestsMicro also define; the env plugin, ncclOsSetEnv and logging are seamed in the test TU. initEnv() and NCCL_NO_CACHE latch behind std::call_once, so those cases run in isolated processes. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-15T19:55:44Z

@argus review

amd-argus

ARGUS

This PR changes 1,590 lines across 10 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-2820-param-parsers-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

aduforest · 2026-10-16T16:20:45Z

@argus review

amd-argus

ARGUS

This PR changes 1,588 lines across 10 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>

aduforest · 2026-10-16T21:27:25Z

@argus review

amd-argus

ARGUS

This PR changes 1,601 lines across 10 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-2820-param-parsers-host-microtests # Conflicts: # projects/rccl/test/host/CMakeLists.txt

aduforest · 2026-10-17T02:50:25Z

@argus review

amd-argus

ARGUS

This PR changes 1,601 lines across 10 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-2820-param-parsers-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

aduforest · 2026-10-19T17:25:01Z

@argus review

amd-argus

ARGUS

This PR changes 1,641 lines across 10 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-2820-param-parsers-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/tools/scripts/test_runner/lib/test_executor.py

aduforest · 2026-10-19T17:53:43Z

@argus review

amd-argus

ARGUS

This PR changes 1,631 lines across 10 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-2820-param-parsers-host-microtests

develop's #13328 added a third config source after /etc/rccl.conf. Co-Authored-By: Claude <noreply@anthropic.com>

aduforest · 2026-10-20T01:33:40Z

@argus review

amd-argus

ARGUS

This PR changes 1,603 lines across 9 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.}

systems-assistant · 2026-10-21T13:32:22Z

TheRock Submodule Bump Activity

Newest first

2026-10-21 — Included in TheRock via ROCm/TheRock#9398.

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

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

amd-argus Bot reviewed Oct 10, 2026

View reviewed changes

amd-argus Bot reviewed Oct 11, 2026

View reviewed changes

Comment thread projects/rccl/test/host/param-parsers-test.cc

Comment thread projects/rccl/test/host/param-parsers-test.cc

amd-argus Bot reviewed Oct 12, 2026

View reviewed changes

Comment thread projects/rccl/test/host/param-parsers-test.cc

Comment thread projects/rccl/test/host/param-parsers-test.cc

amd-argus Bot reviewed Oct 12, 2026

View reviewed changes

Comment thread projects/rccl/test/host/param-parsers-test.cc

amd-argus Bot reviewed Oct 12, 2026

View reviewed changes

aduforest marked this pull request as ready for review October 12, 2026 22:24

aduforest requested review from a team as code owners October 12, 2026 22:25

pvallem reviewed Oct 13, 2026

View reviewed changes

Comment thread projects/rccl/test/host/param-parsers-test.cc

Comment thread projects/rccl/test/host/param-parsers-test.cc

Comment thread projects/rccl/test/host/param-parsers-test.cc

Comment thread projects/rccl/test/host/param-parsers-test.cc

aduforest force-pushed the users/aduforest/aicomrccl-2820-param-utils-host-microtests branch from a13a778 to 5f834a3 Compare October 13, 2026 17:53

aduforest force-pushed the users/aduforest/aicomrccl-2820-param-parsers-host-microtests branch from aeabfda to 5843190 Compare October 13, 2026 17:54

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

aduforest force-pushed the users/aduforest/aicomrccl-2820-param-utils-host-microtests branch from 5f834a3 to a724e4e Compare October 13, 2026 18:32

aduforest force-pushed the users/aduforest/aicomrccl-2820-param-parsers-host-microtests branch from 5843190 to dea81a5 Compare October 13, 2026 18:32

amd-argus Bot reviewed Oct 13, 2026

View reviewed changes

aduforest changed the title ~~test(rccl): add host microtests for the param parsers and registry~~ test(rccl): add host microtests for the param subsystem Oct 15, 2026

pvallem approved these changes Oct 15, 2026

View reviewed changes

amd-argus Bot reviewed Oct 15, 2026

View reviewed changes

amd-argus Bot reviewed Oct 16, 2026

View reviewed changes

test(rccl): name the param pins KnownBug_ and update the README

00bf15a

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

amd-argus Bot reviewed Oct 16, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into users/aduforest/ai…

79c44b2

…comrccl-2820-param-parsers-host-microtests # Conflicts: # projects/rccl/test/host/CMakeLists.txt

amd-argus Bot reviewed Oct 17, 2026

View reviewed changes

amd-argus Bot reviewed Oct 19, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into users/aduforest/ai…

90c6b32

…comrccl-2820-param-parsers-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/tools/scripts/test_runner/lib/test_executor.py

amd-argus Bot reviewed Oct 19, 2026

View reviewed changes

aduforest and others added 2 commits October 19, 2026 18:23

Merge remote-tracking branch 'origin/develop' into users/aduforest/ai…

f42b26b

…comrccl-2820-param-parsers-host-microtests

test(rccl): expect initEnv to read /var/run/rccl.conf

50eda67

develop's #13328 added a third config source after /etc/rccl.conf. Co-Authored-By: Claude <noreply@anthropic.com>

amd-argus Bot reviewed Oct 20, 2026

View reviewed changes

aduforest merged commit 9da5b71 into develop Oct 20, 2026
71 of 78 checks passed

aduforest deleted the users/aduforest/aicomrccl-2820-param-parsers-host-microtests branch October 20, 2026 11:46

This was referenced Oct 21, 2026

[rocjitsu] Correct legacy floating min/max and median selection #13898

Merged

[rocjitsu] Expose backed CDNA3 and CDNA4 hardware identity fields #13899

Merged

[rocjitsu] Implement scratch LDS transfers and scalar cache discard #13900

Merged

Conversation

aduforest commented Oct 9, 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!

aduforest commented Oct 10, 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!

Uh oh!

Uh oh!

aduforest commented Oct 10, 2026

Uh oh!

aduforest commented Oct 11, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

aduforest commented Oct 12, 2026

Uh oh!

amd-argus Bot commented Oct 12, 2026

Uh oh!

aduforest commented Oct 12, 2026

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!

aduforest commented Oct 12, 2026

Uh oh!

aduforest commented Oct 12, 2026

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!

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 13, 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!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Oct 16, 2026

Uh oh!

aduforest commented Oct 9, 2026 •

edited

Loading