fix(therock): resolve ROCm releases from the current multi-arch pip index - #272
juhovainio wants to merge 4 commits into
Conversation
juhovainio
added
installation
|
Reviewed current head |
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Nice fix for the flat multi-arch index and the missing device-* extra — the reasoning in the description and the new unit tests for therock_rocm_extras/therock_index_urls are solid and I don't see a bug in that part.
One thing in the new e2e coverage looks like it will cause flakiness for unrelated tests, so I'd like that addressed before merge:
The new runtime-install-sdk-release-index-shape scenario runs rocm install sdk --family gfx110X-all --dry-run, which still resolves the real repo.amd.com pip index over the network (dry-run only skips the venv/download, not the index resolution). That's the exact same CLI invocation as the existing runtime-install-records-the-real-folder scenario a few lines above it in this file — and that sibling scenario is deliberately tagged @nightly, with a comment explaining why: on the no-GPU mock lane, which runs up to 64 scenarios concurrently, the extra network work from this call was measured to push two timing-sensitive scenarios (eai-7960-gen-tps-held-after-scrape-failure and eai-7960-gen-tps-expiry-boundary) past their validity window, failing them 3/3 under load.
The new scenario has neither @nightly nor @requires-gpu, so it will run on every PR's no-GPU mock lane at the same concurrency that was already shown to cause this interference. It looks like it should carry the same @nightly tag (and equivalent justification) as its sibling, or otherwise be isolated from the default concurrent mock lane.
| # auto-detection so this needs no GPU, and `--dry-run` resolves the real | ||
| # index without installing anything. | ||
| @id:runtime-install-sdk-release-index-shape | ||
| Scenario: 4 - Resolving the SDK from the release channel never uses the broken multi-arch URL shape |
There was a problem hiding this comment.
This scenario issues rocm install sdk --family gfx110X-all --dry-run, which resolves the real repo.amd.com pip index over the network before printing the plan (dry-run skips the install, not the index resolution). That's the same call the runtime-install-records-the-real-folder scenario just above makes, and that one is tagged @nightly specifically because the extra concurrent network work on the no-GPU mock lane (64 scenarios at once) was measured to push two other scenarios past their timing budget 3/3 times. This scenario has no @nightly/@requires-gpu tag, so it will run at that same concurrency on every PR and can reintroduce the same flakiness the sibling scenario's comment describes. Consider tagging it @nightly (or otherwise excluding it from the unserialized mock lane) to match the sibling.
Heads up: this will conflict with #329#329 adds ROCm 10 ("next" layout /
|
…ndex
repo.amd.com/rocm/whl-multi-arch is AMD's current release index and is
where 7.14.0+ is published, but therock_index_urls() appended a
per-family path segment to it (a scheme only the older whl/{family}
index uses), so the request always 403'd and the CLI silently fell
back to the stale classic index topping out at 7.13.0.
Try the multi-arch index (flat, no per-family path) first, keeping the
classic per-family index as the second candidate so older families and
explicit version pins that only exist there keep resolving exactly as
before. The multi-arch rocm sdist also needs an explicit device-*
extra to pull in a GPU backend, so requests to it now add
device-gfxNNNN extras for the families with an exact enumerable chip
set, or device-all for the remaining prefix-bucket families.
Signed-off-by: Juho Vainio <juho.vainio@amd.com>
Adds a GPU-free e2e scenario that dry-runs `install sdk --family
gfx110X-all` and asserts the resolved index_url never has the
/whl-multi-arch/{family}/ shape that 403s and used to mask the fix by
silently falling through to the stale classic index every time.
Signed-off-by: Juho Vainio <juho.vainio@amd.com>
Tag the new release-index-shape dry-run scenario @nightly (same eai-7960 timing-fragility reason as the sibling scenario), and clarify the package_policy dry-run prose to mention resolved device extras instead of implying gfx-specific wheels always ship in the pip specs. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
6684567 to
50ecdbb
Compare
Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Addressed the review feedback from both of you: @rominf: rebased onto current `main` (was DIRTY/conflicting after #322 landed). The conflict in `tests/e2e-cucumber/tests/e2e/runtime_steps.rs` was purely adjacent insertions (this PR's new step function next to #322's new step functions), spliced together with no semantic change to either side. Also tagged the new dry-run scenario `@nightly`, for the same reason as the existing `runtime-install-records-the-real-folder` scenario: it resolves the real channel index over the network even under `--dry-run`, and that extra network work on the no-GPU mock lane's 64-way concurrency was pushing `eai-7960-gen-tps-held-after-scrape-failure` / `eai-7960-gen-tps-expiry-boundary` past their validity window. @michaelroy-amd: updated the `package_policy` dry-run prose to say "pinned rocm (with any resolved device extras)" instead of implying the extras are always the same shape, so it reads correctly for both the classic per-family index and the new flat multi-arch index. Also re-verified live against `repo.amd.com`: `--dry-run` now resolves `index_url: https://repo.amd.com/rocm/whl-multi-arch\` with the correct `device-gfx1100,...` extras and no fallback to the broken per-family shape. All CI checks are green on `68934fa` (full run: unit tests, clippy, prek, E2E, Windows build). Ready for another look. |
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
The core fix is right and I could confirm it end to end. Flattening the multi-arch candidate and putting it first is what #271 needed, and this PR's own GPU lane shows it working: the pre-warm resolves 7.14.0 from https://repo.amd.com/rocm/whl-multi-arch where the classic index had it pinned at 7.13.0. @michaelroy-amd's package_policy wording note and @rominf's @nightly tagging both landed.
But the same CI log shows the device-all fallback going wrong on exactly the hardware that matters most, and the new regression scenario doesn't fail on unpatched main.
device-all on Instinct. Six of the sixteen families return None from known_therock_family_device_chips, including gfx94X-dcgpu and gfx950-dcgpu. Job 99541673879 (MI300X) shows the result: Installing rocm[libraries,devel,device-all]==7.14.0, then 24 rocm-sdk-device-* wheels totalling 4451 MiB downloaded onto a machine that needs one of them, and the post-install probe reporting rocm_sdk_target_family: gfx1010 on a gfx943 host. Four of those six buckets are cleanly enumerable from the wheels the index already publishes — details inline, including that the fix keeps the existing round-trip invariant test green.
The new scenario passes with the bug present. !stdout.contains("whl-multi-arch/gfx110X-all") is satisfied on main, because main tries the classic index first, it succeeds (stale 7.13.0), the loop returns, and the broken URL is never printed — failed candidates only surface through bail!, which is unreachable when the first one works. AGENTS.md §3 wants the opposite.
Two consumers the change breaks silently. scripts/therock_sdk_install_test.py:499 asserts on the literal rocm[libraries,devel]==, which is no longer a substring of rocm[libraries,devel,device-gfx1151]==; it's manual-only so CI can't catch it, and docs/testing.md:167 states the old shape as the expected plan. Both are unchanged files that this PR invalidates.
On "All CI checks are green on 68934fa". It isn't — E2E tests (GPU) is FAILURE with 4 unexpected failures and E2E tests (Strix Halo, Ubuntu) was CANCELLED. I dug into whether they're yours, and the answer is partly. The same four scenario names also fail on #251's lane, so the lane was red before you. But the mechanism here is new: your version bump installs a fresh 7.14.0 runtime, and this branch's base (283811fc) predates #314's unconditional ensure_default_engine in the pre-warm, so the engine venv stays behind on 7.13.0 and every serve dies with vLLM is not installed in a Linux/WSL ROCm Python environment — a message that appears zero times on either control lane. Rebasing onto current main picks up #314 and should clear that half. Worth correcting the comment either way (AGENTS.md §4).
The #329 collision you flagged is real and worse than the file-level conflict: #329's legacy_pip_index_candidates still builds format!("{}/{family}", release_pip_multi_arch_index_base()), i.e. this exact bug. Note inline.
Checked and clean: leak scan on the diff and PR text; signature/sign-off gate; is_multi_arch_pip_index trailing-slash handling; the nightly path is untouched; known_therock_family_device_chips_round_trip_to_their_family is a genuine invariant, not a tautology; the unit tests for the two new helpers are well-targeted.
(One process note: my read here was thorough on the diff but I don't have an Instinct host, so the device-all behavior is from your CI log, not from running it.)
| async fn assert_index_not_broken_multi_arch(world: &mut E2eWorld) { | ||
| let stdout = world.cli_output.as_deref().expect("no dry-run output"); | ||
| assert!( | ||
| !stdout.contains("whl-multi-arch/gfx110X-all"), |
There was a problem hiding this comment.
This assertion is satisfied on unpatched main, so the scenario doesn't guard the fix.
On main, therock_index_urls(Release, family) returns [whl/{family}, whl-multi-arch/{family}] — classic first. resolve_pip_runtime_with_timeout loops the candidates and returns on the first success. The classic index does resolve (that's the premise of #271: it works, it's just pinned at 7.13.0), so the loop returns immediately and the dry-run prints:
index_url: https://repo.amd.com/rocm/whl/gfx110X-all
The broken whl-multi-arch/gfx110X-all URL only ever reaches stdout through the bail! at the bottom of that loop, which is unreachable when a candidate succeeds. So !stdout.contains("whl-multi-arch/gfx110X-all") holds with the bug present, and holds after the fix, and would keep holding if someone reverted therock_index_urls tomorrow. AGENTS.md §3: "test fails before fix and passes after fix."
Asserting the positive fixes it, and it's a one-line change:
assert!(
stdout.contains("index_url: https://repo.amd.com/rocm/whl-multi-arch\n"),
"release install did not resolve the flat multi-arch index:\n{stdout}"
);That fails on main (which prints the classic URL) and passes here. Worth also asserting package_specs: carries a device- extra, since that's the other half of the fix and currently has only unit coverage.
One edge worth naming: if the classic index ever stops resolving entirely, main would bail! and run_rocm_ok's rc == 0 check would panic first — the scenario would fail, but on the command failing, not on the assertion. The positive form is correct in both worlds.
| } | ||
| None => extras.push_str(",device-all"), | ||
| } | ||
| extras |
There was a problem hiding this comment.
This fallback fires on every Instinct/datacenter part, and your own CI shows what it costs.
known_therock_family_device_chips returns None for six of the sixteen known families — gfx90X-dgpu, gfx90X-dcgpu, gfx94X-dcgpu, gfx950-dcgpu, gfx101X-dgpu, gfx103X-dgpu. The two gfx94X/gfx950 entries are MI300/MI350; that's the datacenter audience taking the device-all path by default.
From job 99541673879 (MI300X, detected_gfx_target: gfx943 → gfx94X-dcgpu):
Installing rocm[libraries,devel,device-all]==7.14.0 ... from https://repo.amd.com/rocm/whl-multi-arch
Downloading rocm-sdk-device-gfx1030 (458.5MiB)
Downloading rocm-sdk-device-gfx90a (450.7MiB)
Downloading rocm-sdk-device-gfx950 (650.9MiB)
... 24 device wheels, 4451 MiB total
and then the post-install probe:
rocm_sdk_target_family: gfx1010
So on a gfx943 host we download ~4.3 GiB to use one wheel, and the probe reports the wrong target family — gfx1010 is just the first of the 24. That value is printed to the user and stored in the manifest. The exact-chip lanes in the same run resolve correctly (gfx1151→gfx1151, gfx120X-all→gfx1201), which localises it to device-all.
There's no size guard either: preflight_tarball_space is called only from install_tarball_runtime (:1119), so the wheel path has no disk preflight at all. A constrained host gets a multi-GiB surprise with no warning.
Four of the six are enumerable right now, from the wheel names the same CI log lists:
"gfx94X-dcgpu" => Some(&["gfx942"]),
"gfx950-dcgpu" => Some(&["gfx950"]),
"gfx101X-dgpu" => Some(&["gfx1010", "gfx1011", "gfx1012"]),
"gfx103X-dgpu" => Some(&["gfx1030", "gfx1031", "gfx1032", "gfx1033",
"gfx1034", "gfx1035", "gfx1036"]),I checked these against normalize_therock_family — gfx942→gfx94X-dcgpu (the starts_with("gfx94") arm), gfx950→gfx950-dcgpu, gfx101*→gfx101X-dgpu, gfx103*→gfx103X-dgpu — so known_therock_family_device_chips_round_trip_to_their_family stays green with all four added. The remaining two (gfx90X-dgpu/gfx90X-dcgpu) are genuine catch-alls that overlap the exact gfx900/gfx906/gfx908/gfx90a families, and device-all is a defensible answer there.
The better answer, if you're reconciling with #329 anyway, is #329's shape: derive the extra from the detected raw arch (device-{raw_arch}) and keep the family table only as the fallback when no raw arch is known. Either way, when device-all is chosen, say so in the dry-run output — right now the user sees it in package_specs with no indication it means "every GPU ROCm supports."
| ) -> Vec<String> { | ||
| vec![ | ||
| format!("rocm[libraries,devel]=={}", package_versions.rocm), | ||
| format!("rocm[{rocm_extras}]=={}", package_versions.rocm), |
There was a problem hiding this comment.
This shape change breaks a documented acceptance test that CI can't see.
scripts/therock_sdk_install_test.py — unchanged by this PR — asserts:
THEROCK_SDK_PACKAGE_SPEC = "rocm[libraries,devel]" # :29
...
assert_contains(install_output, f"{THEROCK_SDK_PACKAGE_SPEC}==", "sdk install") # :499After this change the release-channel output is rocm[libraries,devel,device-gfx1151]==7.14.0, and "rocm[libraries,devel]==" is not a substring of that. The script's default is --channel release (:412), so every documented invocation in docs/testing.md:180/186/193/200 and docs/manual-testing.md:140 fails on the first release run. It's manual-only — not in ci.yml — so nothing catches it, and AGENTS.md §8 names docs/testing.md checks as part of the gate for touched behavior.
The fix is small: make the constant a prefix, "rocm[libraries,devel", and assert on assert_contains(install_output, THEROCK_SDK_PACKAGE_SPEC). The existing assert_not_contains(install_output, "rocm[devel]") negative still does its job.
Same class of staleness in prose at docs/testing.md:167, which lists the expected plan as "pinned rocm[libraries,devel], torch, torchvision, and torchaudio versions" — worth a sentence noting the release channel now adds device-*.
| @@ -3368,9 +3402,12 @@ fn parse_version(value: &str) -> Option<ParsedVersion> { | |||
|
|
|||
| fn therock_index_urls(channel: TheRockChannel, family: &str) -> Vec<String> { | |||
There was a problem hiding this comment.
On the #329 collision you flagged — one detail worth pinning down before you agree a merge order, because it's not just a textual conflict.
#329's replacement for this function still contains the bug:
// pr/329 apps/rocm/src/therock.rs:3632-3638
TheRockChannel::Release => vec![
format!("{}/{family}", release_pip_index_base()),
format!("{}/{family}", release_pip_multi_arch_index_base()),
],Classic first, then the per-family multi-arch shape — byte-for-byte main's behavior. #329's new Next candidate doesn't cover this: it points at a different host (whl-next) and returns None outright when raw_arch is unknown, so it never reaches repo.amd.com/rocm/whl-multi-arch. If #329's therock.rs wins the reconciliation, #271 regresses with no test failing — including the new scenario above, for the reason in that comment.
The other direction is the good news: #329's therock_pip_package_specs already derives rocm[libraries,devel,device-{raw_arch}] from the detected arch (and extends it to torch[device-*]/torchvision[device-*], which this PR doesn't), which is the better answer to the device-all problem. So the reconciled version wants this PR's flat candidate plus #329's arch-driven extras — a three-variant generation enum (Legacy / MultiArch / Next) rather than either side winning wholesale.
Whoever merges second should re-verify against #271's actual repro, not just resolve conflicts and trust the suite.
| # nightly lanes instead, where scenarios are serialized. | ||
| @id:runtime-install-sdk-release-index-shape @nightly | ||
| Scenario: 4 - Resolving the SDK from the release channel never uses the broken multi-arch URL shape | ||
| When the user dry-runs installing the SDK for a known family |
There was a problem hiding this comment.
Scenario number 4 is already taken — :30, "Reinstalling the SDK leaves the installed engine's requirements satisfied."
Nothing fails: the harness's uniqueness assert (tests/e2e-cucumber/tests/e2e.rs:1179) covers @id, not the numeric prefix. But the numbers are how the file and the HTML report are navigated, and two "Scenario: 4"s in one feature make them indistinguishable there. Next free in this file is 9.
|
Resolver ownership update: #308 is the canonical owner for current release/nightly aggregate source selection, shared provenance, and runtime-composition behavior. #329 is the ROCm 10 Next-layout extension and will be stacked on #308. This PR's #271 regression behavior will be transferred into #308 with scenario evidence; after that transfer is verified, #272 will be closed as superseded rather than merged. #314/current For new canonical and Next installs, unknown or incomplete source layouts must fail closed—no silent legacy or alternate-channel fallback. Existing managed runtimes with legacy manifests remain readable and usable for backward compatibility. Related: #308, #329, #314. Tracking: EAI-8268, EAI-8431, EAI-7956. |
|
Closing as superseded by merged #308. The complete #271 behavior is now on |
3 participants
Summary
Fixes #271.
therock_index_urls()appended a/{family}path segment torepo.amd.com/rocm/whl-multi-arch, but that index is flat (no per-familypath); the request 403'd, so the CLI silently fell through to the stale
classic
whl/{family}index (stuck at 7.13.0) every time.to the classic per-family index. Reuses the existing "try each index in
order, return on first full success" resolution loop as-is, so older
releases/families that only exist on the classic index keep resolving
exactly as before - the previous source stays a working fallback.
rocmsdist needs an explicitdevice-*pip extra to pullin any GPU backend at all. Added
known_therock_family_device_chipsinrocm-core(mirrors the existingnormalize_therock_familybucketing) torequest exact
device-gfxNNNNextras for the 10 families with anenumerable chip set, falling back to
device-allfor the remaining 6prefix-bucket families where exact membership isn't derivable. The classic
index path is unaffected (still gets plain
libraries,devel).Test plan
cargo build -p rocm-core -p rocmcargo clippy -p rocm-core -p rocm --all-targets(clean)cargo test -p rocm-core(296 passed)cargo test -p rocm(459 passed), including new coverage:therock_index_urls_prefers_multi_arch_then_classic_for_release,therock_index_urls_nightly_unchanged,therock_rocm_extras_classic_index_is_unchanged,therock_rocm_extras_multi_arch_adds_exact_device_chips,therock_rocm_extras_multi_arch_falls_back_to_device_all_for_ambiguous_bucket,and
known_therock_family_device_chips_round_trip_to_their_familyinrocm-core.rocm install sdk --channel release --dry-run) to confirm 7.14.0 and the rightdevice-*extraare resolved for a real host - not run in this environment, worth a
sanity check before merge.