Skip to content

test(rccl): add binary-mode Recorder RedOp record tests - #12704

Merged
pvallem merged 3 commits into
users/pvallem/GIN_TESTS-combined-miscfrom
users/pvallem/GIN_TESTS-recorder-binary
Oct 2, 2026
Merged

pvallem merged 3 commits into
users/pvallem/GIN_TESTS-combined-miscfrom
users/pvallem/GIN_TESTS-recorder-binary

Conversation

@pvallem

@pvallem pvallem commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Only the JSON path of Recorder RedOp records is tested, so raw rcclApiCall fields written in binary mode can regress unseen.

Fix: read both RedOp records back from the binary dump and check each field; stacked on #12641.

JIRA ID: AICOMRCCL-2699

Recorder.RedOpRecordJson only sees what the JSON format strings print,
so the raw rcclApiCall fields written in binary mode, and anything
Destroy must not copy, are unchecked. Recorder.RedOpRecordBinary reads
both records back from the binary dump and is registered next to
RedOpRecordJson in every test_runner config.

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM once the Destroy assertions at _RecorderTests.cpp:190 pin the recorded values with EXPECT_EQ.

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

Comment thread projects/rccl/test/_RecorderTests.cpp Resolved
@pvallem

pvallem commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

@argus re-review

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM.

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

@pvallem
pvallem merged commit e8fdafb into users/pvallem/GIN_TESTS-combined-misc Oct 2, 2026
48 of 49 checks passed
@pvallem
pvallem deleted the users/pvallem/GIN_TESTS-recorder-binary branch October 2, 2026 20:55
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