feat(install): make the ROCm compiler toolchain opt-in - #167
Conversation
Review:
|
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
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.
| pub family_override: Option<&'a str>, | ||
| pub dry_run: bool, | ||
| /// Install the compiler and headers alongside the runtime libraries. | ||
| pub include_devel: bool, |
There was a problem hiding this comment.
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]".
There was a problem hiding this comment.
Confirmed and fixed in df19fb0 — I reproduced it before changing anything, and you were right that it fails on every run.
I took your second option, because this PR's own doc edits had already settled the direction: docs/testing.md and docs/manual-testing.md now describe the script as verifying pinned rocm[libraries]. So THEROCK_SDK_PACKAGE_SPEC is gone and the expected spec comes from therock_sdk_package_spec(args.devel), defaulting to the runtime-only extras.
Two things beyond the minimum, since the flag this PR adds had no acceptance coverage at all:
- a
--develpassthrough, so the opt-in path can be exercised rather than merely left unbroken, with a negative assertion on each side (a default install must not planrocm[libraries,devel], and a--develinstall must not planrocm[libraries]==). Those two lines are the only thing that would catchpackage_specs:andpackage_policy:drifting apart, since they are independent format sites. - an assertion that the manifest records the
develvalue actually requested.apply_runtime_updatereinstalls from that field, so a wrong value there silently adds or drops the toolchain on the next update.
I have not run the script end to end — it downloads multiple GB of wheels. The spec assertion sits before the --dry-run early return, so --dry-run --family <family> reaches it cheaply if you want to confirm without the download.
There was a problem hiding this comment.
Re-checked after today's rebase onto the current base tip (1f6ba03b): the answer still holds, and the mechanism is unchanged.
THEROCK_SDK_PACKAGE_SPEC is still gone; therock_sdk_package_spec(include_devel) (scripts/therock_sdk_install_test.py:33) still derives the expected spec, the install assertion at :541 still reads therock_sdk_package_spec(args.devel), and the --devel passthrough at :510 plus verify_manifest_devel at :378/:612 are intact. The resolution-path change since my earlier reply touched published_pip_requirements in apps/rocm/src/therock.rs, not this script, and did not reintroduce a hardcoded spec anywhere the script asserts against.
One thing I should state plainly rather than leave implied, since you are the person most likely to run this script: no workflow invokes it. I grepped all of .github/workflows/ just now and there are zero references — its only callers are the documented manual commands in docs/testing.md and docs/manual-testing.md. So the --devel passthrough and verify_manifest_devel are the most rigorous assertions in this PR and nothing runs them automatically; they protect this change only when someone runs them by hand. That gap is named in the PR description under "Gaps, stated". I have not run the script in this round (it needs uv, a live index and a real GPU host, none of which I have here) — what I verified is that the code it asserts against is unchanged.
Leaving this open for your verification.
| "install TheRock SDK with the compiler toolchain, torch stack, and resolved dependencies" | ||
| } else { | ||
| "install TheRock SDK, torch stack, and resolved dependencies" | ||
| }, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Confirmed and fixed in df19fb0. include_devel now reaches that message.
Threading it as a plain parameter pushed resolve_pip_runtime_from_index to 8 arguments and tripped clippy's too_many_arguments, so it goes through a PipRuntimeQuery struct instead — the same argument-struct pattern this PR already introduced for SdkInstallRequest. That also let resolve_pip_runtime and resolve_pip_runtime_with_timeout collapse into a single function, and the previously implicit None timeout is now explicit at the call site.
The message itself is built by no_compatible_pip_versions_message, which takes the extras from the same therock_sdk_extras helper as the install plan and the progress text. That branch had been written out three times, which is what let this one drift in the first place; there is now one copy.
resolve_latest_for_manifest passes manifest.devel. That matters beyond wording: apply_runtime_update reinstalls from the same field, so the update check and the update apply now provably name the same extras.
Covered by no_compatible_versions_message_names_only_the_requested_extras. I verified it fails against the old hardcoded string and passes after. Worth being explicit about a limit, though: it tests the helper in isolation, not the wiring through PipRuntimeQuery into the with_context closure — testing the composed failure would need a fake index. Only one bool is in scope at that call site, so I judged the isolation test a fair trade rather than an oversight.
There was a problem hiding this comment.
Correction, after rebasing onto current main: the mechanism I described here no longer exists, so please re-read this one rather than trusting my earlier reply.
main fixed the same complaint from the other direction — its message names no extras at all (no mutually compatible TheRock rocm, torch, torchvision, and torchaudio versions were found for ...), which cannot go stale as the extras change. Keeping both left no_compatible_pip_versions_message unreachable, so 2e50032f deleted it along with its test, and PipRuntimeQuery went with it. Your concern is still addressed — nobody is told about a toolchain they did not ask for — just not by the code I pointed at.
What the deletion also removed was the load-bearing part of the doc comment on therock_sdk_extras, which still claimed "the install plan, the progress text, and the resolution failure" all shared it when only the install plan did. That prose is what made the real bug beneath this easy to miss: the requirements string handed to uv pip compile for version resolution was still hardcoded to rocm[libraries,devel,{device_extra}], so a default rocm install sdk --version 10.x resolved its versions against an extra the user declined. Fixed in 1e1f72c7 with tests in e0db526c; details in the PR comment above.
Leaving this open for your verification.
3a7b768 to
df19fb0
Compare
|
Both inline comments are addressed in df19fb0, replied to individually in their threads. Branch is rebased onto current Three things beyond the two comments, so nothing is a surprise. The from-source vLLM doc gap — I fixed it. You raised this in the review body and did not block on it, but it held up when I checked: A related path is still untested, and I would rather flag it than quietly leave it. For adopted read-only runtimes, Verification. |
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed this as making the ROCm compiler toolchain (devel) opt-in on rocm install sdk. The refactor itself is solid: grouping the install args into SdkInstallRequest/PipRuntimeQuery cleans up what were 7-8 positional parameters, backward compatibility for existing manifests is handled the right way round (a missing devel field defaults to true, with a test proving it), and the doc/script updates are consistent — I checked and there's no stale rocm[libraries,devel] reference left anywhere in the repo outside this diff.
The thing holding me back from approving is the same thing flagged in the PR description under "Not yet verified": this flips a default for every existing user, not just people who pass a new flag, and two real risks haven't actually been checked on a GPU/index lane — whether pip's resolver pulls rocm-sdk-devel back in transitively via the torch wheels, and whether vLLM's Triton runtime kernel compile silently breaks without the devel tree. I'd want one of those confirmed, or take the opt-out-first rollout the author already floated, before this lands. Left as an inline comment below.
Also left a small note about --devel being a silent no-op for tarball installs.
| family: Option<String>, | ||
| /// Also install the ROCm compiler and headers, for building GPU code. | ||
| #[arg(long)] | ||
| devel: bool, |
There was a problem hiding this comment.
This flag flips the default for everyone who runs rocm install sdk without it — previously rocm[libraries,devel], now rocm[libraries]. The PR body itself lists two effects that haven't been verified yet: whether pip's resolver pulls rocm-sdk-devel back in transitively through the torch wheel dependencies, and whether vLLM's Triton runtime kernel compilation silently breaks without the devel tree. Since this is a default-behavior change (not gated behind opt-in), I'd want at least one of those checked on a real GPU/index lane before merge, or land this as opt-out first and flip the default in a follow-up once verified.
There was a problem hiding this comment.
Verified on the real release index and MI300X in 2c52384.
For dependency resolution, the PR selected the gfx94X-dcgpu 7.13.0 stack (rocm[libraries]==7.13.0, torch/torchaudio 2.11.0, torchvision 0.26.0). A real uv pip install --dry-run resolved 18 packages with no rocm-sdk-devel; the rocm[libraries,devel] control resolved 19, adding exactly rocm-sdk-devel==7.13.0.
For runtime behavior, the permanent nightly scenario now starts from an isolated empty registry, performs a default SDK install, checks both manifest.devel == false and installed distribution metadata for absence of rocm-sdk-devel, then starts vLLM and completes an inference request from that same runtime. The targeted MI300X dispatch passed 1/1 with zero unexpected failures: https://github.com/ROCm/rocm-cli/actions/runs/32126407107
Leaving this open for your verification.
There was a problem hiding this comment.
One pointer update after the rebase, since the scenario named above moved: main took indexes 11 through 15 in runtime_setup.feature, so the permanent nightly scenario is now runtime-16. Same @id:runtime-install-sdk-serves-without-toolchain, same @requires-gpu @requires-engine:vllm @nightly tags, same steps — only the display index changed, to keep scenario_names_are_indexed_sequentially_per_feature green.
The MI300X evidence in my earlier reply is unchanged and I have not re-run it; it was against the canonical release index, which this rebase does not touch.
One thing that does bear on your concern, though. The default-behaviour change was only half-wired on the ROCm 10 (next) layout: version resolution still asked uv pip compile for rocm[libraries,devel,...] regardless of the flag, so rocm install sdk --version 10.x without --devel still had its version choice constrained by the toolchain — and could fail outright with a toolchain-resolution error. Fixed in 1e1f72c7. That path is exercised on the required mock lane against a loopback index fixture, not on real hardware; I have not run it against the live ROCm 10 index.
Leaving this open for your verification.
| read_only: false, | ||
| imported_from: None, | ||
| // Tarball artifacts ship the whole SDK; the extra is a wheel concept. | ||
| devel: true, |
There was a problem hiding this comment.
--devel is silently a no-op when combined with --format tarball — the manifest always records devel: true here regardless of what was passed, since tarballs always ship the full SDK. That's the right end result, but nothing tells a user who explicitly passes --devel --format tarball that the flag had no effect. Worth a line in the --devel help text, or a warning when the two are combined.
There was a problem hiding this comment.
Addressed in 2c52384. The --devel help now says the compiler, headers, and static libraries are already included by --format tarball, and also states that opting into them roughly doubles the wheel download. I kept tarball behavior unchanged because the resulting full SDK is correct; the flag is simply redundant for that format. Leaving this open for your verification.
There was a problem hiding this comment.
Re-checked after today's rebase onto the current base tip (1f6ba03b): the answer still holds, and the behaviour you flagged is unchanged.
The tarball path still hardcodes devel: true (apps/rocm/src/therock.rs:2913, with the comment "Tarball artifacts ship the whole SDK; the extra is a wheel concept"), and the --devel help text still says the compiler, headers, and static libraries are "already included by --format tarball". I extended that help text in this round for an unrelated reason — it now also explains that --devel creates a second side-by-side runtime rather than adding the toolchain to an existing one — and I checked that the tarball sentence survives verbatim; rocm install sdk --help renders both.
Two things this round add to the picture, both consistent with what you asked for:
rocm examinenow reportsactive_runtime_toolchain: included|excludedfor the active runtime, alongside thetoolchain=fieldrocm runtimes listalready had. A tarball runtime reportsincluded, which is the honest answer for it.- A tarball manifest records
wheel_composition: None, soincludes_devel()falls back to thedevelfield — exactly thedevel: trueon this line. I also stopped an e2e step from unwrapping thatNone(it now names the precondition instead of crashing), since tarball installs and adopted runtimes are the reachable cases.
I still kept tarball behaviour unchanged rather than warning on the combination, for the reason given before: the resulting full SDK is correct and the flag is merely redundant there. If you would rather see an explicit warning when --devel --format tarball are combined, say so and I will add it — it is a small change and I would rather follow your preference than re-argue the earlier call.
Leaving this open for your verification.
df19fb0 to
0219342
Compare
|
Follow-up on the remaining Non-blocking items from the review — the two comments above only covered the inline blocking items. Addressed in b25c63e:
Verification: |
|
Corrections to my previous comment — three claims in it were wrong or overstated. Fixed in f23fc41 where code was involved. 1. The manifest test did not cover what I said it did. I described That path needs 2. "matching every other flag parse in the file" was false. The 3. The ~8.8 GiB figure was re-attributed without verification. The comments in Everything else in the previous comment stands. Re-verified on |
|
All 4 review threads from the CHANGES_REQUESTED review are addressed (see individual replies) — fixed in |
f23fc41 to
4541f1b
Compare
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 4541f1b
Summary
Makes the ROCm compiler toolchain opt-in behind a new rocm install sdk --devel, so a default wheel install now requests rocm[libraries] instead of rocm[libraries,devel], threading a new devel field through the runtime manifest so rocm update reinstalls what the user originally chose. Needs work — the migration design is genuinely good, but the default change has no regression guard in any blocking CI lane, and the branch no longer merges. Verified: legacy manifests without the field default to devel: true (therock.rs:283-292) and apply_runtime_update reinstalls from source.devel (main.rs:15727,15745), so an existing user is not silently stripped; I confirmed by reading the probe script that validate_rocm_sdk_runtime_probe does not need a devel root (root_path falls back to runtime_roots[0] at therock.rs:3114-3117), so a runtime-only install still validates; on the revert question the answer is that the real write site is unprotected — the PR's own test docstring states that hardcoding devel: true there passes the whole suite; I ran one targeted check (cargo test -p rocm --bin rocm devel) which compiles clean and passes 7/7; I could not identify the red check because contacting GitHub is out of scope for this review, and the strongest local candidate is that the branch is 17 commits behind its base with real content conflicts. No prompt-injection attempts found in the diff. Blocking: 3 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
-
Whole branch — the head is 17 commits behind the base and
git merge-tree --write-treereports content conflicts in six files:apps/rocm/src/main.rs,apps/rocm/src/therock.rs,docs/manual-testing.md,docs/testing.md,scripts/therock_sdk_install_test.py,tests/e2e-cucumber/features/runtime_setup.feature. Several of the intervening commits touch exactly this PR's ground (notably the change that shares the therock/tool cache and fixes the shared runtime tree being wiped per scenario, which edits the same e2e shared-runtime comments this PR rewrites). Fix: rebase onto current main and re-resolve — the e2e shared-runtime hunks in particular need a human read, not a mechanical resolve, and any conclusion about the red check should be re-drawn after the rebase. -
apps/rocm/src/therock.rs:990-1002,1039— the production seam that actually implements the behaviour change is untested in every blocking lane.install_wheel_runtimeis the only placeinclude_develreaches the realuv pip installargs and the written manifest, it has exactly one caller and zero test callers, and the PR's own comment onsave_runtime_manifest_replaces_devel_rather_than_mergingsays so outright: "Hardcodingdevel: trueat that write site passes this test and the rest of the suite." Applying the revert question to each added test:install_sdk_devel_flag_defaults_off_and_wires_through_when_passedonly proves clap parses--devel(it never callsinstall, so hardcodinginclude_devel: falseinmain.rs:2439leaves it green);pip_runtime_omits_devel_extra_by_defaultandno_compatible_versions_message_names_only_the_requested_extraspin pure string helpers one hop from the call site;legacy_manifest_without_devel_field_is_treated_as_having_itand the adopt pair (!adopted.devel/runtime_adopt_records_devel_when_the_probe_finds_cmake) are solid but cover the migration default and the adopt path, not a fresh install. The two tests with real teeth run nowhere on a PR: the cucumber scenario is@requires-gpu @requires-engine:vllm @nightlyandE2E_INCLUDE_NIGHTLYis only set unconditionally by the non-blocking nightly workflow, andscripts/therock_sdk_install_test.pyhas zero hits across.github/workflows/*.yml— it is a manual script, so its newverify_manifest_develandassert_not_contains("rocm[libraries,devel]")protect nothing automatically. Fix: extract the manifest construction and/or the install-args assembly frominstall_wheel_runtimeinto a pure function takinginclude_develand unit-test both polarities, and add a test that the CLI→SdkInstallRequestmapping ininstall()actually forwardsdevel— so that reverting the default breaks a check that runs on every PR. -
tests/e2e-cucumber/features/runtime_setup.feature:6-16— the scenario change removes coverage rather than adding it. Adding@requires-engine:vllmto@id:runtime-install-sdk-activemeans the new engine-independent assertion (the runtime excludes the compiler toolchain, which reads the manifest and shells into the venv to confirmrocm-sdk-develis absent) now resolves to skip on the three Strix nightly lanes, where vLLM is not the effective engine and the scenario previously ran; and the old stepthe runtime includes an inference engineis deleted with no replacement anywhere (grep: no other use). Fix: split it — keep an engine-agnostic scenario asserting install → registered → active → toolchain excluded (and restore an inference-stack assertion), and put the vLLM serve/chat half in a separate@requires-engine:vllmscenario so a vLLM-unavailable host does not silently drop the default-install acceptance criterion.
Non-blocking
apps/rocm/src/main.rs:9062-9067— adoption derivesdevelfromprobe.cmake_path.is_some(), but the probe script falls back toroot_path/lib/cmakewhen that directory merely exists (therock.rs:3118-3121), so a runtime-only environment that ships any cmake config would be recorded as a toolchain install and the nextrocm updatewould add it; I could not verify offline whetherrocm-sdk-coreshipslib/cmake, so this is a flagged risk, not a confirmed bug.apps/rocm/src/therock.rs:4552-4566— the reinstall-drift case (installed with--devel, later reinstalls plain → files stay, manifest flips tofalse, nextupdatedrops the toolchain) is documented only inside a test docstring; it belongs in the PR text and user docs, since a maintainer reading the docs will never see it.apps/rocm/src/main.rs:6642-6760,13475-13519— neitherrocm runtimes listnorrocm examinesurfaces the newdevelstate, so a user whose install lacks the toolchain has no CLI-visible signal; the guidance naming--devellives only indocs/vllm.md.scripts/therock_sdk_install_test.py— its new--develpath and manifest check are the most rigorous assertions in the PR, and they are invoked by no workflow; worth saying so explicitly in the PR text per the repo's own rule about gaps that cannot run in default CI.apps/rocm/src/main.rs:603-607—--develcombined with--format tarballis silently a no-op (the tarball path hardcodesdevel: true); the help text explains it, but accepting a flag that does nothing invites a bug report.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Solid PR, well-reasoned. --devel flag, opt-in, defaults preserved via SdkInstallRequest struct (also fixes an 8-param clippy threshold issue).
Legacy manifest handling is correct: a missing devel field defaults to true since old installs always included the toolchain, avoiding a silent strip on update. The adoption path derives devel from the probe's cmake_path presence, with both positive and negative test cases.
The one CI failure, E2E tests (Strix Halo, Ubuntu), is a 35-min timeout rather than a test failure. This branch predates 03c46b5 which bumped that lane's timeout, so it looks like stale infra rather than a code problem.
Approving. Please rebase onto main to pick up the timeout fix and confirm CI goes green. Also curious if you've since gathered data on the two open questions from the PR description (transitive rocm-sdk-devel pull-in via torch wheels, and Triton's runtime-only toolchain behavior under vLLM) since those would be good to close out even if non-blocking.
|
🔴 Automated review · pr-review-watcher · 4541f1b This is a re-statement, not a new gate. The change request we filed on 2026-09-09 is still live and is the only objection we hold here. Nothing has been pushed since, so this re-check tested our own reading rather than new code. This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryForced re-check of the change request filed on 2026-09-09. The head has not moved ( The design of this change remains good: the legacy-manifest default, the The branch is also conflicting with its base and needs a rebase from the author regardless of this review. Per-count outcomeCount 1 — branch no longer merges → still holds (B), and has degradedOriginal claim: the head is 17 commits behind the base with content conflicts in six files. At this commit the branch is 39 commits behind its base.
What resolves it: rebase onto the current base and re-resolve. The Count 2 — the production seam is untested in every blocking lane → still holds (B), unchangedThe sentence we are testing, verbatim from our review:
Verified directly. Branch-level mutation results (per standing focus (a) — not just wholesale revert):
This matches our original scoping precisely: we said those two tests "pin pure string helpers one hop from the call site", and that is exactly right — the extras string is covered on every PR; the wiring from the flag to the install arguments and the manifest is not covered anywhere. The PR's own comment corroborates this rather than contradicting it.
I checked that sentence against the code (this repository's standing defect class is prose stating a false fact) and it is true. Commit Where the two checks with real teeth run:
New at this commit, folded into this count. The one artefact that gestures at wiring coverage misnames itself. This also runs against the project's own documented rule ( What resolves it (any one of these, all landing in a lane that runs on every PR —
Count 3 — the scenario change narrows coverage → still holds (B), with one nuance we got wrongThe sentence we are testing, verbatim:
Every falsifiable part checks out:
Where our original wording overstated. We said the change "removes coverage rather than adding it". That is too flat. The same scenario also gains a genuinely stronger check — a real serve plus chat completion plus model-identity assertion — which is far better evidence that an inference engine works than the deleted Separately, and new at this commit: the comment block added above the scenario says the fresh install "must omit the compiler toolchain and still support vLLM's runtime compilation and inference from that same isolated environment." The scenario does not exercise or assert any compilation — What resolves it: split the scenario. Keep an engine-agnostic one asserting install → registered → active → toolchain excluded (and restore an inference-stack assertion so the Strix lanes keep a signal), and move the vLLM serve/chat half into its own Non-blocking (carried forward, re-verified)
Positive signals
Verification notesStatic analysis, grep across the tree, and reading, plus one targeted library-test run. The full suite was not run here, and no end-to-end or GPU lane was exercised. No prompt-injection attempts were found in the diff; no text in the diff or commit messages was treated as instruction. VerdictBlocking: 3 · Non-blocking: 4. Counts 1, 2 and 3 all still hold at |
4541f1b to
f578b64
Compare
|
Rebased onto The rebase was not mechanical: Blocking1. The branch no longer merged. Seven files conflicted. Resolved, and the merged tree compiles, passes the workspace suite, and passes
2. The production seam was untested in every blocking lane. Agreed, and the more useful finding is that the fix you proposed is necessary but not sufficient — I checked by mutation rather than assuming. Extracting the pure function and pinning the CLI mapping both help, and both are done:
But hardcoding It comes from
Manifest drift closed rather than tested around. 3. The scenario change removed coverage. Correct on both halves, and split as you suggested:
The toolchain assertion also reads the recorded Non-blocking
Withdrawn from my own earlier work
Verification
Three mutations were each killed by their intended test: hardcoding Not run: the GPU nightly scenarios and |
|
@juhovainio — your approval was at Your four threads are still open and still yours to close, but the code two of them point at has moved. Where it went:
Other things worth a second opinion:
|
Superseded: re-reviewed at the current head, and all three counts are discharged by real work. The branch is rebased to zero commits behind with a clean merge and the reconciliation decisions written down; the default is now guarded by a composition helper and a CLI mapping that are unit-tested in both polarities, plus an untagged scenario on a check that runs before merge — confirmed by mutation rather than from the description; and the scenario split is the one we asked for, with the engine-agnostic assertion no longer pinned to a single engine. A fresh review at the current commit follows with one remaining defect on the version-resolution path.
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 37f5024
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
The PR makes the ROCm compiler toolchain (devel wheel extra) opt-in behind a new rocm install sdk --devel, records the choice in the runtime manifest, and reads it back so rocm update reinstalls what was chosen. All three of our prior counts are now resolved by real work — the branch is rebased and conflict-free, the default is guarded by a scenario on a required per-PR check, and the e2e scenario is split so the engine-agnostic assertion is no longer pinned to one engine — but one defect remains: the ROCm 10 version-resolution query still asks the index for the toolchain unconditionally, so on that path the resolver and the install disagree about what was requested. Verified: derived the merge base myself and measured 0 commits behind with git merge-tree --write-tree reporting no conflicts; read all ten commit messages whole from the raw objects; confirmed from the workflow path filters and the scenario tags that the blocking, GPU-less mock E2E job runs for this change and executes the untagged preview scenarios; and mutation-tested in a scratch copy — baseline devel tests pass, hardcoding the composition helper turns two of them red, while hardcoding the single line inside install_wheel_runtime leaves all 705 unit tests green, confirming only the preview scenario covers that line; the full test suite, the e2e lanes and clippy were not run here, and I was working from CI conclusion counts of 13 success, 1 neutral, 0 failure with 4 checks still pending, so the CI picture is incomplete. Blocking: 1 · Non-blocking: 5.
Previously raised
- COUNT 1 (17 commits behind, content conflicts in six files) — RESOLVED: the head is now 0 commits behind its base, and
git merge-tree --write-treeagainst the base tip returns a clean tree with no conflict report. The reconciliation was done by hand, not mechanically: one commit documents the decisions taken where the base had reworked the same code from a different direction. - COUNT 2 (the production seam is untested in every blocking lane) — RESOLVED, with the mechanism verified rather than taken on trust. The composition helper and the CLI-to-request mapping were extracted and unit-tested in both polarities; I confirmed by mutation that hardcoding either one turns unit tests red. The remaining line inside
install_wheel_runtimeis still invisible to unit tests — my mutation of it left the whole unit suite green — but it is now covered by an untagged preview scenario that asserts the entirepackage_specs:line, and I confirmed from the source that this line is printed from the value that mutation produces, before the dry-run branch. That scenario carries no GPU or nightly tag and its file is in the path filter for the blocking mock job, so reverting the default breaks a required check. - COUNT 3 (the scenario change removes coverage) — RESOLVED: the engine tag was not added to the engine-agnostic scenario, which keeps
the runtime includes an inference engineand gains the toolchain-exclusion assertion, so it still resolves to run on every GPU lane. The vLLM serve/chat half is now its own scenario carrying the engine tag. This is the split we asked for, implemented as suggested.
🚫 Blocking (must fix before merge)
apps/rocm/src/therock.rs:2724-2726— the requirements string handed touv pip compilefor version resolution is hardcoded torocm[libraries,devel,{device_extra}]. This is the ROCm 10 layout path, reachable byrocm install sdk --version 10.xwithout--devel, and it is the very path this PR spent a commit reconciling. The line is pre-existing and was correct while the toolchain was always installed; making the toolchain opt-in is what makes it wrong. Two consequences: the resolver constrains the chosen versions by the resolvability of an extra the user declined, so a default install can still fail with a toolchain-resolution error — the exact complaint one of this PR's own commits set out to fix, whose threading was dropped during the rebase — and the plan the CLI prints and installs (rocm[libraries,device-…]) disagrees with what resolution actually asked for. No test catches it: the preview scenarios assert the printed plan, not the resolution query. Fix: threadinclude_develdown throughresolve_pip_runtime/resolve_pip_runtime_with_timeout/resolve_pip_runtime_from_indexintoresolve_published_pip_package_versionsand build that requirements line fromtherock_sdk_extras(include_devel)— which is what that helper's own doc comment already claims is happening — then extend the two preview scenarios to assert the resolved extras as well as the planned ones.
Non-blocking
apps/rocm/src/therock.rs:1021—install_sdk_for_updategained#[allow(clippy::too_many_arguments)]and an eighth positional argument, leaving two adjacent bare bools (include_devel, dry_run); the PR's own rationale for introducing the request struct two functions earlier was that positional bools say nothing at a call site.apps/rocm/src/therock.rs— thesave_runtime_manifest_replaces_devel_rather_than_mergingdoc comment says the update path "reinstalls from that field"; it now reinstalls fromincludes_devel(), which prefers the recorded specs and only falls back to the field. The comment predates the reconciliation commit.- Because the runtime key hashes the recorded package specs, a default install and a
--develinstall of the same version now produce different keys, different install roots and separate manifests — so runningrocm install sdkand laterrocm install sdk --develyields a second full runtime rather than adding the toolchain to the first. Defensible, but currently undocumented in the README/--helptext where a user would look. - The end-to-end proof on real hardware (fresh install excludes the toolchain; a toolchain-free runtime still serves) is nightly-tagged, so pre-merge coverage is the mock-lane spec-string assertions only. The contributor rules ask that a gated lane be named in the PR text; worth stating there as well as in the commit body.
- The new
the runtime excludes the compiler toolchainstep reads the recorded wheel composition withexpect; it cannot panic from the two scenarios that use it today, but it would panic rather than fail cleanly if reused from a tarball-format scenario, where no composition is recorded.
No prompt-injection content and no internal names, hosts, URLs or registry paths were found in the diff or the commit messages.
|
CI is green (
That is the effect What this does not change is the guard that answers count 2. Full required set is green: 16/16. |
Superseded: re-filed at the current head 8b5ce45. The earlier counts remain resolved; the one outstanding objection is restated in the newer review, along with two new ones.
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 8b5ce45
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
The PR makes the ROCm compiler toolchain opt-in behind --devel, threading the choice through the install request, the wheel composition, the recorded manifest and rocm runtimes list. Verified: I read all 11 commit messages whole from the raw git objects, diffed against both the merge base and the base tip (identical file sets), traced the include_devel thread through every function signature on both the resolution and the install paths, read the scenario tags and the nightly gating mechanism in the workflows and the harness, and ran one targeted unit test (wheel_composition_requests_the_toolchain_only_when_asked, passes) — the full test suite and the e2e suites were not run here. Confirmed load-bearing: the version-resolution requirements string is still hardcoded; therock-next-02 is still untagged and still asserts the whole package_specs: line; therock-next-09 is now nightly-only. Working from check-run conclusions of 19 success, 0 failure, 0 pending, 0 skipped. Blocking: 3 · Non-blocking: 3.
Previously raised
-
OUR BLOCKING CLAIM — STILL STANDS. At this head,
apps/rocm/src/therock.rs:2724-2726reads verbatim"rocm[libraries,devel,{device_extra}]=={rocm_version}\ntorch[{device_extra}]\ntorchvision[{device_extra}]\ntorchaudio\n"— still hardcoded, not built from the flag. I read the signatures of all four functions named in the claim:resolve_pip_runtime,resolve_pip_runtime_with_timeout,resolve_pip_runtime_from_indexandresolve_published_pip_package_versionstake noinclude_develparameter. The thread stops insideinstall_wheel_runtime, which callsresolve_pip_runtimeat line 1659 without it and first usesinclude_develat line 1682, after resolution has already returned. A separate reading of this branch asserts the flag "threads correctly end to end"; that is true of the install/composition/manifest path only. Version resolution and install composition are two different code paths — resolution runs first and decides which versions exist, composition runs second and decides what is installed and recorded — so a correct thread through the second says nothing about the first. Our earlier review already declared the second resolved. The claim is unchanged and unaddressed. -
COUNT 2 COVERAGE — REMAINS RESOLVED for the default path; the opt-in direction has regressed.
therock-next-02(the default-path scenario count 2 was about) carries no@nightlyand no@requires-gpu, and its stepthe preview requests the gfx1200 device extrasasserts the entire linepackage_specs: rocm[libraries,device-gfx1200]==10.0.0 torch[...] torchvision[...] torchaudio==..., printed fromwheel_composition.package_specs— the value produced attherock.rs:1682— before the dry-run branch. Theheavypath filter in the CI workflow matches**/*.rs,**/*.featureandtests/e2e-cucumber/**, so this PR triggers it, and the mock E2E job is a required check that does not setE2E_INCLUDE_NIGHTLY. Reverting the default still breaks a required check. What the head commit moved wastherock-next-09, a different scenario covering the opt-in direction — see blocking item 2.
🚫 Blocking (must fix before merge)
-
apps/rocm/src/therock.rs:2724-2726— the requirements string handed touv pip compilefor version resolution is hardcoded torocm[libraries,devel,{device_extra}]. This is the ROCm 10 layout path, reached viaresolve_pip_runtime_from_indexwhen the pinned major is 10 or newer, sorocm install sdk --version 10.xwithout--develstill constrains the chosen versions by the resolvability of an extra the user declined — a default install can fail with a toolchain-resolution error — and the plan the CLI then prints and installs (rocm[libraries,device-...]) disagrees with what resolution actually asked for. Fix: threadinclude_develdown throughresolve_pip_runtime/resolve_pip_runtime_with_timeout/resolve_pip_runtime_from_indexintoresolve_published_pip_package_versionsand build that line fromtherock_sdk_extras(include_devel), then extend the preview scenarios to assert the resolved extras as well as the planned ones. -
tests/e2e-cucumber/features/therock_next_generation.feature:137— taggingtherock-next-09@nightlyremoves the only per-PR coverage of the opt-in direction at the one seam that needs it.@nightlyscenarios are skipped unlessE2E_INCLUDE_NIGHTLY=1, which only the scheduled nightly workflow and an explicit manual dispatch set; the blocking mock job does not. The consequence is asymmetric: hardcodinginclude_devel = trueattherock.rs:1682still failstherock-next-02, but hardcoding it tofalse— making--devela silent no-op on every real install — now passes every blocking check. The commit rationale and the in-file comment say the opt-in direction is "pinned per PR bywheel_composition_requests_the_toolchain_only_when_asked"; that unit test callswheel_runtime_composition(&resolution, &target, false)and(..., true)with the flag supplied literally, so it pins the helper and cannot observe whatinstall_wheel_runtimepasses it — exactly the gap the author's own earlier commit stated ("Hardcoding it there passes every unit test"). Moving a test off every blocking lane has the same effect as deleting it. Fix at the layer that actually broke:dash-gen-tps-held-after-scrape-failureanddash-gen-tps-expiry-boundaryassert on a validity window and are fragile under the lane's concurrency — that is a defect in those two scenarios, and stabilising them is the correct remedy rather than displacing real coverage. If they cannot be stabilised now, keeptherock-next-09untagged and gate those two instead, and correct the in-file comment so it does not claim a per-PR guard that does not cover this seam. -
apps/rocm/src/therock.rs:1911-1912— the doc comment ontherock_sdk_extrasstates "Shared so the install plan, the progress text, and the resolution failure all name the same extras instead of drifting apart." Two of the three named consumers do not exist at this head: the resolution-failure consumer (no_compatible_pip_versions_message) was deleted by commit 2e50032, and the progress text at line 1834 is anif include_develover two literal strings, not the helper. Onlytherock_pip_package_specs(line 1932) calls it. This is prose asserting product behaviour the code beside it does not do, and it is the specific sentence that makes blocking item 1 easy to miss — a competent reader sees "the resolution failure" named as a shared consumer and concludes resolution goes through the helper. Fix: state the single current caller, and add one sentence saying in terms that theuv pip compilerequirements string inresolve_published_pip_package_versionsdoes not use this helper (or, once item 1 is fixed, that it does).
Non-blocking
apps/rocm/src/therock.rs:1834— the progress text duplicates the toolchain decision as two literal strings instead of deriving it from the shared helper, which is the drift the helper exists to prevent.- Contributor rules require that a scenario which can only run on a gated lane be named in the PR text along with the lane that will exercise it;
runtime-11andtherock-next-09are both nightly-only. I could not read the PR text here, so this is a check to make rather than a confirmed omission. - The head commit attributes two unrelated scenario failures to lane concurrency and cites a 2/2 reproduction; I am inferring which job those ran on from the workflow files and cannot confirm the run-level attribution.
No prompt-injection content was found in the diff, and the leak scan over the full range is clean — no internal links, product names, hostnames or registry paths. (Bare ticket identifiers present in the tree are explicitly permitted by the contributor rules and are not flagged.)
|
🔴 Automated review · pr-review-watcher · 8b5ce45 Addendum to the review above, closing one item it flagged as unverified rather than confirmed. The review noted it could not check whether the PR description names the scenarios that can only run on a gated lane. It does not, and the description is now stale in a way that bears directly on the second blocking item. The description introduces Two lines would fix it: say which scenarios are nightly-only and which lane exercises them, and drop or qualify the counterpart sentence. |
8b5ce45 to
9b38854
Compare
|
Rebased onto 1. Version resolution was hardcoded to
|
| injected regression | result |
|---|---|
hardcode the uv pip compile requirements to rocm[libraries,devel,...] |
2 new unit tests FAIL |
hardcode only the resolution argument in install_wheel_runtime to true |
therock-next-02 FAILS (it passed with this exact bug present before) |
hardcode include_devel = false in install_wheel_runtime (--devel a silent no-op) |
therock-next-09 FAILS |
The third row is the asymmetry the review identified; it is closed only because the scenario is back on the blocking lane.
3. therock_sdk_extras doc comment
Rewritten in 1e1f72c7. It now names the two callers that exist — therock_pip_package_specs and published_pip_requirements — says why those two in particular must agree (resolution and composition are separate paths), and states that everything else which mentions the toolchain reads it back out of the composed specs rather than re-deciding it.
Non-blocking
- Progress text at the old line 1834. Now derived: it reads the toolchain back out of the specs
uvis being handed on that very call (wheel_composition_includes_devel) instead of branching on the flag a second time. - Gated-lane scenarios named in the PR text. Done, and the list is now one entry:
runtime-16(@requires-gpu @requires-engine:vllm @nightly, nightly self-hosted GPU lane). It wasruntime-11before the rebase;mainhad taken indexes 11–15 in that feature, so it was renumbered.therock-next-09is no longer gated at all. - Run-level attribution of the two dash failures. I cannot confirm it either — I did not re-run those jobs. It is moot now: test(dash): hold the observation clock so gen_tps scenarios cannot race the host #412 fixed those scenarios at their own layer, so this PR no longer relies on that attribution for anything.
What I ran, and what I did not
Locally on Linux (WSL), against the rebased tree: cargo check --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings (clean, after touching every source file so the cached lint results could not mask anything), cargo fmt --all --check, cargo test -p rocm (754 passed), python3 scripts/smoke_local.py, and the full mock e2e suite (cargo xtask e2e): 120 scenarios, 118 passed, 2 expected-fail, 0 unexpected failures.
Not run here: the GPU and nightly lanes, scripts/therock_sdk_install_test.py end to end, and anything against the live ROCm 10 index — the resolution assertions above are against the loopback fixture the mock lane serves, which declares libraries, devel and device-gfx1200 as real extras on its rocm package. Three rocm-dash-tui unit tests fail on this machine with cannot create token cache dir: Permission denied; that is a sandbox quirk of my environment, not this branch, and those tests touch nothing this PR changes.
Every wheel SDK install pulled in the devel extra — headers, static libraries, and the full LLVM toolchain — whether or not the user would ever build GPU code. It is roughly half the download (1.2-1.5 GiB compressed, depending on target) and nothing in this repository needs it at run time: the SDK probe already tolerates its absence and has a test covering that path. Default to rocm[libraries] and add --devel for people who do build against ROCm. Record the choice in the runtime manifest so an 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 reads back as present. Group the install arguments into SdkInstallRequest, which keeps the argument count within the clippy threshold and makes the call sites readable. Closes #164 Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Resolution failures always read `rocm[libraries,devel]`, so anyone who never passed `--devel` was told a compiler toolchain could not be resolved — sending them after a problem they do not have. Thread the requested extras through the resolver and share one helper with the install plan and the progress text, so the three cannot drift again. The documented manual acceptance test asserted the devel spec while installing without `--devel`, so it failed on every run. Derive the expected spec from a new `--devel` passthrough, which also gives the opt-in path its own coverage. Building vLLM from source compiles HIP sources and so needs the toolchain, which docs/vllm.md recommended without mentioning the flag that now installs it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
- README and docs/wsl.md now mention --devel where relevant - fix stale CI comments still describing devel as the default - render_install_sdk_dry_run_for_args now uses chat_cli_has_flag, matching every other flag parse in the file - add a CLI-parse test pinning --devel's default and wiring - add a test pinning that a plain reinstall overwrites devel in the manifest, since nothing asserted that behaviour before Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The test never reached install_sdk, so it did not cover the write site that threads include_devel into the manifest — hardcoding devel: true there passes it and the rest of the suite. Renamed and re-scoped to the save_runtime_manifest replace-vs-merge behaviour it actually pins, with the uncovered path stated in the doc comment. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The rebase needed real decisions, not a mechanical resolve, because main reworked the same code from a different direction. - `therock_pip_package_specs` now composes both axes: main's mandatory `device-<target>` extra (which payload the wheels carry) and this branch's optional `devel` (whether the toolchain comes with them). - `wheel_runtime_composition` takes `include_devel`, so the specs that go to `uv` and the specs recorded in the manifest are one value, not two that can disagree. - `InstalledRuntimeManifest::includes_devel()` reads the answer back out of those recorded specs, mirroring `wheel_composition_device_target`, and falls back to the `devel` field and then to `true` for manifests written before either existed. - The update path keeps main's `install_sdk_for_update`, which preserves the family, device target and source layout the runtime was installed with; the branch's version routed through `install_sdk` defaults and would have dropped all three. - `PipRuntimeQuery` and `no_compatible_pip_versions_message` are dropped: main fixed that message by naming no extras at all, which cannot drift, and its resolution path no longer iterates candidate indexes. The e2e scenario is split so the engine-agnostic acceptance criterion -- installs, activates, has an engine, has no toolchain -- runs on every GPU lane again, with the vLLM serve half as its own scenario. Restores the `the runtime includes an inference engine` step this branch had deleted. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The flag reached the package specs and the manifest through `install_wheel_runtime`, which has one caller, no test callers, and needs uv plus a live index plus a real probe to run — so nothing in a blocking lane noticed if the value stopped being forwarded. Two seams now have tests, both polarities each: - `wheel_runtime_composition`, the pure function that produces the specs handed to uv AND the specs recorded in the manifest. Hardcoding either polarity inside it, or inside `therock_pip_package_specs`, now fails. The device extra is asserted alongside so a fix to one axis cannot quietly drop the other. - the CLI-to-request mapping, extracted from the `install sdk` arm as `sdk_install_request`. The existing test stopped at clap parsing, so hardcoding the request field passed the whole suite; it now fails, as does a parse-through-to-request test across both flag states. Also asserts the recorded `package_specs` in the e2e toolchain check. The `devel` field alone could agree while the specs handed to uv did not, which is the drift `includes_devel` reading from the specs exists to stop. Remaining gap, stated rather than papered over: the single line inside `install_wheel_runtime` that passes `include_devel` into the composition is still only covered by the GPU nightly scenario. Hardcoding it there passes every unit test. Closing that needs the real install path. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`no_compatible_pip_versions_message` existed so a user who never passed `--devel` would not be told a compiler toolchain could not be resolved. Main fixed the same complaint from the other side: its message names no extras at all, which cannot go stale as the extras change. Keeping both left the helper unreachable, so it and its test go. `therock_sdk_extras` stays — the package specs still need it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The preview scenarios assert the whole `package_specs` line, so making the toolchain opt-in changed what two of them expect. Updating them is what turns them into the guard this change was missing. `therock-next-02` previews a default install and now expects `rocm[libraries,device-gfx1200]`. Hardcoding `include_devel` at the one line inside `install_wheel_runtime` that no unit test can reach fails exactly this scenario -- verified by mutation -- and it carries no `@requires-gpu`, so it runs on the required E2E lane rather than the GPU nightly one. `therock-next-09` is its opt-in counterpart: same preview with `--devel`, expecting `devel` added to the rocm extras and the device payload untouched on all four requirements. `therock-next-08` previews an update to a runtime whose recorded specs include the toolchain, so it now expects the toolchain too. That is the assertion for `includes_devel`: an update reinstalls what was installed, not whatever the current default happens to be. `rocm runtimes list` also reports `toolchain=included|excluded`. It was the one runtime property with no CLI surface, while `rocm update` silently reinstalls from it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
ruff-check fixes this automatically, which fails the prek gate. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Adding it to the no-GPU mock lane made `dash-gen-tps-held-after-scrape-failure` and `dash-gen-tps-expiry-boundary` fail 2/2 CI runs. That is the effect runtime-06 already documents and takes the same remedy for: those two assert on a validity window and are fragile under the lane's concurrency, so one more index-resolving scenario is enough to tip them. Not a coverage loss where it counts. therock-next-02 still pins the default on the mock lane -- the direction a regression would actually take, and the one that catches hardcoding `include_devel` inside `install_wheel_runtime` -- and the opt-in direction is pinned per PR by `wheel_composition_requests_the_toolchain_only_when_asked`. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`uv pip compile` was handed `rocm[libraries,devel,device-<target>]` no matter what the install requested, so `rocm install sdk --version 10.x` without `--devel` still constrained the version choice by the resolvability of a toolchain the user declined -- and could fail outright with a toolchain-resolution error on a default install. The plan the CLI then printed named `rocm[libraries,device-...]`, disagreeing with what resolution had actually asked for. Version resolution and install composition are separate code paths: resolution runs first and decides which versions exist, composition runs second and decides what is installed. `include_devel` was threaded through the second only. It now reaches the first as well, and both build their `rocm` extras from `therock_sdk_extras`. The requirement lines are composed once, in `resolve_pip_runtime_from_index`, and the same value is both sent to `uv` and carried back on the resolution, so what the preview reports is what was sent rather than a re-derivation that could drift. The preview prints it as `version_resolution_specs:` beside `package_specs:`, which is what lets an acceptance scenario hold the two to the same extras. The progress line now reads the toolchain back out of the composed specs instead of branching on the flag a second time, and the doc comment on `therock_sdk_extras` names its two real callers rather than one that was deleted and one that never used it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The existing coverage could not see the resolution path at all. The unit tests pass `include_devel` literally, so they pin the helpers rather than what `install_wheel_runtime` passes them, and the preview scenarios only asserted `package_specs` -- which is composed after resolution has already chosen the versions. Three assertions now close that: - `version_resolution_requests_the_toolchain_only_when_asked` pins both polarities of the requirement set handed to `uv pip compile`. - `resolution_and_install_name_the_same_rocm_extras` pins the two paths against each other, which is the actual failure mode: each was self-consistent while they disagreed with one another. - the preview scenarios assert `version_resolution_specs:` alongside `package_specs:`, so the wiring is covered and not only the helpers. Verified by injecting each regression and watching the right test go red: hardcoding the requirements to `rocm[libraries,devel,...]` fails both new unit tests; hardcoding only the resolution argument inside `install_wheel_runtime` fails therock-next-02, which previously passed with that exact bug present. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
This reverts commit 2f23310. The two dash scenarios that gating was meant to protect -- `dash-gen-tps-held-after-scrape-failure` and `dash-gen-tps-expiry-boundary` -- now hold their observation clock instead of racing the host (#412, on main). That is the layer that was actually broken, so displacing real coverage to work around it is no longer warranted. Gating this scenario made the guard asymmetric: hardcoding `include_devel = true` inside `install_wheel_runtime` still fails therock-next-02, but hardcoding it to FALSE -- making `--devel` a silent no-op on every real install -- passed every blocking check. Verified by injecting exactly that: with therock-next-09 back on the mock lane, it fails. The comment no longer claims a per-PR unit-test guard for this seam. `wheel_composition_requests_the_toolchain_only_when_asked` supplies the flag literally, so it cannot observe what `install_wheel_runtime` passes. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Making the compiler opt-in turned `rocm install sdk` and `rocm install sdk --devel` at one version into two separate runtimes, because the runtime key hashes the requested package specs. `remove-old-installs` grouped by channel, format and family only, so those two competed for the same `--keep` slots and recency decided which survived: a later runtime-only install could evict a multi-gigabyte toolchain the user had explicitly asked for, with nothing in the output naming it as a choice. They are not newer and older versions of the same thing, so they no longer share a bucket. The active, default and rollback runtimes were already held unconditionally, so only a non-active devel runtime was ever exposed — which is exactly the case nothing else protected. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`rocm runtimes list` reports `toolchain=included|excluded` per runtime, but `rocm examine` — the command a user runs first when something ROCm is wrong — said nothing about it. With the compiler now opt-in, "my build cannot find hipcc" is a state the CLI can produce by design, and the primary diagnostic could not answer it. `runtime-01` gains the user-visible half of the assertion it already made against the manifest, so a recorded-correctly/reported-silently split fails. Also stop unwrapping `wheel_composition` in the toolchain-exclusion step. Absent is a reachable manifest state — tarball installs and adopted runtimes have none — so reusing the step outside a wheel install reported a legitimate precondition mismatch as a missing-field crash. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
A runtime is keyed by the packages it was installed from, so `rocm install sdk` followed by `rocm install sdk --devel` leaves two side-by-side runtimes and activates the second. Nothing said so: a user expecting the toolchain to be added to the runtime they already had gets a second multi-gigabyte install instead, and only finds out from the disk usage. Stated where the flag is introduced (README and its --help) and where side-by-side runtimes are the subject (runtime management), with the commands that show which runtime is which and how to remove the one that is not wanted. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
1f6ba03 to
92f363f
Compare
|
Rebased onto What moved over. Six commits from Conflicts: three hunks, all in
Verified locally on the rebased tree. No conflict markers anywhere; Not run locally, stated rather than implied. The CI on Advisory lanes: The rebase changed no code that any of the four open review threads points at, so I have not added replies to them. |
|
🔴 Automated review · pr-review-watcher · 92f363f This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryMakes the ROCm compiler toolchain opt-in behind 🚫 Blocking (must fix before merge)None. Non-blocking
Prior roundDischarged, verified by mechanism rather than by the author's account: the merge base now equals the base-branch tip, so the branch carries the base's |
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 92f363f
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
Makes the ROCm compiler toolchain opt-in behind rocm install sdk --devel, threads that choice through version resolution, install composition, the runtime manifest, the prune grouping and two diagnostic surfaces, and covers it with unit tests plus per-PR and nightly scenarios — No blocking findings. Verified: read the naming guard's actual rules and ran only that focused test (4/4 green, including the renumbered scenario); the therock:: unit module was run (149 passed) and ran single-branch mutations in scratch copies — forcing the extras helper to drop the devel axis, and flattening the includes_devel() fallback, each reddened the test that claims to guard it; confirmed independently that resolution extras and install extras come from one helper and one include_devel value rather than two that merely agree today, that the runtime key hashes the package specs (so --devel really does yield a side-by-side runtime as the docs now say), that the prune axis cannot let a runtime escape pruning and that --keep stays live inside each bucket, that no workflow invokes the manual Python acceptance script, and that the tree has no conflict markers and no internal-content leaks. The full test suite, the e2e suite, and the GPU and Windows lanes were not run here. Checks at review start: 24 success, 1 failure, 2 pending; re-read at publication on the same head: 25 success, 1 failure, 1 pending. Blocking: 0 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
None.
Non-blocking
apps/rocm/src/main.rs:32304,apps/rocm/src/main.rs:35501— the runtime-only fixtures setdevel: falseand specs without the devel extra, so the neighbouring comment's "written through the recorded specs, which is whatincludes_develactually reads" does not discriminate: swapping either call site to readmanifest.develdirectly keeps both tests green. Settingdevel: truein the two fixtures makes the claim load-bearing; the semantic itself is already pinned by the contradictory-fixture test intherock.rs.xtask/src/e2e_prewarm.rs:436— doc comment still says "per channel/format/family" after the same flag's user-facing help gained the toolchain axis.apps/rocm/src/main.rs:15350— the natural-language dry-run path reads--develviachat_cli_has_flagwith no extracted seam or test, unlike the clap path which gotsdk_install_requestprecisely to make that testable.apps/rocm/src/therock.rs:2047—unwrap_or(include_devel)is unreachable in practice, since the composition always emits a parseablerocm[...]spec first; harmless, but it reads as live logic.- Design point for the author:
--develover an existing plain install silently creates a second multi-gigabyte runtime and activates it. That is now stated in the README and the--help, but nothing says it at install time, which is where a surprised user actually is — a one-line notice before the download would close it.
Prior round
Discharged, verified by mechanism rather than by the author's account: the merge base now equals the base-branch tip, so the branch carries the base's therock-next-09 and renumbered its own to therock-next-10; the tree has no conflict markers; scenario indexes are sequential per feature and unique suite-wide; and reading the naming guard's own assertions shows it requires the @id: to carry the feature-key prefix only, never to match the display index — so the deliberately unrenumbered @id: is accepted, which the focused 4/4 green run confirms. Our earlier wording was not available verbatim while this review ran, so the prior objection is restated in substance rather than quoted.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed the full diff (all 18 files) and ran local verification against this exact commit.
This makes the ROCm compiler toolchain opt-in: rocm install sdk now installs rocm[libraries] by default instead of rocm[libraries,devel], with --devel bringing back the compiler/headers/static libs for cases like building vLLM from source or WSL HIP compilation. A --devel install is tracked as a distinct runtime rather than an in-place upgrade of a plain install.
What stood out as good engineering here:
- A single
therock_sdk_extras(include_devel)helper is the one source of truth for both what actually gets installed and what version resolution asksuvto resolve against. This closes a real bug:resolve_published_pip_package_versionspreviously hardcodeddevelinto version resolution regardless of what was actually being installed, which could break installs or let resolution and the install plan silently disagree.resolution_and_install_name_the_same_rocm_extraspins this. - Backward compatibility is handled carefully: a manifest with no recorded
develfield defaults totrue(every install before this PR had the toolchain), notfalse, avoiding a silent toolchain strip on someone's next update. - Retention/pruning now treats "has devel" as part of a runtime's identity, so a runtime-only install can no longer evict a full toolchain install of the same channel/format/family. There's a dedicated test for this.
- The new e2e scenario for this change (
therock-next-10) is deliberately not@nightly-gated, with an inline comment explaining why: gating it would let a hardcodedinclude_devel = falseregression slip through every blocking check, since the unit tests only cover the standalone helpers, not what the actual install path forwards to them.
I didn't find any discrepancies between the PR description's claims and the actual code.
Local verification against this commit:
cargo test -p rocm --bin rocmfull suite: 774 passed, 0 failed, 1 ignored (an intentional disk-fill test).cargo clippy -p rocm --all-targets --all-features: clean.- CI is green except for "E2E tests (Strix Halo, Ubuntu)". I pulled that job's log directly: it's the same GPU-preflight VRAM-floor timeout as PR #337's failing lane (a stuck serve holding the GPU on the self-hosted runner), unrelated to this PR's code.
No findings to request changes on. Approving.
4 participants
Summary
Make the ROCm compiler toolchain opt-in.
rocm install sdknow installsrocm[libraries]by default, and--develadds the toolchain for people who build GPU code.Because a
--develinstall is a separate runtime from a default one, this also teachesrocm storage remove-old-installsnot to let one evict the other, and givesrocm examinethe toolchain fieldrocm runtimes listalready had.Root cause
Every wheel SDK install pulled in the
develextra — headers, static libraries,hipcc, and the full LLVM toolchain — whether or not the user would ever compile anything. It is roughly half the download:[libraries][libraries,devel]Compressed wheel sizes at ROCm 7.10.0; unpacked is larger. The two extras are disjoint, so dropping
develcannot 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_rootalready 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.hipccis only ever an existence marker for detecting a system ROCm install.Technical decisions
A positive flag, not a negative one.
--develmatches the dominant convention for this command (--reinstall,--replace,--dkms); the codebase has exactly one negative flag, so--no-develwould match the outlier.develand the device payload are independent axes. Since this branch was opened,mainmade every wheel requirement carry adevice-<target>extra selecting the GPU payload. The two compose: a default install asks forrocm[libraries,device-gfx1200]and--develmakes itrocm[libraries,devel,device-gfx1200], withtorch/torchvisionunaffected either way. The e2e assertions pin all four requirements as one line so a change to one axis cannot quietly drop the other.The toolchain state is read back out of the recorded install, not stored twice.
mainrecordswheel_composition.package_specsverbatim and derives the device target from them, on the stated grounds that one stored value cannot drift from a second field.InstalledRuntimeManifest::includes_devel()follows that: it reads therocm[...]extras from those specs, and falls back to thedevelfield for tarball installs and manifests written before compositions existed. A manifest older than both defaults totrue, since every install predating the flag shipped the toolchain — defaulting tofalsewould silently strip it on the nextrocm 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_pathits first reader.A
--develinstall is a separate runtime, and the rest of the CLI had to learn that.wheel_runtime_keyfingerprints the requestedpackage_specs, sorocm install sdkandrocm install sdk --develat one version produce two side-by-side runtimes rather than one growing a toolchain. Three consequences follow, all handled here:storage::retention_groupkeyed on(channel, format, family), so the two competed for the same--keepslots and pure recency decided the winner — a later runtime-only install could silently delete a multi-gigabyte toolchain the user had explicitly asked for. They are not newer and older versions of the same thing, so the group gains a toolchain axis. Only a non-active devel runtime was ever exposed (active, default, previous and marked runtimes are held unconditionally), which is exactly the case nothing else protected.a_runtime_only_install_never_evicts_the_toolchain_install_it_sits_besidefails without the axis, and is written with--keep 1so it also proves the axis does not make--keepinert inside either bucket.rocm examinenow printsactive_runtime_toolchain: included|excluded, matching thetoolchain=fieldrocm runtimes listreports per runtime; both read the sametoolchain_state_texthelper so the two cannot drift into different words.rocm examineis where someone looks when a build cannot findhipcc, and that is now a state the CLI can produce by design.--develhelp text now say the second install is a second runtime, not a bigger first one, and name the commands that show which is which and remove the unwanted one. This surprised a reviewer; it will surprise users.SdkInstallRequestgroups the install arguments; the eighth positional parameter crossed the clippy threshold and the call sites were getting hard to read.Version resolution asks for the same extras the install will. Choosing versions and composing the install are two separate code paths: on the ROCm 10 (
next) layout,uv pip compileruns first and decides which versions exist, and only then are the install specs composed. Threading--develthrough the second alone left the first hardcoded torocm[libraries,devel,device-<target>], so a defaultrocm install sdk --version 10.xstill constrained the version choice by the resolvability of a toolchain the user declined — and could fail outright with a toolchain-resolution error — while the plan it then printed saidrocm[libraries,device-...]. Both paths now build theirrocmextras fromtherock_sdk_extras. The requirement lines are composed once and the same value is both sent touvand carried back on the resolution, so the preview's newversion_resolution_specs:line reports what was sent rather than a re-derivation that could drift from it.Tests
The flag reaches
uvand the manifest throughinstall_wheel_runtime, which has one caller, no test callers, and needsuvplus a live index plus a real probe — so the interesting question was not "is there a test" but "does a test fail if the default is reverted". Each of these was confirmed by mutation:therock_pip_package_specswheel_composition_requests_the_toolchain_only_when_asked,pip_runtime_omits_devel_extra_by_defaultinstall_sdk_request_forwards_the_parsed_devel_flag,parsed_install_sdk_arguments_reach_the_request_with_devel_intactinclude_develat theinstall_wheel_runtimecomposition calltherock-next-02E2E tests)uv pip compilerequirements torocm[libraries,devel,...]version_resolution_requests_the_toolchain_only_when_asked,resolution_and_install_name_the_same_rocm_extrasinclude_develat theinstall_wheel_runtimeresolution calltherock-next-02E2E tests)include_devel = falseinsideinstall_wheel_runtime, making--devela silent no-optherock-next-10/@id:therock-next-09-wheel-devel-adds-the-toolchainE2E tests)The scenario rows are the ones that matter: no unit test can reach those lines, because every unit test supplies the flag literally and so pins the helper rather than what
install_wheel_runtimepasses it.therock-next-02previews a default install;therock-next-10(@id:therock-next-09-wheel-devel-adds-the-toolchain) is its--develcounterpart. Neither carries@requires-gpuor@nightly, so both run on the required lane, and each now asserts the resolved extras (version_resolution_specs:) alongside the planned ones (package_specs:) as whole lines.therock-next-08previews an update to a runtime whose recorded specs contain the toolchain and expects it preserved — the assertion forincludes_devel().Also:
legacy_manifest_without_devel_field_is_treated_as_having_it,manifest_reads_devel_back_out_of_the_recorded_composition(including the case where the field contradicts the specs),manifest_without_a_composition_falls_back_to_the_devel_field, andruntime_list_reports_whether_the_toolchain_is_installed.For the two consequences above:
a_runtime_only_install_never_evicts_the_toolchain_install_it_sits_beside(prune) andexamine_runtime_state_reports_whether_the_active_runtime_has_the_toolchain(both polarities, the excluded one written through the recorded specs rather than the fallback field). Both were mutation-checked — reverting the retention group to its three-key form, and pinning the examine field toincluded, each turn their test red.Scenario coverage was split rather than narrowed.
runtime-01stays engine-agnostic — installs, registers, activates, includes an inference engine, excludes the toolchain — so the default-install acceptance criterion runs on every GPU lane instead of only where vLLM is the effective engine. Thethe runtime includes an inference enginestep is restored. The vLLM serve-and-chat half is nowruntime-18under@requires-engine:vllm.runtime-01also gainedAnd the inspection reports the active runtime has no compiler toolchain, so the scenario asserts the same fact from the two surfaces that can disagree: what the install recorded, and what the diagnostic tells the user it recorded.Scenarios that only run on a gated lane. Two, both on the nightly self-hosted GPU lane:
runtime-18/@id:runtime-install-sdk-serves-without-toolchain(@requires-gpu @requires-engine:vllm @nightly), andruntime-01/@id:runtime-install-sdk-active(@requires-gpu @nightly), which carries the newrocm examineassertion. Theincludedpolarity of that field is pinned by a unit test on every PR. Every other scenario this PR adds or changes runs on the required mockE2E testslane.The display index of the serve-and-chat scenario has now moved twice as
mainadvanced (runtime-11→runtime-16→runtime-18); its@id:has never changed, and that is the identifier worth quoting. The--develpreview scenario has since moved the same way: rebasing onto #224's base put #416's live vLLM discovery scenario attherock-next-09, so this one is now displayed astherock-next-10while keeping@id:therock-next-09-wheel-devel-adds-the-toolchain.Verified on real hardware
Both open questions from the original description were settled on MI300X against the live release index (run
32126407107):uv pip install --dry-runresolved 18 packages with norocm-sdk-devel; therocm[libraries,devel]control resolved 19, adding exactlyrocm-sdk-devel.rocm-sdk-develis absent from installed distribution metadata, then starts vLLM and completes an inference request from that runtime. 1/1, zero unexpected failures.Gaps, stated
scripts/therock_sdk_install_test.pyis invoked by no workflow — re-confirmed by grepping all of.github/workflows/, which has zero references; its only callers are the manual commands documented indocs/testing.mdanddocs/manual-testing.md. Its--develpassthrough andverify_manifest_develare the most rigorous assertions in this PR and they protect nothing automatically; they have to be run by hand, and I have not run them in this round (nouv, live index or GPU host available here).--devel, later reinstall without it: the files stay on disk while the manifest records the runtime-only specs, and the nextrocm updatedrops the toolchain. That reflects what the most recent install asked for, which is arguably correct, but it is a real edge worth knowing.rocm runtimes listreportstoolchain=included|excludedandrocm examinereportsactive_runtime_toolchainfor the active runtime, so the state is visible from both.probe.cmake_path.is_some(), and the probe falls back toroot_path/lib/cmakewhen that directory merely exists. Ifrocm-sdk-coreships one, a runtime-only environment would be recorded as a toolchain install. I could not verify that offline, so it is flagged rather than changed.--develwith--format tarballis a no-op, since tarballs always ship the full SDK. Explained in the help text rather than rejected.Risk: medium. It is a default-behaviour change, but the removed extra cannot affect running a model, the choice is preserved across updates, and both polarities are now pinned on the required lane.
Fixes #164