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

aduforest · 2026-09-29T02:31:01Z

JIRA ID: AICOMRCCL-2460

What

Adds host-only microtests for src/ras/diagnostics_env.cc, covering environment collection, rank summaries, and error handling without changing production code.

The 32 enabled tests cover:

local NCCL environment collection and non-NCCL filtering;
empty, exact-capacity, and truncated payloads;
reporter and payload validation failures;
consistent, incomplete, mismatched, unset, and truncated rank summaries;
multiple communicators, malformed payloads, display truncation, and control-character sanitization;
stable rank grouping with repeated values and reporter-error propagation across every summary path;
multi-key ordering/deduplication and allocation failures in collection and summarization.

One additional disabled regression tracks the production reporting defect AICOMRCCL-2743.

How

ras-diagnostics-env-test.cc includes the hipified ras/diagnostics_env.cc directly in the existing rccl-UnitTestsMicro binary. It reuses the real shared diagnostics helpers and the communicator registry already supplied by the existing RAS microtests.

Coverage

Previously reported local coverage for this source file is listed below. Coverage was not regenerated for this review follow-up; the CI results below are historical.

Functions: 20/20 (100.00%)
Lines: 298/298 (100.00%)
Regions: 557/763 (73.00%)
Branches: 286/428 (66.82%)

Mutation Testing

The following results are preserved from the previously reported local mutation runs. Current-head mutation results have not been verified.

Previously reported targeted behavioral mutations: 13/13 killed.

The NCCL-prefix filter, exact-capacity boundary, value equality, covered-rank grouping, reporter-error propagation, key-length ordering and matching, key deduplication, truncated nonempty payload handling, full communicator identity, candidate-key sizing, and allocation-failure mutations were all caught by the suite.

Historical CI

Host unit tests (CPU-only) CI passed for commit 6a5dedc6b8be after merging develop and retaining both the RAS and tuning test registrations.

Check	Result
`RasDiagnosticsEnvMicrotest.*`	25/25 passed in the shared binary
Full `rccl-UnitTestsMicro`	1468/1468 passed
Host unit tests (CPU-only) job	Passed
Production source changes	None; changes are confined to `projects/rccl/test/`

Review follow-up

Centralized environ save/override/restore in fakes/env_fakes.{h,cc}. During an enumeration override, SetMicroEnviron, SetMicroEnv, micro_getenv, and getenv agree; tests cover updates, absence, duplicates, malformed entries, and restoration.
Environment, common diagnostics, and RAS suites share mutex-protected communicator registry installation/reset, including sorted-state reset. Common diagnostic owned-communicator/reporter helpers are shared too.
Wrapped collector output in RAII, collapsed the allocation forwarders, removed internal allocation-layout assertions, and derive the failure-injection sweep from a successful baseline run.
Split reporter-error paths and negative-size validation into individually named tests; derive the displayed byte budget from RAS_DIAG_ENV_BYTES.

Deferred production finding

AICOMRCCL-2743, under AICOMRCCL-2397, tracks the environment reporters' omission of host/process hashes. Different communicator identities sharing commHash currently look identical in reports. Production grouping is not alleged broken.

DISABLED_Summarize_HashCollisionsReportFullCommIdentity specifies full IDs for consistent, mismatch, and mismatch-summary output. Explicitly enabling it reproduces all three expectation failures on current production. It remains disabled pending a separate production fix. The enabled multi-communicator test still checks grouping without requiring the shortened identity format.

Current local validation

32/32 enabled environment tests, plus 23 common diagnostics tests and 71 RAS tests, passed 20 shuffled repetitions (126 tests per iteration). One production regression is disabled as described above.
Environment/common diagnostics and the environment fake use ASan + UBSan. The RAS translation unit uses ASan only, because an existing unrelated malformed-notification fixture loads enum value 99 and trips enum UBSan in ras.cc. Leak detection is disabled.
Changed translation units were rebuilt and linked against cached shared fake objects and GoogleTest. This is focused validation, not a fresh full host-binary build. Current-head full CI is pending; coverage and the historical 13-mutation set have not been rerun.

aduforest · 2026-09-29T02:31:05Z

@argus review

therock-pr-bot · 2026-09-29T02:31:44Z

✅ 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?

amd-argus

ARGUS

Bottom line: Worth settling before approval: no test goes above 2 ranks, so the covered[] rank grouping is unpinned, and no test installs a failing emit, so every reporter-error path stays untaken.

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

aduforest · 2026-09-29T07:50:52Z

@argus re-review

amd-argus

ARGUS

Bottom line: Worth settling before approval: no Summarize test feeds two distinct env keys, leaving the key dedup and comparator in diagnostics_env.cc unpinned; the allocation-failure seam from the last round is also still open.

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

aduforest · 2026-09-29T18:05:12Z

@argus re-review

amd-argus

ARGUS

