Skip to content

fix(therock): pin torchaudio to the requested ROCm 10 build - #630

Open
rominf wants to merge 5 commits into
mainfrom
fix/rocm10-pin-torchaudio-build
Open

rominf wants to merge 5 commits into
mainfrom
fix/rocm10-pin-torchaudio-build

Conversation

@rominf

@rominf rominf commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

rocm install sdk --version 10.0.0 fails for every arch:

published package metadata selected torchaudio 2.11.0.3+rocm10.1.0, which does not share ROCm build 10.0.0

--version 10.1.0 works only while 10.1.0 is the newest ROCm 10 build on the index; it fails the same way once a newer build publishes torchaudio. Found while reviewing #616, whose README documents installing 10.0.

Root cause

On the ROCm 10 (whl-next) layout, version resolution hands uv pip compile an exact rocm==<version> pin plus unversioned torch, torchvision and torchaudio. torch and torchvision follow the rocm pin, because their metadata depends on the exact ROCm build. torchaudio's metadata names no ROCm build, so uv took the newest torchaudio on the index, from whatever ROCm build that is. The existing consistency check then correctly refused the mixed stack.

Fix

Before resolving, list torchaudio from the index and pin it with torchaudio==<version>:

  • It pins the torchaudio release verified on hardware for the requested ROCm line, when that build is published for this Python and platform. The release is read from the vLLM ROCm 10.x discover table through rocm_engine_vllm::verified_torchaudio_release. That is one source of truth, and no new crate edge: rocm already depends on the engine crate.
  • Otherwise, or when the line has no verified release (10.0 today), it pins the newest build published for the requested ROCm version, and the plan's torchaudio_selection: line says the build is not hardware-verified. That line and the install-log note are rendered from the selection value itself.
  • Recency alone isn't enough: whl-next has published a CPU-only 2.11.0 under the same +rocm10.1.0 label as the working 2.11.0.2 and 2.11.0.3. That is why the vLLM table pins 2.11.0.2 exactly.
  • Builds are compared in PEP 440 order, with final releases ahead of pre-releases, which is how uv chooses.
  • The existing major.minor.patch parser can't be reused, because torchaudio publishes rebuilds as a fourth release segment (2.11.0.2, 2.11.0.3). That parser would drop every rebuild.

uv still resolves and validates the whole set together. If the requested build publishes no installable torchaudio, or none with a readable version, the install stops and says which.

This costs one extra index fetch, with its own single-fetch timeout.

Tests

  • New e2e scenarios in therock_next_generation.feature:
    • therock-next-11 publishes two ROCm 10 builds whose dependency metadata matches the live index's (torch requires its rocm build, torchvision its torch, torchaudio neither). It includes a four-segment torchaudio rebuild and a rebuild wheel for another platform, and asserts the plan uses only the pinned build. It fails on main with the reported error.
    • therock-next-12 covers the refusal when the pinned build has no installable torchaudio. It asserts the message and that resolution stopped before any plan. It fails on main.
    • therock-next-13 pins ROCm 10.1.0 against a fixture that publishes 2.11.0, 2.11.0.2 and 2.11.0.3, all +rocm10.1.0. It asserts the plan takes the verified 2.11.0.2, says so, and never mentions 2.11.0.3 or "not hardware-verified".
    • therock-next-11 also asserts the not-hardware-verified line, and that it names the same torchaudio the plan installs.
    • All three run on the mock lane (no GPU tags).
  • Unit tests cover PEP 440 ordering (numeric suffixes, dev/pre/post, epochs, trailing zeros), the final-over-pre-release preference, unreadable versions being skipped, and that resolution and install name the same torchaudio.
  • Mutations checked locally:
    • removing the wheel filter fails scenarios 11 and 12;
    • leaving torchaudio unversioned fails the composition test;
    • collapsing .devN numbers fails the ordering test;
    • removing the parser's trailing-garbage guard fails two unit tests;
    • ignoring the verified release fails three unit tests and therock-next-13;
    • refusing instead of falling back fails the two fallback tests;
    • a prefix match instead of an exact version fails torchaudio_pin_matches_the_verified_release_exactly;
    • removing the printed selection line fails scenarios 11 and 13.

