Repository navigation
Conversation
…s pinned An unpinned release-channel rocm install sdk stayed on the canonical index and installed ROCm 7.14. It now resolves the newest stable version on the next layout, with that layout's Python, and still leaves pinned 7.x and nightly auto-select on the canonical stream. Signed-off-by: Roman Inflianskas <roman.inflianskas@amd.com>
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · 71f1117
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.
Review — needs work
Full review of the whole change (19 files, +588/-551 per git diff cc56179...71f1117). No ticket named in the PR description.
Blocking
-
clippyCI check fails on code this diff introduces —tests/e2e-cucumber/tests/e2e/therock_steps.rs:338-340
Theclippyjob log shows:error: \format!(..)` appended to existing `String`(clippy::format_push_string, denied via-D warnings) in the newpip_indexes_for_unpinned_newest_stable` given-step:html.push_str(&format!( "<a href=\"{prerelease}\" data-dist-info-metadata=\"true\">{prerelease}</a>\n" ));
leading to
could not compile \e2e-cucumber` (test "e2e") due to 1 previous errorand job conclusionfailure. Confidence 100 (observed directly in the CI log). Category: mechanical. Fix: usewrite!(html, ...)` as clippy's own suggestion in the log proposes. -
"Commit signatures + sign-off" CI check fails — commit
71f11175(the PR's sole commit)
CI log:71f11175 ... -> signature is not GitHub-"Verified",error: commit signature/sign-off enforcement failed, job conclusionfailure.
Confidence 100. Category: mechanical/process.
Fix: sign the commit (SSH or GPG) and register the key on GitHub, or re-sign per the job's own printed instructions, then re-push. -
PR description omits a significant deletion: the EAI-8051/
comfyui-14end-to-end regression guard is removed in full, with no replacement coverage left anywhere —tests/e2e-cucumber/features/comfyui.feature(scenariocomfyui-14/@id:comfyui-install-preserves-the-rocm-runtimeremoved, file now ends at line 278),tests/e2e-cucumber/tests/e2e/comfyui_steps.rs(all its step defs + helperssole_runtime_python/torch_stack_versions/installed_distributions/nvidia_distributions/nvidia_onlyremoved, file now ends at line 1561),tests/e2e-cucumber/tests/e2e.rs:144-148,~320(the twoE2eWorldfields it used removed),docs/testing.md(its documentation paragraph removed).
The PR body's bullet about removed tests names only the separate two-component-version-pin test ("a test that required--version 10.1to stay on the canonical index was removed because..."); it never mentionscomfyui-14. The removal itself is mechanically clean (no dangling references, confirmed by repo-wide grep), but a repo-wide search also confirms no other scenario, unit test, or CI workflow still exercises "installing ComfyUI must not replace the managed ROCm runtime's torch/torchvision/torchaudio stack with CUDA (nvidia-*) packages" (the historical #298 regression this scenario guarded). That guarantee now has zero test coverage, undisclosed.
Confidence 90. Category: architectural (undisclosed coverage removal).
Fix: either restore the scenario — updating itsinstall sdk --yesGiven-step to pass an exact--family, the same pattern this diff already applies elsewhere inruntime_steps.rs/therock_steps.rs— or at minimum disclose the removal and the resulting coverage gap explicitly in the PR description so reviewers can judge it. -
A Gherkin scenario's own step text now misdescribes what it executes —
tests/e2e-cucumber/features/download_progress_pty.feature:21vstests/e2e-cucumber/tests/e2e/therock_steps.rs:658,685
The feature step reads "the user installs the tarball SDK for family gfx120X-all under a real terminal." Its bound#[when(...)]step (identical literal string) was edited by this diff to invoke--family gfx1200(RAW_ARCH) instead ofgfx120X-all(GROUP_FAMILY) — the step's own updated doc-comment even states "--family gfx1200is an exact arch; a group label is refused on this route," so the author clearly knew the value changed but left the Gherkin text (and the unchanged@id/scenario name) describing the old, no-longer-true behavior.
Confidence 95. Category: mechanical.
Fix: update both the.featureline and the matching#[when(...)]literal to say "arch gfx1200" (or similar).
Non-blocking
- Stale "both channels" claim left beside the scenario this PR rewrote to contradict it —
tests/e2e-cucumber/features/runtime_setup.feature:147
The untouched preamble above scenario runtime-08 still says "Both channels now resolve from one canonical aggregate index each..." Ten lines later, this same diff rewrote the adjacent runtime-09 comment to say an unpinned release install now reads the ROCm 10 index instead of the canonical aggregate — so the blanket "both channels" claim two scenarios up is no longer true for the release channel.
Confidence 80. Category: mechanical.
Fix: scope line 147's claim to the nightly channel, or cross-reference the now-different release-channel behavior described below it.
Decisions for the author
- Breaking-change blast radius for unpinned release installs — tradeoff
Making the release channel's unpinned default route to ROCm 10/"Next" is an intentional, test-covered change (seetherock-next-15), but it means any existing automation that ranrocm install sdkwith no--versionand only a grouped--family(or on a host whose GPU only resolves to a group) previously got ROCm 7.14 silently and will now hard-refuse, asking for an exact arch. Confirm this is the intended blast radius for existing users/scripts, and that it's communicated (see Deployment notes). - Deleting vs. updating
comfyui-14— non-blocking-improvement
Separate from the disclosure problem above (which is blocking), consider whether the Given-step's bareinstall sdk --yescould instead have been updated to pass an exact--family, the same pattern this diff already used inruntime_steps.rs/therock_steps.rs, preserving the regression guard instead of losing it outright.
Positive signals
- The core routing decision (
selects_next_layout/select_source_layout/requested_source_layout) is reasoned through every channel × selector × layout-override combination, each path is backed by a unit test that would fail if the logic were reverted, and the newif layout_override.is_none()guard ininstall_wheel_runtimecorrectly prevents the new unpinned-release-selects-Next rule from misrouting an existing runtime'srocm update. - The PR's own test plan is honest about what wasn't verified locally ("Full workspace
cargo testand clippy were not run... A later build of theorigin/maincheckout does not prove this branch compiles") rather than claiming false completeness — and that admission correctly predicted the clippy failure found in this review.
Deployment notes
- Breaking change: unpinned
rocm install sdkon the release channel now installs the newest stable ROCm 10 release and requires an exact GPU arch, where it previously installed ROCm 7.14 and accepted a grouped family. Existing scripts relying on the old implicit default need--version 7.14.1to keep the old behavior. - The GPU self-hosted/nightly E2E lanes switch their shared prewarm cache to
e2e-prewarm-next-v1(rename verified complete and consistent across both workflow files); the first post-merge run on each long-lived runner rebuilds that tree from scratch rather than reusinge2e-prewarm-multi-arch-v2. - At review time, two GPU-hardware E2E checks (W7900, MI300X) were still queued and two more (Strix Halo Ubuntu nightly/release) were in progress; full hardware-lane results weren't available yet, separate from the clippy/commit-signature failures already observed.
What this covered
Read the full diff (19 files) between merge-base cc56179ced9dd1be42a3000b2592ace9394dbff1 and head 71f11175424a00135c2f62eedb511443ad8b52f4, fetched read-only via git fetch (never checked out), plus the surrounding code each changed function calls into or is called from. Fanned out to 4 independent workers grouped by coupling: (1) core routing logic in therock.rs/main.rs, (2) therock test steps + feature files, (3) the comfyui-14 removal cluster, (4) CI workflow/docs prewarm-directory rename. Consolidated against my own direct reading of every file plus the actual CI job logs (clippy, Commit signatures + sign-off) rather than assuming what they covered.
Did not run as separate passes: history (git blame/git log) and prior-changes (no earlier review rounds exist on this PR to reconcile against) — not needed given this is a fresh single-commit PR with no visible prior review threads. Not reconciled against any discussion, since none existed at review time.
One fan-out worker (CI/infra/docs pass) reported it accidentally ran git checkout pr-820-head -- . against the shared local clone mid-pass and then reset the stage; it stated its actual analysis was done via git show/git diff against the untouched main-ref/pr-820-head refs, which were never touched. Noted here for transparency; it does not appear to have affected that pass's findings.
|
Reviewed at 71f1117. The core routing logic reads as sound, but three things block merge and the red GPU lanes need attributing. Blocking
Red GPU lanes, not yet attributed
Non-blocking
Checked by reading the diff: no CPU fallback is introduced, no workflow actions changed, and both the CLI and chat install paths pick up the new default. I didn't build or run tests locally, so the sort order in |
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
I went through the full diff and found a few things that should be fixed before this merges.
Blocking
- The clippy job fails on new code (
clippy::format_push_stringintherock_steps.rs), and the e2e-cucumber test target doesn't compile as a result. - The commit-signatures check fails: 71f1117 isn't GitHub-"Verified".
- Three GPU e2e lanes are red: MI300X, Strix Halo Ubuntu release, and Strix Halo Ubuntu nightly. The failures look like consequences of the unpinned default now installing ROCm 10.1 (for example a llama-server
rocm-10.1asset that can't be downloaded, and a vLLM 400 about--enable-auto-tool-choice), not runner flakes. I couldn't compare with main because its e2e-selfhosted run was cancelled, so please diagnose or rerun. - The PR body doesn't match the diff. It says the test requiring
--version 10.1to stay canonical was removed, but the diff adds one. It also doesn't mention the version-first ordering of release tarballs for all layouts, thelayout_overrideguard in the wheel install path, or the three e2e steps switched to--channel nightly. The test-plan boxes are unchecked while the text says tests passed locally.
Non-blocking
- The
layout_override.is_none()guard has no test that goes red when it's removed. - The MANIFEST/README claim that a tarball-installed runtime stays canonical on
rocm updatehas no code or test in this diff. - The release ordering change is only tested for the next layout, not canonical.
For the author to confirm: an unpinned rocm install sdk now hard-refuses, even with --dry-run, when only a group family or no GPU is detected. Please confirm that's intended for CI, container and no-GPU hosts and for other unpinned callers.
Details are in the inline comments.
| let rocm_index = served.join("next").join("rocm").join("index.html"); | ||
| let mut html = std::fs::read_to_string(&rocm_index).expect("rocm index was just written"); | ||
| let prerelease = format!("rocm-{NEWER_PRERELEASE_ROCM_VERSION}-{ANY_WHEEL_TAG}.whl"); | ||
| html.push_str(&format!( |
There was a problem hiding this comment.
clippy::format_push_string fails the clippy job (denied by -D warnings). Use write!(html, ...) with std::fmt::Write.
| // this host or channel can never actually use. An update already recorded | ||
| // its layout; re-deciding from the absent selector would treat a canonical | ||
| // runtime's refresh as an unpinned release install. | ||
| if layout_override.is_none() { |
There was a problem hiding this comment.
No test goes red if this guard is removed; the added unit test only covers requested_source_layout(Some(..)). Please add a test or scenario for updating a canonical runtime with no exact arch.
| .partial_cmp(&right.0.mtime) | ||
| .unwrap_or(Ordering::Equal) | ||
| .then_with(|| compare_version_strings(&left.1, &right.1)) | ||
| if matches!(channel, TheRockChannel::Release) { |
There was a problem hiding this comment.
Release ordering is now version-first for every layout, including the canonical tarball catalog, but the new test only exercises SourceLayout::Next. Add a canonical case, and mention the change in the PR body.
| "3.12" | ||
| ); | ||
| // A two-component selector does not parse as stable >= 10. | ||
| let line_pin = RuntimeVersionSelector::Version("10.1".to_owned()); |
There was a problem hiding this comment.
This pins --version 10.1 -> Canonical, while the PR body says that test was removed. Drop it or fix the body.
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
Review — needs work
Full review of the whole change. No ticket named in the PR description.
Blocking
-
clippyCI check fails on code this diff introduces —tests/e2e-cucumber/tests/e2e/therock_steps.rs:338
The new#[given]steppip_indexes_for_unpinned_newest_stabledoeshtml.push_str(&format!(...)), which is exactlyclippy::format_push_string(denied via-D warningsin this repo's lint config). Confirmed both in the code and in the CI check result (clippy= FAILURE on this head commit viagh pr view --json statusCheckRollup).
Confidence 100 · mechanical · Fix: usewrite!(html, "...")instead ofhtml.push_str(&format!(...)). -
"Commit signatures + sign-off" CI check fails on this PR's sole commit (
71f11175).
Confirmed viagh pr view --json statusCheckRollupon the current head: conclusion FAILURE.
Confidence 100 · mechanical/process · Fix: sign and/or DCO sign-off the commit per the job's own guidance, then re-push. -
"E2E tests (MI300X, current)" CI check fails on this head commit.
Confirmed viagh pr view --json statusCheckRollup: conclusion FAILURE. (Two other GPU lanes — Strix Halo Ubuntu release and nightly — show CANCELLED rather than FAILURE at this head; worth confirming they were re-run and are green before merge.)
Confidence 100 · mechanical · Fix: diagnose and either fix or re-run; a failing required check blocks merge regardless of cause. -
PR description misstates what this diff does to the two-component-version test. The Summary says: "A test that required
--version 10.1to stay on the canonical index was removed because that behavior is fixed on the separatefix/sdk-two-component-version-pinbranch." The diff does the opposite: it adds a new case torequested_source_layout_reads_the_request_not_the_resolution(apps/rocm/src/therock.rs, theline_pinblock) asserting a bare"10.1"selector staysSourceLayout::Canonical(with the comment "a two-component selector does not parse as stable >= 10"). No such assertion exists in the pre-change file at all — I diffedtherock.rsat base (cc56179) against head and confirmed"10.1"as a selector string appears only at head. Separately, the description also never mentions thatselect_tarball_candidate's reordering (see next finding) now reaches the pre-existing canonical/release tarball stream, not only the new "next" catalog.
Confidence 90 · architectural (description/diff mismatch) · Fix: correct the Summary bullet to describe what was actually added, and mention the broadened tarball-ordering scope. -
A Gherkin scenario's own step text now misdescribes what it executes —
tests/e2e-cucumber/features/download_progress_pty.feature:21vstests/e2e-cucumber/tests/e2e/therock_steps.rs:658,685.
The feature step reads "the user installs the tarball SDK for family gfx120X-all under a real terminal." Its bound#[when(...)]step (identical literal string, line 658) was edited by this diff to passRAW_ARCH("gfx1200", an exact arch) instead ofGROUP_FAMILY("gfx120X-all") at line 685 — the step's own updated doc comment even says "a group label is refused on this route." The.featuretext and the@id/scenario name were not updated to match.
Confidence 90 · mechanical · Fix: update the feature step's wording (and the bound#[when]string) to say "arch gfx1200". -
select_tarball_candidate's reordering silently broadens beyond the "next" layout it's framed around, and that broader case has no dedicated test.apps/rocm/src/therock.rs(select_tarball_candidate, ~line 3462): the sort comparator branches onchannel == TheRockChannel::Release, not onlayout, so the version-first ordering now also applies toSourceLayout::Canonicalrelease-channel tarballs (e.g. a pinned ROCm 7.x release), not only the newNextcatalog the surrounding doc comment discusses. The one existing test that exercisesCanonical+Release(tarball_selection_never_crosses_channels) has only one candidate surviving the stable-version filter on that branch (the other is a prerelease that gets filtered out), so it cannot distinguish old mtime-first from new version-first ordering — verified by reading the test directly.unpinned_next_tarball_picks_the_newest_stable_not_a_later_prereleaseonly coversSourceLayout::Next.
Confidence 80 · logic (untested behavior change) · Fix: either scope the version-first branch tolayout == SourceLayout::Nextif that was the intent, or add aCanonical+Releasetest with disagreeing mtime/version order to pin the broadened behavior deliberately.
Non-blocking
- Stale "both channels" claim beside a scenario this PR rewrote to contradict it —
tests/e2e-cucumber/features/runtime_setup.feature:147.
The untouched preamble above scenario runtime-08 still says "Both channels now resolve from one canonical aggregate index each..." Ten lines below, this same diff rewrote the adjacent runtime-09 comment/scenario to say an unpinned release install now reads the separate ROCm 10 ("next") index instead of the canonical aggregate — so the "both channels" claim is no longer accurate for the release channel (code/test behavior governs; the comment is stale).
Confidence 80 · mechanical · Fix: scope line 147's claim to the nightly channel.
Decisions for the author
- Breaking-change blast radius for unpinned release installs — tradeoff.
An unpinnedrocm install sdkon the release channel used to silently install ROCm 7.14 even when only a grouped--family(or no GPU) could be resolved. It now hard-refuses in that situation and asks for an exact arch, including under--dry-run. This is intentional and test-covered (therock-next-15), but confirm it's the intended blast radius for existing scripts/CI/container hosts that relied on the old silent default, and that it's called out prominently (it is already in the PR's own Summary).
Positive signals
selects_next_layout(pure routing predicate) is cleanly separated fromselect_source_layout(routing + refusal-with-message), andrequested_source_layoutreuses the same predicate rather than re-deriving it — "which interpreter" can't drift from "which layout."unpinned_release_without_an_exact_arch_refuses_instead_of_falling_backexplicitly asserts the refusal message does not contain--version, guarding against a plausible regression where the message is copy-pasted from the pinned-version path and invents a bogus suggestion for an unpinned request.- The new/changed unit tests in
therock.rs(unpinned_release_selects_the_next_layout,release_auto_select_picks_the_newest_stable_not_a_newer_prerelease,unpinned_next_tarball_picks_the_newest_stable_not_a_later_prerelease, etc.) were traced by hand and each would fail if the specific production change it covers were reverted, and each exercises the actual new branch rather than only the pre-existing pinned route.
Deployment notes
- Breaking change: unpinned
rocm install sdkon the release channel now installs the newest stable ROCm 10 release and requires an exact GPU arch, where it previously installed ROCm 7.14 and accepted a grouped family. Existing scripts need--version 7.14.1to keep the old default. - The GPU self-hosted/nightly E2E prewarm cache directory rename (
e2e-prewarm-multi-arch-v2→e2e-prewarm-next-v1) is complete and consistent — verified no remainingmulti-arch-v2occurrences in either.github/workflows/e2e-selfhosted.ymlor.github/workflows/nightly.yml, and the corresponding fixture-path rename inscripts/reclaim-gpu.shis cosmetic only (no runtime-detection regex in that script matches on the old/new name). - At review time (re-checked immediately before publishing), CI on this head commit shows:
clippyandCommit signatures + sign-offFAILURE (both blocking above),E2E tests (MI300X, current)FAILURE, andE2E tests (Strix Halo, Ubuntu, release/nightly, current)CANCELLED;W7900,Strix Halo Windows, andStrix Halo WSL2lanes are SUCCESS.
What this covered
Read the full diff (16 files, +587/-202, matching the PR's own reported stats exactly) between merge-base cc56179ced9dd1be42a3000b2592ace9394dbff1 and head 71f11175424a00135c2f62eedb511443ad8b52f4, fetched read-only via git fetch into a scratch repo (never checked out), plus the surrounding code each changed function calls into or is called from. Fanned out to 3 independent workers grouped by coupling: (1) core routing logic in apps/rocm/src/therock.rs, read in full (12815 lines); (2) apps/rocm/src/main.rs CLI help text plus README.md/MANIFEST.md; (3) the e2e feature/step files plus xtask/src/workflow_contract.rs's CI-contract test. A planned 4th worker (CI workflow YAML + remaining docs + reclaim-gpu.sh) could not be launched due to a transient shared-account subagent-capacity limit; I covered that scope directly myself instead. Every worker's candidate findings were independently re-verified by me against the actual code/files before inclusion; one candidate (a prior-review-style claim about an unrelated test file being removed) was checked and found to not apply to this diff at all, so it is not included here.
Did not run as a separate pass: git blame/git log beyond a surface skim of therock.rs's commit history, which surfaced nothing beyond ordinary incremental development. Not reconciled against any PR review discussion — this report was produced blind to it, independently re-deriving everything it states as a finding.
Summary
rocm install sdkstayed on the canonical index and installed ROCm 7.14. It now resolves the newest stable version on the ROCm 10 layout, with that layout's Python. A pinned 7.x release and nightly auto-select stay on the canonical stream. GPU e2e prewarm directories move toe2e-prewarm-next-v1so those lanes do not keep a ROCm 7.14 tree.@id:therock-next-01-unpinned-release-selects-newest-stablecovers newest-stable selection and skips a newer prerelease.@id:therock-next-15-unpinned-release-group-family-refusedcovers the grouped-family refusal.@id:therock-next-16-older-pin-stays-canonicalkeeps a ROCm 7 pin on the canonical index.@id:runtime-resolve-next-releasedry-runs the live ROCm 10 index and is@requires-gfx-target.--version 10.1are a separate change. This PR does not include that parser, so reviewers should not expect--version 10.1behavior here. A test that required--version 10.1to stay on the canonical index was removed because that behavior is fixed on the separatefix/sdk-two-component-version-pinbranch.Test plan
@id:therock-next-01-unpinned-release-selects-newest-stable,@id:therock-next-15-unpinned-release-group-family-refused, and@id:therock-next-16-older-pin-stays-canonical.@id:runtime-resolve-next-releaseis@requires-gfx-targetand dry-runs the live index.cargo testand clippy were not run. This worktree was not rebuilt withcargo build --workspace --all-targets. A later build of theorigin/maincheckout does not prove this branch compiles.