Bottom line: Worth settling before approval: the new two-key test uses NCCL_Z/NCCL_A, which share no prefix, so the key-prefix handling at diagnostics_env.cc:51 and :82 is still unpinned.

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

amd-argus

ARGUS

Bottom line: Worth settling before approval: no test feeds a multi-rank comm whose env is consistent, so deleting the value compare at diagnostics_env.cc:277 leaves all 24 tests green.

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

aduforest · 2026-09-30T05:39:29Z

@argus re-review

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: ras-diagnostics-env-test.cc:339 and 6 more. Worth clearing before approval.

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

aduforest · 2026-09-30T08:25:48Z

@argus review

amd-argus · 2026-09-30T08:30:48Z

@aduforest the current head (991a70a0) 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-09-30T17:32:05Z

@argus review

amd-argus

ARGUS

Bottom line: LGTM.

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

amd-argus

ARGUS

Bottom line: Worth settling before approval: the suite's only multi-comm case has empty env payloads, so no test ever compares keys for a comm after the first.

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

aduforest · 2026-10-02T06:24:39Z

@argus re-review

amd-argus

ARGUS

Bottom line: LGTM.

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

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: ras-diagnostics-env-test.cc:501. Worth clearing before approval.

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

amd-argus

ARGUS

Bottom line: LGTM.

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

amd-argus

ARGUS

Bottom line: LGTM.

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

JeniferC99 · 2026-10-06T20:46:13Z

Cancelling the remaining gfx950 jobs on this run so the TheRock submodule bump PRs can be prioritized on the gfx950 queue.

This change does not target gfx950, and gfx950 was the only work still outstanding. Jobs that already finished are unchanged. Re-run failed jobs if you need gfx950 results before merge.

amd-argus

ARGUS

Bottom line: LGTM.

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

systems-assistant · 2026-10-09T23:38:50Z

TheRock Submodule Bump Activity

Newest first

2026-10-09 — Included in TheRock via ROCm/TheRock#8885.

JIRA ID: AICOMRCCL-2460 ## What Adds host-only microtests for `src/ras/diagnostics_env.cc`, covering environment collection, rank summaries, and error handling without changing production code. The 32 enabled tests cover: - local NCCL environment collection and non-NCCL filtering; - empty, exact-capacity, and truncated payloads; - reporter and payload validation failures; - consistent, incomplete, mismatched, unset, and truncated rank summaries; - multiple communicators, malformed payloads, display truncation, and control-character sanitization; - stable rank grouping with repeated values and reporter-error propagation across every summary path; - multi-key ordering/deduplication and allocation failures in collection and summarization. One additional disabled regression tracks the production reporting defect AICOMRCCL-2743. ## How `ras-diagnostics-env-test.cc` includes the hipified `ras/diagnostics_env.cc` directly in the existing `rccl-UnitTestsMicro` binary. It reuses the real shared diagnostics helpers and the communicator registry already supplied by the existing RAS microtests. ## Coverage Previously reported local coverage for this source file is listed below. Coverage was not regenerated for this review follow-up; the CI results below are historical. - Functions: 20/20 (100.00%) - Lines: 298/298 (100.00%) - Regions: 557/763 (73.00%) - Branches: 286/428 (66.82%) ## Mutation Testing The following results are preserved from the previously reported local mutation runs. Current-head mutation results have not been verified. Previously reported targeted behavioral mutations: 13/13 killed. The NCCL-prefix filter, exact-capacity boundary, value equality, covered-rank grouping, reporter-error propagation, key-length ordering and matching, key deduplication, truncated nonempty payload handling, full communicator identity, candidate-key sizing, and allocation-failure mutations were all caught by the suite. ## Historical CI [Host unit tests (CPU-only) CI](https://github.com/ROCm/rocm-systems/actions/runs/36798053397/job/110165926508) passed for commit `6a5dedc6b8be` after merging develop and retaining both the RAS and tuning test registrations. | Check | Result | | --- | --- | | `RasDiagnosticsEnvMicrotest.*` | 25/25 passed in the shared binary | | Full `rccl-UnitTestsMicro` | 1468/1468 passed | | Host unit tests (CPU-only) job | Passed | | Production source changes | None; changes are confined to `projects/rccl/test/` | ## Review follow-up - Centralized `environ` save/override/restore in `fakes/env_fakes.{h,cc}`. During an enumeration override, `SetMicroEnviron`, `SetMicroEnv`, `micro_getenv`, and `getenv` agree; tests cover updates, absence, duplicates, malformed entries, and restoration. - Environment, common diagnostics, and RAS suites share mutex-protected communicator registry installation/reset, including sorted-state reset. Common diagnostic owned-communicator/reporter helpers are shared too. - Wrapped collector output in RAII, collapsed the allocation forwarders, removed internal allocation-layout assertions, and derive the failure-injection sweep from a successful baseline run. - Split reporter-error paths and negative-size validation into individually named tests; derive the displayed byte budget from `RAS_DIAG_ENV_BYTES`. ## Deferred production finding **AICOMRCCL-2743**, under **AICOMRCCL-2397**, tracks the environment reporters' omission of host/process hashes. Different communicator identities sharing `commHash` currently look identical in reports. Production grouping is not alleged broken. `DISABLED_Summarize_HashCollisionsReportFullCommIdentity` specifies full IDs for consistent, mismatch, and mismatch-summary output. Explicitly enabling it reproduces all three expectation failures on current production. It remains disabled pending a separate production fix. The enabled multi-communicator test still checks grouping without requiring the shortened identity format. ## Current local validation - **32/32 enabled environment tests**, plus **23 common diagnostics tests and 71 RAS tests**, passed **20 shuffled repetitions** (126 tests per iteration). One production regression is disabled as described above. - Environment/common diagnostics and the environment fake use **ASan + UBSan**. The RAS translation unit uses **ASan only**, because an existing unrelated malformed-notification fixture loads enum value 99 and trips enum UBSan in `ras.cc`. Leak detection is disabled. - Changed translation units were rebuilt and linked against cached shared fake objects and GoogleTest. This is focused validation, not a fresh full host-binary build. Current-head full CI is pending; coverage and the historical 13-mutation set have not been rerun.