Verification

  • Local:

    • cargo test --workspace --all-targets: 3599 passed, 0 failed.
    • cargo clippy --workspace --all-targets -- -D warnings and cargo clippy -p e2e-cucumber --test e2e -- -D warnings.
    • cargo fmt --check.
    • A Windows-target cargo check.
    • python scripts/smoke_local.py.
    • The therock_next_generation feature, 9/9 applicable.
  • Live index, --dry-run:

    Version Arch Before After
    10.0.0 gfx1200, gfx1151 failed torch 2.13.0, torchvision 0.28.0, torchaudio 2.11.0.2, all +rocm10.0.0
    10.1.0 gfx1200, gfx942 worked torchaudio 2.11.0.2+rocm10.1.0, the verified build the vLLM install also pins (derived from the code, not re-run live)
  • A real rocm install sdk --version 10.0.0 --family gfx1200 install succeeds. torchaudio's native libraries load with all symbols resolved (RTLD_NOW) against torch 2.13.0+rocm10.0.0, and a native torchaudio op runs.

  • At 0adef257:

    • rocm --bin rocm (1045 passed) and rocm-engine-vllm (102)
    • xtask (247), e2e-cucumber lib (142) and feature_naming
    • check --test e2e and check-crate-edges
    • both Linux clippy invocations
    • Windows-target clippy on this branch merged with current main
    • therock-next-11, -12 and -13, which pass locally; -07 and -09 need a GPU
  • Not verified: GPU execution. The host has no AMD GPU. The pin assumes the newest torchaudio of a ROCm build is the one built against that build's torch. Nothing on the index states that pairing; the ABI load above is the evidence for 10.0.0.

Related, not changed here

  • The canonical-layout selector (select_matching_pip_package_versions) also uses the major.minor.patch parser and would skip four-segment rebuilds. It is only reachable if a canonical feed ever carries such builds.
  • The verified pin is keyed on the ROCm line, not on the torch version it was checked against (2.13). If the SDK ever resolves a different torch for 10.1, that pin needs re-verifying.
  • The vLLM ROCm 10.0 discover row still matches torchaudio by series (Series("2.11")). Its own comment documents the same flat-index risk, which needs a live 10.0 box to settle.

  • If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No ticket; no rows mention torchaudio or ROCm 10.)
  • If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md (the clap declaration and dispatch wiring staying in main.rs/lib.rs is expected, not a violation). (Not applicable.)
  • Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. Covers claims of an outcome, remediation advice naming a command, and promises that something will not happen. (Both refusals are unit-tested; the "none published" refusal is asserted in therock-next-12 together with the stop before planning.)

Since the review of 0adef257

  • Behaviour: an unverified torchaudio pin is also announced as a Note: in a real install's log. When the verified release is published here only under another ROCm build label, the reason names that build instead of blaming the Python and platform.
  • Tests:
    • unverified_torchaudio_note_is_logged_only_for_an_unverified_pin
    • torchaudio_pin_falls_back_to_newest_when_the_verified_release_is_published_for_another_rocm_build
    • therock-next-14: a real install reaches the unverified-torchaudio note and is refused at the consent gate before uv runs, so it needs no GPU and no download. It asserts the note together with the refusal and an unchanged registry.
    • All four mutations (the note condition, the other-build lookup, the labelled-only filter, the progress_line call) turn a test red.
  • Deployment note: for ROCm 10.1, the composed package_specs change from 2.11.0.3 to 2.11.0.2+rocm10.1.0. That changes wheel_runtime_key, so a 10.1.0 runtime installed before this change gets a new runtime key on its next install or update.
  • Not run locally: therock-next-07 and -09, which need a GPU.

@rominf
rominf requested a review from a team as a code owner October 12, 2026 07:58
@rominf
rominf requested a review from siloteemu October 12, 2026 07:58

@jussielo-amd jussielo-amd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review — request changes

Ran a full independent review (review-code method: 4-way fan-out plus follow-up tracing). One finding is attached as an inline comment on the lines it concerns; everything else is here.

