feat(therock): support ROCm 10 "next" install layout, additive (EAI-8431) - #329
Conversation
juhovainio
changed the title
Heads up: this will conflict with #272Verified with a real trial merge (
|
Conflict check against #308Verified via a real trial merge ( The two PRs are not solving the same problem, but they rewrite the same seam of code:
Whichever of these merges second will need a real rebase, not a mechanical conflict resolution: #308 already changes legacy's package-spec shape, URL candidate list, and runtime-key scheme, which this PR's "legacy stays untouched" premise assumes doesn't happen. The Flagging early so whoever rebases second isn't surprised by the depth of the conflict. |
michaelroy-amd
left a comment
michaelroy-amd
left a comment
There was a problem hiding this comment.
PR Review: #329 — feat(therock): support ROCm 10 "next" install layout, additive (EAI-8431)
Reviewed: 2026-08-31
Author: @juhovainio
Branch: therock-rocm10-next-layout → main
Head: f245f8246b79c38d83b4bbd4e22c1dcbbf06b3c8
Decision: REQUEST CHANGES
Summary
The generation-aware resolver is structurally sound and the live ROCm 10 indexes match the implemented package/tarball naming. Three blockers remain: the new tarball scenario runs on unsupported native Windows and fails CI, existing managed runtimes suppress raw-arch detection and therefore bypass the new pip index, and the new gfx103X-all grouped alias is incorrectly reused as a raw device extra. The branch also fails the required formatting gate.
Findings
CRITICAL
None.
HIGH
-
Existing managed runtimes prevent normal ROCm 10 wheel discovery —
apps/rocm/src/therock.rs:1561-1576resolve_familyreturns the grouped family fromdetect_managed_therock_familywithraw_arch: Nonebefore attemptingdetect_host_gfx_target.therock_pip_index_candidatesthen removes the Next candidate wheneverraw_archis absent (apps/rocm/src/therock.rs:3627-3664). Once a user has any managed runtime, a plain release wheel install can therefore remain on Legacy instead of trying ROCm 10 first, even when the host exposes a usable raw target. This contradicts the PR's primary Next-before-Legacy behavior for the common upgrade path.Fix: preserve managed-family precedence, but attach the detected raw target when it normalizes to the same family (or return raw arch from the managed manifest). Add a regression scenario with an existing managed runtime and no explicit
--family. -
The new grouped alias can generate an invalid raw-arch pip extra —
apps/rocm/src/therock.rs:1523-1538,crates/rocm-core/src/lib.rs:3910-3912gfx103X-allis newly accepted as a grouped alias but is not present inknown_therock_families, whose canonical value remainsgfx103X-dgpu.family_override_raw_arch("gfx103X-all")therefore returnsSome("gfx103X-all"); Next package specs becomedevice-gfx103X-allinstead of a specific raw target such asdevice-gfx1030. The same helper accepts any larger string from whichnormalize_therock_familycan extract a gfx token.Fix: only return
raw_archfor a validated specific raw target (gfxfollowed by digits), not for grouped labels or free-form strings. Add cases forgfx103X-all,gfx103X-dgpu, and an embedded/free-form gfx token. -
The new tarball E2E scenario is not gated to Linux and fails the blocking Windows lane —
tests/e2e-cucumber/features/therock_next_generation.feature:17-22The scenario invokes
rocm install sdk --format tarball, but native Windows intentionally rejects tarball installs inapps/rocm/src/therock.rs:587-591. CI run33400341053, job99514948516, fails this exact scenario as an unexpected regression. Repository E2E conventions provide@requires-os:linuxfor OS-specific premises.Fix: add
@requires-os:linuxto@id:therock-next-tarball-skips-tests-artifact. Keep the wheel scenario cross-platform.
MEDIUM
-
Required formatting gate fails —
tests/e2e-cucumber/tests/e2e/therock_steps.rs:29-31cargo fmt --all --checkrequires the longformat!expression to be split across lines. The blockingprek (lint / hygiene)CI job fails on the same diff.Fix: run
cargo fmt --alland commit the result.
LOW
None.
Standards
No additional documented-standard violations found. New Rust source has the required license header; both user-visible behaviors have Gherkin coverage; legacy and nightly candidate ordering has focused unit coverage. Optional tuple/context abstractions would add weight without fixing a current defect.
Spec
The source dispatch, tarball sibling filtering, URL overrides, family additions, and Linux fixture-backed scenarios implement most of the PR description. Spec fidelity is incomplete because managed-runtime precedence prevents Next dispatch in the ordinary upgrade state, and the accepted gfx103X-all alias is not safely distinguished from a raw device target. The cache-key concern initially considered is not a finding: download_text_cached rejects a cached entry whose recorded URL differs (apps/rocm/src/therock.rs:2303-2304).
Validation Results
| Check | Result |
|---|---|
cargo fmt --all --check |
Fail — therock_steps.rs:29 formatting diff |
cargo clippy --workspace --all-targets -- -D warnings |
Pass |
cargo test --workspace --all-targets |
Fail — unrelated gen_tps_expiry_boundary_held_then_unavailable; targeted retry passed |
cargo build --workspace |
Pass |
cargo xtask e2e (local WSL) |
Fail — six pre-existing/environment-specific scenarios; both new TheRock scenarios passed |
python3 scripts/smoke_local.py --skip-build |
Pass |
GitHub prek (lint / hygiene) |
Fail — same formatter diff |
GitHub E2E tests (Strix Halo, Windows) |
Fail — new tarball scenario lacks OS gate |
| GitHub clippy / Rust analysis / CodeQL | Pass |
Files Reviewed
apps/rocm/src/comfyui.rs— Modifiedapps/rocm/src/main.rs— Modifiedapps/rocm/src/storage.rs— Modifiedapps/rocm/src/therock.rs— Modifiedcrates/rocm-core/src/lib.rs— Modifiedtests/e2e-cucumber/features/therock_next_generation.feature— Addedtests/e2e-cucumber/tests/e2e.rs— Modifiedtests/e2e-cucumber/tests/e2e/therock_steps.rs— Added
|
Thanks for the review! Pushed 1a3eeb0 addressing all four findings:
|
volen-silo
left a comment
volen-silo
left a comment
There was a problem hiding this comment.
Three blocking issues:
-
crates/rocm-core/src/lib.rs:3335—detect_host_therock_family()has no callers or tests after its logic was inlined. Please remove the dead helper. -
apps/rocm/src/therock.rs:1305— a pinned ROCm 10+ version with a grouped family (for example,--version 10.0.0 --family gfx120X-all) produces no candidates or network requests, then renders an empty error bullet while recommending the same unsupported family. Please fail early with guidance requiring a specific architecture such asgfx1200. -
apps/rocm/src/therock.rs:1404-1441— release tarball resolution always tries the new-generation repository first. Once an artifact appears there, an unchanged--format tarballcommand can silently jump from legacy ROCm to ROCm 10, with no pinning or opt-out. This contradicts the PR description's compatibility claim. Please gate the new tarball source or explicitly define and document the major-version transition.
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Most of this change reads well — the device-extras spec building, the -tests- sibling filtering, the per-generation cache-key namespacing, and confining the gfx103X-all filename quirk to the tarball call site rather than the shared normalizer are all nicely scoped. The new family alias round-trips correctly, and raw_arch being #[serde(default)] means existing manifests still deserialize.
My concern is with the framing: the description says the change is additive, with older ROCm versions installing byte-for-byte unchanged. As far as I can tell that holds only for an explicitly pinned --version, which is the case most users don't hit. On the default path therock_tarball_base_candidates(Release) returns [Next, Legacy] unconditionally and the resolver returns the first candidate that succeeds, so Legacy is reached only when Next errors. The day stable.repo.amd.com carries an artifact for a family, an ordinary rocm install sdk --format tarball starts returning ROCm 10 instead of 7.x — and since install_sdk rejects version_selector for tarball, there's no way for a user to pin back.
I don't think that's necessarily the wrong destination, but it's a behavior change on a major-version boundary rather than an additive one, and it currently ships with no opt-out. Worth being deliberate about before this lands.
The rest is smaller: one error path renders a blank bullet list, the new version-parse filter also narrows legacy selection, and one new match arm looks like it's already covered by an existing one.
1a3eeb0 to
ee01da7
Compare
|
Thanks both for the review. Pushed ee01da7 addressing all outstanding findings. @volen-silo, your three blocking points:
@rominf, on the framing concern in your review body: agreed the additive claim didn't hold on the default tarball path, and #2/#3 above are the same underlying fix your inline comments also flagged. The remaining three (parse_version filter scoped to Full commit message on ee01da7 has the itemized list. Verified locally: |
feedback addressed
|
Resolver ownership update: this PR/EAI-8431 is the ROCm 10 Next-layout extension. It will be rebased on #308, which owns current release/nightly aggregate source selection, shared provenance, and runtime-composition behavior. #272's #271 regression behavior will transfer into #308 and be verified there before #272 closes as superseded. #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. While stacked, this PR will remain draft and explicitly depend on #308. Related: #272, #308, #314. Tracking: EAI-8268, EAI-8431, EAI-7956. |
|
UX/behavior notes beyond what's already fixed per michaelroy-amd/volen-silo/rominf's threads:
Minor: the static |
ee01da7 to
4cf7cef
Compare
|
Rebased and rebuilt on merged #308/current |
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 4cf7cef
Summary
Adds a second TheRock source layout (SourceLayout::Next) for ROCm 10 — separate pip index and tarball catalog, opt-in only via an explicit --version >= 10 pin plus an exact GFX arch — with env-overridable artifact bases behind a trust gate, layout provenance recorded in runtime manifests, and 5 new Gherkin scenarios. Verdict: Needs work — the production logic is sound, but three added tests do not discriminate, including the one guarding the new security gate. Verified: I traced every added test against the revert question — 11 of 14 are genuinely discriminating, but the ROCM_CLI_THEROCK_ALLOW_BASE_OVERRIDE gate can be deleted outright with the whole suite still green, next_tarball_selection_skips_the_tests_sibling passes identically with SourceLayout::Canonical, and a previously-reported gfx103X-all dead-arm finding is half-fixed — the dead match arm is gone at this head (base already handled it via starts_with("gfx103"), and no such arm exists now), but the vacuous test it accompanied was kept and still passes verbatim on unpatched main; on the red check, every static gate (hawkeye, cargo fmt --check -p rocm, clippy arg counts, feature-naming drift guard, Sphinx include anchors, TPN, smoke scripts) traces clean and the e2e harness APIs and asserted output tokens all exist, so the failure is most plausibly the E2E tests job — the only gate that exercises the new scenarios at runtime, and a red result there is expected-ish for a PR whose whole payload is new e2e dispatch, not dismissable as flake. Blocking: 3 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
-
apps/rocm/src/therock.rs:209+apps/rocm/src/therock.rs:5006— the override trust gate is untested end-to-end.env_override_baseis the only thing that consultstherock_base_override_allowed(), and nothing exercises it:base_overrides_are_inert_without_the_explicit_opt_intests the pure helperselect_base_overridedirectly,next_layout_resolves_its_own_basesonly sees defaults because CI has no vars set, and all five e2e scenarios callallow_base_overrides(world). Change line 209 toselect_base_override(true, ...)— deleting the gate thatdocs/release-trust.mdsays prevents a stray shell var from redirecting a real install to an untrusted host — and every test in this PR still passes. Fix: add a sixth scenario using the same fixtures but omittingallow_base_overrides, asserting the preview resolves the hardcoded default artifact base and never mentions the loopback fixture. That is one Given + one Then and it is the only thing that makes the gate real. -
apps/rocm/src/therock.rs:5065—next_tarball_selection_skips_the_tests_siblingis vacuous with respect to this PR. The-tests-sibling is excluded by the pre-existing release-channelis_stable_runtime_versionfilter (tests-10.0.0failsparse_version), not by anything new; passSourceLayout::Canonicalinstead ofNextand the assertions are byte-identical. It only fails on revert because the signature changed. Worse, it hides a real gap: nothing tests thatselect_tarball_candidateusestarball_family_token—next_tarball_token_renames_only_the_gfx103x_family(line 5127) tests the helper in isolation. Fix: make this test discriminating by selecting forfamily = "gfx103X-dgpu"underSourceLayout::Nextagainst a file namedtherock-dist-<platform>-gfx103X-all-10.0.0.tar.gz, and assert the same call underCanonicalreturnsNone. Only the new rename path can make that pass. -
crates/rocm-core/src/lib.rs:9094—normalize_therock_family_aliases_gfx103x_all_to_canonical_dgpu_labelpasses on unpatched main. Base already hadvalue if value.starts_with("gfx103") => Some("gfx103X-dgpu"), which catches bothgfx103X-allandgfx1030; this PR changes nothing in that path. (The companion test at line 9082 forgfx1250is discriminating — base returnsNone— so keep it.) Fix: either delete this test, or make it assert the invariant the PR actually introduces by round-tripping through the new helper:normalize_therock_family(tarball_family_token(SourceLayout::Next, "gfx103X-dgpu")) == Some("gfx103X-dgpu"). That fails the moment the rename and the normalizer drift apart, which is the thing worth protecting.
Non-blocking
apps/rocm/src/therock.rs:282—select_source_layoutre-destructures the selector and re-parses the version with fallbacks thatnext_layout_requestedalready proved unreachable; theelse { Ok(Canonical) }arm and themap_or(THEROCK_NEXT_MIN_MAJOR, ...)default are dead. Havenext_layout_requestedreturn the parsed major and thread it through.apps/rocm/src/therock.rs:4656and:4709—canonical_wheel_resolution_hint/family_resolution_hintunconditionally suggest retrying with--channel nightly, butselect_source_layouthard-bails on any non-release channel once a ROCm 10 pin is present; a failing next-layout install therefore recommends a retry that cannot work. Suppress the nightly suggestion when the layout isNext.apps/rocm/src/therock.rs:5635,:5674,:6737—canonical_channels_use_only_their_aggregate_streamsand the two provenance tests now callresolve_source, which reads process environment, without takingPROCESS_ENV_TEST_LOCK(whichnext_layout_resolves_its_own_basesdoes take). Not red today since no unit test sets those vars, butcanonical_sourceused to be aconst fnand this quietly turns three tests into env-sensitive ones.apps/rocm/src/therock.rs:320,:452,:1520,:1952,:2110— user-facing strings say "canonical" unconditionally, so a next-layout dry run prints acanonical_source:key pointing at the next-layout host, and next-layout failures blame "the canonical source". The e2e steps assert on that literal key, so it looks deliberate — worth confirming rather than inheriting.- Commit message is a bare subject line for a 1369-line change that adds a network trust boundary and a new artifact host; a body explaining the opt-in rule and the override gate would help the next reader. No prompt-injection or internal-reference leakage found in the diff.
|
Final handoff: PR #308 merged to |
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 92d2f76
Summary
Adds a ROCm 10 ("next") source layout to the TheRock installer — a pinned-version + exact-arch opt-in path with its own artifact bases — plus, in the latest three commits, size bounds on remote reads and filename validation on the tarball catalog. Verdict: Needs work — the production code is sound and the new bounding work is a real improvement, but all three prior blocking findings are test-coverage gaps and none of them was addressed. Verified: prior finding 1 (override trust gate untested e2e) still open — the new therock-next-06 sets no base-override env at all, so deleting the gate still leaves it green, and it is @requires-gpu @nightly so it never runs in blocking CI; prior finding 2 (next_tarball_selection_skips_the_tests_sibling vacuous) still open, re-derived independently — tarball_family_token only special-cases gfx103X-dgpu, and the test uses gfx90a, so Canonical and Next produce an identical prefix; prior finding 3 (family-alias test) still open — no family-alias test was touched by these commits (I could not re-identify the exact test from the diff alone, so that one carries forward on the earlier round's evidence, not mine); on the revert question, the new tarball_catalog_rejects_paths_and_control_characters fails if the whole change is reverted but passes if only the call site at therock.rs:4611 is dropped, and every other new assertion (tarball_family_token, select_source_layout, resolve_source, family_override_raw_arch) exercises functions that do not exist on the base and would fail on revert; relied on CI (20 passing, 0 failing) rather than running the suite, per this run's verification policy. Blocking: 3 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
-
tests/e2e-cucumber/features/therock_next_generation.feature:82/apps/rocm/src/therock.rs:201— the artifact-base trust gate is still untested end to end.therock-next-06's onlyGiven(a machine with no CLI-managed runtimes,tests/e2e-cucumber/tests/e2e/runtime_steps.rs:9) never touchesworld.command_env, andrun_rocm_with_scenario_envdrains an empty map — so the child process sees neitherROCM_CLI_THEROCK_ALLOW_BASE_OVERRIDEnor anyROCM_CLI_THEROCK_*_BASE. With no override value present,select_base_override(allowed, None, default)returnsdefaultfor either value ofallowed, so removingtherock_base_override_allowed()entirely leaves this scenario passing. The unit test attherock.rs:5032still calls the pure helper directly, and the five fixture scenarios all opt in. On top of that, the scenario is tagged@requires-gpu @nightly, so it is skipped both by the GitHub-hosted blockinge2ejob and by the nightly gate attests/e2e-cucumber/tests/e2e.rs:1114unlessE2E_INCLUDE_NIGHTLY=1on the non-blocking self-hosted lane — it adds zero merge-blocking coverage. Fix: add a hermetic scenario that starts the fixture server and setsROCM_CLI_THEROCK_NEXT_PIP_BASE(or..._RELEASE_PIP_BASE) without callingallow_base_overrides, then asserts the CLI reports the hardcoded defaultcanonical_source:and never contacts the loopback server. That assertion flips the moment the gate is deleted. -
apps/rocm/src/therock.rs:5108—next_tarball_selection_skips_the_tests_siblingis still vacuous with respect to this PR.select_tarball_candidateconsultslayoutin exactly one place,tarball_family_token(layout, family)(therock.rs:2183), and that function (therock.rs:2156) only rewritesgfx103X-dgpu. The test passes"gfx90a", soSourceLayout::Canonicalyields a byte-identical prefix and the-tests-sibling is excluded by the pre-existingTheRockChannel::Releasestable-version filter attherock.rs:2193— nothing next-specific is proven. Fix: either drive the case through agfx103X-dgpufamily (where the layout actually changes the token) or assert directly thatCanonicalandNextdiverge, and rename the test to say what it checks. -
Carried forward from the earlier round, unaddressed: the family-alias normalisation test that passed on unpatched
mainbecause an existing prefix match already covered the case.git diff 4cf7cefb..HEADtouches no family-alias test, so nothing about that finding changed. Flagging honestly: I could not re-identify which test this referred to from the diff (tarball_family_token,is_raw_gfx_arch_code,family_override_raw_archandraw_arch_agreeing_with_familyare all absent from the base, so the tests covering them are not vacuous) — please cross-check against the earlier round before dismissing it.
Non-blocking
apps/rocm/src/therock.rs:4611— the newvalidate_tarball_file_namehas no integration coverage:tarball_catalog_rejects_paths_and_control_characters(therock.rs:5049) calls the helper directly, so dropping the call site fromparse_tarball_index_htmlwhile keeping the helper leaves the suite green. Add one test that feedsparse_tarball_index_htmla catalog containing../escape.tar.gzand asserts it errors.apps/rocm/src/therock.rs:4616— the validator rejects/,\, non-Normalcomponents and control characters, but not:(an NTFS alternate-data-stream reference such asa.tar.gz:evilparses as a singleNormalcomponent on every platform), Windows reserved stems (CON.tar.gz), or trailing dots/spaces. Currently unreachable because tarball installs are refused on Windows (therock.rs:1003), so this is defence-in-depth rather than a live hole — cheap to close now.tests/e2e-cucumber/tests/e2e/therock_steps.rs:501—discover_latest_next_rocm_versionfetches a live index withreqwest::get(no timeout) andpanic!s on any network error or unrecognised page shape, so an upstream outage or an index-format change turns the nightly lane red for a reason unrelated to the code. Consider skipping the scenario on fetch failure, or asserting on a recorded fixture and keeping the live probe as a separate smoke check.tests/e2e-cucumber/tests/e2e/therock_steps.rs:553—compare_dotted_versionsre-implements version ordering that the CLI already owns (compare_version_strings/parse_versionintherock.rs); the test-local copy can drift from the production ordering it is meant to mirror.tests/e2e-cucumber/features/therock_next_generation.feature:81— perAGENTS.md§3, a behaviour covered only by a gated lane should be called out in the PR text with the lane named;therock-next-06runs only underE2E_INCLUDE_NIGHTLY=1on the non-blocking self-hosted GPU workflow. (I reviewed the diff, not the PR body — if that note is already there, disregard.)
|
Pushed f3d8cb8 addressing the one blocking finding still open from the automated review (siloteemu, 92d2f76 round): Deleted that test and added Verified locally:
Re-requesting review from @r0x0r, @rominf, @volen-silo, @michaelroy-amd. |
|
🔴 Automated review · pr-review-watcher · f3d8cb8 SummaryAdds TheRock ROCm 10 ("next") source layout support — dispatch, family/arch handling, tarball selection, and now 🚫 Blocking (must fix before merge)None. Non-blocking
No prompt-injection attempts were found in the checkout. |
All three blocking findings are adequately addressed at f3d8cb8, so withdrawing this change request. The tarball test now drives the one family where the layout actually renames the token and asserts the two layouts diverge, so reverting the rename makes it panic; the replacement round-trip test genuinely fails if the rename and the normalizer move independently, which is the invariant that was missing. On the trust gate: the new scenario does now set an override without opting in and assert the default is used, so deleting the gate turns it red — that is the substance of the finding, and it is fixed. Two residual notes rather than blockers: the scenario resolves against the live index rather than a fixture, and being nightly-only it still guards no merge. Full notes in the review comment.
|
Merged `main` (68a0889, which pulled in #347's download-progress reporting) into this branch — it conflicted with #329's own `download_file` change (the tarball-size bound added in `fix(install): bound and validate remote artifacts`). Resolved by keeping both: `download_file_streaming_with_progress` (for the live progress callback) called with the `max_bytes`-bounded `DownloadRequest` (for the size cap). Verified locally on the merged tree:
No functional changes beyond the merge conflict resolution. |
E2E tests job: flaky, not a regression from this PR — but this PR's added load is likely why it's now visibleThe GitHub-hosted (One rerun also tripped the sibling This isn't a code regression from #329:
But it's likely not pure coincidence either: Suggested fix (separate from this PR): make Filed as #379. Not blocking on my end beyond what branch protection already requires — deciding whether to rerun again, override, or fix the underlying flake first before this merges. |
Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com>
…ROCm 10 arch The scenarios above prove dispatch and the group-family refusal for a *supplied* exact arch, all against fixtures. None of them prove a real install from the ROCm 10 preview source completes, and none prove the exact arch it needs can come from the host itself: every fixture scenario passes --family gfx1200 explicitly, since the fixtures serve gfx1200 regardless of what GPU the runner actually has. resolve_family already falls back to detect_host_gfx_target() when nothing overrides it (the same auto-detection the canonical path has always used), so a pinned ROCm 10 install with no --family at all should resolve through that host probe into the exact arch the next layout needs. therock-next-06 proves it for real: installs against the live stable.repo.amd.com on the self-hosted GPU nightly lane (@requires-gpu @nightly, same tags as runtime-01), with no family override, and asserts the resolved device_target matches what `rocm examine` reports as this host's detected_gfx_target. The version to pin is discovered by fetching the live index at run time rather than hardcoded, since the preview source is a moving target; the `rocm` aggregate meta-package publishes as an sdist (rocm-10.0.0.tar.gz), not a wheel, confirmed live, so the discovery step checks both suffixes the same way the CLI's own parse_simple_index_version_candidate does. Jira: EAI-8431 Signed-off-by: Juho Vainio <juho.vainio@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com>
normalize_therock_family_aliases_gfx103x_all_to_canonical_dgpu_label
passed on unpatched main: the pre-existing `starts_with("gfx103")`
arm already maps both gfx103X-all and gfx1030 to gfx103X-dgpu, so it
proved nothing this PR introduced. Delete it and add a round-trip
test where the actual new behavior lives: tarball_family_token's
next-layout rename must map back through normalize_therock_family to
gfx103X-dgpu, which only holds if the rename and the normalizer agree.
Addresses the last open blocking finding from the automated review.
Signed-off-by: Juho Vainio <juho.vainio@amd.com>
69ce7ab to
59998d3
Compare
There was a problem hiding this comment.
I went through this at 59998d3b — the generation-aware dispatch itself reads well, and the additions since the last round (the metadata size bounds, the catalog filename validation, the discriminating gfx103X alias test, the hermetic-vs-live split in the scenarios) are real improvements. Three things look like blockers to me, all of them behaviour rather than style, plus two smaller ones. Inline below.
The short version:
- A pinned nightly ROCm 10.x install now fails outright.
parse_version("10.1.0a20260822")gives major 10, so nightly alphas tripnext_layout_requestedand get refused with a suggestion that also can't work.mainhas a test (nightly_accepts_future_prerelease_major_without_cli_changes) that exists specifically to say the canonical nightly stream serves ROCm 10 without CLI changes — it stays green only because it calls the inner helper below the new dispatch. - Updating a ROCm 10 wheel runtime always fails on grouped families — including
gfx125X-dcgpu, the family this PR adds for gfx1250. The manifest carries the family, not the arch, so the update path can never produce one. - The 2-second startup update budget is silently bypassed on the ROCm 10 path:
uv pip compileis spawned with no wall-clock bound from a code path whose whole point is to stay under two seconds.
Things I chased that turned out to be fine, for what it's worth: the metadata size cap only sits on index/catalog/signature reads, never on artifact downloads; --index-url without PyPI matches what the install step already does, so the uv pip compile invocation isn't inconsistent with it; and the new -tests- sibling handling plus the gfx103X rename are correctly confined to the catalog call site rather than leaking into normalize_therock_family.
On the red E2E tests check: it isn't yours. All five hermetic therock-next scenarios pass; the two failures are dash-gen-tps-held-after-scrape-failure and dash-gen-tps-expiry-boundary, wall-clock-window assertions in dash-tui that this PR doesn't touch. But they pass on main and have now failed on two separate commits of this branch, so I don't think it's pure noise either — my guess is contention, since scenario 02 now runs a real uv pip compile subprocess inside the suite and those two scenarios assert on a clamp(3 x instance_tick, 6s, 30s) window. Worth a re-run to confirm before assuming it's unrelated.
| let major = parse_version(version).map_or(THEROCK_NEXT_MIN_MAJOR, |parsed| parsed.major); | ||
| if !matches!(channel, TheRockChannel::Release) { | ||
| bail!( | ||
| "ROCm {major} and newer is published only on the release channel; re-run `rocm install sdk --channel release --version {version}`" |
There was a problem hiding this comment.
A pinned nightly ROCm 10 install is refused here, and the suggested fix can't work either.
parse_version("10.1.0a20260822") returns major 10, so next_layout_requested (:269) is true for nightly alpha builds, and this bail fires before anything else. The user is told to re-run with --channel release --version 10.1.0a20260822, which the release stable-version filter then rejects — so they get two failures in a row with no working path.
main already models ROCm 10.x on the canonical nightly stream, and has a test named for exactly that intent: nightly_accepts_future_prerelease_major_without_cli_changes (now at :5961). It still passes because it calls select_matching_pip_package_versions directly, underneath this dispatch — so the flow it documents is broken while the test that encodes it stays green. That also reads against the SourceLayout doc comment at :63-70 ("a canonical install resolves exactly the URLs, device target, and package specs it resolved before the layout existed"), and against THEROCK_NEXT_MIN_MAJOR's "published only in the next layout".
Would gating on a stable pin (VersionStage::Stable) rather than any major >= 10 work? That keeps nightly 10.x pins on the canonical nightly stream and still makes --version 10.0.0 the ROCm 10 opt-in. Either way a select_source_layout case for nightly-channel-with-a-10.x-alpha-pin would catch it.
| // already carried a validated exact arch, and that arch — not a second probe | ||
| // of whatever card happens to be plugged in — is what the pin asked to | ||
| // install for. | ||
| let detected_target = match layout { |
There was a problem hiding this comment.
Updating a ROCm 10 wheel runtime looks like it can never succeed when the family is a group label.
The chain: resolve_latest_for_manifest passes Some(manifest.family) as the family override (:1254). resolve_family early-returns on that branch (:2284-2292) with raw_arch: family_override_raw_arch(value) — and this PR's own test asserts family_override_raw_arch("gfx120X-all") == None. So for Next, detected_target here is None, AggregateDeviceTarget::resolve returns Undetermined, and resolve_pip_runtime_from_index bails at :2031 with "no AMD GPU target was detected on this host" — on the very machine that has the GPU, because this arm deliberately doesn't probe the host.
That covers gfx120X-all, gfx103X-dgpu, gfx110X-dgpu and gfx125X-dcgpu — the last being the family this PR adds for gfx1250, so the flagship ROCm 10 arch installs fine and then can never be updated. Only families that are themselves exact arch codes (gfx942, gfx950, gfx90a, ...) survive, which is likely why the live MI300X scenario wouldn't catch it.
The data to fix it is already recorded: wheel_composition.rocm_sdk_target holds gfx1200. It's just consulted at :1266, after the ? on this call has already failed. Recovering the arch from the composition (or recording raw_arch in InstalledRuntimeManifest) before resolving would do it. Worth a regression test for updating a next-v1 manifest whose family is gfx120X-all — nothing currently exercises resolve_latest_for_manifest with a next-layout manifest at all.
| }) | ||
| } | ||
|
|
||
| fn resolve_published_pip_package_versions( |
There was a problem hiding this comment.
This spawns uv pip compile and wait_with_output()s on it with no wall-clock bound, and resolve_pip_runtime_from_index never passes download_timeout_secs down here — so the timeout the caller asked for is silently dropped on the ROCm 10 path.
That matters because refresh_startup_update_check_quietly runs before nearly every command (apps/rocm/src/main.rs:1750) with STARTUP_UPDATE_CHECK_TIMEOUT_SECS = 2, and it reaches this function for any active ROCm 10 wheel runtime. UV_HTTP_TIMEOUT defaults to 600s per request (crates/rocm-core/src/uv.rs:27), and ensure_uv_binary just above may download uv first with no bound at all. So once every 12 hours, a plain rocm examine or rocm dash can block for minutes on a network resolve that was budgeted at two seconds.
Either thread download_timeout_secs through and enforce it on the child (wait with a deadline, then kill), or skip the uv metadata resolve entirely when a timeout budget is supplied and let freshness fall back to the version comparison.
| # reaches the live default index, so it runs on the scheduled network lane. | ||
| @id:therock-next-06-base-override-requires-opt-in @nightly | ||
| Scenario: therock-next-06 - A ROCm 10 source override is ignored without explicit trust | ||
| Given an untrusted ROCm 10 pip base override |
There was a problem hiding this comment.
AGENTS.md section 3 asks that a scenario which can only run on a gated lane be called out in the PR text with the lane named — this one is @nightly and therock-next-07 is @requires-gpu @nightly, and neither is mentioned in the description.
That's not just bookkeeping here: this scenario is the only end-to-end proof of ROCM_CLI_THEROCK_ALLOW_BASE_OVERRIDE, which docs/release-trust.md describes as the thing standing between a stray shell variable and a real install being redirected to an untrusted host. Because it's nightly-only, that trust boundary currently has zero merge-blocking coverage — the unit test at :5301 exercises the pure helper, and the other five scenarios all opt in.
A hermetic variant would fix both: same loopback fixture, set ROCM_CLI_THEROCK_NEXT_PIP_BASE without calling allow_base_overrides, assert the default base is resolved and the fixture never appears. That runs on the blocking lane and flips the moment the gate is removed.
(Minor, while you're in the description: the Verification section still says 5 scenarios and 112 unit tests — there are 7 scenarios now.)
| @id:therock-next-06-base-override-requires-opt-in @nightly | ||
| Scenario: therock-next-06 - A ROCm 10 source override is ignored without explicit trust | ||
| Given an untrusted ROCm 10 pip base override | ||
| When the user previews a wheel SDK install for arch gfx1200 pinned to ROCm 10.0.0 |
There was a problem hiding this comment.
This scenario resolves the live default index but hardcodes --version 10.0.0, and the step goes through preview_ok, which asserts rc == 0. So it needs stable.repo.amd.com to keep publishing exactly 10.0.0, with a jointly-installable gfx1200 stack, indefinitely.
therock-next-07 makes the opposite call for the same reason it should apply here — its comment says the preview source is a moving target, and discover_latest_next_rocm_version looks the version up at run time. As written this one goes red on the nightly lane the first time 10.0.0 is superseded or withdrawn, for a reason that has nothing to do with the trust gate it's meant to prove.
Reusing discover_latest_next_rocm_version would do it — though if you take the hermetic-scenario suggestion on the Given above, the problem disappears along with the live fetch.
… support - A pinned nightly ROCm 10.x prerelease (e.g. `10.1.0a20260822`) was refused outright: `next_layout_requested` gated on major >= 10 alone, so a nightly alpha pin tripped the next-layout dispatch and was told to retry on the release channel, which the stable-version filter would then also reject. Gate on a *stable* pin instead, matching what the canonical nightly stream already promises for future majors. - Updating a ROCm 10 wheel runtime whose family is a group label (every family this PR adds, including gfx125X-dcgpu) always failed: the manifest only records the family, and resolving with it as the override deliberately produces no raw arch, so the next layout's device target resolution came up undetermined on the very host the runtime is installed on. Recover the installed arch from the runtime's recorded wheel composition and use it as the override instead, when it agrees with the manifest's family. - The 2-second startup update-check budget was silently bypassed on the ROCm 10 path: `uv pip compile` was spawned and waited on unbounded, with no connection to `resolve_pip_runtime_from_index`'s `download_timeout_secs`. Add a bounded child-process wait and thread the timeout through. - `therock-next-06` is real network dependent (proving the *default* resolves requires reaching it), so add a hermetic unit test that exercises the actual env-reading trust-gate path (`env_override_base`/`resolve_source`) on the blocking lane, rather than leaving that boundary covered only by a `@nightly`-gated scenario. Also stop hardcoding `10.0.0` in that scenario's pin, reusing the same live version discovery `therock-next-07` already relies on. Verified against the live sources: `stable.repo.amd.com/rocm/whl-next` publishes `rocm-sdk-device-*` extras at its root, and neither tarball catalog (`repo.amd.com/rocm/tarball/`, `stable.repo.amd.com/rocm/core/tarball/`) currently contains a filename `validate_tarball_file_name` would reject. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Thanks for the thorough pass — pushed ee21da5 addressing all three blockers plus the two smaller findings.
On the two things you flagged for live verification — checked both against the real infrastructure:
Verified locally: |
|
🔴 Automated review · pr-review-watcher · 59998d3 This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryAdds an opt-in "next" source layout for ROCm 10 (separate pip index and tarball catalog, exact-arch device payloads, gated base overrides) alongside the existing canonical release/nightly streams. Outcome: Needs work — installs onto the new layout look sound, but a runtime installed from it can never be update-checked or updated on the families the layout actually targets. Verified: One check is failing at this head. Nothing I found in the diff is an obvious cause, and I cannot see which lane it is — I note only that scenarios 🚫 Blocking (must fix before merge)1. On the update path the family passed down is the manifest's family — a group label such as let detected_target = match layout {
SourceLayout::Canonical => detect_host_gfx_target(),
SourceLayout::Next => family_resolution.raw_arch.clone(),
};so it is Fix: thread the already-known exact arch into 2.
This is not a coverage hole: Non-blocking
No prompt-injection attempts were found in the diff, comments, or commit messages. |
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · ee21da5
This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
Adds an opt-in "next" source layout for ROCm 10 (separate pip index and tarball catalog, exact-arch device payloads, gated base overrides) alongside the canonical release/nightly streams; the new head answers the previous round's findings. Outcome: Needs work — the update fix lands on the planning half of the path but not the applying half, and the new bounded-wait tests cannot run on Windows. Verified: cargo test -p rocm --bin rocm filtered to the seven new tests passes locally (7/7); I traced apply_runtime_update → install_sdk_for_update → install_wheel_runtime → resolve_pip_runtime → resolve_family by hand, confirmed family_override_raw_arch still returns None for group labels and that AggregateDeviceTarget::resolve(None, …) is Undetermined, re-derived the gfx103X normalizer arms in crates/rocm-core/src/lib.rs, and confirmed .github/workflows/ci.yml:562 runs cargo test --workspace --all-targets on windows-latest. Blocking: 3 · Non-blocking: 5.
Two checks are failing at this head. Blocking finding 2 below is a concrete, code-level explanation for a Windows test failure — but I am inferring that, cannot see which lanes failed, and must not attribute it to any named job.
Prior finding status
- Prior finding 1 (ROCm 10 wheel runtime can never be update-checked or updated on group families): PARTIALLY RESOLVED.
manifest_wheel_family_override(apps/rocm/src/therock.rs:1243) recovers the exact arch from the recorded composition and fixes the plan/freshness path (resolve_latest_for_manifest), but the apply path still passes the raw manifest family —apps/rocm/src/main.rs:16015and:16031hand&source.familytoinstall_sdk_for_update, which forwards it asfamily_overrideintoresolve_pip_runtime, so the next layout still resolves an undetermined device target and bails. Detail in Blocking 1. - Prior finding 2 (the round-trip test still passes with the production change reverted): STILL STANDING.
next_tarball_family_token_round_trips_through_normalize_therock_family(apps/rocm/src/therock.rs:5551) is byte-for-byte unchanged at this head and the reasoning still holds:crates/rocm-core/src/lib.rs:4002and:4009both yieldgfx103X-dgpu, so the assertion cannot distinguish the rename from its absence.
Neither prior finding was wrong; one is half-fixed, one is untouched.
🚫 Blocking (must fix before merge)
1. apps/rocm/src/main.rs:16015 (and :16031), with apps/rocm/src/therock.rs:1995 / :2055 — the fix for prior finding 1 covers planning but not applying, so rocm update on a ROCm 10 wheel runtime still fails, dry-run included.
manifest_wheel_family_override is applied only at apps/rocm/src/therock.rs:1274, inside resolve_latest_for_manifest. The apply path does not go through that function. apply_runtime_update calls therock::install_sdk_for_update(paths, &source.channel, &source.format, &source.family, plan.device_target.as_deref(), …) — the unrecovered manifest family — at both apps/rocm/src/main.rs:16015 (dry-run) and :16031 (real apply). install_sdk_for_update (therock.rs:992) puts that string into InstallSourceOverride.family, and install_wheel_runtime passes it to resolve_pip_runtime as family_override. For a group label such as gfx125X-dcgpu or gfx120X-all, family_override_raw_arch (therock.rs:2292) returns None because is_raw_gfx_arch_code rejects anything but a bare arch code, so family_resolution.raw_arch is None. At therock.rs:1995 the next layout takes its device target solely from that field, AggregateDeviceTarget::resolve(None, …) returns Undetermined (therock.rs:437), and the next-layout branch hard-bails at therock.rs:2055 with "cannot resolve ROCm X package metadata without an exact device target".
The exact arch is available at both call sites — it is plan.device_target, passed in the very next argument — but install_wheel_runtime applies device_target_override only after resolve_pip_runtime returns (therock.rs:1541), which is too late: the next layout needs the exact target before it can query package metadata at all. Every family this PR adds is a group label, so this is the whole feature's update path, not an edge case.
Fix: apply the same recovery at the apply boundary — either pass manifest_wheel_family_override(&source) from main.rs instead of source.family, or (cleaner, since the caller already has it) have install_wheel_runtime seed the family override from device_target_override when it is Some and the layout is Next. Add a test at the install_sdk_for_update level, not just at manifest_wheel_family_override — the three new unit tests exercise the helper in isolation and therefore cannot detect that the helper is not wired into this path.
On reviewability (this is why the gap is easy to miss): the doc comment at therock.rs:1236-1242 describes the problem as "the whole resolve would bail" and reads as a statement about updating in general, while the fix is scoped to one of two resolve entry points. A competent reader will make the same wrong inference next time. Cheap prevention: say in that comment which path it covers, and add a one-line comment at main.rs:16015 stating that the family handed to install_sdk_for_update must already carry an exact arch for next-layout runtimes.
2. apps/rocm/src/therock.rs:6617 — the Windows spawn_test_sleep does not sleep, so both new wait_with_output_bounded tests fail on the Windows lane.
Command::new("cmd")
.args(["/C", "timeout", "/t", &seconds.to_string(), "/nobreak"])
.stdin(Stdio::null())Windows timeout.exe requires a console on stdin; when stdin is redirected — and Stdio::null() is a redirection to NUL — it prints "ERROR: Input redirection is not supported, exiting the process immediately" and exits at once, with a non-zero status, regardless of /nobreak. Consequences, with .github/workflows/ci.yml:562 running cargo test --workspace --all-targets on windows-latest:
wait_with_output_bounded_kills_a_slow_child_at_the_deadline(:6630): the "30s" child has already exited when the poll loop runs,try_waitreturnsSome,wait_with_output_boundedreturnsOk, and.expect_err(…)panics.wait_with_output_bounded_waits_unbounded_when_no_timeout_is_given(:6647): the "1s" child exits with the redirection error, soassert!(output.status.success())fails.
I am inferring that this is behind a failing check; I cannot see which lanes failed and am not attributing it to a named job. Either way the helper is wrong on Windows and the tests prove nothing there. Fix: use a child that genuinely blocks without a console — e.g. powershell -NoProfile -Command "Start-Sleep -Seconds N", or the same cmd trick without stdin redirection — or #[cfg(not(windows))]-gate both tests and say why in a comment.
3. apps/rocm/src/therock.rs:5551 — prior finding 2 is untouched: the test that was added to close the earlier round still passes with the production change reverted.
next_tarball_family_token_round_trips_through_normalize_therock_family asserts normalize_therock_family(tarball_family_token(SourceLayout::Next, "gfx103X-dgpu")) == Some("gfx103X-dgpu"). With the rename (therock.rs:2217) the inner call yields "gfx103X-all", which lowercases to gfx103x-all and is caught by value if value.starts_with("gfx103") at crates/rocm-core/src/lib.rs:4009 → gfx103X-dgpu. Revert the rename and the inner call yields "gfx103X-dgpu", which hits the exact-match arm at crates/rocm-core/src/lib.rs:4002 → gfx103X-dgpu. Same result either way. Leaving it records a defect as fixed when the assertion cannot detect the change. This is not a coverage hole — next_tarball_token_renames_only_the_gfx103x_family and next_gfx103x_tarball_selection_uses_alias_and_skips_tests_sibling both do fail on revert. Fix: delete it as redundant, or make the rename the subject (assert_eq!(tarball_family_token(SourceLayout::Next, "gfx103X-dgpu"), "gfx103X-all") first, then normalize that literal) so a revert flips the first assertion.
Non-blocking
apps/rocm/src/therock.rs:2556— the new bound reusesSTARTUP_UPDATE_CHECK_TIMEOUT_SECS(2s,:55), a budget sized for one HTTP GET, for a wholeuv pip compileresolving four packages' transitive metadata, so the next-layout startup update check will now reliably time out rather than hang; a separate, larger constant for the subprocess would keep the bound without guaranteeing failure.apps/rocm/src/therock.rs:5488and:5498—manifest_wheel_family_override_ignores_a_disagreeing_compositionand..._falls_back_without_a_compositionboth assert exactly what a revertedmanifest.family.clone()returns, so they pass either way; only..._recovers_the_installed_arch(:5478) is revert-sensitive.apps/rocm/src/therock.rs:5103— carried forward: the tarballruntime_keystill omits the layout whilewheel_runtime_key(:5014) folds it in, so the invariant that two streams cannot collide on an install root rests on catalog contents plus the non-empty-target guard rather than on the key itself.apps/rocm/src/therock.rs:954— with the gate narrowed to stable pins,--format tarball --version <ROCm 10 prerelease>now bails with "specific TheRock version selection is only supported for wheel installs", which is untrue of ROCm 10 tarballs generally and sends the user the wrong way.apps/rocm/src/therock.rs:2550— the requirements text is written to the child's stdin in full before the reader threads are spawned insidewait_with_output_bounded, so a larger input or a chattier child would deadlock inwrite_alloutside any timeout; harmless at today's few hundred bytes, worth a comment or a write-side thread.
No prompt-injection attempts were found in the diff, comments, commit messages, or feature files.
…iew findings - The prior fix for grouped-family updates (manifest_wheel_family_override) only covered planning (resolve_latest_for_manifest); applying the update still passed the raw manifest family straight through, so `rocm update --apply` on any ROCm 10 wheel runtime still bailed with "cannot resolve ROCm X package metadata without an exact device target" - the whole feature's update path, since every family this PR adds is a group label. Extract family_override_or_recovered_arch and use it in install_wheel_runtime itself (seeded from device_target_override before resolve_pip_runtime is called), so the fix lives where every current and future caller of the wheel install path benefits, not just the two call sites that happen to pass a device target today. Added therock-next-08, a hermetic e2e scenario that plants a next-layout manifest with a grouped family and asserts `update --apply --dry-run` resolves past it; confirmed it fails with the exact reported error when the fix is reverted. - spawn_test_sleep's Windows branch used `cmd /C timeout`, which refuses to run at all with redirected stdin and exits immediately instead of sleeping - both new wait_with_output_bounded tests would fail on Windows for the wrong reason. Switched to `powershell -Command Start-Sleep`. - next_tarball_family_token_round_trips_through_normalize_therock_family asserted only the round-trip's final value, which normalize_therock_family produces identically whether or not the gfx103X rename happens (that's the point of the alias), so it couldn't detect a reverted rename. Split it into two assertions, the first pinning the rename itself. - The uv pip compile bound for ROCm X metadata resolution reused download_timeout_secs verbatim, the same per-fetch budget the canonical path applies once per package (rocm, torch, torchvision, torchaudio: 4 sequential fetches). uv resolves that same four-package set in one call, so give it a comparable aggregate budget instead of one that reliably times out a startup update check before it can ever succeed. - Reworded the tarball version-selector refusal: it previously read as if no tarball install could ever pin a version, when a stable ROCm 10+ pin can. - Noted (ponytail comment) that the uv requirements payload is written to stdin before wait_with_output_bounded's reader threads start, so a larger payload could deadlock outside the timeout. Harmless at today's size. Verified: cargo test -p rocm --bin rocm therock (125 passed), cargo clippy --workspace --all-targets -D warnings (clean), cargo fmt --check (clean), and the 6 hermetic therock-next e2e scenarios plus therock-next-06 against the live index all pass. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Thanks for catching that the previous fix was half-done — pushed fa95d0c.
Non-blocking, all addressed:
Verified: |
|
🔴 Automated review · pr-review-watcher · fa95d0c This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryAdds additive support for TheRock's "next" ROCm 10 install/update layout (separate pip index and tarball catalog, exact-arch device extras, bounded remote artifacts) behind an explicit stable 🚫 Blocking (must fix before merge)None. Explicitly, on the three parts of our open change request:
On cause (focus c): part 1 was the codebase inviting the half-fix — planning ( Non-blocking
|
Withdrawing this change request: all three blocking parts are resolved, and each was confirmed by experiment rather than from the commit message. The apply path is fixed at the shared helper so every caller of the wheel install path gets the recovery, and the new hermetic scenario fails with exactly the originally reported error when that fix is reverted. The round-trip test now pins the renamed token first and is revert-sensitive. The Windows sleep helper swap is the right fix, though it could not be executed from a Linux container and rests on CI. Remaining points are non-blocking and are in the review comment.
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Re-reviewed at fa95d0ca. All five things I raised are addressed, and two of them better than I asked for — approving.
Going through them:
- Pinned nightly ROCm 10.x — gating
next_layout_requestedon a stable pin is the right layer, and citingnightly_accepts_future_prerelease_major_without_cli_changesby name in the doc comment at:272is what stops the next person re-introducing it.nightly_prerelease_pin_of_a_future_major_stays_canonicalcovers the case directly. - The update path — I only named the planning half (
resolve_latest_for_manifest); you found the apply half as well and fixed both atfamily_override_or_recovered_archrather than patching the two call sites that happen to pass a device target today. Gating the recovery throughraw_arch_agreeing_with_familyso a runtime moved to a different card falls back to the group label instead of installing the wrong kernels is the detail I'd have missed.therock-next-08being untagged means it actually guards this on the blocking lane. - The startup budget — bounded, with a kill-at-the-deadline test rather than just a shorter timeout. The x4 aggregate is a fair reading of what the call is doing.
- The trust gate —
env_override_is_ignored_end_to_end_without_the_opt_inputs it on the blocking lane, which was the substance of my concern; the nightly scenario being the live-default proof on top of that is a sensible split. - Scenario 06 — discovering the published version instead of hardcoding it also removes the last reason that scenario could go red for something unrelated to what it tests.
One thing left, and it's the PR description rather than the code: AGENTS.md section 3 asks that scenarios which only run on a gated lane be called out in the PR text with the lane named, and 06 (@nightly) and 07 (@requires-gpu @nightly) still aren't mentioned. The Verification block also still reads "5 scenarios, 24 steps" when there are eight now. Worth refreshing before merge so the description matches what landed — not worth holding the PR for.
Two non-blocking notes, both of which you've already documented in-tree, so just flagging that I agree with your read: ensure_uv_binary sits outside the new timeout bound, so a first-run uv download on the startup path is still unbounded; and the ponytail comment at :2577 about writing the requirements payload before the reader threads start is the right call at today's payload size.
To be clear about what this approval rests on: a read of the diff and its surrounding code, plus the check results on this head — I didn't build or run the suite myself, and I can't verify from here that the live ROCm 10 index publishes the rocm-sdk-device-* links AggregateDeviceTarget::resolve gates on. The nightly and GPU lanes are what will actually prove that one.
6 participants
Summary
rocm,torch, andtorchvision-tests-tarball siblingsgfx125X-dcgpu, preserve legacy-manifest readability, and keep source layout in runtime/update provenanceDependency status
#308 has merged into
mainas60abf955, so this branch is rebased directly onto currentmain; it is no longer a stacked PR. #314/currentmainremains authoritative for torch owning-runtime settlement.Behavior
Normal unpinned release/nightly installs do not jump to Next and never fall back across layouts. ROCm 10 Next resolution requires an explicit version pin and exact raw architecture such as
--version 10.0.0 --family gfx1200. Group labels and free-form strings fail with actionable guidance.Fixture source overrides remain inert unless
ROCM_CLI_THEROCK_ALLOW_BASE_OVERRIDEis explicitly enabled.Verification
cargo test -p rocm --bin rocm therock -- --test-threads=1— 112 passed, 1 ignoredcargo test -p rocm-core normalize_therock_family— 6 passedcargo xtask e2e -- -n 'therock-next'— 5 scenarios, 24 steps passedcargo clippy -p rocm -p rocm-core -p e2e-cucumber --all-targets -- -D warningsTracking: EAI-8431.