aduforest requested review from a team September 29, 2026 02:31

github-actions Bot added the project: rccl label Sep 29, 2026

systems-assistant Bot added the organization: ROCm label Sep 29, 2026

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

aduforest force-pushed the users/aduforest/ras-diagnostics-env-host-microtests branch from 09e80bb to dc2bdaa Compare September 29, 2026 07:50

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

aduforest force-pushed the users/aduforest/ras-diagnostics-env-host-microtests branch from dc2bdaa to aead397 Compare September 29, 2026 18:02

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

aduforest force-pushed the users/aduforest/ras-diagnostics-env-host-microtests branch from aead397 to 069e797 Compare September 30, 2026 00:36

amd-argus Bot reviewed Sep 30, 2026

View reviewed changes

Comment thread projects/rccl/test/host/ras-diagnostics-env-test.cc Resolved

test(rccl): add host microtests for diagnostics_env.cc

991a70a

aduforest force-pushed the users/aduforest/ras-diagnostics-env-host-microtests branch from 069e797 to 991a70a Compare September 30, 2026 05:32

amd-argus Bot reviewed Sep 30, 2026

View reviewed changes

test(rccl): address diagnostics environment microtest review findings

f67f934

amd-argus Bot reviewed Sep 30, 2026

View reviewed changes

aduforest marked this pull request as ready for review September 30, 2026 20:20

Merge develop and preserve both RAS and tuning test registration

6a5dedc

amd-argus Bot reviewed Oct 1, 2026

View reviewed changes

Comment thread projects/rccl/test/host/ras-diagnostics-env-test.cc Resolved

test(rccl): cover environment offsets across communicators

68767d5

amd-argus Bot reviewed Oct 2, 2026

View reviewed changes

mch approved these changes Oct 2, 2026

View reviewed changes

test(rccl): address RAS diagnostics review feedback

3ac5eae

amd-argus Bot reviewed Oct 3, 2026

View reviewed changes

pvallem approved these changes Oct 5, 2026

View reviewed changes

Merge develop and align diagnostics report expectations

8dc7d92

amd-argus Bot reviewed Oct 6, 2026

View reviewed changes

Merge develop after diagnostics host tests

c4e8f1d

amd-argus Bot reviewed Oct 6, 2026

View reviewed changes

aduforest added the Blocked by CI Waiting for CI to pass before reviews or merging label Oct 7, 2026

Merge develop after ras_param host tests

4b8302f

amd-argus Bot reviewed Oct 8, 2026

View reviewed changes

aduforest merged commit 40d322b into develop Oct 9, 2026
94 of 106 checks passed

aduforest deleted the users/aduforest/ras-diagnostics-env-host-microtests branch October 9, 2026 07:18

peymanr mentioned this pull request Nov 6, 2026

test(rccl): check config parsers keep the caller's strtok state #12648

Open

1 task done

Conversation

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

Uh oh!

What

How

Coverage

Mutation Testing

Historical CI

Review follow-up

Deferred production finding

Current local validation

Uh oh!

aduforest commented Sep 29, 2026

Uh oh!

therock-pr-bot Bot commented Sep 29, 2026

✅ All Policy Checks Passed

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 Sep 29, 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!

aduforest commented Sep 29, 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!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduforest commented Sep 30, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

aduforest commented Sep 30, 2026

Uh oh!

amd-argus Bot commented Sep 30, 2026

Uh oh!

aduforest commented Sep 30, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduforest commented Oct 2, 2026

Uh oh!

amd-argus Bot left a comment

aduforest commented Sep 29, 2026 •

edited

Loading