feat(install): make the ROCm compiler toolchain opt-in

[rominf]

Summary

Make the ROCm compiler toolchain opt-in. rocm install sdk now installs rocm[libraries] by default, and --devel adds the toolchain for people who build GPU code.

Root cause

Every wheel SDK install pulled in the devel extra — headers, static libraries, hipcc, and the full LLVM toolchain — whether or not the user would ever compile anything. It is roughly half the download:

target	[libraries]	[libraries,devel]	saved
gfx110X-dgpu, Linux	0.56 GiB	2.04 GiB	1.48 GiB (72%)
gfx110X-dgpu, Windows	0.78 GiB	1.97 GiB	1.19 GiB (60%)
gfx94X-dcgpu, Linux	1.70 GiB	3.06 GiB	1.36 GiB (44%)
gfx1151, Linux	0.47 GiB	1.81 GiB	1.34 GiB (74%)

Compressed wheel sizes at ROCm 7.10.0; unpacked is larger. The two extras are disjoint, so dropping devel cannot remove anything needed to run a model.

Nothing in this repository needs it at run time. The SDK probe already tolerates its absence and backfills paths from the runtime packages, and runtime_only_rocm_sdk_probe_validates_without_devel_root already covered that path before this change. No codepath compiles against ROCm headers: the vLLM install is a plain wheel install, Lemonade downloads prebuilt backends, and ComfyUI has no custom-node build path. hipcc is only ever an existence marker for detecting a system ROCm install.

Technical decisions

A positive flag, not a negative one. --devel matches the dominant convention for this command (--reinstall, --replace, --dkms); the codebase has exactly one negative flag, so --no-devel would match the outlier.

The choice is recorded in the runtime manifest so rocm update reinstalls what the user picked rather than silently adding or dropping the toolchain. Manifests written before this change have no such field and were all toolchain installs, so a missing field deserializes as true — defaulting to false there would silently strip the toolchain on the next update.

For an adopted environment, the probe's CMake path tells us whether the toolchain is present, so adoption records what the environment actually has. This also gives cmake_path its first reader.

SdkInstallRequest groups the install arguments; the eighth positional parameter crossed the clippy threshold and the call sites were getting hard to read.

Tests

• pip_runtime_omits_devel_extra_by_default — the default spec is rocm[libraries] and the torch stack is unaffected.
• legacy_manifest_without_devel_field_is_treated_as_having_it.
• The existing pinned-spec test still covers the --devel path.

Verified against the live index: the default dry run resolves rocm[libraries]==7.13.0, and --devel resolves rocm[libraries,devel]==7.13.0.

Not yet verified

I could not run a real install here, so two things want checking on a GPU lane before this is trusted in anger:

• Whether the resolver pulls rocm-sdk-devel back in transitively via the torch wheels. A --dry-run resolve on a runner would settle it.
• Triton compiles kernels at run time under vLLM. On ROCm it bundles its own toolchain and uses the runtime packages, and nothing here points it at the devel tree — but a runtime-only vLLM serve run should confirm that.

If maintainers would rather land this behaviour-neutral first, it could ship as an opt-out and flip the default later; given the evidence I think that mostly costs a second flag rename, but I'm happy either way.

Fixes #164

[volen-silo]

Review: feat(install): make the ROCm compiler toolchain opt-in

Change type/scope: behaviour change to rocm install sdk defaults, plus a manifest schema addition and an argument-struct refactor. 6 files, +204/−47, single commit on d17fc0c.

Overall assessment: needs work. The core change is well-reasoned, well-tested at the unit level, and the PR body is unusually honest about its own gaps. Two concrete follow-ons were missed, and the CI evidence is weaker than it appears.