Blocking

  1. Same-ROCm-suffix torchaudio builds are picked by recency alone, and this repo already has direct, same-package evidence that recency isn't a safe discriminator. See the inline comment on apps/rocm/src/therock.rs.

  2. Three required E2E hardware lanes are still QUEUED, not green, as of this review's snapshot — E2E tests (MI300X), E2E tests (Strix Halo, Windows), E2E tests (Strix Halo, WSL2). Everything else (clippy, coverage, commit-signatures, docs-links, CodeQL, Linux/Windows build-and-test, Strix Halo Ubuntu E2E) is green. This isn't a code defect, just not yet a resolved precondition — please confirm these land green before merging.

Positive signals

  • The doc comment explaining why the existing major.minor.patch parser (parse_version) can't be reused for torchaudio is specific and checks out: traced parse_version's splitn(3, '.') behavior against a 4-segment version and confirmed it would indeed drop every rebuild.
  • therock-next-11/therock-next-12 are genuine regression tests, not cosmetic ones: the mock index is shaped to reproduce the real ambiguity (unconstrained torchaudio, a four-segment rebuild, a same-series wheel for another platform), traced to confirm it would fail on reverted code; and the refusal scenario asserts actual resulting state (no package_specs: line printed), not just the message text.

Deployment notes

  • Adds one extra index fetch (to list torchaudio versions) per whl-next SDK install, with its own single-fetch timeout — worth knowing for environments with restricted access to the TheRock index.

What this covered

Full diff (README.md, apps/rocm/src/therock.rs, tests/e2e-cucumber/features/therock_next_generation.feature, tests/e2e-cucumber/tests/e2e/therock_steps.rs) read completely via independent passes (implementation+README, e2e tests, git history/prior-PR search, AGENTS.md adherence), plus direct reading of engines/vllm/src/install.rs for the cross-cutting finding above. Did not re-run the e2e cucumber suite myself (relied on CI's green status); did not have AMD GPU hardware to verify the author's live-install claim for --version 10.0.0 --family gfx1200 (same limitation the PR description discloses).

Comment thread apps/rocm/src/therock.rs
Comment on lines +3750 to +3763
/// The newest of `versions` built for exactly `rocm_version`, chosen the way
/// `uv` would choose among them: in PEP 440 order, with final releases ahead of
/// any pre-release.
///
/// Nothing on the index pairs a torchaudio build with a torch build, so this
/// assumes the newest torchaudio of a ROCm build is the one built against that
/// build's torch — the same choice `uv` made unaided when the pinned build was
/// also the newest one on the index.
///
/// Not [`select_latest_stack_package`]: [`parse_version`] reads only
/// `major.minor.patch` and rejects the rest, and torchaudio publishes rebuilds
/// as extra release segments (`2.11.0.2`, `2.11.0.3`), so that would drop every
/// rebuild and settle on the original `2.11.0` build.
fn latest_build_for_rocm(versions: &[String], rocm_version: &str) -> Option<String> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This "pick the newest build sharing the ROCm suffix" heuristic has already failed, for this exact package, on this exact index, elsewhere in this repo.

engines/vllm/src/install.rs (lines ~149-159) documents that whl-next has published three torchaudio builds all tagged +rocm10.1.0 — 2.11.0, 2.11.0.2, 2.11.0.3 — where 2.11.0 is a CPU-only rebuild with no HIP kernels at all, indistinguishable from the other two by ROCm-suffix matching alone. vLLM's own discover table deliberately pins an exact, hardware-verified release (DiscoverVersion::Exact("2.11.0.2")) instead of trusting recency, specifically because "a series match would resolve the newest… so pin the lower of the two confirmed-working builds instead" (and for the 10.0 row, it explicitly flags trusting a series/newest match as an unverified risk it's accepting for lack of hardware to check it).

latest_build_for_rocm here has no safeguard against the same failure mode: package_versions_matching_rocm filters only by the +rocm<version> suffix, and among the survivors this always picks the PEP 440-newest. Today that happens to land on a working build (2.11.0.3), but nothing stops a future rebuild from publishing a broken build as the newest under an identical suffix — at which point rocm install sdk would pick it automatically, for every ROCm version, with no human verification and no code change required.

