feat(rccl): embed CSV tuner configs in librccl.so by mustafabar · Pull Request #13038 · ROCm/rocm-systems

mustafabar · 2026-10-07T17:28:39Z

Motivation

The built-in CSV tuner (src/plugin/tuner/csv_tuner.cc) can only read its tuning table from a file on disk. On a multi-node cluster that means staging the CSV on every node or putting it on NFS, which is an operational burden and a silent mis-tune whenever one node's copy drifts. Compiling the shipped configs into the library removes that requirement.

Technical Details

tuner/*.csv is compiled into librccl.so as a std::map<std::string, std::string> (key = file name, value = CSV text) and used as the stock default. New generator cmake/GenerateEmbeddedTunerConfigs.cmake emits the content as raw string literals and fails the build if a CSV ever contains the raw-string terminator. It runs from an always-run target and rewrites the header only when the content changes, so deleting a CSV stops shipping it without churning rebuilds.

Resolution order is unchanged for steps 1-7 (NCCL_TUNER_CONFIG_FILE, then the <libdir>/tuner/ and share/rccl/tuner/ locations). The embedded map is a new step 8, so a customer-placed CSV still overrides it.
rocm_install(DIRECTORY tuner ...) and file(COPY tuner ...) are dropped, so the binary is the only stock source and the build tree cannot shadow it.
When the GPU arch is known but has no entry, the tuner falls back only to a generic rccl_tuner.csv, otherwise stays inactive. It never borrows another arch's tuning, matching the existing disk rule. When the arch cannot be determined, any embedded config may be used, same as the existing directory scan.
RCCL_TUNER_EMBEDDED_CONFIG=0 disables only the embedded defaults; NCCL_TUNER_PLUGIN=none still disables the tuner entirely.
The fgets loop became a buffer-based one shared by both sources, preserving fgets truncate-and-resume on over-long lines so file-backed and embedded configs cannot diverge. Parsing is otherwise identical to before, with one deliberate exception noted below.

One intentional parsing change

A trailing \r is now stripped from each line. This only matters for CRLF config files whose line ends on a trailing comma, where the stray CR previously became its own token:

input "allreduce,0,1023,tree,ll,4,1,8,\r\n"
before   9 tokens, numPipeOps = atoi("\r") = 0   (matches only numPipeOps == 0)
after    8 tokens, numPipeOps = -1               (matches any numPipeOps)

The old value was wrong. LF files are unaffected.

Issue Tracking

JIRA ID: AICOMRCCL-2762

Test Plan

New host microtests in test/host/csv-tuner-test.cc: that the generator embeds each tuner/*.csv byte for byte, embedded/file parse parity, arch-key selection, NCCL_TUNER_CONFIG_FILE and share/rccl/tuner/ overrides beating the embedded map, the disable knob, CRLF, and over-long-line chunking. A separate differential harness compares the old and new parsers across 32 inputs (shipped CSV, 8/9/10/11 field counts, whitespace, CRLF variants, unknown enums, no trailing newline, 255/256-char boundaries, over-long lines with valid and invalid tails).

On Ruby mi350-cluster-2 (gfx950), single-node 8 GPU, all_reduce_perf -b 1K -e 256M -f 2 -g 8 with NCCL_DEBUG=INFO NCCL_DEBUG_SUBSYS=TUNING, in four configurations: stock, NCCL_TUNER_CONFIG_FILE pointed at a copy of the same bytes, RCCL_TUNER_EMBEDDED_CONFIG=0, and NCCL_TUNER_PLUGIN=none.

Test Result

31 of the 32 differential inputs parse identically between the old and new parsers. The single difference is the CRLF trailing-comma case described above. rccl-UnitTestsMicro passes, including the new cases.

On Ruby: the stock run logs Using built-in CSV tuner, config: <embedded>/rccl_tuner_gfx950.csv and loads 7 configs. Embedded vs file source give identical Applied config decisions after dedup and busbw within run-to-run noise at all 19 sizes; #wrong=0 in every run. Both negative controls resolve no CSV tuner. strings librccl.so.1.0 confirms the CSV bytes are in the binary and build/release/tuner/ is gone.

Not covered: the nNodes=2,nRanks=16 rows are exercised by the unit test but not on hardware. The 16-rank run fails at init on this develop-HEAD build (sched_setaffinity failed: Invalid argument, src/os/linux.cc:612); it reproduces with NCCL_TUNER_PLUGIN=none and stock /opt/rocm RCCL passes the identical job, so it is pre-existing and not from this change.

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

tuner/*.csv is compiled into the library and used as the stock tuning default, so clusters no longer need the file staged on every node or on NFS. Disk locations and NCCL_TUNER_CONFIG_FILE still override it. RCCL_TUNER_EMBEDDED_CONFIG=0 disables the embedded map.

github-actions · 2026-10-07T17:29:35Z

No undocumented RCCL or NCCL environment-variable changes.

therock-pr-bot · 2026-10-07T17:29:53Z

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

Copilot

🟡 Changes recommended

New CSV discovery and test-target generation dependencies can leave configs omitted or cause clean parallel builds to fail.

3 open findings

Add unit test dependency on generated tuner config header · New
Reconfigure when new tuner CSV files are added · New
Clarify tuner inactivity when no specific or generic config exists · New

What changed in this PR

Embeds RCCL CSV tuner defaults into librccl.so while preserving disk-based overrides.

Changes:

Generates an embedded configuration header from tuner/*.csv.
Adds buffer-backed parsing and embedded-config resolution.
Adds host microtests and updated documentation.

File	Description
`projects/rccl/CMakeLists.txt`	Adds embedded-config generation and removes CSV installation.
`projects/rccl/src/CMakeLists.txt`	Makes RCCL depend on generated configs.
`projects/rccl/cmake/GenerateEmbeddedTunerConfigs.cmake`	Generates the embedded CSV header.
`projects/rccl/src/plugin/tuner/csv_tuner.cc`	Resolves and parses embedded configurations.
`projects/rccl/src/include/tuner.h`	Exposes embedded-config helpers.
`projects/rccl/test/host/csv-tuner-test.cc`	Tests parsing, resolution, and application.
`projects/rccl/test/host/CMakeLists.txt`	Adds the tuner microtests.
`projects/rccl/test/test_categories_micro.yaml`	Registers the new test suite.
`projects/rccl/tuner/README.md`	Documents embedded defaults and overrides.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Document RCCL_TUNER_EMBEDDED_CONFIG in CHANGELOG.md and env-variables.rst. Make the micro-test targets depend on the generated header so a clean parallel build cannot race it, and glob the CSVs with CONFIGURE_DEPENDS so a newly added file is picked up. Clarify the generic-config fallback in the tuner README.

amd-argus

ARGUS

Findings

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

projects/rccl/test/host/csv-tuner-test.cc:264 (Low) The target-row filter omits numPipeOps and regBuff, which csvTunerGetCollInfo also requires The target-row filter checks only nNodes and nRanks, but csvTunerGetCollInfo (csv_tuner.cc:696-697) also gates on numPipeOps and regBuff against the 1 and 0 passed at line 278. Every shipped row ends -1,-1 today, so the first retune that sets either value makes target unmatchable and line 281 fails for a reason unrelated to the code under test.

Bottom line: Worth settling before approval: the installed-override example at tuner/README.md:35 copies the CSV under a name no disk probe looks for, so the documented override is inert.

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

Add a test that the generator embeds each CSV verbatim, and tests that a share-path CSV beats the embedded map. Strengthen the over-long-line and CRLF cases so they can actually fail. Regenerate the header from an always-run target so a deleted CSV stops shipping. Fix the README install example filename and qualify the arch claim in env-variables.rst.

amd-argus

ARGUS

Bottom line: Looks approval ready; all ten findings from the last round are fixed. The one worth a second look is the unbounded config read at csv_tuner.cc:409.

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

corey-derochie-amd

LGTM. The unresolved Argus notes all look valid, so best to review and consider addressing those before merge.

Cap the config file read so a wrong path cannot grow the allocation, check the standalone generator's exit status, make the generic share-path test use an arch the embedded map carries, and note that stale tuner/ copies from earlier builds still win.

PedramAlizadeh

LGTM! Just the introduced conflicts need to be resolved.
Thanks Mustafa!

amd-argus

ARGUS

Bottom line: Looks approval ready. Nothing this round rises above Low.

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

Restrict the generator glob to rccl_tuner*.csv so a stray CSV cannot become the arch-unknown pick, and reject names that would break the string literal. Cover the arch-unknown, no-trailing-newline and oversized-file branches, derive the arch from the embedded map instead of hardcoding gfx950, and make the cost-table case select on every field getCollInfo gates on.

mustafabar · 2026-10-08T20:44:21Z

@argus re-review

amd-argus

ARGUS

Bottom line: Worth settling before approval: csvTunerInit's disk arm has no test, so dropping the resolved-path capture at csv_tuner.cc:651 would go unnoticed.

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

csvTunerInit's disk arm had no case: every existing test resolved to the embedded map, so dropping the resolved-path capture left the suite green while a share-path CSV silently loaded nothing. Also split the knob case so it distinguishes ignoring the embedded map from disabling the tuner.

amd-argus

ARGUS

Bottom line: LGTM.

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

systems-assistant · 2026-10-14T12:02:33Z

TheRock Submodule Bump Activity

Newest first

2026-10-14 — Included in TheRock via ROCm/TheRock#8996.

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

mustafabar marked this pull request as ready for review October 7, 2026 17:31

mustafabar requested review from a team and a balanced review from Copilot October 7, 2026 17:31

Copilot started reviewing on behalf of mustafabar October 7, 2026 17:32

View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Comment thread projects/rccl/test/host/CMakeLists.txt Resolved

Comment thread projects/rccl/CMakeLists.txt Resolved

Comment thread projects/rccl/tuner/README.md Resolved

wenkaidu approved these changes Oct 7, 2026

View reviewed changes

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

github-actions Bot added the documentation Improvements or additions to documentation label Oct 7, 2026

amd-argus Bot reviewed Oct 7, 2026

View reviewed changes

corey-derochie-amd approved these changes Oct 8, 2026

View reviewed changes

PedramAlizadeh approved these changes Oct 8, 2026

View reviewed changes

Merge branch 'develop' into users/mustafabar/embedded-csv-tuner

a301f5e

amd-argus Bot reviewed Oct 8, 2026

View reviewed changes

Comment thread projects/rccl/test/host/csv-tuner-test.cc Resolved

Comment thread projects/rccl/test/host/csv-tuner-test.cc Resolved

amd-argus Bot reviewed Oct 8, 2026

View reviewed changes

mustafabar merged commit cbaf38c into develop Oct 13, 2026
69 of 72 checks passed

mustafabar deleted the users/mustafabar/embedded-csv-tuner branch October 13, 2026 15:20

akankjai mentioned this pull request Oct 19, 2026

[RCCL] Cherry-pick gfx1250 tuning and AlltoAllv fixes (#12984, #13336, #13416, #13466) [bkc 20261013] #13736

Merged

Conversation

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

Uh oh!

Motivation

Technical Details

One intentional parsing change

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

github-actions Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

therock-pr-bot Bot commented Oct 7, 2026

✅ All Policy Checks Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

corey-derochie-amd left a comment

Choose a reason for hiding this comment

Uh oh!

PedramAlizadeh 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mustafabar commented Oct 8, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

systems-assistant Bot commented Oct 14, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

mustafabar commented Oct 7, 2026 •

edited

Loading

github-actions Bot commented Oct 7, 2026 •

edited

Loading