Executed here: cargo fmt --all -- --check and cargo clippy --locked --workspace --all-targets -- -D warnings both clean; cargo test --workspace --all-targets --no-fail-fast passes except two known-local WSL2 process-tree tests unrelated to this PR (#168/#169); cargo xtask manifest --check clean.

---

Blocking

1. scripts/therock_sdk_install_test.py still asserts the old default and will now fail every run

scripts/therock_sdk_install_test.py:29:

THEROCK_SDK_PACKAGE_SPEC = "rocm[libraries,devel]"

asserted at line 499:

assert_contains(install_output, f"{THEROCK_SDK_PACKAGE_SPEC}==", "sdk install")

The script's own rocm install sdk invocation never passes --devel, and the script has no --devel option at all.

Built the PR binary and ran the dry-run it exercises:

$ ./target/debug/rocm install sdk --family gfx110X-all --dry-run
  package_specs: rocm[libraries]==7.13.0 torch==2.11.0+rocm7.13.0 ...

rocm[libraries,devel]== is not present anywhere in the output, so this assertion fails deterministically.

This is not merely adjacent — this PR edited the documentation of this exact script. docs/testing.md:167-169, changed in this diff, now reads:

a single TheRock-index pip install plan for pinned rocm[libraries], torch, torchvision, and torchaudio versions (--devel adds the compiler toolchain, giving rocm[libraries,devel])

The doc says one thing; the script it documents asserts the other. The script is not CI-gated, which is why checks are green — but it is the repo's documented live acceptance test for SDK installs, invoked from docs/testing.md:181,187,194,201 and docs/manual-testing.md:134.

Suggested fix: split the constant (THEROCK_SDK_PACKAGE_SPEC_BASE / ..._DEVEL), assert the base spec for the default invocation, and add a --devel flag to the script with a matching assertion so the opt-in path stays covered.

2. The repo's documented vLLM from-source build needs the toolchain, and its docs were not updated

docs/vllm.md:16-19 (unchanged by this PR):

For rocm-cli managed TheRock runtimes, prefer building vLLM from source against the existing TheRock PyTorch stack.

and docs/vllm.md:61-75:

current vLLM source required the GPTQ compatibility guard in csrc/libtorch_stable/quantization/gptq/compat.cuh to include HIP 7.13 … Without that patch, q_gemm.hip fails to compile because TheRock 7.13 headers do not expose the half/half2 atomicAdd overloads …

docs/testing.md:897-900 repeats it. Compiling .hip sources against TheRock headers is precisely what the devel extra provides. After this change, a user following the repo's own recommended vLLM setup on a default install has no headers and no compiler, and nothing in the output or docs tells them to pass --devel.

This qualifies rather than refutes the PR body's "No codepath compiles against ROCm headers" — true of rocm-cli's own code, but the repo documents a user-facing build workflow that does. docs/vllm.md and the vLLM section of docs/testing.md should say --devel is required for the from-source path. (git diff d17fc0c..HEAD -- docs/vllm.md docs/testing.md touches only line 167-169 of testing.md.)

3. Stale user-facing error message still names rocm[libraries,devel] on the default path

apps/rocm/src/therock.rs:1255-1260, in resolve_pip_runtime_from_index:

format!(
    "no mutually compatible TheRock rocm[libraries,devel], torch, torchvision, and torchaudio versions were found for {requested} in {index_url}"
)

Reached unconditionally from install_wheel_runtime (therock.rs:1179) regardless of include_devel, so a user who runs plain rocm install sdk and hits a resolution failure is told about a spec they did not request. Every other user-facing string in the same file was correctly parameterised on include_devel (therock.rs:889-897, 967-971), so this reads as a missed spot. One-line fix; blocking only because it is an objective factual inaccuracy introduced by this change.

The resolver itself is fine — version selection queries the plain rocm package from the simple index and never checks devel-extra availability, so only the message needed changing.

---

Design review

CI green does not settle the two risks the PR body flags

The PR body says a runtime-only vLLM serve run is needed to confirm Triton still works without the devel tree. All checks are green including the three GPU lanes. That is not evidence for the new default:

1. The GPU lane's install never ran under the new default. .github/workflows/ci.yml:880 gates the pre-warm on if [ ! -d "$E2E_SHARED_RUNTIMES_DIR/registry" ], and the comment at ci.yml:855-856 says it plainly: "Persisted across runs on RUNNER_WORKSPACE, so after the first run ever the pre-warm below is a no-op." The e2e-gpu log for head 1ae9877 shows shared runtime already present … — skipping pre-warm. The serve scenarios ran against a runtime installed by an earlier, pre-PR run — a rocm[libraries,devel] install. There is no flag- or version-based cache invalidation.
2. The vLLM inference scenarios are not gating. tests/e2e-cucumber/expectations.toml:41-46 and :76-81 register serve-vllm-inference and serve-readiness-contract as flaky = true xfail under EAI-7333 when effective_engine = "vllm". They XPASSed, but a genuine toolchain regression could sit inside that tolerated bucket without turning the build red.

Also, --devel has zero E2E coverage — no scenario passes it. The only scenario doing a genuinely fresh install sdk under the new default (runtime-install-sdk-active) is @nightly-gated, skipped per-PR, and asserts nothing about toolchain presence.

The transitive-rocm-sdk-devel question (PR risk #1) cannot be settled from the repo: the dry-run path only echoes a constructed uv pip install string and never invokes a real resolver.

Recommendation: before trusting this, either force a pre-warm invalidation for this change so one GPU lane actually installs under the new default and then serves, or run the confirmation manually and record it. Landing on green checks alone would be landing on evidence that does not cover the change.

The devel flag records intent, but nothing keeps it aligned with reality

• runtime_key (therock.rs:3129-3141) excludes devel, so devel and non-devel installs of the same version share one install_root and one manifest.
• ensure_uv_venv (therock.rs:2275-2294) reuses an existing venv, and uv_pip_install_base (crates/rocm-core/src/uv.rs:82-89) issues a plain uv pip install with no --reinstall/sync — installs are strictly additive.
• save_runtime_manifest (therock.rs:3159-3181) unconditionally overwrites, with devel: include_devel (therock.rs:1000).

So a user who has the toolchain and re-runs the plain rocm install sdk (the command the README documents) keeps hipcc/headers on disk but silently has their manifest rewritten to devel: false. The next rocm update, which builds a new environment at a newer version, then omits the toolchain — silently. Nothing warns, and neither rocm runtimes list nor rocm examine prints devel (render_runtimes_text, main.rs:5628-5748; append_examine_runtime_state, main.rs:11074-11167), so the state driving the next update is invisible.

The counter-argument is real — the user did omit --devel — which is why this is not blocking. But it undercuts the PR's own stated goal ("reinstalls what the user picked rather than silently adding or dropping the toolchain"), so it deserves a deliberate decision. Options: make the flag sticky (a plain reinstall can add devel but never clear it), derive it from the probe uniformly, or surface it in rocm runtimes list so drift is visible.

devel: probe.cmake_path.is_some() has a confirmed false-negative path

main.rs:6674-6676 infers the flag for adopted environments from probe.cmake_path. In the embedded probe (therock.rs:2432-2481), cmake_path is set in the from rocm_sdk import _devel branch (2445) or by a fallback checking root_path/lib/cmake (2478-2481).

• False negative (confirmed reachable): the pre-existing test runtime_only_rocm_sdk_probe_validates_without_devel_root (therock.rs:4284-4330) shows a probe carrying root_path_error: "ModuleNotFoundError: …" is an accepted, validating state. There cmake_path stays None unless some other package root happens to expose lib/cmake — so an adopted environment that genuinely has the toolchain can be recorded devel: false.
• False positive (plausible, unconfirmed): the fallback would fire if any runtime-only package root ships lib/cmake. I could not confirm this without inspecting real TheRock wheels.

Consequential rather than inert: neither select_runtime_update_source (main.rs:13237-13256) nor current_runtime_manifest (main.rs:6806-6819) filters on manifest.read_only, so an adopted manifest can be the source for rocm update and a wrong inference flows straight into include_devel. A comment noting the heuristic's limits, or gating on root_path_error, would help.

---

Non-blocking

• README.md:258-260 — the rocm install sdk flag synopsis is the primary user-facing flags reference and does not list --devel.
• --devel is silently ignored with --format tarball. Executed: rocm install sdk --family gfx110X-all --format tarball --devel --dry-run succeeds with no mention of the flag; install_sdk (therock.rs:475-493) only threads include_devel into the wheel branch. The sibling flag one line above hard-errors instead: bail!("specific TheRock version selection is only supported for wheel installs"). Behaviour is safe (tarballs ship the full SDK), but the asymmetry is confusing — a note in the help ("implied for --format tarball") or a matching diagnostic would close it.
• Help text omits the cost. main.rs:517: "Also install the ROCm compiler and headers, for building GPU code." The roughly-doubled download — the entire motivation for the change — is the fact a user needs at the moment they decide. The after_help EXAMPLES block (main.rs:493-497) shows no --devel example either.
• render_install_sdk_dry_run_for_args hand-rolls flag parsing. main.rs:10512: args.iter().any(|arg| arg == "--devel"), where the file's convention is chat_cli_has_flag (main.rs:9334, used at 9211, 9222, 14633, 14646) — and the sibling values in the same function use chat_cli_arg_value. Unreachable today (providers.rs's install_sdk_dry_run schema has no devel property), but a landmine if that parameter is added.
• docs/wsl.md:110 — the edit to rocm[libraries] is accurate for that section (it is scoped to running pre-built HIP apps). Unlike the other two edited docs it never mentions --devel, so a WSL reader who does need to build has no signpost.
• Stale CI comments. .github/workflows/ci.yml:39 calls the nightly scenario a "cold devel install"; ci.yml:852 and tests/e2e-cucumber/tests/e2e.rs:131 cite "an ~8.8 GiB devel tarball" as the pre-warm cost driver. Both now misdescribe the default. Informational — the real effect is a CI speedup.
• Test gaps. Nothing asserts adopt_runtime_from_probe's new devel inference (runtime_adopt_records_read_only_manifest_from_probe, ~main.rs:22254, builds a probe with cmake_path: None and never checks adopted.devel — one assert_eq! away); nothing covers the CLI-level --devel wiring; nothing covers the install-then-reinstall manifest-overwrite path above.

---

Tradeoffs

• devel excluded from runtime_key. Collapsing devel and non-devel into one runtime matches the additive install model and avoids doubling disk usage for what users perceive as one install. Including it would eliminate the manifest drift at the root, at the cost of two registered runtimes. The choice looks right; the drift issue is a consequence of it that wants handling separately.
• #[derive(Default)] on SdkInstallRequest<'a> with &str fields. channel/format default to "", which hits TheRockChannel::parse("") → Err("unsupported TheRock channel: ") before anything else, so it fails loudly. All four call sites set both explicitly. The stringly-typed boundary (format_name at main.rs:2021-2033 stringifies the typed InstallFormat enum to fit the struct) is inherited from the pre-change 6-arg signature, not introduced here.
• Direct default flip vs. opt-out first. Repo precedent is on this PR's side: 9ab576b (pin release signing key, verify by default) and 4108ea7 (deployment summary by default) both flipped user-visible defaults directly in one PR with docs updated alongside, and there is no CHANGELOG. It is the docs-updated-alongside half of that precedent that is incomplete (blocking item 2).

---

Positive signals

• devel_default_for_legacy_manifest (therock.rs:255-262) is a named function with a comment explaining why the default is true rather than a bare #[serde(default)] — exactly the failure mode (silently stripping the toolchain on the next update) a bare default would have caused.
• Both new tests are real, not tautological. Mutation-tested (then reverted): forcing extras = "libraries,devel" unconditionally fails pip_runtime_omits_devel_extra_by_default; flipping the legacy default to false fails legacy_manifest_without_devel_field_is_treated_as_having_it.
• The SdkInstallRequest refactor is proportionate — it converts a growing positional list at the point a 7th argument would have crossed the readability threshold, rather than as speculative cleanup.
• The PR body's "Not yet verified" section is candid and specific. That is what let this review target the CI-evidence question directly.

---

Not verified

• No real (non-dry-run) SDK install and no GPU work. Every install-path claim is from dry-run output plus source tracing. I never observed a rocm[libraries] environment actually running a model.
• PR risk Let Lemonade auto-select its llama.cpp backend #1 (transitive rocm-sdk-devel via the torch wheels) is unresolved — needs a live resolve on a runner.
• PR risk Enable native-certs for ureq across all crates #2 (Triton runtime compilation under vLLM without the devel tree) is unresolved, and the green GPU lanes do not settle it.
• I did not run scripts/therock_sdk_install_test.py end to end — venv creation failed here (missing python3.12-venv). Its breakage is proven from the CLI's dry-run output plus the script's literal assertion, not from a script run.
• I did not inspect real TheRock wheel contents, so the cmake_path false-positive scenario stays plausible-unconfirmed.
• I did not run the e2e-cucumber GPU scenarios locally — CI claims come from reading the e2e-gpu log for head 1ae9877.

[juhovainio]

I reviewed this PR and found two issues worth fixing before merge — left as inline comments below. Both are real regressions caused by the new opt-in-devel default, not style nitpicks.

I also looked into a few other concerns that have come up around this change (the from-source vLLM docs not mentioning --devel is now needed, CI not really exercising the new default because of GPU-lane pre-warm caching, and a claimed bug in cmake_path.is_some()), but none of them held up as clear bugs worth blocking on — the cmake_path one in particular looks like it's based on a test that actually shows the check working correctly, not failing.

[juhovainio]

include_devel now defaults to false, which flips the SDK's default extras from rocm[libraries,devel] to rocm[libraries].

scripts/therock_sdk_install_test.py (not touched by this PR) installs without --devel but still asserts the output contains rocm[libraries,devel]== (THEROCK_SDK_PACKAGE_SPEC at line 29, asserted at line 499). This documented manual smoke test (linked from docs/testing.md and docs/manual-testing.md) will now fail every time it's run.

Either pass --devel in the script's install invocation, or update THEROCK_SDK_PACKAGE_SPEC to "rocm[libraries]".

[juhovainio]

This is the pattern used everywhere else in the file to make user-facing strings reflect include_devel — but resolve_pip_runtime_from_index's error message (around line 1257) doesn't receive include_devel at all, and always says:

no mutually compatible TheRock rocm[libraries,devel], torch, torchvision, and torchaudio versions were found for {requested} in {index_url}

Someone who didn't pass --devel and hits this failure gets an error naming a toolchain they never asked for.

Thread include_devel into resolve_pip_runtime_from_index (or its caller) and branch the message the same way this block does.