This doesn't look wrong for the versions actually exercised here (10.0.0 was manually ABI-verified per the PR description), but the default path for the primary SDK install command has no defense against a failure this same codebase has already hit once for this exact artifact. Worth at least cross-referencing this precedent in the doc comment above, and ideally hardening with a curated-pin fallback or an automated symbol/ABI check (mirroring what was already done by hand for 10.0.0) before trusting "newest" unconditionally.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, and fixed in 0adef257. The pin now prefers the build verified on hardware:

  • The source: install sdk reads the vLLM ROCm 10.x discover table's exact torchaudio through a new rocm_engine_vllm::verified_torchaudio_release. That's the same data, not a copy, since rocm already depends on the engine crate.
  • Verified build published for this Python and platform: it is pinned. For 10.1 that's 2.11.0.2.
  • Verified build not published, or none for the line (10.0 today): the newest build is pinned as before, and the plan's torchaudio_selection: line says it isn't hardware-verified. That line and the install-log note are rendered from the selection value itself.
  • The doc comment now cites the CPU-only 2.11.0+rocm10.1.0 case and the vLLM table.
  • Tests: four unit tests, one per branch plus exact matching, each checked against a mutation that drops it. The new therock-next-13 runs against a fixture that publishes 2.11.0/2.11.0.2/2.11.0.3, all as +rocm10.1.0, and asserts the plan takes 2.11.0.2 and says so.

@rominf

rominf commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 0adef257 for the blocking finding. Since the review, torchaudio prefers the hardware-verified release from the vLLM ROCm 10.x discover table, and falls back to the newest build with a printed "not hardware-verified" line. The visible effect on the live index: --version 10.1.0 now pins 2.11.0.2+rocm10.1.0, the same build the vLLM install pins, instead of 2.11.0.3. The description is updated.

On the second point, I'll make sure the MI300X and Strix Halo Windows/WSL2 lanes are green on the new head before asking to merge.

@siloteemu siloteemu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Automated review · pr-review-watcher · 0adef25

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.
The description says what the change does: it pins torchaudio for ROCm 10 wheel installs, preferring the vLLM table's hardware-verified release over the newest build. It names no ticket. Its checklist claim that every new user-facing message has a test asserting it does not hold for the install-log note (see the first Blocking item).

Blocking

  • new · The new install-log Note: for an unverified torchaudio has no test, and inverting its condition keeps every test green — apps/rocm/src/therock.rs:1978-1986
    This is new user-visible output. The code says it exists so the unverified choice shows "in the install log, not only in the summary block", and the description's checklist says every new user-facing message is asserted. Nothing reaches it:

    • All three new scenarios (therock-next-11/12/13) run with --dry-run. install_wheel_runtime returns the plan at if dry_run { … return Ok(SdkInstallResult::plan(output)); } before this block.
    • The live real-install scenarios (-07, -09) install the newest ROCm 10, which takes the verified path, so they never print the note.

    Changing .filter(|pin| !pin.is_verified()) to .filter(|pin| pin.is_verified()) and running cargo test -p rocm --bin rocm therock gave 177 passed, 0 failed. The e2e side was traced, not run.
    Confidence 85 · logic · Fix: move the note text and its condition into a helper and unit-test that it appears for both unverified bases and not for Verified. Or add a non-dry-run fixture scenario that reaches this point. It only has to get past the note, not complete an install.

  • new · The VerifiedNotPublished message blames "this Python and platform" when the real cause can be the ROCm build label, and a test locks that wording in — apps/rocm/src/therock.rs:3793-3795, apps/rocm/src/therock.rs:8527 (torchaudio_pin_falls_back_to_newest_when_the_verified_release_is_unpublished)

    • published_build_of searches built_for_rocm, which package_versions_matching_rocm has already narrowed to the exact +rocm<rocm_version> label.
    • verified_torchaudio_release matches by major.minor line, so a 10.1.1 or a nightly 10.1.0a… request still gets Some("2.11.0.2").
    • When 2.11.0.2 exists only under another label of the same line, the printed reason is "the verified release, 2.11.0.2, is not published for this Python and platform". That is false: it is published for this Python and platform, just for a different ROCm build.
    • The unit test's own doc names that case ("A verified release published only for another ROCm build"). Its fixture publishes 2.11.0.2+rocm10.1.0's only build as +rocm10.0.0, and it asserts the inaccurate sentence.
    • torchaudio_pin's own doc rejects this kind of misdirection: "reporting it as 'none published' would send the user to the wrong place".

    The code governs here, because the reason string has to describe what published_build_of actually searched.
    Confidence 85 · logic · Fix: say "is not published for ROCm {rocm_version} for this Python and platform", or tell the two cases apart. Then update the test to assert the corrected reason.

