fix(install): resolve canonical ROCm streams (EAI-8268) - #308
Conversation
fredespi
left a comment
fredespi
left a comment
There was a problem hiding this comment.
I reviewed this against the current head (69ee614) and checked the new canonical streams directly rather than trusting the constants: both aggregate indexes serve the expected rocm/, torch/, torchvision/ and torchaudio/ package pages, and a resolver run against each one from a clean environment produces a complete, mutually consistent stack — release and nightly, Linux and Windows, without any family segment and without needing an amd-torch-device-* package. Device coverage on the aggregate stream matches what the old family indexes offered. So the central move is sound, and deleting the dead whl/<family> fallback is a real improvement.
Five things I would fix before merge. None of them break resolution; three are user-visible and two are test hygiene.
The one that matters most: the wheel index is now family-independent, but the surrounding code still treats the family as the thing that selects wheels. On a resolution failure the CLI still tells the user to re-run with an explicit --family, which can no longer change the outcome, and the managed runtime is still keyed by family, so two family values now produce two identical multi-gigabyte installs. Details inline.
Also worth noting for the record: the new scenario is untagged, so it puts a live dependency on the nightly index into the blocking merge gate for the whole repository.
A few things I suspected and then ruled out, so they do not need your time: the metadata signature policy is inert by default (the pinned key is empty), so the new root fetch does not require a new .sig sidecar for anyone who has not opted in; the aggregate stream is not missing older families; and the multi-arch payload is not larger than the family-specific one it replaces.
| ) | ||
| .with_context(|| { | ||
| format!( | ||
| "failed to resolve TheRock {} wheel runtime from canonical source {}\n\n{}", |
There was a problem hiding this comment.
The wheel index is now family-independent, but the failure guidance still tells the user their family is the problem.
resolve_pip_runtime_from_index is now called with source.wheel_index and no /<family> segment, so family_resolution no longer influences which wheels are found — it is only carried into the result for display. When resolution fails, this context still appends family_resolution_hint(...), which for an auto-detected family says:
Re-run with an explicit package family: `rocm install sdk --family <FAMILY>`.
Recognized families: ...
On the wheel path that advice can no longer change anything: every --family value now yields the identical index URL and the identical resolution, so a user who follows it gets the same error a second time.
Two related spots while you are here:
- the dry-run still prints
family:/family_source:immediately undercanonical_source:(line 904), which reads as though the family scoped the selection; runtime_key(channel, "wheel", &resolution.family, ...)(line 877) still keys the managed venv by family, so--family Aand--family Bon one host now build two separate multi-gigabyte environments with identical content.
Suggestion: for format == "wheel", drop the family half of the hint and keep the --channel / --format half, which is still actionable — then decide deliberately whether the family should stay in the wheel runtime key.
There was a problem hiding this comment.
Fixed in 05edff0. Wheel failures now use aggregate-specific guidance without --family; dry-run labels the value as target_family; and managed wheel keys are now channel-wheel-multi-arch-version, preventing duplicate aggregate environments across family values.
| let _ = writeln!(output, " channel: {}", channel.as_str()); | ||
| let _ = writeln!(output, " canonical_source: {source_url}"); | ||
| let _ = writeln!(output, " selected_rocm_version: {version}"); | ||
| let _ = writeln!(output, " build_date: {build_date}"); |
There was a problem hiding this comment.
build_date: <not published by canonical source> will print on every release-channel install.
runtime_version_build_date only returns a date when the version string embeds eight date digits. Nightly versions do (10.1.0a20260822); stable release versions never do (7.14.0). So every --channel release preview reports <not published by canonical source>, which reads as a provenance gap at the source when nothing is actually missing — a stable version simply does not carry a build date.
It also contradicts the convention already set in this file: runtime_version_display("7.14.0") returns plain 7.14.0 and just omits the (build ...) suffix when there is no embedded date (see runtime_version_display_mentions_embedded_build_date).
Suggestion: omit the build_date line when there is no embedded date, or use neutral wording such as not encoded in this version.
There was a problem hiding this comment.
Fixed in 05edff0. Stable versions now report build_date: not encoded in stable version; dated nightly versions continue to report the parsed date.
| const THEROCK_RELEASE_PIP_INDEX_BASE: &str = "https://repo.amd.com/rocm/whl"; | ||
| const THEROCK_RELEASE_PIP_MULTI_ARCH_INDEX_BASE: &str = "https://repo.amd.com/rocm/whl-multi-arch"; | ||
| const THEROCK_NIGHTLY_PIP_INDEX_BASE: &str = "https://rocm.nightlies.amd.com/whl-multi-arch"; | ||
| const THEROCK_RELEASE_PIP_INDEX_BASE: &str = "https://repo.amd.com/rocm/whl-multi-arch"; |
There was a problem hiding this comment.
Not about this line specifically — anchored here because MANIFEST.md is not part of the diff.
MANIFEST.md lines 649-650 still document the endpoints this PR removes:
Release channel wheels are served from `https://repo.amd.com/rocm/whl/<gpu-family>/`.
Nightly channel wheels are served from `https://rocm.nightlies.amd.com/v2/<gpu-family>/`.
Both are now wrong in host and in shape — the family segment is gone. That section is the repository's statement of what the CLI downloads at runtime, so it is worth keeping exact. The tarball paragraph directly below it is still correct and needs no change.
There was a problem hiding this comment.
Fixed in 05edff0. MANIFEST.md now documents the canonical release and nightly whl-multi-arch endpoints without family segments.
| Then the adoption is refused | ||
| And the error explains which install types can be adopted | ||
|
|
||
| @id:runtime-resolve-canonical-nightly |
There was a problem hiding this comment.
This scenario carries no capability tag, so it runs in the blocking mock E2E tests job — and it needs live network access to pass. install sdk --dry-run still performs the full resolution: the new root layout fetch plus four simple-index fetches.
Every other scenario on that lane is served locally or by the in-process mock server. This one makes an external service's availability and layout a merge gate for the whole repository, so an outage or a layout change lands as a red required check on unrelated PRs rather than on the change that needs to know.
It does pass today — I confirmed the full nightly stack resolves for cp312 on Linux and Windows from the aggregate index. The concern is the standing coupling, not the current result.
Options: tag it for a lane where a live-stream dependency is expected, or split it so the blocking lane asserts the rendered provenance against a locally served index and only the tagged lane touches the real stream.
There was a problem hiding this comment.
Fixed in 05edff0. The live canonical-stream scenario is now tagged @nightly, so it no longer adds an external network dependency to the blocking mock lane.
| And the error explains which install types can be adopted | ||
|
|
||
| @id:runtime-resolve-canonical-nightly | ||
| Scenario: 4 - Previewing a nightly SDK install reports canonical provenance |
There was a problem hiding this comment.
Duplicate scenario number: Scenario: 4 - Reinstalling the SDK leaves the installed engine's requirements satisfied is already number 4 in this file (line 30). The numbers are used for name filtering (cargo xtask e2e -- -n ...) and in the report grid, so this one probably wants to be 5.
There was a problem hiding this comment.
Fixed in 05edff0. The canonical provenance scenario is now numbered 5.
fredespi
left a comment
fredespi
left a comment
There was a problem hiding this comment.
I re-reviewed the two follow-up commits against the full current PR. The five issues from my previous review are addressed: wheel guidance and runtime identity now match the aggregate stream, stable provenance wording is neutral, the manifest is current, and the live scenario is isolated to the nightly lane. The merge from main introduced one small numbering regression that still needs correction before merge; details inline.
| And the error explains which install types can be adopted | ||
|
|
||
| @id:runtime-resolve-canonical-nightly @nightly | ||
| Scenario: 5 - Previewing a nightly SDK install reports canonical provenance |
There was a problem hiding this comment.
This is again a duplicate Scenario: 5. Commit 05edff0c correctly changed this scenario from 4 to 5, but the subsequent merge brought in The update check reports the active runtime's freshness as scenario 5 at line 45. Scenario numbers are used in name filters and report output, so please renumber this new canonical-provenance scenario to 6.
There was a problem hiding this comment.
Fixed in 0eefc89. The canonical-provenance scenario is now numbered 6. The same commit also activates the refreshed canonical shared runtime in isolated GPU scenarios, addressing the multi-runtime CI failure exposed by the new aggregate key.
|
Re-review complete at |
fredespi
left a comment
fredespi
left a comment
There was a problem hiding this comment.
I reviewed only the new pre-warm commit against the previously cleared head. Scenario numbering remains correct, and the shared-runtime activation path is unchanged. One control-flow issue prevents this commit from repairing the persisted runtime state it targets; details inline.
| } | ||
| } | ||
|
|
||
| ensure_default_engine(&rocm, prewarm_dir)?; |
There was a problem hiding this comment.
This call is unreachable when the runtime is reused: Decision::Reuse returns at line 231. That is the important repair case here. The previous run already updated the persistent shared tree to release-wheel-multi-arch-7-14-0 without vLLM; on the next run rocm update reports that runtime as up_to_date, decide returns Reuse, and pre-warm exits before installing the engine, so the same serve scenarios still fail. Please arrange for the default-engine check/install to run for a reused current runtime too (while keeping the intended degraded-index behavior), and cover the reuse path.
There was a problem hiding this comment.
Fixed in 9f63cfb. The decision match now joins at one shared ensure_default_engine call, so Reuse cannot return before engine validation. Repair is a distinct decision path and both reuse and repair routing are covered.
|
Re-review complete at |
fredespi
left a comment
fredespi
left a comment
There was a problem hiding this comment.
I reviewed only the new aggregate-device-payload commit against the head cleared in the last round. The reuse-path engine repair, unique scenario numbering, and shared-runtime activation remain intact. One cache-identity issue means this commit does not repair the persistent runtime that exposed the missing device payload; details inline. This is now an accumulating design problem rather than another isolated test slip: the persistent runtime cache is version-keyed, while successive fixes change the required package composition without changing that identity.
Separately, device-all installs every device wheel available for the current OS, and individual published device wheels can be hundreds of megabytes to over 1.5 GB. If the generic or target-specific device extra can satisfy the detected family, it would avoid a very large managed environment; please confirm that all payloads are truly required.
| vec![ | ||
| format!("rocm[libraries,devel]=={}", package_versions.rocm), | ||
| format!( | ||
| "rocm[libraries,devel,device-all]=={}", |
There was a problem hiding this comment.
Adding device-all changes the required runtime contents, but it does not change the runtime identity or freshness decision. The persistent MI300X tree already contains release-wheel-multi-arch-7-14-0 without device payloads. Its manifest still says version 7.14.0, and runtime_update_plan compares only that version with the index, so the next pre-warm reports up_to_date, takes Decision::Reuse, and only ensures vLLM; it never reruns this SDK install. The same cached environment therefore remains without the payload needed to fix the observed Failed to infer device type failures. Please add a repair/migration for same-version runtimes, or include the package/layout composition in runtime identity and freshness, with coverage for an existing old-composition cache.
There was a problem hiding this comment.
Fixed in 9f63cfb. device-all is removed. Aggregate installs require one target-specific device extra (including gfx943 -> device-gfx942); absent or mismatched exact detection now fails clearly instead of installing every published payload.
fredespi
left a comment
fredespi
left a comment
There was a problem hiding this comment.
I re-reviewed the cache design at the full current head, not only the latest patch. The reuse-path engine repair, scenario numbering, and shared-runtime activation remain intact. However, the round-6 cache-identity issue is not resolved: this commit narrows the payload chosen for a new install, but it still does not invalidate or repair an existing same-version runtime. Details inline.
| version_selector, | ||
| )?; | ||
| let device_target = aggregate_device_target(&resolution.family); | ||
| let rocm_extras = device_target.as_deref().map_or_else( |
There was a problem hiding this comment.
This improves fresh installs by selecting device-gfx942 instead of every device payload, but it still changes only the install arguments. wheel_runtime_key remains channel + version, InstalledRuntimeManifest records no package/layout composition, and runtime_update_plan still compares only manifest.version with latest_version. Therefore the persistent release-wheel-multi-arch-7-14-0 that lacks any device payload still reports up_to_date; pre-warm takes Reuse, only ensures the engine, and never executes this target-specific SDK install. No migration or invalidation path was added. Please either include the required composition in runtime identity/freshness, or explicitly detect and repair same-version runtimes, with a regression test starting from the old 7.14.0 cache shape.
There was a problem hiding this comment.
Fixed in 9f63cfb. Managed wheel manifests now record source-layout generation plus exact pinned package specs. Freshness reports repair_available when either composition or its composition-keyed runtime identity is stale. Applying that repair creates and activates a side-by-side runtime selected by its exact target key. Regression coverage starts from the old 7.14.0 manifest shape with no composition.
fredespi
left a comment
fredespi
left a comment
There was a problem hiding this comment.
The round-6/7 cache-identity issue is STILL OPEN at this head. I traced the complete existing-cache path again. The new launch environment handling does not migrate, invalidate, repair, or re-key a same-version runtime. The previously cleared engine-prewarm repair, unique scenario numbering, and shared-runtime activation remain intact.
|
|
||
| fn apply_therock_env(command: &mut ProcessCommand, runtime: &VllmRuntime) -> Result<()> { | ||
| command.env("VLLM_TARGET_DEVICE", "rocm"); | ||
| if let Some(target_family) = therock_device_target(runtime) { |
There was a problem hiding this comment.
This is a launch-time workaround that only runs when sdk_library_paths already contains an _rocm_sdk_device_* directory. The stale persistent 7.14.0 runtime lacks that device package, so therock_device_target returns None and this adds nothing. More importantly, the actual cache path is unchanged: runtime_update_plan compares only the version, reports up_to_date, e2e-prewarm takes Decision::Reuse, runs only ensure_default_engine, and returns. No SDK reinstall, migration, invalidation, or re-key occurs, so this commit cannot repair the old cache. Please make composition participate in runtime identity/freshness, or explicitly detect and repair the same-version runtime before reuse.
There was a problem hiding this comment.
Addressed at the cache boundary in 9f63cfb. The launch variable remains necessary after a target package exists, but it is no longer treated as the migration: an old same-version manifest now becomes repair_available and installs a composition-keyed replacement before reuse.
62db3c9 to
46a5967
Compare
Conflict check against #329Verified 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: this PR already changes legacy's package-spec shape, URL candidate list, and runtime-key scheme, which #329'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. |
tomastola
left a comment
tomastola
left a comment
There was a problem hiding this comment.
Not a full review of the canonical-stream work — this is findings from debugging #314's red GPU lane that turned out to be about this PR, plus one concrete rebase instruction. Two are actionable here; two are evidence that the design in this PR is right.
1. This PR's hardware runs contaminate the shared pre-warm cache for every other PR
This is the one worth acting on. It is measured, not inferred, and it cost about a day of red lanes on #314.
The self-hosted GPU runners keep the pre-warm runtime tree in retained shared storage across jobs. This branch can compose a runtime main cannot, and it leaves it behind. Registry entries found in the shared cache:
release-wheel-gfx94x-dcgpu-7-13-0 version 7.13.0 index_url https://repo.amd.com/rocm/whl/gfx94X-dcgpu
release-wheel-gfx94x-dcgpu-7-14-0 version 7.14.0 index_url https://repo.amd.com/rocm/whl-multi-arch
That second index_url cannot be produced by main: therock_index_urls builds whl/{family} and whl-multi-arch/{family}, the latter 403s, so the newest version main can reach for this family is 7.13.0. The flat whl-multi-arch path is reachable only from this branch, which redefines THEROCK_RELEASE_PIP_INDEX_BASE to it.
The reason it then sticks is a design decision in xtask/src/e2e_prewarm.rs::decide — and it is doing exactly what it says:
// `ahead_of_index` means the installed runtime is NEWER than anything the
// index offers (a hand-placed or pinned build). Reuse it rather than
// "updating" backwards.7.14.0 is ahead of main's reachable index, so every subsequent lane — running main's code, which cannot compose a multi-arch runtime — resolves Decision::Reuse onto it and inherits a runtime it has no way to complete. On one runner the active-runtime pointer had been left on 7.14.0 and on the other on 7.13.0, so the failure looked intermittent and PR-dependent when it was neither.
Note the fence here is one-way. source_layout_generation is hashed into the composition key (therock.rs:3911), so this branch correctly refuses to reuse a main-composed runtime. Nothing protects the other direction: main has no notion of layout generation and reuses anything newer. So the identity work in this PR, which is right, does not remove the hazard — it removes it only for this PR's own runs.
Worth fixing before the full hardware matrix runs again. Options, roughly in order of how much I like them:
- Give the pre-warm cache an identity that includes the layout generation (install root or
runtime_key), so the two generations coexist instead of one shadowing the other. This also fits the round-6/7 cache-identity thread already open on this PR. - Or fence
decide: treatahead_of_indexas reusable only when the installed runtime'sindex_urlis one this build can itself reach, and install alongside otherwise. - Or isolate the pre-warm cache per branch. Simplest, but gives up the warm-tree coverage that the reuse path exists to provide.
I cleaned the 7.14.0 runtime out of the shared cache to unblock #314 (details in #314). main cannot re-create it, so that is not a treadmill — but it will come straight back the next time this PR's matrix runs.
2. settle_engine_install here still has the bug #314 just fixed — drop this copy on rebase
At c0c4409, settle_engine_install(paths, engine, runtime_key, response) derives the target torch build and the device-probe library paths from a caller-supplied selector, but installs into the environment's own interpreter. Three of the four call sites (main.rs:3802, 4011, 4329) pass a runtime_id, and runtime_id is shared by every side-by-side install of one channel and family:
runtime_key: release-wheel-gfx94x-dcgpu-7-13-0 version 7.13.0
runtime_key: release-wheel-gfx94x-dcgpu-7-14-0 version 7.14.0
runtime_id : therock-release:gfx94X-dcgpu <- both
So with two runtimes installed side by side the selector can name a different runtime than the one the engine env lives in, and the alignment installs one tree's SDK torch build into the other tree's python. Side-by-side is not an edge case here — the pre-warm's Decision::Update path installs alongside on purpose.
#314 fixes this by resolving the owning runtime from response.python_executable (longest containing install_root, compared both verbatim and canonicalized, since the CLI stores install_root canonicalized while an engine adapter echoes back whatever path it was handed). Selector stays as the fallback for interpreters outside every install root.
Concretely, when rebasing this PR onto the resulting main: drop this PR's torch-alignment commits rather than merging them, and keep #314's settle_engine_install. The two implementations collide across main.rs, therock.rs and engines/vllm/src/lib.rs, and this one lacks the ownership resolution.
3. Supporting evidence that the composition work in this PR is the right fix
Measured on hardware while debugging the above, comparing the two published torch 2.11.0 wheels:
| wheel | .hip_fatbin |
libtorch_hip.so |
offload targets | device check |
|---|---|---|---|---|
+rocm7.14.0 (multi-arch) |
NOBITS, 0 bytes |
136 MB | none | hipErrorInvalidImage |
+rocm7.13.0 (family index) |
PROGBITS |
188 MB | gfx942 | usable, 1 device |
The multi-arch wheel is not corrupt and 7.14 is not a bad build — on-disk size matches the published wheel byte for byte, and +rocm7.13.0 under whl-multi-arch is stripped identically. Multi-arch wheels ship without device code by design; kernels live in .kpack archives carried by the aggregate device package. Generalised: resolving a version published only in the multi-arch channel into a family-specific tree always yields a kernel-less torch, and the tree that had it was carrying the family-specific _rocm_sdk_libraries_gfx94X_dcgpu (no .kpack) rather than the aggregate.
That is precisely the failure WheelRuntimeComposition / required_aggregate_device_target / the rocm[libraries,devel,device-{target}] extras exist to prevent, so this is a concrete argument for the approach, not against it.
It also settles an earlier reading of the red MI300X lane as "reported target is gfx943 and no compatible published device package is available". That is not what was happening — the target resolved fine; the failure was a kernel-less torch from a cross-composed tree. therock.rs:4699 already covers required_aggregate_device_target(Some("gfx943"), "gfx94X-dcgpu").
4. Known gap neither PR closes
engines/vllm/src/lib.rs still builds the env id from runtime_id:
env_id = format!("external-vllm-{}", stable_id_component(runtime_id))which drops the version, so both side-by-side runtimes map to one env id and the registry keeps whichever registered first. The active runtime is therefore not necessarily the one an engine uses. #314 makes the used runtime work; it does not make the right runtime get used, and this PR does not change it either. Not a blocker for either — flagging it so it does not get assumed fixed. Happy to take it as a separate change.
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
I looked at this mainly from the resolution-correctness angle, since @fredespi and @tomastola have already covered the pre-warm cache identity and the settle_engine_install selector issues in depth. Version handling is the part I'd most expected to find problems in and it's genuinely solid: parse_version strips +local, ParsedVersion/VersionStage derive Ord with Alpha < Rc < Stable, and compare_version_strings only falls back to byte comparison when both sides fail to parse. "Latest" is a descending sort with a per-candidate torch/torchvision/torchaudio completeness check, and Release additionally filters to is_stable_runtime_version. That's semver, not lexicographic, and it's the right shape.
What I found is mostly around the new mandatory device-target gate and one thing I think is an accident.
The accident first: the wheel install root stopped going through resolved_install_root, so it no longer resolves symlinks. #317 (ff78075b) is in this branch's history — I confirmed with git merge-base --is-ancestor — and the tarball path still calls it, so this is one-sided rather than a deliberate removal. It matters because it re-opens the exact bug #317 fixed, and the e2e harness creates the very symlink that triggers it.
The device-target gate is the bigger design question. required_aggregate_device_target hard-errors when host detection returns None, and it runs before the dry_run early return, so a preview on a machine with no AMD GPU now fails — including via --family, since the gate re-checks detection rather than trusting the flag. Two scenarios in this PR's own feature file depend on that no-GPU preview working, one of them pre-existing and one added here. Both are @nightly and the run had E2E_INCLUDE_NIGHTLY empty, so CI didn't catch it.
On the GPU lane failure: I traced run 33096888884 and I don't think it's a flake — classify_dependency_details requires every detail to name the realigned package, and realigning torch predictably drags triton out of pin too, so the deliberate divergence gets reported as a violation and the run exits 1. Details inline.
Also worth raising: this bundles canonical stream resolution with a ~1300-line torch-alignment/device-probe subsystem. AGENTS.md §11 asks for one logical change per PR, and here it's load-bearing — the torch-alignment defect above is what's keeping the stream-resolution work red.
There's an ordering problem with #329 and #272 that isn't visible from any single PR; I've noted it in the last comment.
| ); | ||
| let install_root = resolved_install_root(paths, "wheel", &runtime_key, prefix); | ||
| let runtime_key = wheel_runtime_key(channel, &resolution.latest_version, &wheel_composition); | ||
| let install_root = prefix.unwrap_or_else(|| managed_runtime_root(paths, "wheel", &runtime_key)); |
There was a problem hiding this comment.
This drops the symlink resolution that #317 added, and I think unintentionally — the tarball path at :1263 still calls resolved_install_root(paths, "tarball", &runtime_key, prefix), so only the wheel side changed.
main:843 was:
let install_root = resolved_install_root(paths, "wheel", &runtime_key, prefix);which is this same expression plus rocm_core::resolve_path_through_symlinks(&requested). resolved_install_root is still defined on this branch at :4028 and its doc comment still explains exactly why the wheel path needs it:
install sdkwrites this path into three places that all outlive the command: the registry manifest, the sidecar beside the runtime, and — viauv— the#!line of every console script in the venv.
git merge-base --is-ancestor ff78075b pr/308 confirms #317 is in this branch's base, so this reverts it rather than predating it. The consequence is the original bug: reaching data/runtimes through a link bakes the link path into the manifest and every console-script shebang, so removing the link leaves the runtime claiming to live somewhere that isn't there while the files sit next door.
That's not hypothetical here — the e2e harness creates that link for the shared pre-warm tree, and scenario 8 (runtime-install-records-the-real-folder) exists to pin this behavior.
Restoring resolved_install_root(paths, "wheel", &runtime_key, prefix) should be all it needs.
| version_selector, | ||
| )?; | ||
| let device_target = | ||
| required_aggregate_device_target(detect_host_gfx_target().as_deref(), &resolution.family)?; |
There was a problem hiding this comment.
This gate runs unconditionally, before the if dry_run early return at :1055, and required_aggregate_device_target errors on None:
let detected = detected.with_context(|| {
format!("unable to detect an exact GPU target for canonical aggregate family `{family}`; refusing to install every device payload")
})?;So rocm install sdk --dry-run now fails on any host without a detectable AMD GPU. Two scenarios in this PR's feature file depend on it succeeding:
- Scenario 8 (pre-existing,
runtime-install-records-the-real-folder) runsinstall sdk --family gfx110X-all --dry-runthroughrun_rocm_ok(asserts rc==0). Its step comment says why: "--familybecause a host with no AMD GPU has no target to detect, and the preview resolves the install folder before it needs one." That premise is what this gate removes. - Scenario 6 (new,
runtime-resolve-canonical-nightly) has the same shape with--family gfx120X-all, also viarun_rocm_ok, and carries no@requires-gpu— so it's scheduled on hosts that can't satisfy it. On the gfx94X GPU lane it fails the other way, sincecanonical_aggregate_device_targetreturnsNonewhen the detected chip doesn't normalize to the requested family.
Neither ran: both are @nightly and the failing run had E2E_INCLUDE_NIGHTLY: empty.
The --family case seems like the more interesting one. A user passing --family explicitly has stated their target, but resolve_family returns it from the override branch and then this gate re-derives the device target from host detection anyway, so the flag doesn't get them past it. On a preview there's nothing to protect — no payload is downloaded — so skipping the requirement on the dry_run path and rendering something like device_target: <undetermined> would keep both scenarios honest. For the real install path, falling back to the family's device set when --family is supplied would be the equivalent of what #272 does (see the last comment).
| /// | ||
| /// Only a divergence about the realigned package is expected; anything else in | ||
| /// the same run is still a violation and must keep saying so. | ||
| fn classify_dependency_details( |
There was a problem hiding this comment.
I think this is what's red on the GPU lane, and it looks like a normal case rather than an edge one.
details.iter().all(|d| d.contains(&marker)) only tolerates the divergence when every violation names the realigned package. But realigning torch drags its stack along, so a second violation is the expected outcome, not an anomaly. From run 33096888884:
violation: The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.14.0` is installed
violation: The package `vllm` requires `triton==3.6.0`, but `3.7.1+git0263a6a6.rocm7.14.0` is installed
dependency_check: violated
device_check: no_devices
1: rocm::unusable_runtime_error
The torch line is the deliberate realignment this code exists to tolerate; the triton line is its consequence. Because all() sees the triton detail, the whole run is classified Violated, the user is advised to run rocm engines install vllm --reinstall (which would undo the alignment), and install_left_runtime_unusable then exits 1.
Partitioning instead of requiring all() would fit the doc comment's intent better — expected-divergence details for the realigned package and the packages that pin against it, genuine violations for everything else — and the --reinstall action line should be suppressed whenever any detail is an expected divergence, since following it makes things worse.
(The no_devices half of this is the selector/env_path mismatch @tomastola described at runtime_id: release-wheel-multi-arch-7-14-0-… vs env_path: …/release-wheel-gfx94x-dcgpu-7-13-0/bin, which I gather #314 fixes — flagging only the classification half here.)
| @@ -14511,8 +15209,8 @@ | |||
| false, | |||
| )?; | |||
| let manifests_after = therock::load_runtime_manifests(paths)?; | |||
There was a problem hiding this comment.
apply_runtime_update predicts a runtime key with one family, then installs with another.
plan.target_runtime_key comes from runtime_update_plan → resolve_latest_for_manifest (therock.rs:734), which passes Some(manifest.family.as_str()) as the family override. But the apply path just above calls install_sdk(paths, &source.channel, &source.format, None, None, None, false) — family_override = None. So the install re-derives the family from resolve_family's detection chain, and the family feeds the device-<target> extra, which feeds the composition, which feeds the key.
What makes that fatal now rather than merely surprising is the lookup narrowing on the next line. main matched on channel/format/family/version:
manifest.channel == source.channel && manifest.format == source.format
&& manifest.family == source.family && manifest.version == latest_versionThis PR replaces it with an exact manifest.runtime_key == target_runtime_key. Any divergence between predicted and actual now aborts with "runtime install completed but the target runtime manifest was not found" — after a multi-gigabyte install has already succeeded and been written to disk.
The dry-run branch a few lines up has the same None, so the preview describes a different install than the apply performs.
Passing Some(&source.family) on both branches would keep the planned and applied keys in agreement. Worth doing regardless of whether a divergence is reachable today — xtask e2e-prewarm's new Decision::Repair drives exactly this path, and the failure mode is expensive.
| }) | ||
| } | ||
|
|
||
| fn canonical_aggregate_device_target(detected: &str, family: &str) -> Option<String> { |
There was a problem hiding this comment.
Only gfx94* is canonicalized to a published package name; everything else passes the raw detected chip straight through as device-<target> (:1203).
That's fine for the single-chip families, but normalize_therock_family maps four chips into gfx110X-all (gfx1100/1101/1102/1103) and two into gfx120X-all (gfx1200/1201). So a gfx1103 host resolves family gfx110X-all and then requests rocm[libraries,devel,device-gfx1103]. Whether that resolves depends on the index publishing a device payload per detected chip rather than per family bucket — and if it doesn't, it's an install-time resolution failure, not a warning.
I couldn't verify the index contents from here (the endpoint isn't reachable from my environment), so I can't tell you whether device-gfx1103 exists today. But #272 came to a different conclusion about this: it added known_therock_family_device_chips in rocm-core mapping gfx110X-all -> [gfx1100, gfx1101, gfx1102, gfx1103] and requests all of a family's chips, with a device-all fallback for unmapped families.
Either the two PRs disagree about the index layout, or one of them is carrying a workaround the other doesn't need — worth settling explicitly rather than by merge order. The safest local version would be to verify the requested device-<target> extra exists in the index before pinning it, so a wrong guess produces a clear message instead of a resolver error.
| Ok(output) | ||
| } | ||
|
|
||
| fn runtime_freshness( |
There was a problem hiding this comment.
Runtimes installed before this PR look permanently repairable, and the daemon re-notifies about them every six hours.
A manifest written by main has wheel_composition: None, so at equal versions the Ordering::Equal guard's manifest.wheel_composition.as_ref() != required_composition is always true and freshness is RepairAvailable. The repair then installs a new composition-keyed runtime alongside rather than amending the old manifest, so the old one keeps reporting RepairAvailable on every subsequent check.
That state reaches the daemon: rocmd/src/lib.rs:1229-1231 folds repair_available into update_output_reports_update_available, and :3861 calls record_update_available_notification whenever it's true, which emits "A ROCm runtime repair is available because its package composition changed" — every THEROCK_UPDATE_INTERVAL_MS (6h), forever, for a repair the user already performed.
xtask/src/e2e_prewarm.rs::decide was given an explicit up_to_date-wins guard (:124) which suggests this state is known-reachable; the daemon has no equivalent.
Suppressing RepairAvailable when a sibling runtime of the same channel/format/family already carries the required composition would cover both. Backfilling legacy manifests would work too, but the sibling check also handles the case where the user repaired and kept the old runtime deliberately.
| ] | ||
| } | ||
|
|
||
| fn wheel_runtime_composition( |
There was a problem hiding this comment.
Not about this function specifically — anchoring here because it's where the three PRs collide. Raising it because it isn't visible from inside any one of them.
#329 builds a two-generation index model: TheRockIndexGeneration{Legacy, Next}, whl-next plus a second release host, raw_arch plumbing, and per-generation cache-key namespacing. It keeps therock_index_urls() and adds candidate ordering on top.
#308 goes the other way: one hardcoded CanonicalSource per channel with a constant layout_generation, and removes therock_index_urls() entirely.
These aren't mergeable in sequence — whichever lands second has to be substantially rewritten against the other's model, and a clean textual rebase would produce something neither author designed.
#272 overlaps differently: it makes the identical therock_pip_package_specs(package_versions, rocm_extras) signature change this PR makes, with a competing extras policy (see the device-<target> comment above). Same root cause, two incompatible fixes; these will conflict semantically even if git merges them cleanly.
Probably worth a maintainer picking the index model first and having the other two rebase onto it, rather than resolving it at merge time. Happy to be told this is already settled somewhere I haven't seen.
c0c4409 to
4748eb7
Compare
|
@michaelroy-amd Rebased this branch onto latest While verifying the rebased result (
Full verification after the fixups: tests and clippy clean, E2E suite 71/74 passing (2 expected xfail, 1 pre-existing environment-only failure unrelated to this branch, confirmed it reproduces identically on unmodified |
|
Resolver ownership update: this PR/EAI-8268 is the canonical owner for current release/nightly aggregate source selection, shared provenance, and runtime-composition behavior. #329/EAI-8431 will be rebased as the ROCm 10 Next-layout extension on top of this model. #272's #271 regression behavior will be transferred and verified here with scenario evidence, after which #272 will close 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. Related: #272, #329, #314. Tracking: EAI-8268, EAI-8431, EAI-7956. |
Signed-off-by: Michael Roy <michael.roy@amd.com> Signed-off-by: Juho Vainio <juho.vainio@amd.com> (cherry picked from commit ef2aaa9) Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com> (cherry picked from commit db7e7d5) Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com> (cherry picked from commit 1a1fad7) Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com> (cherry picked from commit 5df4c8d) Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com> (cherry picked from commit 9f7f36d) 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>
A managed wheel runtime was identified by channel and version alone, so two installs that differ in what they actually install — above all the `device-<target>` payload that decides whether the runtime can launch a kernel at all — resolved to the same runtime key. A corrected composition overwrote the existing tree in place instead of landing beside it, and `rocm update` reported the stale environment as up to date because its version had not moved. Record the exact install intent (source-layout generation and pinned package specs) in the runtime manifest, fold a fingerprint of it into the wheel runtime key, and compare it during freshness resolution. A same-version runtime whose composition or identity is stale now reports `repair_available` and installs a composition-keyed replacement side-by-side. Manifests written before this field load unchanged and are repaired once. An `ahead_of_index` build is never offered a repair: the index cannot reproduce it, so the only install available would be a rollback. Update apply now selects the installed manifest by the runtime key the plan predicted, not by version: a repair produces a sibling sharing the source's channel, format, family and version, which a version match would have resolved back to the runtime being replaced. The install is pinned to the source runtime's family for the same reason, so the applied key and composition match the predicted ones on a host whose GPU differs from the runtime being updated. An already-installed replacement suppresses further repairs of the manifest it superseded, so a retained legacy entry cannot re-trigger the same install or notification on every run. Each `rocm update` line now carries `target=<key>`, the key an apply from that line would produce. The E2E pre-warm uses it to activate the runtime a reuse actually means — after a repair that is the replacement, not the superseded manifest — before installing the default engine, and the shared pre-warm tree is namespaced by source-layout generation so a branch on a new layout cannot poison the cache. 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>
78eecdb to
7fa847c
Compare
|
Rebased and narrowed onto current |
Signed-off-by: Michael Roy <michael.roy@amd.com>
|
CI diagnosis at |
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>
feedback addressed
Brings the branch up to date with main so GitHub can build refs/pull/273/merge again; the branch had been CONFLICTING, which left every pull_request-triggered workflow with nothing to run. Conflict resolutions: - apps/rocm/src/therock.rs (imports): union of both sides. This branch adds detect_legacy_rocm_summary and interactive_terminal; main adds detect_host_gfx_target and extract_first_gfx_token. - apps/rocm/src/therock.rs (helpers): kept this branch's parse_host_version and dropped therock_index_urls, which main removed in #308 along with its only caller and the THEROCK_RELEASE_PIP_MULTI_ARCH_INDEX_BASE constant it read. - tests/e2e-cucumber/features/runtime_setup.feature: both sides appended scenarios numbered runtime-08 and runtime-09. Kept all four; main's canonical provenance scenarios stay at 08/09 and this branch's --yes scenarios move to runtime-10 and runtime-11 so indexes remain sequential and unique, as feature_naming.rs requires. The @id: tags are unchanged. - tests/e2e-cucumber/tests/e2e/runtime_steps.rs (both hunks): union. The shared runtime preconditions keep this branch's --yes on the fresh `install sdk` and gain main's else branch calling activate_shared_runtime_if_unset. The --yes consent gate and the "replacing the active default" wording are carried through unchanged. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
5 participants
Summary
device-<gfx target>payload and preserve legacy-manifest readabilityScope and dependencies
mainremains authoritative for torch owning-runtime settlement. The obsolete torch/vLLM alignment experiments from the previous history were removed.Behavior
New canonical installs use exactly one aggregate source per channel and fail closed when its layout or exact device payload is unavailable. Existing managed runtimes with legacy manifests remain readable and receive a one-time side-by-side composition repair when reproducible.
Verification
cargo test --workspace --all-targets— 2,467 passed across 30 suites; 8 ignoredcargo clippy --workspace --all-targets -- -D warningspython3 scripts/smoke_local.pycargo xtask manifest --checkcargo xtask tpn --checkcargo xtask verify-commits --base origin/main— all 10 commits signed and signed offcargo xtask e2e -- -n 'Previewing a release SDK install resolves the canonical aggregate'— 1 scenario, 4 steps passeddevice-gfx1103device-gfx1103Local
pythonis unavailable, so the documented smoke script was run with the equivalentpython3interpreter. Local Hawkeye 6.5.1 cannot parse the repository's current config; commits used the CI-pinned, checksum-verified Hawkeye 7.0.0 binary, and the required CI license-header job remains authoritative.