fix(install): resolve canonical ROCm streams (EAI-8268) - #308
fix(install): resolve canonical ROCm streams (EAI-8268)#308michaelroy-amd wants to merge 21 commits into
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.
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>
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>
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>
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>
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>
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>
Signed-off-by: Michael Roy <michael.roy@amd.com>
Two installers write torch into the same managed runtime. `install sdk` writes TheRock's build; an engine install then writes the build from its own index, pinned to an exact version. Letting either side win outright is wrong, and both failures have been seen on real hardware: - With the engine's build, the runtime can hold a torch that loads against the installed SDK and then enumerates no devices. vLLM resolves no platform and serving dies with "Failed to infer device type", long after the install reported success. - With the SDK's build, the runtime can hold a torch *release* the engine does not accept, which breaks the engine a different way. The release and the build answer different questions, so take them from different places: the release from the engine, which was built against it, and the build from the SDK, which owns the libraries the runtime loads. The SDK's torch is read before the engine install, since that install is what overwrites it, and its build identifier is carried onto the release the engine pins. Where the SDK publishes no such build, the engine's own is kept rather than failing a multi-gigabyte install; where the install fails for any other reason the error is reported as it happened, since calling a network or permission failure a missing wheel sends the reader hunting for something that exists. This applies to every path that installs an engine into a managed runtime, not just the auto-install after `install sdk`. A standalone `rocm engines install <engine> --reinstall` is what someone reaches for when a runtime already looks wrong, so it above all must not be the thing that breaks it; it now re-settles torch and leaves the runtime usable. Environments rocm-cli does not own are left alone, and anyone wanting a different torch can still install it into the environment directly. Reporting is adjusted so the result is legible. A new device check asks the runtime whether torch can actually open a GPU, because a satisfied dependency check does not mean a usable environment -- the broken state reports `is_available() == True` with a device count of zero, which is why it stayed invisible until first serve. The check composes the runtime's recorded library paths, not `rocm_sdk.initialize_process()`: measured on a runtime that serves correctly, the former reports 8 devices and the latter 0, so a probe built on it would condemn healthy runtimes. The dependency check learns to tell a deliberate divergence from a real violation, since after alignment the engine's exact pin is unsatisfied by design and the reinstall remedy no longer applies. An install that leaves a runtime unable to open a device now fails instead of exiting 0, but only on that conjunction: an alignment that could not run may still leave a working environment, and a device count of zero is the right answer wherever no GPU is present. `install sdk` needs the distinction, because it deliberately downgrades engine auto-install failures to a warning so a failed engine install does not discard a good multi-gigabyte SDK -- which is still right, and still what happens for every other engine failure. It was catching this one too, so the single case where the install really did produce something broken kept reporting success, which is the reported symptom exactly. The error says the SDK itself installed fine and what to do next, so a transient index failure does not read as a ruined install. The nightly scenario covering a second SDK install moves with it. It asserted the wording of the dependency check, which a settled runtime no longer produces; it now asserts that the runtime can still open a GPU, which is the outcome it was always trying to protect, while still failing on any genuine unmet requirement. Verified end to end on MI300X: an unmodified `install sdk` realigns torch, reports the runtime usable with 8 devices, and serves a real completion; a subsequent `engines install --reinstall` re-settles it rather than breaking it. Signed-off-by: Tomas Saaristola <tsaarist@amd.com> Signed-off-by: Michael Roy <michael.roy@amd.com>
62db3c9 to
46a5967
Compare
Signed-off-by: Michael Roy <michael.roy@amd.com>
3 participants
Summary
Verification
cargo test --workspace --all-targets(2234 passed, 8 ignored)cargo clippy --workspace --all-targets -- -D warningspython3 scripts/smoke_local.py --skip-buildruntime-resolve-canonical-nightlyCucumber scenarioTracking
EAI-8268
If this PR fixes a bug, searched
tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. No matching xfail row exists.