Non-blocking

  • new · Two scenario Givens don't describe the situation their fixtures set up — tests/e2e-cucumber/features/therock_next_generation.feature:223, :237

    • In therock-next-12, "publishes torchaudio only for a newer ROCm 10 build" hides the case that matters. The pinned build does publish torchaudio, but only as a wheel this host cannot install. Only the comment above the scenario says so.
    • therock-next-13 reuses "also publishes a newer ROCm 10 build" while installing that newer build. The situation it actually tests is "several torchaudio rebuilds share one ROCm label", and that too is only in a comment.

    Under the bdd-scenarios rules (situations, not fixture names), the Given should state the situation.
    Confidence 70 · mechanical · Fix: give 13 its own Given, and reword 12's Given to name the uninstallable-wheel situation.

  • new · README overstates when the install refuses, and hard-codes table facts that will go stale — README.md:447, README.md:441, docs/manual-testing.md:166, docs/vllm.md:87

    • "It stops with an error when the requested version publishes no torchaudio" leaves out "for this Python and platform". therock-next-12 refuses a version that does publish torchaudio, for another platform.
    • "today only ROCm 10.1 has one" and "(10.1: 2.11.0.2)" repeat the vLLM table's current contents in three docs. They go stale the moment a row changes, and nothing ties them to the table.

    Confidence 70 · mechanical · Fix: add the platform qualifier, and describe the rule rather than the table's current contents.

Decisions for the author

  • "Verified on hardware" is keyed on the ROCm line, not on the torch it was verified with, nor on the exact build — answered-by-intent
    PR description: "The verified pin is keyed on the ROCm line, not on the torch version it was checked against (2.13). If the SDK ever resolves a different torch for 10.1, that pin needs re-verifying."

    • The SDK leaves torch to uv.
    • docs/vllm.md already says the SDK's torch "may be a different torch version than this row pins".
    • describe() prints "verified on hardware for ROCm {rocm_version}" for any 10.1.x or nightly label that publishes 2.11.0.2.

    Without that passage this would be a non-blocking accuracy finding. Please confirm that keying on the line is still what you intend. Cheaper alternatives would be wording the line as "verified for the ROCm 10.1 line", or downgrading the basis when the resolved torch doesn't match the row's torch prefix.

  • The SDK's torchaudio choice now comes from the vLLM engine's discover table — tradeoff

    • For: one source of truth, no second copy, and no new crate edge (apps/rocm already depends on rocm-engine-vllm).
    • Against: this is the first time apps/rocm reads engine data. Switching a row's torchaudio between Series and Exact for a vLLM-only reason now silently changes rocm install sdk. The guards are a field doc, a docs/vllm.md paragraph, and the pinning test in therock.rs. The table rows themselves don't mention the SDK.

    Please confirm the coupling is deliberate, or consider a one-line pointer on the rows.

Positive signals

  • The torchaudio_selection: line and the install note are both rendered from the TorchaudioPin that made the choice, so the printed claim can't drift from the pinned build.
  • package_release_key skips versions it can't parse instead of guessing. The two refusals (none published, none readable) carry different messages, and a test checks they stay distinct.
  • The e2e fixture reproduces the live index's dependency shape: torch requires rocm, torchvision requires torch, torchaudio requires neither. That is what lets therock-next-11 fail on the base and pass with the fix.

Deployment notes

  • For ROCm 10.1, the composed package_specs change from the torchaudio uv used to choose (the newest, 2.11.0.3+rocm10.1.0 per the description) to 2.11.0.2+rocm10.1.0. wheel_runtime_key (apps/rocm/src/therock.rs:6605) hashes package_specs, so a 10.1.0 runtime installed before this change gets a different runtime key on the next install or update.

What this covered

Every changed file was read completely: apps/rocm/src/therock.rs (changed regions plus the resolution, index-listing, wheel-filter, plan-printing and runtime-key code they reach), engines/vllm/src/install.rs, engines/vllm/src/lib.rs, the feature file, therock_steps.rs, README.md, docs/manual-testing.md and docs/vllm.md. Diff: prw-base...0adef257. Four independent workers ran in parallel: code, e2e/docs, instruction/history/comments, and design questions.

  • Run by the driver: cargo test -p rocm --bin rocm torchaudio (11 passed) and the therock unit tests with one mutation (still green, as above).
  • Not run locally: the e2e cucumber suite, which needs release builds. The e2e conclusions come from reading the code, plus CI: every check that reported on this head passed, and "Skill checks" did not report. No skill files are touched.
  • Did not run: the prior-changes pass, because earlier PR reviews and comments are deliberately not read here. The discussion was not reconciled in this review.

@rominf

rominf commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Addressed in 7a315607, 1095c7d5 and 64dde91e. The branch is also rebased onto current main with no conflicts, and the PR's diff is otherwise unchanged.

Blocking 1: the unverified-torchaudio Note: was untested. Agreed: no test reached it.

  • The text and its condition now live in unverified_torchaudio_note. A unit test checks the note is absent for Verified and present, as Note: {describe()}, for both VerifiedNotPublished and NoVerifiedRelease.
  • The new scenario therock-next-14 reaches the note end to end, without a GPU or a download. It runs a real (non-dry-run) ROCm 10.0.0 install with an active default runtime and no consent flag. That install is refused right after the note and before uv runs. The scenario asserts the note (naming the same build therock-next-11 pins), the refusal, and an unchanged runtime registry together.
  • Mutation checks: inverting the condition fails the unit test, and deleting the progress_line call fails therock-next-14.

Blocking 2: VerifiedNotPublished blamed the Python and platform. Correct: the lookup searches only the exact +rocm<version> label. The two cases are now told apart:

  • If the verified release is published here under another ROCm label, the reason says it "is published for this Python and platform only for another ROCm build (2.11.0.2+rocm10.1.0), not for ROCm 10.1.1".
  • "Not published for this Python and platform" is used only when no ROCm build of it is published here.

Each case has its own test, and each test was mutation-checked.

Non-blocking: the scenario Givens. therock-next-12's Given now names the uninstallable-wheel situation. therock-next-13 has its own Given: several torchaudio rebuilds share one ROCm label.

Non-blocking: the docs. The refusal sentence now says "for this Python and platform". README, docs/manual-testing.md and docs/vllm.md now describe the rule (the vLLM ROCm 10.x row's exact torchaudio pin, where it has one) rather than the table's current values.

Decision: keyed on the ROCm line. Intended. The SDK leaves torch to uv, and the description already flags that the pin needs re-verifying if the SDK resolves a different torch for 10.1. Downgrading the basis on a torch-prefix mismatch would tie the SDK to the vLLM row's torch prefix, which docs/vllm.md says may differ by design.

Decision: the SDK reads vLLM's discover table. Deliberate: one source of truth and no new crate edge. As a cheap guard, both table rows now carry a one-line comment saying rocm install sdk reads their torchaudio pin.

Verified locally:

  • fmt
  • tests for rocm --bin (1107), rocm-engine-vllm (102), rocm-core --features e2e-test-hooks (622), xtask (252) and e2e-cucumber lib (142), plus feature_naming
  • check --test e2e and check-crate-edges
  • both clippy invocations, on Linux and for x86_64-pc-windows-gnu
  • therock-next-10 to -14, all passing; -07 and -09 need a GPU and were not run

The one local test failure was the known spinner timing flake, which passed on re-run.

`rocm install sdk --version 10.0.0` failed for every arch with "selected
torchaudio 2.11.0.3+rocm10.1.0, which does not share ROCm build 10.0.0".

On the ROCm 10 layout, `uv pip compile` picks torch and torchvision from
the `rocm==<version>` pin, because their metadata depends on the exact
ROCm build. torchaudio's metadata names no ROCm build, so left
unversioned it resolved to the newest build on the index. That works for
the newest ROCm 10 release only; every older pin got a mismatched
torchaudio and was refused by the consistency check.

Pin torchaudio before resolving: list torchaudio from the index (one
extra fetch, with its own single-fetch timeout) and take the newest
build published for the requested ROCm version and this Python and
platform, in PEP 440 order with final releases ahead of pre-releases, as
uv would choose. The existing major.minor.patch parser cannot be reused,
because torchaudio publishes rebuilds as a fourth release segment
(2.11.0.2). The newest ROCm build resolves the same versions as before.
When the requested build publishes no installable torchaudio, or none
with a readable version, the install stops and says which.

The e2e scenarios publish two ROCm 10 builds whose dependency metadata
matches the live index's, with a four-segment torchaudio rebuild and a
rebuild wheel for another platform, and cover both the pin and the
refusal.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Picking the newest torchaudio carrying the requested `+rocm<version>`
label is not safe on its own: `whl-next` publishes `2.11.0`, `2.11.0.2`
and `2.11.0.3` all as `+rocm10.1.0`, and `2.11.0` is a CPU-only rebuild
that nothing in its version or label distinguishes. The vLLM ROCm 10.x
discover-build table already pins `2.11.0.2` exactly for that reason.

`install sdk` now reads that same pin rather than keeping a second copy:
the vLLM engine exposes `verified_torchaudio_release`, the row's exact
torchaudio for a ROCm line (`rocm` already depends on the engine crate,
so no new crate edge). When the verified release is published for this
Python and platform, it is pinned. Otherwise, or when the line has no
verified release (ROCm 10.0 today), the newest build is pinned as
before. The plan's `torchaudio_selection:` line, and an install-log note
for the fallback, are both rendered from the selection itself and say
whether the build is hardware-verified.

ROCm 10.1 on the live index now pins `2.11.0.2+rocm10.1.0` instead of
`2.11.0.3+rocm10.1.0`, matching what the vLLM install pins.

therock-next-11 asserts the not-hardware-verified line beside the plan
it describes. The new therock-next-13 pins ROCm 10.1 against a fixture
that also publishes the original and a newer rebuild under the same
label, and asserts the plan takes the verified build and says so.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…e ROCm build in its reason

The install-log note for a torchaudio pin that is not hardware-verified had
no test: its condition lived inline at the call site, after the dry-run
return every scenario takes. Move the text and the condition into
unverified_torchaudio_note and test it for both unverified bases and for a
verified pin.

The VerifiedNotPublished reason blamed "this Python and platform", but the
verified release is looked up only among builds labelled for the requested
ROCm build, while it is matched by ROCm line. When it is published here
only under another label of the same line, the reason now says so and
names that build; it says "not published for this Python and platform"
only when no ROCm build of it is published here at all.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
therock-next-14 runs a real (not dry-run) ROCm 10.0.0 wheel install with an
active default runtime and no consent flag, so it is refused right after the
install-log note and before uv runs or anything changes on disk. It asserts
the note together with the refusal and an unchanged runtime registry.

Also name the situations therock-next-12 and -13 set up in their Givens: a
pinned build whose only torchaudio is a wheel this host cannot install, and
several torchaudio rebuilds sharing one ROCm label.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Describe the verified torchaudio as the vLLM ROCm 10.x row's exact pin
instead of repeating today's values, add "for this Python and platform" to
the refusal, mention the install-log note, and point the discover-table rows
at rocm install sdk, which reads their torchaudio pin.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf dismissed stale reviews from jussielo-amd and siloteemu October 14, 2026 16:40

Addressed: reviewed d4bd401. The blocking finding is fixed in 0adef25: torchaudio prefers the hardware-verified release and prints a "not hardware-verified" note otherwise. That note was then tested in 7a31560, 1095c7d and 64dde91. The head is 64dde91 (replies of 2026-10-12 and 2026-10-13). The GPU-lane point is tracked on the rebased head.

@rominf
rominf force-pushed the fix/rocm10-pin-torchaudio-build branch from 64dde91 to f3ff03d Compare October 14, 2026 18:33
@rominf

rominf commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main 287e08a8 to pick up the E2E harness fixes from #662 and #663; the head is now f3ff03db. The rebase had no conflicts, and the PR's diff is line-for-line identical. The only main commit that touched therock.rs is #648 (gfx90c family resolution), and it does not overlap the torchaudio pin.

Verified locally:

  • fmt;
  • tests: rocm bin (1178), vLLM (99), rocm-core (661), xtask (252), e2e-cucumber lib and feature_naming;
  • cargo check --test e2e;
  • clippy, both invocations, on Linux and for the Windows target;
  • this PR's scenarios therock-next-11 to 14 pass.

@pmoutsias-amd
pmoutsias-amd self-requested a review October 19, 2026 10:51

@pmoutsias-amd pmoutsias-amd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agent-assisted review of #630 at f3ff03db — approve

Reviewed by Claude Code on behalf of @pmoutsias-amd (ROCm Documentation Team), whose docs PR #616 depends on this one. This is an agent-assisted review based on reading the code and the CI logs. It was not run locally and not run on hardware (see "Not covered"). I'm not on @ROCm/rocm-cli-maintainers, so a maintainer approval is still needed to satisfy the merge rule.

Every finding from review 5504479846 is fixed in the code. I found nothing blocking.

Blocking

None.

Non-blocking (docs formatting only)

To be handled in the dependent docs PR #616 after it rebases onto this one. No change needed here:

  • docs/manual-testing.md: one over-long unwrapped line ("On a host with no detectable AMD GPU the preview reports device_target: undetermined…").
  • docs/vllm.md: ragged wrapping in the new paragraph.
  • README.md: the new torchaudio paragraph is dense. Consider splitting the rule from the fallback behavior.

Decisions for the maintainers

  • ROCm 10.0 still trusts "newest build" unconditionally. The open thread from @jussielo-amd (the CPU-only 2.11.0+rocm10.1.0 precedent) is fixed for 10.1: the vLLM-verified 2.11.0.2 is pinned over the newer 2.11.0.3. The vLLM table records no verified torchaudio for 10.0, so there the SDK still takes the newest build, with a printed "not hardware-verified" note in the plan and the install log. A future bad rebuild under the same label would be picked up with no code change. The reviewer's suggested automated ABI/symbol check was not added. I read this as an accepted, visible risk (the 10.0 build was verified by hand per the description), not a blocker. The thread is still unresolved and needs its author or a maintainer to close it.
  • Keying the verified pin on the ROCm line, and the SDK reading the vLLM discover table, were both answered by the author. The pointer comments on the vLLM table rows are in.

Prior findings (review 5504479846)

  • B1, install-log note untested: fixed. unverified_torchaudio_note has a unit test, and scenario therock-next-14 does a real install, asserts the note and the refusal, and checks the runtime registry is unchanged. It ran and passed in CI.
  • B2, wrong reason wording: fixed. The message separates "published only for another ROCm build" from "not published for this Python and platform", with a test for each.
  • Scenario Givens and docs: fixed.

Positive signals

  • The verified release is matched as a whole PEP 440 version, not a prefix.
  • Version ordering is tested on 12 pairs.
  • Unreadable versions are skipped, not guessed, and the two refusal messages are distinct.
  • The e2e fixture constants carry a comment tying them to the vLLM pin.

Deployment notes

  • Existing ROCm 10.1.0 runtimes get a new runtime key and rebuild on their next install or update.
  • Each whl-next SDK install makes one extra torchaudio index fetch.

What this covered

  • The full diff (287e08a8..HEAD, 8 files) and the unit-test bodies, with each prior finding checked against the code.
  • CI logs on f3ff03db: scenarios therock-next-11 to -14 ran and passed, and the MI300X and Strix Halo lanes are green. The only failures in the E2E run are serve-01 and serve-02, which are marked as expected failures.

Not covered

  • No local test run (no Rust toolchain in the review environment).
  • No hardware check that --version 10.1.0 resolves to 2.11.0.2. The scenarios use a mock index, so that rests on the author's testing and the hardware CI lanes.
  • The author's mutation checks were not re-run.

@rominf
rominf enabled auto-merge October 19, 2026 11:21
@rominf
rominf added this pull request to the merge queue Oct 19, 2026
@rominf

rominf commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks to both of you.

  • ROCm 10.0 still takes the newest build: that is the accepted, visible risk. The vLLM table records no verified torchaudio for 10.0. In that case the plan and the install log print the "not hardware-verified" note, so a bad rebuild would show up at install time. An automated ABI or symbol check is a reasonable follow-up, but it is outside this fix.
  • Docs formatting notes: handled in docs: copyedit README, CONTRIBUTING, and vLLM; add uninstall and verify guidance #616, as suggested.

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants