From 930850582392c6db8ca06f06872842d75d7914b8 Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Wed, 26 Aug 2026 11:13:21 +0000 Subject: [PATCH 01/19] fix(install): always keep the SDK's build of the torch an engine pins 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 --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 --- apps/rocm/src/comfyui.rs | 3 + apps/rocm/src/main.rs | 1126 ++++++++++++++++- apps/rocm/src/storage.rs | 1 + apps/rocm/src/therock.rs | 263 ++++ .../features/runtime_setup.feature | 21 +- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 24 +- 6 files changed, 1368 insertions(+), 70 deletions(-) diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 4e0707fae..5ae412265 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -2111,6 +2111,7 @@ mod tests { ], ..therock::RocmSdkPythonProbe::default() }), + sdk_torch: None, read_only: false, imported_from: None, installed_at_unix_ms: 100, @@ -2176,6 +2177,7 @@ mod tests { library_paths: vec![sdk_lib.clone()], ..Default::default() }), + sdk_torch: None, read_only: false, imported_from: None, installed_at_unix_ms: 100, @@ -2272,6 +2274,7 @@ mod tests { ], ..therock::RocmSdkPythonProbe::default() }), + sdk_torch: None, read_only: false, imported_from: None, installed_at_unix_ms: 100, diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 375e851ca..ceff82631 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -2444,8 +2444,8 @@ fn install(target: InstallTarget) -> Result<()> { finalize_successful_sdk_install(&paths)? }; print!("{output}"); - if let Some(finalized) = finalized { - print_sdk_install_success(&finalized); + if let Some(finalized) = &finalized { + print_sdk_install_success(finalized); // The SDK runtime wheel bundles PyTorch, whose ROCm build // links against libatomic.so.1 and the system numactl // runtime (libnuma.so.1 / libnuma_1.2). Ensure both are @@ -2453,41 +2453,22 @@ fn install(target: InstallTarget) -> Result<()> { // engine (if any) is auto-installed below. ensure_libatomic_for_torch(yes); ensure_libnuma_for_torch(yes); - if let Err(error) = - maybe_auto_install_sdk_preferred_engine(&paths, &finalized, yes) - { - record_cli_audit_event( - &paths, - "engine", - "engine_auto_install", - "error", - format!( - "auto-install failed engine=vllm runtime_id={} family={}: {error}", - finalized.runtime_key, finalized.family - ), - None, - ); - eprintln!("warning: automatic vLLM install failed: {error}"); - eprintln!( - "warning: SDK install completed; you can run `rocm engines install vllm --runtime-id {}` after vLLM is available in that runtime", - finalized.runtime_key - ); - } } - record_cli_audit_event( + finish_sdk_install( &paths, - "runtime", + finalized.as_ref(), if dry_run { "install_sdk_dry_run" } else { "install_sdk" }, - "info", format!( "sdk install completed channel={channel} format={format_name} prefix={prefix_display} version_selector={version_selector_display} dry_run={dry_run}" ), - None, - ); + |paths, finalized| { + maybe_auto_install_sdk_preferred_engine(paths, finalized, yes) + }, + )?; } Err(error) => { record_cli_audit_event( @@ -3806,13 +3787,14 @@ fn engines(command: EnginesCommand) -> Result<()> { }, env_root.as_deref(), )?; + settle_engine_install(&paths, &engine, &runtime_id, &response)?; println!("engine install"); println!(" engine: {engine}"); println!(" runtime_id: {runtime_id}"); println!(" reinstall: {reinstall}"); println!(" env_id: {}", response.env_id); println!(" env_path: {}", response.env_path); - for warning in response.warnings { + for warning in &response.warnings { println!(" warning: {warning}"); } if response.managed_env == Some(false) { @@ -4014,7 +3996,7 @@ fn ensure_self_managed_engine_ready( None } else { eprintln!("Preparing {engine} for GPU serving..."); - Some(engine_request_with_env_root::<_, InstallResponse>( + let response = engine_request_with_env_root::<_, InstallResponse>( Some(paths), engine, EngineMethod::Install, @@ -4025,7 +4007,9 @@ fn ensure_self_managed_engine_ready( env_root: env_root.clone(), }, env_root.as_deref(), - )?) + )?; + settle_engine_install(paths, engine, &runtime_id, &response)?; + Some(response) }; let engine_config = config.engine_config_mut(engine); @@ -4342,6 +4326,7 @@ fn resolve_engine_env( }, env_root.as_deref(), )?; + settle_engine_install(paths, engine, &runtime_id, &response)?; Ok(ResolvedEngineEnv { env_id: response.env_id, runtime_id, @@ -7394,6 +7379,9 @@ fn maybe_auto_install_sdk_preferred_engine( engine, runtime_python_for_key(paths, &finalized.runtime_key).as_deref(), &finalized.runtime_key, + // The engine install failed, so no alignment ran and any + // divergence found here is a real one. + None, ); return Err(error); } @@ -7401,7 +7389,7 @@ fn maybe_auto_install_sdk_preferred_engine( println!(" reinstall: false"); println!(" env_id: {}", response.env_id); println!(" env_path: {}", response.env_path); - for warning in response.warnings { + for warning in &response.warnings { println!(" warning: {warning}"); } @@ -7417,16 +7405,7 @@ fn maybe_auto_install_sdk_preferred_engine( } config.save(paths)?; - // The SDK and the engine share this environment, and the SDK's torch stack was - // just written into it. Say plainly whether the engine's own requirements - // survived that, so a runtime the engine cannot use is never reported only as a - // successful install. - report_engine_dependency_check( - paths, - engine, - Some(Path::new(&response.python_executable)), - &finalized.runtime_key, - ); + settle_engine_install(paths, engine, &finalized.runtime_key, &response)?; } record_cli_audit_event( @@ -7452,6 +7431,14 @@ enum EngineDependencyCheck { /// The engine declares requirements the environment does not meet, one line each, /// as the resolver reported them. Violated(Vec), + /// The only unmet requirements are ones the install deliberately diverged from. + /// + /// Torch alignment leaves the runtime holding the SDK's build of the release + /// the engine pins, which the engine's exact-pin metadata cannot express. The + /// distinction matters because the remedy for a real violation — reinstalling + /// the engine — is precisely what would undo the alignment and restore a + /// runtime that cannot open a device. + ExpectedDivergence(Vec), /// The check itself could not run (no usable `uv`, unreadable environment). NotVerified(String), } @@ -7466,8 +7453,9 @@ fn report_engine_dependency_check( engine: &str, python: Option<&Path>, runtime_key: &str, + realigned_package: Option<&str>, ) { - let outcome = engine_dependency_check(paths, engine, python); + let outcome = engine_dependency_check(paths, engine, python, realigned_package); print!("{}", render_engine_dependency_check(engine, &outcome)); let (level, message) = match &outcome { EngineDependencyCheck::Satisfied => ( @@ -7481,6 +7469,13 @@ fn report_engine_dependency_check( details.join("; ") ), ), + EngineDependencyCheck::ExpectedDivergence(details) => ( + "info", + format!( + "engine={engine} runtime_id={runtime_key} dependency_check=expected_divergence: {}", + details.join("; ") + ), + ), EngineDependencyCheck::NotVerified(reason) => ( "info", format!( @@ -7507,10 +7502,617 @@ fn runtime_python_for_key(paths: &AppPaths, runtime_key: &str) -> Option Vec { + let Ok(manifests) = therock::load_runtime_manifests(paths) else { + return Vec::new(); + }; + runtime_manifest_for_selector(&manifests, runtime_key) + .and_then(|manifest| manifest.rocm_sdk.as_ref()) + .map(|probe| probe.library_paths.clone()) + .unwrap_or_default() +} + +/// The wheel index the runtime was installed from. +fn runtime_index_url_for_key(paths: &AppPaths, runtime_key: &str) -> Option { + let manifests = therock::load_runtime_manifests(paths).ok()?; + let manifest = runtime_manifest_for_selector(&manifests, runtime_key)?; + manifest.index_url.clone() +} + +/// Which torch a runtime should hold once an engine has been installed into it. +/// +/// Two installers write torch into the same environment. The SDK install writes +/// TheRock's build; the engine then writes the build from its own index, pinned +/// to an exact version. Letting either side win unconditionally is wrong, and +/// both failures have been observed in the field: +/// +/// * If the engine's build always wins, the runtime can end up with a torch that +/// loads against the installed SDK and then enumerates no devices, so serving +/// fails with an unhelpful error long after the install reported success. +/// * If the SDK's build always wins, the runtime can end up on a torch *release* +/// the engine does not accept, which breaks the engine in a different way. +/// +/// The version and the build answer different questions, so they are taken from +/// different places: the *release* comes from the engine, which is built against +/// it, and the *build* comes from the SDK, which the libraries belong to. That +/// is what this resolves to — the SDK's build of the release the engine pins. +#[derive(Debug, Clone, PartialEq, Eq)] +enum TorchAlignment { + /// The runtime already holds the SDK build of the engine's torch release. + AlreadyAligned { version: String }, + /// The engine's build was replaced with the SDK build of the same release. + Realigned { from: String, to: String }, + /// The SDK publishes no build of that release; the engine's own is kept. + /// + /// Claimed only when the resolver actually said so. Any other failure is an + /// `InstallFailed`, because asserting "not published" over a network or disk + /// error sends the reader hunting for a missing wheel that exists. + Unavailable { wanted: String, kept: String }, + /// The realignment install failed for some other reason, carried verbatim. + InstallFailed { + wanted: String, + kept: String, + error: String, + }, + /// Nothing to decide — no exact pin, no engine metadata, or no SDK torch. + NotApplicable(String), +} + +/// What the alignment rule concludes, before any of it is acted on. +#[derive(Debug, Clone, PartialEq, Eq)] +enum TorchAlignmentPlan { + AlreadyAligned { version: String }, + Install { wanted: String, from: String }, + NotApplicable(String), +} + +/// The rule itself, kept free of I/O so both field failures can be tested. +/// +/// `wanted` is the engine's pinned *release* carrying the SDK's *build*. +fn plan_torch_alignment( + sdk_build: Option<&str>, + installed_torch: Option<&str>, + engine_requirement: Option<&str>, + engine: &str, +) -> TorchAlignmentPlan { + let Some(sdk_build) = sdk_build else { + return TorchAlignmentPlan::NotApplicable( + "the runtime manifest does not identify the SDK's torch build".to_owned(), + ); + }; + let Some(requirement) = engine_requirement else { + return TorchAlignmentPlan::NotApplicable(format!("{engine} does not pin torch")); + }; + let Some(pinned) = therock::requirement_pinned_version(requirement) else { + return TorchAlignmentPlan::NotApplicable(format!( + "{engine} does not pin torch to an exact version ({requirement})" + )); + }; + let wanted = format!("{}+{sdk_build}", therock::split_local_version(pinned).0); + let installed = installed_torch.unwrap_or_default().to_owned(); + if installed == wanted { + TorchAlignmentPlan::AlreadyAligned { version: wanted } + } else { + TorchAlignmentPlan::Install { + wanted, + from: installed, + } + } +} + +/// Whether a failed install means the resolver could not find that version. +/// +/// Deliberately narrow. Anything unmatched is reported as a plain failure with +/// its error, so an unrecognised message degrades to the honest answer rather +/// than to a confident wrong one. +fn install_error_reports_version_unavailable(error: &str) -> bool { + let error = error.to_ascii_lowercase(); + error.contains("no solution found") + || error.contains("were found for") + || error.contains("not found in the package registry") + || error.contains("has no version") +} + +fn align_runtime_torch( + paths: &AppPaths, + python: &Path, + index_url: Option<&str>, + sdk_build: Option<&str>, + engine: &str, +) -> TorchAlignment { + let probe = match therock::probe_torch_alignment(python, engine) { + Ok(probe) => probe, + Err(error) => return TorchAlignment::NotApplicable(error.to_string()), + }; + let plan = plan_torch_alignment( + sdk_build, + probe.installed_torch.as_deref(), + probe.engine_requires_torch.as_deref(), + engine, + ); + let (wanted, from) = match plan { + TorchAlignmentPlan::AlreadyAligned { version } => { + return TorchAlignment::AlreadyAligned { version }; + } + TorchAlignmentPlan::NotApplicable(reason) => { + return TorchAlignment::NotApplicable(reason); + } + TorchAlignmentPlan::Install { wanted, from } => (wanted, from), + }; + let Some(index_url) = index_url else { + return TorchAlignment::NotApplicable( + "the runtime manifest records no wheel index to install from".to_owned(), + ); + }; + match therock::install_pinned_package( + paths, + python, + index_url, + "torch", + &format!("torch=={wanted}"), + ) { + Ok(()) => TorchAlignment::Realigned { from, to: wanted }, + // The SDK index may not publish this release at all. That is a real + // possibility, not an error to abort on: the engine's own build is left + // in place and the device check that follows reports whether it works. + Err(error) => { + let error = format!("{error:#}"); + if install_error_reports_version_unavailable(&error) { + TorchAlignment::Unavailable { wanted, kept: from } + } else { + TorchAlignment::InstallFailed { + wanted, + kept: from, + error, + } + } + } + } +} + +fn render_torch_alignment(outcome: &TorchAlignment) -> String { + let mut output = String::new(); + match outcome { + TorchAlignment::AlreadyAligned { version } => { + let _ = writeln!( + output, + " torch_alignment: already_aligned ({})", + sanitize_log_value(version) + ); + } + TorchAlignment::Realigned { from, to } => { + let _ = writeln!(output, " torch_alignment: realigned"); + let _ = writeln!( + output, + " {} -> {} (the SDK's build of the release {} pins)", + sanitize_log_value(from), + sanitize_log_value(to), + sanitize_log_value("the engine") + ); + } + TorchAlignment::Unavailable { wanted, kept } => { + let _ = writeln!(output, " torch_alignment: unavailable"); + let _ = writeln!( + output, + " the SDK index publishes no {}; keeping {}", + sanitize_log_value(wanted), + sanitize_log_value(kept) + ); + } + TorchAlignment::InstallFailed { + wanted, + kept, + error, + } => { + let _ = writeln!(output, " torch_alignment: install_failed"); + let _ = writeln!( + output, + " could not install {}: {}", + sanitize_log_value(wanted), + sanitize_log_value(error) + ); + let _ = writeln!(output, " keeping {}", sanitize_log_value(kept)); + } + TorchAlignment::NotApplicable(reason) => { + let _ = writeln!( + output, + " torch_alignment: not_applicable ({})", + sanitize_log_value(reason) + ); + } + } + output +} + +/// Returns the package the runtime now deliberately diverges on, if any, so the +/// dependency check can tell that divergence apart from a real violation. +/// +/// Both `Realigned` and `AlreadyAligned` diverge from the engine's exact pin — +/// the second is simply a rerun over a runtime already put right, which is the +/// normal state on every refresh after the first. +fn report_torch_alignment( + paths: &AppPaths, + engine: &str, + python: &Path, + runtime_key: &str, + sdk_build: Option<&str>, +) -> TorchAlignment { + let index_url = runtime_index_url_for_key(paths, runtime_key); + let outcome = align_runtime_torch(paths, python, index_url.as_deref(), sdk_build, engine); + print!("{}", render_torch_alignment(&outcome)); + let (level, message) = match &outcome { + TorchAlignment::AlreadyAligned { version } => ( + "info", + format!( + "engine={engine} runtime_id={runtime_key} torch_alignment=already_aligned version={version}" + ), + ), + TorchAlignment::Realigned { from, to } => ( + "info", + format!( + "engine={engine} runtime_id={runtime_key} torch_alignment=realigned from={from} to={to}" + ), + ), + TorchAlignment::Unavailable { wanted, kept } => ( + "error", + format!( + "engine={engine} runtime_id={runtime_key} torch_alignment=unavailable wanted={wanted} kept={kept}" + ), + ), + TorchAlignment::InstallFailed { + wanted, + kept, + error, + } => ( + "error", + format!( + "engine={engine} runtime_id={runtime_key} torch_alignment=install_failed wanted={wanted} kept={kept}: {error}" + ), + ), + TorchAlignment::NotApplicable(reason) => ( + "info", + format!( + "engine={engine} runtime_id={runtime_key} torch_alignment=not_applicable: {reason}" + ), + ), + }; + record_cli_audit_event(paths, "engine", "torch_alignment", level, message, None); + outcome +} + +/// The package the runtime now deliberately diverges on, if any, so the +/// dependency check can tell that divergence apart from a real violation. +/// +/// Both `Realigned` and `AlreadyAligned` diverge from the engine's exact pin — +/// the second is a rerun over a runtime already put right, which is the normal +/// state on every refresh after the first. +const fn deliberately_diverged_package(outcome: &TorchAlignment) -> Option<&'static str> { + match outcome { + TorchAlignment::Realigned { .. } | TorchAlignment::AlreadyAligned { .. } => Some("torch"), + TorchAlignment::Unavailable { .. } + | TorchAlignment::InstallFailed { .. } + | TorchAlignment::NotApplicable(_) => None, + } +} + +/// Whether the installed runtime can actually open a GPU. +/// +/// The dependency check answers whether the engine's declared requirements are +/// satisfied. That is a question about metadata, and it is not the same question +/// as whether the environment works: a torch built against a different ROCm +/// version than the installed SDK satisfies nothing yet loads cleanly, and then +/// reports no devices. vLLM turns that into `Failed to infer device type` at +/// first serve, long after the install reported success. +#[derive(Debug, Clone, PartialEq, Eq)] +enum RuntimeDeviceCheck { + /// torch imported and reported at least one device. + Usable { + device_count: u32, + torch_version: String, + }, + /// torch imported but reported no devices — the failure this check exists for. + NoDevices { + torch_version: String, + hip_version: String, + }, + /// The question could not be answered; never assume healthy. + NotVerified(String), +} + +fn runtime_device_check(python: Option<&Path>, library_paths: &[PathBuf]) -> RuntimeDeviceCheck { + let Some(python) = python else { + return RuntimeDeviceCheck::NotVerified( + "the runtime's Python environment could not be located".to_owned(), + ); + }; + let probe = match therock::probe_runtime_devices(python, library_paths) { + Ok(probe) => probe, + Err(error) => return RuntimeDeviceCheck::NotVerified(error.to_string()), + }; + if !probe.import_ok { + return RuntimeDeviceCheck::NotVerified( + probe + .error + .unwrap_or_else(|| "torch did not import".to_owned()), + ); + } + let torch_version = probe.torch_version.unwrap_or_else(|| "unknown".to_owned()); + match probe.device_count { + Some(0) => RuntimeDeviceCheck::NoDevices { + torch_version, + hip_version: probe.hip_version.unwrap_or_else(|| "unknown".to_owned()), + }, + Some(device_count) => RuntimeDeviceCheck::Usable { + device_count, + torch_version, + }, + None => RuntimeDeviceCheck::NotVerified( + "torch imported but did not report a device count".to_owned(), + ), + } +} + +fn render_runtime_device_check(outcome: &RuntimeDeviceCheck) -> String { + let mut output = String::new(); + match outcome { + RuntimeDeviceCheck::Usable { + device_count, + torch_version, + } => { + let _ = writeln!( + output, + " device_check: usable ({device_count} device(s), torch {})", + sanitize_log_value(torch_version) + ); + } + RuntimeDeviceCheck::NotVerified(reason) => { + let _ = writeln!( + output, + " device_check: not_verified ({})", + sanitize_log_value(reason) + ); + } + RuntimeDeviceCheck::NoDevices { + torch_version, + hip_version, + } => { + let _ = writeln!(output, " device_check: no_devices"); + let _ = writeln!( + output, + " torch {} (hip {}) imported but reports 0 devices", + sanitize_log_value(torch_version), + sanitize_log_value(hip_version) + ); + // Deliberately no remedy: the reinstall that would satisfy the + // engine's pin is what produces this state, so naming it here would + // send people in a circle. Say what is wrong and let them choose. + let _ = writeln!( + output, + " serving will fail with `Failed to infer device type`; this torch is built \ + for a different ROCm version than the installed SDK" + ); + } + } + output +} + +fn report_runtime_device_check( + paths: &AppPaths, + engine: &str, + python: Option<&Path>, + runtime_key: &str, +) -> RuntimeDeviceCheck { + let library_paths = runtime_library_paths_for_key(paths, runtime_key); + let outcome = runtime_device_check(python, &library_paths); + print!("{}", render_runtime_device_check(&outcome)); + let (level, message) = match &outcome { + RuntimeDeviceCheck::Usable { device_count, .. } => ( + "info", + format!( + "engine={engine} runtime_id={runtime_key} device_check=usable devices={device_count}" + ), + ), + RuntimeDeviceCheck::NoDevices { + torch_version, + hip_version, + } => ( + "error", + format!( + "engine={engine} runtime_id={runtime_key} device_check=no_devices torch={torch_version} hip={hip_version}" + ), + ), + RuntimeDeviceCheck::NotVerified(reason) => ( + "info", + format!("engine={engine} runtime_id={runtime_key} device_check=not_verified: {reason}"), + ), + }; + record_cli_audit_event( + paths, + "engine", + "runtime_device_check", + level, + message, + None, + ); + outcome +} + +/// The build identifier of the torch that belongs to this runtime's SDK. +/// +/// Taken from the manifest, never from whatever torch happens to be installed. +/// The environment tells you what is there now, which after an engine install is +/// the engine's build — trusting that would let a runtime already holding the +/// wrong torch declare itself correct and never recover. +/// +/// Older manifests predate the recorded value, so fall back to deriving it from +/// the SDK version, which is how TheRock names these builds. +fn sdk_torch_build_for_key(paths: &AppPaths, runtime_key: &str) -> Option { + let manifests = therock::load_runtime_manifests(paths).ok()?; + let manifest = runtime_manifest_for_selector(&manifests, runtime_key)?; + if let Some(recorded) = manifest.sdk_torch.as_deref() + && let Some(build) = therock::split_local_version(recorded).1 + { + return Some(build.to_owned()); + } + let version = manifest + .rocm_sdk + .as_ref() + .and_then(|probe| probe.rocm_sdk_version.clone()) + .unwrap_or_else(|| manifest.version.clone()); + (!version.trim().is_empty()).then(|| format!("rocm{version}")) +} + +/// Settle which torch a managed runtime keeps after an engine install, then report. +/// +/// Every path that installs an engine into a managed runtime must call this. +/// Skipping it anywhere lets the engine's own torch win silently and can leave a +/// runtime that cannot open a device — including after an explicit +/// `rocm engines install --reinstall`, which is exactly what someone +/// reaches for when a runtime already looks wrong. +/// +/// External environments are left alone: rocm-cli does not own them. +/// Whether an install finished having produced a runtime that cannot serve. +/// +/// The conjunction is the point. An alignment that could not run may still leave +/// a working environment, and a device count of zero is expected wherever no GPU +/// is present — neither alone justifies failing a multi-gigabyte install. Both +/// together mean the runtime cannot open a device and we could not correct it. +const fn install_left_runtime_unusable( + alignment: &TorchAlignment, + devices: &RuntimeDeviceCheck, +) -> bool { + let unsettled = matches!( + alignment, + TorchAlignment::Unavailable { .. } | TorchAlignment::InstallFailed { .. } + ); + unsettled && matches!(devices, RuntimeDeviceCheck::NoDevices { .. }) +} + +/// An engine install finished having left a runtime that cannot open a device. +/// +/// A distinct type rather than a plain message so `install sdk` can tell this +/// apart from an ordinary engine-install failure. It deliberately tolerates the +/// latter — a failed engine install still leaves a good multi-gigabyte SDK +/// behind, and discarding that would be worse — but a runtime it has just left +/// unable to serve is the exact silent success this change exists to end, so +/// that one has to fail the command. +#[derive(Debug)] +struct UnusableRuntimeAfterInstall(String); + +impl std::fmt::Display for UnusableRuntimeAfterInstall { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl std::error::Error for UnusableRuntimeAfterInstall {} + +fn unusable_runtime_error(engine: &str, runtime_key: &str) -> anyhow::Error { + anyhow::Error::new(UnusableRuntimeAfterInstall(format!( + "the {engine} install left runtime `{runtime_key}` on a torch it cannot use: no GPU could \ + be opened, and the matching build from the SDK index could not be installed. The ROCm \ + SDK itself is installed; re-run once the index is reachable, or install a torch matching \ + the SDK into the runtime environment." + ))) +} + +/// Whether a failed engine auto-install should fail `rocm install sdk` itself. +/// +/// Only the unusable-runtime case does. Everything else — an unreachable engine +/// index, a resolver error, a missing build tool — leaves the SDK installed and +/// usable, so it warns and keeps the successful exit rather than throwing away +/// the install that did succeed. +fn engine_auto_install_failure_is_fatal(error: &anyhow::Error) -> bool { + error.is::() +} + +/// Everything `install sdk` does once the SDK itself is on disk: auto-install the +/// family's engine, record the install, and decide the exit code. +/// +/// The engine auto-install is a parameter so that decision is reachable from a +/// test without a multi-gigabyte install behind it. It is the part worth pinning: +/// a failed engine install must keep the successful exit, because the SDK is +/// installed and usable and only a separately retryable step is missing, while a +/// runtime this install left unable to open a device must not — reporting success +/// for that is the defect being fixed. The SDK's own audit event is recorded +/// either way, before the command fails, because the SDK install did complete. +fn finish_sdk_install( + paths: &AppPaths, + finalized: Option<&SdkInstallFinalization>, + audit_action: &str, + audit_detail: String, + auto_install: impl FnOnce(&AppPaths, &SdkInstallFinalization) -> Result<()>, +) -> Result<()> { + let mut unusable_runtime = None; + if let Some(finalized) = finalized + && let Err(error) = auto_install(paths, finalized) + { + record_cli_audit_event( + paths, + "engine", + "engine_auto_install", + "error", + format!( + "auto-install failed engine=vllm runtime_id={} family={}: {error}", + finalized.runtime_key, finalized.family + ), + None, + ); + if engine_auto_install_failure_is_fatal(&error) { + unusable_runtime = Some(error); + } else { + eprintln!("warning: automatic vLLM install failed: {error}"); + eprintln!( + "warning: SDK install completed; you can run `rocm engines install vllm --runtime-id {}` after vLLM is available in that runtime", + finalized.runtime_key + ); + } + } + record_cli_audit_event(paths, "runtime", audit_action, "info", audit_detail, None); + if let Some(error) = unusable_runtime { + return Err(error); + } + Ok(()) +} + +fn settle_engine_install( + paths: &AppPaths, + engine: &str, + runtime_key: &str, + response: &InstallResponse, +) -> Result<()> { + if response.managed_env == Some(false) { + return Ok(()); + } + let python = Path::new(&response.python_executable); + let sdk_build = sdk_torch_build_for_key(paths, runtime_key); + let alignment = + report_torch_alignment(paths, engine, python, runtime_key, sdk_build.as_deref()); + let diverged = deliberately_diverged_package(&alignment); + report_engine_dependency_check(paths, engine, Some(python), runtime_key, diverged); + let devices = report_runtime_device_check(paths, engine, Some(python), runtime_key); + + // Fail only on the conjunction: torch could not be settled AND the runtime + // cannot open a device. Either alone is survivable — an alignment that could + // not run may still leave a working environment, and a device count of zero + // is expected where no GPU is present. Together they mean this install + // produced a runtime that cannot serve, and reporting success for that is the + // whole failure this change exists to end. + if install_left_runtime_unusable(&alignment, &devices) { + return Err(unusable_runtime_error(engine, runtime_key)); + } + Ok(()) +} + fn engine_dependency_check( paths: &AppPaths, engine: &str, python: Option<&Path>, + realigned_package: Option<&str>, ) -> EngineDependencyCheck { let Some(python) = python else { return EngineDependencyCheck::NotVerified( @@ -7520,21 +8122,40 @@ fn engine_dependency_check( match rocm_core::check_dependencies(paths, python) { Ok(violations) => { let owned = rocm_core::violations_requiring(&violations, engine); - if owned.is_empty() { - EngineDependencyCheck::Satisfied - } else { - EngineDependencyCheck::Violated( - owned - .iter() - .map(|violation| violation.detail.clone()) - .collect(), - ) - } + let details: Vec = owned + .iter() + .map(|violation| violation.detail.clone()) + .collect(); + classify_dependency_details(details, realigned_package) } Err(error) => EngineDependencyCheck::NotVerified(error.to_string()), } } +/// Separate a deliberate divergence from a genuine violation. +/// +/// 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( + details: Vec, + realigned_package: Option<&str>, +) -> EngineDependencyCheck { + if details.is_empty() { + return EngineDependencyCheck::Satisfied; + } + let Some(package) = realigned_package else { + return EngineDependencyCheck::Violated(details); + }; + // `uv pip check` phrases the requirement as ``requires `torch==…` ``, so the + // package name followed by a specifier is what identifies the subject. + let marker = format!("`{package}=="); + if details.iter().all(|detail| detail.contains(&marker)) { + EngineDependencyCheck::ExpectedDivergence(details) + } else { + EngineDependencyCheck::Violated(details) + } +} + fn render_engine_dependency_check(engine: &str, outcome: &EngineDependencyCheck) -> String { let mut output = String::new(); match outcome { @@ -7558,6 +8179,19 @@ fn render_engine_dependency_check(engine: &str, outcome: &EngineDependencyCheck) " action: rocm engines install {engine} --reinstall" ); } + EngineDependencyCheck::ExpectedDivergence(details) => { + let _ = writeln!(output, " dependency_check: expected_divergence"); + for detail in details { + let _ = writeln!(output, " divergence: {}", sanitize_log_value(detail)); + } + // Deliberately not the reinstall remedy. It does not apply: every + // path that installs an engine realigns torch afterwards, so a + // reinstall reproduces this same state rather than resolving it. + let _ = writeln!( + output, + " action: none; the SDK's build is intended here (see torch_alignment above)" + ); + } } output } @@ -7848,6 +8482,9 @@ fn adopt_runtime_from_probe( python_executable: Some(python_executable.display().to_string()), pip_cache_dir: None, rocm_sdk: Some(probe), + // Adoption does not install torch, so the build is derived from the SDK + // version instead. + sdk_torch: None, read_only: true, imported_from: Some(install_root), installed_at_unix_ms: rocm_core::unix_time_millis(), @@ -26003,6 +26640,387 @@ ID_LIKE="suse opensuse" assert!(rendered.contains(" action: rocm engines install vllm --reinstall\n")); } + #[test] + fn a_divergence_on_the_realigned_package_does_not_advise_undoing_it() { + // The reinstall remedy is correct for a real violation and catastrophic + // here: it reinstates the engine's own build and restores a runtime that + // cannot open a device. + let outcome = classify_dependency_details( + vec![ + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed".to_owned(), + ], + Some("torch"), + ); + + assert!(matches!( + outcome, + EngineDependencyCheck::ExpectedDivergence(_) + )); + let rendered = render_engine_dependency_check("vllm", &outcome); + assert!(rendered.contains(" dependency_check: expected_divergence\n")); + assert!( + !rendered.contains("action: rocm engines install vllm --reinstall"), + "the reinstall remedy does not apply to a deliberate divergence: {rendered}" + ); + } + + #[test] + fn a_divergence_on_any_other_package_is_still_a_violation() { + let outcome = classify_dependency_details( + vec![ + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed".to_owned(), + "The package `vllm` requires `numpy==1.26.4`, but `2.0.0` is installed".to_owned(), + ], + Some("torch"), + ); + + assert!(matches!(outcome, EngineDependencyCheck::Violated(_))); + } + + /// The engine's build enumerates no devices against the installed SDK. + /// + /// Observed on MI300X: the engine pins a torch built for a different ROCm + /// version, installs it over the SDK's, and the runtime then reports zero + /// devices. The release is right; only the build is wrong. + #[test] + fn the_engines_build_is_replaced_by_the_sdk_build_of_the_same_release() { + let plan = plan_torch_alignment( + Some("rocm7.13.0"), + Some("2.11.0+gitd0c8b1f"), + Some("torch==2.11.0+gitd0c8b1f"), + "vllm", + ); + + assert_eq!( + plan, + TorchAlignmentPlan::Install { + wanted: "2.11.0+rocm7.13.0".to_owned(), + from: "2.11.0+gitd0c8b1f".to_owned(), + } + ); + } + + /// The SDK picked a torch *release* the engine does not accept. + /// + /// The mirror-image failure: here the SDK's own choice is the wrong one, and + /// simply keeping it would break the engine. The rule takes the release the + /// engine pins while still keeping the SDK's build of it — so neither the + /// engine's build nor the SDK's release selection wins outright. + #[test] + fn the_sdks_release_is_corrected_to_the_one_the_engine_pins() { + let plan = plan_torch_alignment( + Some("rocm7.14.0a20260611"), + Some("2.10.0+git8514f05"), + Some("torch==2.10.0+git8514f05"), + "vllm", + ); + + assert_eq!( + plan, + TorchAlignmentPlan::Install { + wanted: "2.10.0+rocm7.14.0a20260611".to_owned(), + from: "2.10.0+git8514f05".to_owned(), + } + ); + } + + #[test] + fn an_install_is_failed_only_when_it_left_a_runtime_that_cannot_serve() { + let no_devices = RuntimeDeviceCheck::NoDevices { + torch_version: "2.11.0+gitd0c8b1f".to_owned(), + hip_version: "7.2.53211".to_owned(), + }; + let usable = RuntimeDeviceCheck::Usable { + device_count: 8, + torch_version: "2.11.0+rocm7.13.0".to_owned(), + }; + let failed = TorchAlignment::InstallFailed { + wanted: "2.11.0+rocm7.13.0".to_owned(), + kept: "2.11.0+gitd0c8b1f".to_owned(), + error: "dns error".to_owned(), + }; + let realigned = TorchAlignment::Realigned { + from: "2.11.0+gitd0c8b1f".to_owned(), + to: "2.11.0+rocm7.13.0".to_owned(), + }; + + // Could not settle torch, and the runtime sees nothing: unusable. + assert!(install_left_runtime_unusable(&failed, &no_devices)); + // Could not settle torch, but the runtime works anyway: not our call to fail. + assert!(!install_left_runtime_unusable(&failed, &usable)); + // Settled fine; no devices just means no GPU on this host. + assert!(!install_left_runtime_unusable(&realigned, &no_devices)); + } + + fn sdk_install_finalization() -> SdkInstallFinalization { + SdkInstallFinalization { + runtime_key: "wheel-gfx942-7.13.0".to_owned(), + install_root: PathBuf::from("/tmp/does-not-need-to-exist"), + family: "gfx94X-dcgpu".to_owned(), + } + } + + /// Run the real `install sdk` completion path against a stubbed auto-install. + fn finish_sdk_install_with( + paths: &AppPaths, + outcome: Result<()>, + ) -> (Result<()>, SdkInstallFinalization) { + let finalized = sdk_install_finalization(); + let result = finish_sdk_install( + paths, + Some(&finalized), + "install_sdk", + "sdk install completed".to_owned(), + |_, _| outcome, + ); + (result, finalized) + } + + #[test] + fn install_sdk_fails_when_the_install_left_a_runtime_that_cannot_open_a_device() -> Result<()> { + let (_root, paths) = test_paths("install-sdk-unusable"); + + // Exercises the command's own completion path, not just the predicate: if the + // catch goes back to warning and falling through, this fails. + let (result, _) = finish_sdk_install_with( + &paths, + Err(unusable_runtime_error("vllm", "wheel-gfx942-7.13.0")), + ); + let error = result.expect_err("an unusable runtime must fail `install sdk`"); + // And it must still say the SDK survived, so a transient index failure does + // not read as a ruined install that has to be started from scratch. + assert!( + error + .to_string() + .contains("The ROCm SDK itself is installed"), + "the failure must say the SDK survived:\n{error}" + ); + + // The SDK install did complete, so its record is written before the command + // fails — otherwise the audit trail would show an install that never happened. + let actions: Vec = load_recent_audit_events(&paths, 10)? + .into_iter() + .map(|event| event.action) + .collect(); + assert!( + actions.iter().any(|action| action == "install_sdk"), + "the successful SDK install was not recorded before failing: {actions:?}" + ); + Ok(()) + } + + #[test] + fn install_sdk_survives_an_engine_install_failure() -> Result<()> { + let (_root, paths) = test_paths("install-sdk-engine-failure"); + + // The complement, and the reason the catch exists at all: the SDK is installed + // and usable, only a separately retryable step is missing. Failing here would + // throw away a multi-gigabyte install over an unreachable engine index. + let (result, _) = finish_sdk_install_with( + &paths, + Err(anyhow::anyhow!( + "failed to reach the engine index: dns error" + )), + ); + assert!( + result.is_ok(), + "an engine install failure must not discard a good SDK install: {:?}", + result.err() + ); + + let actions: Vec = load_recent_audit_events(&paths, 10)? + .into_iter() + .map(|event| event.action) + .collect(); + assert!(actions.iter().any(|action| action == "install_sdk")); + assert!( + actions.iter().any(|action| action == "engine_auto_install"), + "the engine failure must still be recorded: {actions:?}" + ); + Ok(()) + } + + #[test] + fn the_unusable_runtime_failure_survives_being_wrapped_in_context() { + // The error reaches the catch through several `?` hops. Flattening it into a + // plain message is the realistic way a later change silently restores the + // exit-0 bug, since every failure would then look recoverable. + let propagated: Result<()> = Err(unusable_runtime_error("vllm", "wheel-gfx942-7.13.0")) + .context("automatic vLLM install failed"); + let propagated = propagated.expect_err("expected the unusable-runtime error"); + assert!(engine_auto_install_failure_is_fatal(&propagated)); + assert!(!engine_auto_install_failure_is_fatal(&anyhow::anyhow!( + "uv exited with status 1" + ))); + } + + #[test] + fn an_install_failure_is_not_reported_as_a_missing_wheel() { + // Saying "the index publishes no such build" over a network or disk error + // sends the reader hunting for a wheel that exists. Anything the resolver + // did not actually attribute to a missing version degrades to the honest + // answer: the failure, verbatim. + assert!(!install_error_reports_version_unavailable( + "failed to launch uv: Permission denied" + )); + assert!(!install_error_reports_version_unavailable( + "error sending request: dns error: failed to lookup address" + )); + assert!(install_error_reports_version_unavailable( + "No solution found when resolving: torch==2.10.0+rocm7.14.0a20260611" + )); + } + + #[test] + fn an_unresolvable_version_still_reads_as_unavailable() { + let rendered = render_torch_alignment(&TorchAlignment::Unavailable { + wanted: "2.10.0+rocm7.14.0a20260611".to_owned(), + kept: "2.10.0+git8514f05".to_owned(), + }); + + assert!(rendered.contains(" torch_alignment: unavailable\n")); + assert!(rendered.contains("publishes no 2.10.0+rocm7.14.0a20260611")); + } + + #[test] + fn a_failed_realignment_reports_the_error_it_actually_hit() { + let rendered = render_torch_alignment(&TorchAlignment::InstallFailed { + wanted: "2.11.0+rocm7.13.0".to_owned(), + kept: "2.11.0+gitd0c8b1f".to_owned(), + error: "failed to launch uv: Permission denied".to_owned(), + }); + + assert!(rendered.contains(" torch_alignment: install_failed\n")); + assert!(rendered.contains("Permission denied")); + assert!( + !rendered.contains("publishes no"), + "an install failure must not be described as a missing wheel: {rendered}" + ); + } + + #[test] + fn an_already_correct_runtime_is_left_alone() { + let plan = plan_torch_alignment( + Some("rocm7.13.0"), + Some("2.11.0+rocm7.13.0"), + Some("torch==2.11.0+gitd0c8b1f"), + "vllm", + ); + + assert_eq!( + plan, + TorchAlignmentPlan::AlreadyAligned { + version: "2.11.0+rocm7.13.0".to_owned() + } + ); + } + + #[test] + fn a_loose_torch_requirement_is_not_second_guessed() { + // Without an exact pin there is no release to carry over, so the engine's + // resolution stands rather than being overridden on a guess. + let plan = plan_torch_alignment( + Some("rocm7.13.0"), + Some("2.11.0+gitd0c8b1f"), + Some("torch>=2.10"), + "vllm", + ); + + assert!( + matches!(plan, TorchAlignmentPlan::NotApplicable(reason) if reason.contains("exact")), + "a non-pinned requirement must be left alone" + ); + } + + #[test] + fn an_unidentified_sdk_build_is_not_acted_on() { + // Nothing to carry over, so guessing a build would be worse than doing + // nothing and letting the device check report the result. + let plan = plan_torch_alignment( + None, + Some("2.11.0+gitd0c8b1f"), + Some("torch==2.11.0+gitd0c8b1f"), + "vllm", + ); + + assert!(matches!(plan, TorchAlignmentPlan::NotApplicable(_))); + } + + /// A runtime already holding the engine's build must still be corrected. + /// + /// The build identifier comes from the manifest, so this stays right even + /// when the environment has already been overwritten — the case where + /// inferring it from the installed torch would call the wrong build correct + /// and leave the runtime broken for good. + #[test] + fn a_runtime_already_on_the_engines_build_is_still_corrected() { + let plan = plan_torch_alignment( + Some("rocm7.13.0"), + Some("2.11.0+gitd0c8b1f"), + Some("torch==2.11.0+gitd0c8b1f"), + "vllm", + ); + + assert_eq!( + plan, + TorchAlignmentPlan::Install { + wanted: "2.11.0+rocm7.13.0".to_owned(), + from: "2.11.0+gitd0c8b1f".to_owned(), + } + ); + } + + #[test] + fn a_runtime_that_sees_no_devices_names_the_torch_that_cannot_use_the_sdk() { + // The install succeeded and the engine's requirements are satisfied, so + // every other surface reports this runtime healthy. Measured on MI300X: + // this torch loads against a ROCm 7.13 SDK and enumerates nothing, and + // the first symptom would otherwise be a serve failure. + let rendered = render_runtime_device_check(&RuntimeDeviceCheck::NoDevices { + torch_version: "2.11.0+gitd0c8b1f".to_owned(), + hip_version: "7.2.53211".to_owned(), + }); + + assert!(rendered.contains(" device_check: no_devices\n")); + assert!(rendered.contains("2.11.0+gitd0c8b1f")); + assert!(rendered.contains("7.2.53211")); + assert!(rendered.contains("Failed to infer device type")); + // No remedy is offered on purpose: reinstalling to satisfy the engine's + // pin is what produces this state. + assert!( + !rendered.contains("action:"), + "a remedy that recreates the fault must not be suggested: {rendered}" + ); + } + + #[test] + fn a_usable_runtime_states_how_many_devices_it_found() { + let rendered = render_runtime_device_check(&RuntimeDeviceCheck::Usable { + device_count: 8, + torch_version: "2.11.0+rocm7.13.0".to_owned(), + }); + + assert_eq!( + rendered, + " device_check: usable (8 device(s), torch 2.11.0+rocm7.13.0)\n" + ); + } + + #[test] + fn a_device_check_without_an_interpreter_is_not_verified_not_healthy() { + // Same rule as the dependency check: absence of an answer is never + // allowed to read as a passing one. + let outcome = runtime_device_check(None, &[]); + + assert_eq!( + outcome, + RuntimeDeviceCheck::NotVerified( + "the runtime's Python environment could not be located".to_owned() + ) + ); + assert!(render_runtime_device_check(&outcome).contains("not_verified")); + } + #[test] fn an_unlocatable_environment_is_reported_not_assumed_healthy() { // The engine install can fail before it reports its own interpreter. That path @@ -26010,7 +27028,7 @@ ID_LIKE="suse opensuse" // than fall through to `satisfied`. let (root, paths) = test_paths("engine-dependency-no-python"); - let outcome = engine_dependency_check(&paths, "vllm", None); + let outcome = engine_dependency_check(&paths, "vllm", None, None); let _ = fs::remove_dir_all(root); assert_eq!( @@ -27070,6 +28088,7 @@ ID_LIKE="suse opensuse" ], ..therock::RocmSdkPythonProbe::default() }), + sdk_torch: None, read_only: false, imported_from: None, installed_at_unix_ms, @@ -27108,6 +28127,7 @@ ID_LIKE="suse opensuse" python_executable: Some("python".to_owned()), pip_cache_dir: None, rocm_sdk: None, + sdk_torch: None, read_only: false, imported_from: None, installed_at_unix_ms: 1, diff --git a/apps/rocm/src/storage.rs b/apps/rocm/src/storage.rs index fcc0163cc..f8e910dbe 100644 --- a/apps/rocm/src/storage.rs +++ b/apps/rocm/src/storage.rs @@ -968,6 +968,7 @@ mod tests { python_executable: None, pip_cache_dir: None, rocm_sdk: None, + sdk_torch: None, read_only: false, imported_from: None, installed_at_unix_ms, diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index cf5fcb7e1..d8cda40dc 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -266,6 +266,14 @@ pub(crate) struct InstalledRuntimeManifest { pub pip_cache_dir: Option, #[serde(default)] pub rocm_sdk: Option, + /// The torch this SDK install wrote, e.g. `2.11.0+rocm7.13.0`. + /// + /// Recorded because an engine install later overwrites torch in the same + /// environment. Reading the environment afterwards tells you what is there + /// now, not which build belongs to these libraries; only the manifest still + /// knows that, and it has to survive repeat installs to be worth anything. + #[serde(default)] + pub sdk_torch: Option, #[serde(default)] pub read_only: bool, #[serde(default)] @@ -977,6 +985,7 @@ fn install_wheel_runtime( python_executable: Some(env_python.display().to_string()), pip_cache_dir: None, rocm_sdk: Some(rocm_sdk_probe.clone()), + sdk_torch: Some(resolution.package_versions.torch.clone()), read_only: false, imported_from: None, installed_at_unix_ms: unix_time_millis(), @@ -1105,6 +1114,7 @@ fn install_tarball_runtime( python_executable: None, pip_cache_dir: None, rocm_sdk: None, + sdk_torch: None, read_only: false, imported_from: None, installed_at_unix_ms: unix_time_millis(), @@ -2630,6 +2640,214 @@ fn parse_rocm_sdk_probe(output: &str) -> Result { serde_json::from_str(output.trim()).context("failed to parse rocm_sdk probe output") } +/// What the runtime's torch reports about the GPUs it can actually open. +/// +/// [`validate_rocm_sdk_runtime_probe`] establishes that the SDK's libraries are +/// present and resolvable. That is not the same question as whether the torch +/// sharing the venv can enumerate a device: a torch built against a different +/// HIP version loads happily against those libraries and then reports no +/// devices at all. +#[derive(Debug, Clone, Default, serde::Deserialize)] +pub(crate) struct RuntimeDeviceProbe { + pub import_ok: bool, + pub torch_version: Option, + pub hip_version: Option, + /// `None` when torch never imported, so "unknown" stays distinct from "zero". + pub device_count: Option, + pub error: Option, +} + +/// Ask the runtime's own interpreter how many devices its torch can open. +/// +/// `library_paths` must be the runtime's recorded ROCm library directories (see +/// [`RocmSdkPythonProbe::library_paths`]). They are prepended to +/// `LD_LIBRARY_PATH` for the child, which is how a served process resolves them. +/// +/// This cannot be replaced with an in-process `rocm_sdk.initialize_process()` +/// call in the probe script. Measured on MI300X against a runtime that serves +/// correctly: with the library directories on `LD_LIBRARY_PATH` torch reports 8 +/// devices, and with `initialize_process()` alone it reports 0. A probe built on +/// the latter would fail healthy runtimes and send people to reinstall them, +/// which is the operation that breaks them. +pub(crate) fn probe_runtime_devices( + python_executable: &Path, + library_paths: &[PathBuf], +) -> Result { + let mut env = Vec::new(); + if !library_paths.is_empty() { + let mut entries = library_paths.to_vec(); + if let Some(existing) = std::env::var_os(LIBRARY_PATH_ENV) { + entries.extend(split_runtime_path(&existing)); + } + let joined = std::env::join_paths(entries) + .context("failed to compose the runtime library path for the device probe")?; + env.push(( + LIBRARY_PATH_ENV.to_owned(), + joined.to_string_lossy().into_owned(), + )); + } + let text = capture_python_stdout_with_env( + python_executable, + RUNTIME_DEVICE_PROBE_SCRIPT, + &env, + "launch runtime device probe", + ) + .with_context(|| { + format!( + "failed to launch runtime device probe via {}", + python_executable.display() + ) + })?; + serde_json::from_str(text.trim()).context("failed to parse runtime device probe output") +} + +/// The loader search-path variable used to expose the runtime's ROCm libraries. +#[cfg(windows)] +const LIBRARY_PATH_ENV: &str = "PATH"; +#[cfg(not(windows))] +const LIBRARY_PATH_ENV: &str = "LD_LIBRARY_PATH"; + +/// Reports what torch sees, never raising: an unusable runtime must be +/// described, not turned into a probe crash. +const RUNTIME_DEVICE_PROBE_SCRIPT: &str = r#" +import json + +out = { + "import_ok": False, + "torch_version": None, + "hip_version": None, + "device_count": None, + "error": None, +} + +try: + import torch + + out["import_ok"] = True + out["torch_version"] = getattr(torch, "__version__", None) + out["hip_version"] = getattr(getattr(torch, "version", None), "hip", None) + out["device_count"] = int(torch.cuda.device_count()) +except Exception as exc: + out["error"] = type(exc).__name__ + ": " + str(exc) + +print(json.dumps(out)) +"#; + +/// What torch is installed, and what torch the engine's metadata demands. +#[derive(Debug, Clone, Default, serde::Deserialize)] +pub(crate) struct TorchAlignmentProbe { + /// The installed torch, e.g. `2.11.0+rocm7.13.0`. `None` if torch is absent. + pub installed_torch: Option, + /// The engine's pinned requirement, e.g. `torch==2.11.0+gitd0c8b1f`. `None` + /// when the engine is not installed yet or does not pin torch. + pub engine_requires_torch: Option, +} + +/// Read installed/required torch from distribution metadata, without importing +/// torch. Metadata questions must not depend on the runtime being usable — this +/// is called both before and after an engine install, and in the broken state +/// importing torch is exactly what fails. +pub(crate) fn probe_torch_alignment( + python_executable: &Path, + engine_distribution: &str, +) -> Result { + let env = vec![( + "ROCM_CLI_PROBE_DIST".to_owned(), + engine_distribution.to_owned(), + )]; + let text = capture_python_stdout_with_env( + python_executable, + TORCH_ALIGNMENT_PROBE_SCRIPT, + &env, + "launch torch alignment probe", + )?; + serde_json::from_str(text.trim()).context("failed to parse torch alignment probe output") +} + +const TORCH_ALIGNMENT_PROBE_SCRIPT: &str = r#" +import json +import os +import re +import importlib.metadata as md + +out = {"installed_torch": None, "engine_requires_torch": None} + +try: + out["installed_torch"] = md.version("torch") +except Exception: + pass + +try: + for raw in md.requires(os.environ.get("ROCM_CLI_PROBE_DIST", "vllm")) or []: + # `Requires-Dist` entries carry environment markers after ';'. Only the + # requirement itself matters here. + requirement = raw.split(";")[0].strip() + # The distribution name runs up to the first specifier, extra or space — + # and there is usually no space at all, as in `torch==2.11.0+gitd0c8b1f`. + matched = re.match(r"[A-Za-z0-9._-]+", requirement) + if matched is None: + continue + if matched.group(0).lower().replace("_", "-") == "torch": + out["engine_requires_torch"] = requirement + break +except Exception: + pass + +print(json.dumps(out)) +"#; + +/// Split a version into its public part and its local segment. +/// +/// `2.11.0+rocm7.13.0` -> `("2.11.0", Some("rocm7.13.0"))`. The local segment is +/// the build identifier: for TheRock wheels it names the ROCm build, and for the +/// engine's own index it is an opaque commit tag. +pub(crate) fn split_local_version(version: &str) -> (&str, Option<&str>) { + match version.split_once('+') { + Some((base, local)) => (base, Some(local)), + None => (version, None), + } +} + +/// The version pinned by a `==` requirement, e.g. `torch==2.11.0+git…` -> the +/// version. Returns `None` for any looser requirement, since only an exact pin +/// tells us which release the engine was built against. +pub(crate) fn requirement_pinned_version(requirement: &str) -> Option<&str> { + let (_, version) = requirement.split_once("==")?; + let version = version.trim(); + if version.is_empty() || version.contains(',') { + return None; + } + Some(version) +} + +/// Install one exact package version from `index_url` into `python_executable`. +/// +/// Used to put the SDK's build of a package back after another installer has +/// replaced it. `--reinstall-package` is required: without it uv treats the +/// already-present distribution as satisfying the request and does nothing. +pub(crate) fn install_pinned_package( + paths: &AppPaths, + python_executable: &Path, + index_url: &str, + package: &str, + requirement: &str, +) -> Result<()> { + let uv = rocm_core::uv::ensure_uv_binary(paths) + .context("failed to acquire uv binary for the torch alignment install")?; + let mut args = rocm_core::uv::uv_pip_install_base(python_executable); + args.push("--extra-index-url".to_owned()); + args.push(index_url.to_owned()); + args.push("--reinstall-package".to_owned()); + args.push(package.to_owned()); + args.push(requirement.to_owned()); + let borrowed = args.iter().map(String::as_str).collect::>(); + run_command( + &uv, + &borrowed, + "install the SDK build of the engine's torch", + ) +} + pub(crate) fn validate_rocm_sdk_runtime_probe(probe: &RocmSdkPythonProbe) -> Result<()> { if !probe.import_ok { bail!( @@ -2835,6 +3053,50 @@ fn capture_command_output_with_temp_files(program: &Path, args: &[&str]) -> Resu }) } +/// Run a probe script with extra environment set and return its stdout. +/// +/// The script goes to a temp file rather than `python -c`, because the loader +/// search path this exists to set must reach the child through its environment, +/// and a file keeps the invocation identical on every platform. +fn capture_python_stdout_with_env( + python_executable: &Path, + script: &str, + env: &[(String, String)], + context_text: &str, +) -> Result { + let temp_root = if runtime_is_windows() { + windows_temp_dir("rocm-cli-python-probe")? + } else { + linux_temp_dir("rocm-cli-python-probe")? + }; + let script_path = temp_root.join("probe.py"); + fs::write(&script_path, script) + .with_context(|| format!("failed to write {}", script_path.display()))?; + + let mut command = Command::new(python_executable); + command.arg(&script_path); + for (key, value) in env { + command.env(key, value); + } + let output = command + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output() + .with_context(|| format!("failed to launch {}", python_executable.display())); + let _ = fs::remove_dir_all(&temp_root); + let output = output?; + + if !output.status.success() { + bail!( + "{context_text}: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + String::from_utf8(output.stdout) + .with_context(|| format!("{context_text}: failed to decode Python output")) +} + fn capture_python_stdout( python_executable: &Path, script: &str, @@ -5319,6 +5581,7 @@ echo Python 3.12.10 python_executable: Some("python".to_owned()), pip_cache_dir: None, rocm_sdk: None, + sdk_torch: None, read_only: false, imported_from: None, installed_at_unix_ms, diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index b66cb92e8..44f0a84ab 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -19,18 +19,21 @@ Feature: Runtime configuration When the user inspects the system Then the managed runtime folder path is not recursively nested - # The SDK and the engine share one Python environment, so a second - # `install sdk` wrote the SDK's torch stack over the build the engine pins. The - # engine still resolved, so the install reported success and every health surface - # kept saying `ready` — the first signal was a serve failure naming neither. Needs - # a real SDK install, a real engine install, and a second SDK install, so it runs - # on the nightly GPU lane. `@requires-engine:vllm` because only vLLM shares the - # runtime environment; Lemonade manages its own. + # The SDK and the engine share one Python environment and both write torch into + # it, so a second `install sdk` could leave a torch that one of the two cannot + # use. Every health surface still reported `ready` and the install still exited + # 0 — the first signal was a serve failure naming neither. The runtime now + # settles on the SDK's build of the release the engine pins, so this asserts the + # outcome that actually matters rather than the wording of a check: can the + # runtime still reach the GPU afterwards. Needs a real SDK install, a real engine + # install, and a second SDK install, so it runs on the nightly GPU lane. + # `@requires-engine:vllm` because only vLLM shares the runtime environment; + # Lemonade manages its own. @id:runtime-sdk-reinstall-keeps-engine-consistent @requires-gpu @requires-engine:vllm @nightly - Scenario: 4 - Reinstalling the SDK leaves the installed engine's requirements satisfied + Scenario: 4 - Reinstalling the SDK leaves the installed engine able to use the GPU Given a managed runtime with an inference engine already installed When the user installs the SDK again - Then the install reports the engine's requirements as satisfied + Then the runtime can still use the GPU # The GPU E2E lanes no longer install the shared runtime once and keep it # forever: `xtask e2e-prewarm` asks `rocm update` whether the channel index has diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index caae41986..cc9ad08c3 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -172,16 +172,23 @@ async fn user_reinstalls_sdk(world: &mut E2eWorld) { user_installs_sdk(world).await; } -#[then("the install reports the engine's requirements as satisfied")] -async fn assert_engine_requirements_satisfied(world: &mut E2eWorld) { +#[then("the runtime can still use the GPU")] +async fn assert_runtime_can_still_use_the_gpu(world: &mut E2eWorld) { let output = world.cli_output.as_deref().expect("no install output"); + // The functional signal rather than a diagnostic string: the device check asks + // the runtime's own torch how many devices it can open. A runtime left holding + // a torch that one of the two installers cannot use reports none, and that is + // the failure this scenario exists to catch. assert!( - !output.contains("dependency_check: violated"), - "the reinstall left the engine's declared requirements unmet:\n{output}" + output.contains("device_check: usable"), + "the reinstall left a runtime that cannot open a GPU:\n{output}" ); + // A genuine unmet requirement must still fail the scenario. Torch itself is + // expected to diverge from the engine's exact pin once it is settled on the + // SDK's build of the same release, and that is reported as a divergence. assert!( - output.contains("dependency_check: satisfied"), - "the install did not report on the engine's requirements at all:\n{output}" + !output.contains("dependency_check: violated"), + "the reinstall left a genuine requirement unmet:\n{output}" ); } @@ -192,8 +199,9 @@ async fn assert_engine_requirements_satisfied(world: &mut E2eWorld) { /// violated — that false green is the very thing this feature's scenario exists /// to catch — so asserting it afterwards would pass whether or not the fix /// works. Teaching that surface to notice a violated pin is tracked separately; -/// until it does, the `dependency_check: satisfied` assertion is the only -/// falsifiable signal available. +/// until it does, the device check is the falsifiable signal, because it asks +/// the runtime how many GPUs it can actually open rather than whether it looks +/// installed. fn assert_engine_ready(world: &mut E2eWorld) { let (stdout, _, _) = crate::run_rocm(world, &["engines", "list"]); assert!( From 06691850118a9a63975f209430873d0572783159 Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Fri, 28 Aug 2026 11:44:03 +0000 Subject: [PATCH 02/19] fix(install): keep the engine from undoing the SDK torch alignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The alignment left one of vLLM's own pins permanently unsatisfied, and the engine reads exactly that signal to decide a runtime needs repairing. So the two mechanisms fought: `uv pip check` reported the intended divergence as vLLM's violation, the engine reinstalled vLLM and rewrote torch back to its own build, the CLI realigned it, and the next invocation started over. Two full torch-stack flips per `engines install` or `install sdk`, a repeated multi-gigabyte download on cache-cold hosts, and a warning claiming a repair that had just undone the intended state. The engine now applies the same rule the CLI does — release from the pin, build from the SDK — reading the build from the runtime manifest rather than the environment, for the reason the CLI already does: after an install the environment holds the engine's build, so trusting it would call the wrong torch correct. A torch of the wrong release, of a build belonging to neither side, or in a runtime whose manifest identifies no SDK build all still force the reinstall. Also from review: - Skip settling for engines that manage their own runtime. Lemonade reports `managed_env: Some(true)` but its `python_executable` is a native binary, so it was being spawned twice with a generated `.py` path as `argv[1]`, printing torch and device checks for a runtime that has neither — on the serve path. - Run the realignment install through the uv environment every other uv call uses, restoring `UV_HTTP_TIMEOUT` and the managed `UV_CACHE_DIR` (without which uv silently copies the whole stack per environment) and the e2e shared cache. It also captures stderr on Windows, where the caller classifies this install's outcome by matching the resolver's message. - Make the SDK index authoritative for that install and add `--no-deps`, so a build the index does not publish cannot resolve against PyPI behind the `Unavailable` check, and a surgical swap cannot move the rest of the stack. - Keep the device probe's real exception instead of replacing it with the generic no-count line; a raising `device_count()` is the diagnostic it exists to capture. - Settle after the install header so the check blocks print under the lines they describe, and the config bookkeeping still lands when settling fails. - Name the engine in the realigned line, which sanitized a literal before. - Split a merged doc block that left `settle_engine_install` undocumented, and correct `report_torch_alignment`'s, which described another function. Test coverage follows the fixes: the manifest lookup the repair path rests on was untested and now covers both the recorded value and the reconstruction older manifests need; a byte-identical duplicate test is replaced by one that exercises the facet it claimed; and the two alignment states a healthy run produces were unasserted. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 224 +++++++++++++++---- apps/rocm/src/therock.rs | 39 +++- crates/rocm-core/src/lib.rs | 6 +- crates/rocm-core/src/uv.rs | 132 +++++++++++ engines/vllm/src/lib.rs | 427 ++++++++++++++++++++++++++++++------ 5 files changed, 703 insertions(+), 125 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index ceff82631..d82c83b6c 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -3787,7 +3787,6 @@ fn engines(command: EnginesCommand) -> Result<()> { }, env_root.as_deref(), )?; - settle_engine_install(&paths, &engine, &runtime_id, &response)?; println!("engine install"); println!(" engine: {engine}"); println!(" runtime_id: {runtime_id}"); @@ -3813,6 +3812,12 @@ fn engines(command: EnginesCommand) -> Result<()> { config.save(&paths)?; let _ = seeded_preference; } + // Settle last, matching `maybe_auto_install_sdk_preferred_engine`. The + // check blocks then print under the `engine:`/`runtime_id:`/`env_id:` + // lines they describe instead of above them, and the config bookkeeping + // above still lands when settling fails — the engine did install; it is + // the runtime it left behind that is being reported on. + settle_engine_install(&paths, &engine, &runtime_id, &response)?; record_cli_audit_event( &paths, "engine", @@ -4008,7 +4013,12 @@ fn ensure_self_managed_engine_ready( }, env_root.as_deref(), )?; - settle_engine_install(paths, engine, &runtime_id, &response)?; + // No `settle_engine_install` here. This function returns at the top unless + // `engine_manages_own_runtime(engine)`, and that is exactly the case + // `settles_runtime_torch` declines: the runtime holds the engine's own + // binary, not an interpreter with a torch in it. Calling it would be inert + // at best, and a call that provably cannot act invites someone to "fix" the + // gate later. Some(response) }; @@ -7674,7 +7684,13 @@ fn align_runtime_torch( } } -fn render_torch_alignment(outcome: &TorchAlignment) -> String { +/// Render one alignment outcome as the `torch_alignment:` check block. +/// +/// `engine` names the engine whose pin decided the release, so the realigned line +/// reads `... the release vllm pins` rather than an anonymous "the engine". It +/// arrives from the engine selection and is sanitized like every other +/// interpolated value. +fn render_torch_alignment(outcome: &TorchAlignment, engine: &str) -> String { let mut output = String::new(); match outcome { TorchAlignment::AlreadyAligned { version } => { @@ -7691,7 +7707,7 @@ fn render_torch_alignment(outcome: &TorchAlignment) -> String { " {} -> {} (the SDK's build of the release {} pins)", sanitize_log_value(from), sanitize_log_value(to), - sanitize_log_value("the engine") + sanitize_log_value(engine) ); } TorchAlignment::Unavailable { wanted, kept } => { @@ -7728,12 +7744,12 @@ fn render_torch_alignment(outcome: &TorchAlignment) -> String { output } -/// Returns the package the runtime now deliberately diverges on, if any, so the -/// dependency check can tell that divergence apart from a real violation. +/// Settle the runtime's torch, print the `torch_alignment:` block, and record it. /// -/// Both `Realigned` and `AlreadyAligned` diverge from the engine's exact pin — -/// the second is simply a rerun over a runtime already put right, which is the -/// normal state on every refresh after the first. +/// The outcome is returned so the dependency and device checks that follow can be +/// read against it — `deliberately_diverged_package` turns it into the one package +/// whose divergence is intended, and `install_left_runtime_unusable` pairs it with +/// the device count. fn report_torch_alignment( paths: &AppPaths, engine: &str, @@ -7743,7 +7759,7 @@ fn report_torch_alignment( ) -> TorchAlignment { let index_url = runtime_index_url_for_key(paths, runtime_key); let outcome = align_runtime_torch(paths, python, index_url.as_deref(), sdk_build, engine); - print!("{}", render_torch_alignment(&outcome)); + print!("{}", render_torch_alignment(&outcome, engine)); let (level, message) = match &outcome { TorchAlignment::AlreadyAligned { version } => ( "info", @@ -7850,8 +7866,15 @@ fn runtime_device_check(python: Option<&Path>, library_paths: &[PathBuf]) -> Run device_count, torch_version, }, + // A `device_count()` that raises — HIP init failures are the live example — + // reports no count and puts the real exception in `error`. That exception is + // precisely the diagnostic this probe exists to capture, so prefer it over + // the generic line, which is only right when the probe returned nothing at + // all to explain itself. None => RuntimeDeviceCheck::NotVerified( - "torch imported but did not report a device count".to_owned(), + probe + .error + .unwrap_or_else(|| "torch imported but did not report a device count".to_owned()), ), } } @@ -7953,6 +7976,15 @@ fn report_runtime_device_check( fn sdk_torch_build_for_key(paths: &AppPaths, runtime_key: &str) -> Option { let manifests = therock::load_runtime_manifests(paths).ok()?; let manifest = runtime_manifest_for_selector(&manifests, runtime_key)?; + sdk_torch_build_from_manifest(manifest) +} + +/// The build identifier one manifest names, with the fallback for older manifests. +/// +/// Split out from the lookup so the decision can be tested without a registry on +/// disk: the lookup is a directory scan, but this is the part that has to be right +/// for a runtime already stuck on the engine's build to recover. +fn sdk_torch_build_from_manifest(manifest: &therock::InstalledRuntimeManifest) -> Option { if let Some(recorded) = manifest.sdk_torch.as_deref() && let Some(build) = therock::split_local_version(recorded).1 { @@ -7966,15 +7998,6 @@ fn sdk_torch_build_for_key(paths: &AppPaths, runtime_key: &str) -> Option --reinstall`, which is exactly what someone -/// reaches for when a runtime already looks wrong. -/// -/// External environments are left alone: rocm-cli does not own them. /// Whether an install finished having produced a runtime that cannot serve. /// /// The conjunction is the point. An alignment that could not run may still leave @@ -8079,13 +8102,36 @@ fn finish_sdk_install( Ok(()) } +/// Whether rocm-cli owns the torch in the runtime this install just produced. +/// +/// Two kinds of environment are left alone, because there is no torch here that +/// rocm-cli put in place. External environments are the obvious one. The subtler +/// one is an engine that manages its own runtime: those report +/// `managed_env: Some(true)` — rocm-cli did create the runtime — but their +/// `python_executable` is the engine's native binary, not an interpreter, and no +/// torch ever lived in there. Probing one anyway spawns that binary with a +/// generated `.py` path as `argv[1]` and prints check blocks about a package the +/// runtime never had. +fn settles_runtime_torch(engine: &str, managed_env: Option) -> bool { + managed_env != Some(false) && !engine_manages_own_runtime(engine) +} + +/// Settle which torch a managed runtime keeps after an engine install, then report. +/// +/// Every path that installs an engine into a managed runtime must call this. +/// Skipping it anywhere lets the engine's own torch win silently and can leave a +/// runtime that cannot open a device — including after an explicit +/// `rocm engines install --reinstall`, which is exactly what someone +/// reaches for when a runtime already looks wrong. +/// +/// See `settles_runtime_torch` for the environments this deliberately skips. fn settle_engine_install( paths: &AppPaths, engine: &str, runtime_key: &str, response: &InstallResponse, ) -> Result<()> { - if response.managed_env == Some(false) { + if !settles_runtime_torch(engine, response.managed_env) { return Ok(()); } let python = Path::new(&response.python_executable); @@ -26873,10 +26919,13 @@ ID_LIKE="suse opensuse" #[test] fn an_unresolvable_version_still_reads_as_unavailable() { - let rendered = render_torch_alignment(&TorchAlignment::Unavailable { - wanted: "2.10.0+rocm7.14.0a20260611".to_owned(), - kept: "2.10.0+git8514f05".to_owned(), - }); + let rendered = render_torch_alignment( + &TorchAlignment::Unavailable { + wanted: "2.10.0+rocm7.14.0a20260611".to_owned(), + kept: "2.10.0+git8514f05".to_owned(), + }, + "vllm", + ); assert!(rendered.contains(" torch_alignment: unavailable\n")); assert!(rendered.contains("publishes no 2.10.0+rocm7.14.0a20260611")); @@ -26884,11 +26933,14 @@ ID_LIKE="suse opensuse" #[test] fn a_failed_realignment_reports_the_error_it_actually_hit() { - let rendered = render_torch_alignment(&TorchAlignment::InstallFailed { - wanted: "2.11.0+rocm7.13.0".to_owned(), - kept: "2.11.0+gitd0c8b1f".to_owned(), - error: "failed to launch uv: Permission denied".to_owned(), - }); + let rendered = render_torch_alignment( + &TorchAlignment::InstallFailed { + wanted: "2.11.0+rocm7.13.0".to_owned(), + kept: "2.11.0+gitd0c8b1f".to_owned(), + error: "failed to launch uv: Permission denied".to_owned(), + }, + "vllm", + ); assert!(rendered.contains(" torch_alignment: install_failed\n")); assert!(rendered.contains("Permission denied")); @@ -26898,6 +26950,64 @@ ID_LIKE="suse opensuse" ); } + #[test] + fn a_realigned_runtime_names_both_builds_and_the_engine_that_decided_the_release() { + // The line a successful run actually prints, and the one a user reads when + // deciding whether the divergence reported just below it is expected. + let rendered = render_torch_alignment( + &TorchAlignment::Realigned { + from: "2.11.0+gitd0c8b1f".to_owned(), + to: "2.11.0+rocm7.13.0".to_owned(), + }, + "vllm", + ); + + assert!(rendered.contains(" torch_alignment: realigned\n")); + assert!(rendered.contains("2.11.0+gitd0c8b1f -> 2.11.0+rocm7.13.0")); + assert!( + rendered.contains("the release vllm pins"), + "the engine that pinned the release is named rather than left anonymous: {rendered}" + ); + } + + #[test] + fn an_already_aligned_runtime_reports_the_version_it_kept() { + // Every refresh after the first lands here, so this is the most frequently + // printed of the five outcomes. + let rendered = render_torch_alignment( + &TorchAlignment::AlreadyAligned { + version: "2.11.0+rocm7.13.0".to_owned(), + }, + "vllm", + ); + + assert!(rendered.contains(" torch_alignment: already_aligned (2.11.0+rocm7.13.0)\n")); + } + + #[test] + fn a_managed_python_runtime_is_settled() { + assert!(settles_runtime_torch("vllm", Some(true))); + assert!( + settles_runtime_torch("vllm", None), + "an engine that does not report the field is still assumed managed, as before" + ); + } + + #[test] + fn an_external_runtime_is_left_alone() { + assert!(!settles_runtime_torch("vllm", Some(false))); + } + + #[test] + fn an_engine_that_manages_its_own_runtime_is_left_alone() { + // Lemonade reports `managed_env: Some(true)` — rocm-cli did create the + // runtime — but its `python_executable` is the Lemonade binary, not an + // interpreter, and no torch ever lived there. Settling it would spawn that + // binary twice with a generated `.py` path as `argv[1]` and print torch and + // device check blocks for a runtime that has neither, on the serve path. + assert!(!settles_runtime_torch("lemonade", Some(true))); + } + #[test] fn an_already_correct_runtime_is_left_alone() { let plan = plan_torch_alignment( @@ -26946,30 +27056,54 @@ ID_LIKE="suse opensuse" assert!(matches!(plan, TorchAlignmentPlan::NotApplicable(_))); } - /// A runtime already holding the engine's build must still be corrected. + /// The build comes from the manifest, not from whatever torch is installed. /// - /// The build identifier comes from the manifest, so this stays right even - /// when the environment has already been overwritten — the case where - /// inferring it from the installed torch would call the wrong build correct - /// and leave the runtime broken for good. + /// This is what makes a runtime already overwritten by the engine recoverable: + /// reading the environment would call the engine's build the SDK's and leave the + /// runtime broken for good. `plan_torch_alignment` takes the build as an opaque + /// argument, so the property lives here, in the lookup that produces it. #[test] - fn a_runtime_already_on_the_engines_build_is_still_corrected() { - let plan = plan_torch_alignment( + fn the_sdk_build_is_read_from_the_manifest_not_the_environment() { + let mut manifest = + test_runtime_manifest_for_update("wheel-gfx94x", "gfx94x", "gfx94x-dcgpu", "7.13.0"); + manifest.sdk_torch = Some("2.11.0+rocm7.13.0".to_owned()); + + assert_eq!( + sdk_torch_build_from_manifest(&manifest).as_deref(), Some("rocm7.13.0"), - Some("2.11.0+gitd0c8b1f"), - Some("torch==2.11.0+gitd0c8b1f"), - "vllm", + "the recorded SDK torch names the build" + ); + } + + /// Manifests written before `sdk_torch` existed still have to yield a build. + /// + /// These are the runtimes already on real machines, so this fallback is the + /// repair path rather than a nicety. TheRock names the build `rocm`. + #[test] + fn a_manifest_without_a_recorded_torch_derives_the_build_from_the_sdk_version() { + let manifest = + test_runtime_manifest_for_update("wheel-gfx94x", "gfx94x", "gfx94x-dcgpu", "7.13.0"); + assert!( + manifest.sdk_torch.is_none(), + "this test is about the pre-change manifest shape" ); assert_eq!( - plan, - TorchAlignmentPlan::Install { - wanted: "2.11.0+rocm7.13.0".to_owned(), - from: "2.11.0+gitd0c8b1f".to_owned(), - } + sdk_torch_build_from_manifest(&manifest).as_deref(), + Some("rocm7.13.0") ); } + /// A manifest that names no version at all must not invent a build. + #[test] + fn a_manifest_with_no_version_identifies_no_build() { + let mut manifest = + test_runtime_manifest_for_update("wheel-gfx94x", "gfx94x", "gfx94x-dcgpu", " "); + manifest.sdk_torch = None; + + assert_eq!(sdk_torch_build_from_manifest(&manifest), None); + } + #[test] fn a_runtime_that_sees_no_devices_names_the_torch_that_cannot_use_the_sdk() { // The install succeeded and the engine's requirements are satisfied, so diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index d8cda40dc..6a0bc3521 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -2801,11 +2801,12 @@ print(json.dumps(out)) /// `2.11.0+rocm7.13.0` -> `("2.11.0", Some("rocm7.13.0"))`. The local segment is /// the build identifier: for TheRock wheels it names the ROCm build, and for the /// engine's own index it is an opaque commit tag. +/// +/// Defined in `rocm-core` because the vLLM engine has to make the same split to +/// recognise a runtime the CLI deliberately realigned; two copies of this would be +/// two places for the two sides to drift apart. pub(crate) fn split_local_version(version: &str) -> (&str, Option<&str>) { - match version.split_once('+') { - Some((base, local)) => (base, Some(local)), - None => (version, None), - } + rocm_core::uv::split_local_version(version) } /// The version pinned by a `==` requirement, e.g. `torch==2.11.0+git…` -> the @@ -2825,6 +2826,21 @@ pub(crate) fn requirement_pinned_version(requirement: &str) -> Option<&str> { /// Used to put the SDK's build of a package back after another installer has /// replaced it. `--reinstall-package` is required: without it uv treats the /// already-present distribution as satisfying the request and does nothing. +/// +/// `--index-url`, not `--extra-index-url`, so the SDK index is the only place a +/// candidate can come from. This matches how `install_therock_runtime` installs +/// from the same index, and it is load-bearing rather than cosmetic: with PyPI +/// left in the candidate set, a `+rocm` build the SDK index does not publish can +/// resolve against PyPI instead, and the caller's `Unavailable` classification — +/// the whole point of which is to say "the SDK index has no such build" — never +/// gets the resolver error it keys on. +/// +/// `--no-deps` because this is a surgical swap of one build for another build of +/// the *same release*: the environment already carries a resolved dependency tree +/// and re-resolving it here is free to move `torchvision`/`torchaudio` as a side +/// effect, which is the mixed stack this change is trying not to create. A build +/// that genuinely needs a different dependency is not silently ignored — the +/// `uv pip check` that runs immediately after reports it as a violation. pub(crate) fn install_pinned_package( paths: &AppPaths, python_executable: &Path, @@ -2835,15 +2851,26 @@ pub(crate) fn install_pinned_package( let uv = rocm_core::uv::ensure_uv_binary(paths) .context("failed to acquire uv binary for the torch alignment install")?; let mut args = rocm_core::uv::uv_pip_install_base(python_executable); - args.push("--extra-index-url".to_owned()); + args.push("--index-url".to_owned()); args.push(index_url.to_owned()); + args.push("--no-deps".to_owned()); args.push("--reinstall-package".to_owned()); args.push(package.to_owned()); args.push(requirement.to_owned()); let borrowed = args.iter().map(String::as_str).collect::>(); - run_command( + // `run_command_with_env`, not `run_command`, for two reasons. The uv environment + // carries `UV_HTTP_TIMEOUT` (uv has no `--timeout` flag) and `UV_CACHE_DIR`; + // without the latter uv falls back to `$HOME/.cache/uv`, loses hardlinking when + // that is on another filesystem, and silently copies the whole torch stack per + // environment — and the e2e lanes' shared cache is threaded through the same + // helper. It also captures stderr on every platform, where `run_command`'s + // Windows branch reports only an exit status; the caller classifies this + // install's outcome by matching the resolver's message, so on Windows an + // unpublished build would otherwise be reported as a generic failure. + run_command_with_env( &uv, &borrowed, + &rocm_core::uv::uv_command_env(paths), "install the SDK build of the engine's torch", ) } diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index e0369ecab..e6b495578 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -71,9 +71,9 @@ pub use runtime::{ }; pub use uv::{ DEFAULT_UV_TIMEOUT_SECS, DependencyViolation, UV_CACHE_DIR_ENV, UV_CACHE_DIR_OVERRIDE_ENV, - UvCacheSource, check_dependencies, ensure_uv_binary, uv_binary_name, uv_cache_source, - uv_command_env, uv_http_timeout_secs, uv_pip_check_args, uv_pip_freeze_args, - uv_pip_install_base, uv_venv_args, violations_requiring, + UvCacheSource, ViolationSubject, check_dependencies, ensure_uv_binary, split_local_version, + uv_binary_name, uv_cache_source, uv_command_env, uv_http_timeout_secs, uv_pip_check_args, + uv_pip_freeze_args, uv_pip_install_base, uv_venv_args, violation_subject, violations_requiring, }; pub const DEFAULT_LOCAL_PORT: u16 = 11_435; diff --git a/crates/rocm-core/src/uv.rs b/crates/rocm-core/src/uv.rs index ef99cf503..68bc24694 100644 --- a/crates/rocm-core/src/uv.rs +++ b/crates/rocm-core/src/uv.rs @@ -295,6 +295,78 @@ pub fn violations_requiring<'a>( .collect() } +/// What a violation line is *about*: the required package and what is there instead. +/// +/// `DependencyViolation` keeps `uv`'s line verbatim, which is right for display but +/// leaves every caller wanting to reason about a violation to re-parse it. Both the +/// CLI and the vLLM engine need the same two fields to tell a deliberate divergence +/// from a defect, so the parse lives here once. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ViolationSubject { + /// The package whose requirement is unsatisfied — `torch` in + /// ``requires `torch==2.11.0+rocm7.13.0` ``. + pub package: String, + /// The exact version the requirement pins, or `None` for any looser requirement. + /// + /// Only an exact pin says which release the requirer was built against, which is + /// the question callers reasoning about a divergence actually have. + pub required: Option, + /// The version installed instead, or `None` when `uv` reported the package as + /// not installed at all. + pub installed: Option, +} + +/// Pull the required package and the installed version out of one violation line. +/// +/// `uv` frames the line as ``The package `` requires ``, but +/// `` is installed``. Returns `None` for anything that does not carry both +/// halves of that frame, so a differently-shaped diagnostic yields no subject rather +/// than a confidently wrong one. +pub fn violation_subject(detail: &str) -> Option { + let (_, rest) = detail.split_once(UV_CHECK_REQUIREMENT_INFIX)?; + let (spec, rest) = rest.split_once('`')?; + let package = spec + .split(|character: char| { + !(character.is_alphanumeric() + || character == '-' + || character == '_' + || character == '.') + }) + .next() + .filter(|name| !name.is_empty())? + .to_owned(); + // A compound specifier (`>=1.0,<2.0`) pins nothing, and neither does a single + // inequality; only a lone `==` names the release the requirer was built against. + let required = spec + .split_once("==") + .map(|(_, version)| version.trim()) + .filter(|version| !version.is_empty() && !version.contains(',')) + .map(str::to_owned); + // `, but it's not installed` carries no version and correctly yields `None`. + let installed = rest + .split_once(", but `") + .and_then(|(_, tail)| tail.split_once('`')) + .map(|(version, _)| version.to_owned()); + Some(ViolationSubject { + package, + required, + installed, + }) +} + +/// Split a version into its public part and its local segment. +/// +/// `2.11.0+rocm7.13.0` -> `("2.11.0", Some("rocm7.13.0"))`. The local segment is the +/// build identifier: for TheRock wheels it names the ROCm build, and for an engine's +/// own index it is an opaque commit tag. Telling those two apart is what lets a +/// caller take the release from one source and the build from another. +pub fn split_local_version(version: &str) -> (&str, Option<&str>) { + match version.split_once('+') { + Some((base, local)) => (base, Some(local)), + None => (version, None), + } +} + /// Pull the unsatisfied-requirement lines out of a `uv pip check` stderr body. /// /// `uv` frames one as ``The package `` requires ``, but `` is @@ -836,4 +908,64 @@ All installed packages are compatible assert_eq!(slug("latest"), "latest"); assert_eq!(slug("weird/version space"), "weird-version-space"); } + + #[test] + fn a_violation_line_yields_the_package_and_both_versions() { + let subject = violation_subject( + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed", + ) + .expect("a well-formed violation line has a subject"); + + assert_eq!(subject.package, "torch"); + assert_eq!(subject.required.as_deref(), Some("2.11.0+gitd0c8b1f")); + assert_eq!(subject.installed.as_deref(), Some("2.11.0+rocm7.13.0")); + } + + #[test] + fn a_missing_package_has_no_installed_version() { + let subject = violation_subject( + "The package `vllm` requires `triton==3.5.0`, but it's not installed", + ) + .expect("the line still names a requirement"); + + assert_eq!(subject.package, "triton"); + assert_eq!(subject.required.as_deref(), Some("3.5.0")); + assert_eq!(subject.installed, None, "nothing is installed to name"); + } + + #[test] + fn a_loose_requirement_pins_no_release() { + // Only an exact `==` says which release the requirer was built against. A + // range or a compound specifier must not be mistaken for one. + for detail in [ + "The package `tilelang` requires `cloudpickle>=3.0`, but `2.2.1` is installed", + "The package `foo` requires `bar>=1.0,<2.0`, but `2.5` is installed", + ] { + let subject = violation_subject(detail).expect("still a violation line"); + assert_eq!( + subject.required, None, + "a looser requirement pins nothing: {detail}" + ); + } + } + + #[test] + fn a_line_without_the_requirement_frame_has_no_subject() { + // `uv` reports several other conditions under the same opening frame, and a + // confidently wrong parse of one of those is worse than no answer. + assert_eq!(violation_subject("Checked 214 packages in 12ms"), None); + assert_eq!( + violation_subject("The package `vllm` has an invalid METADATA file"), + None + ); + } + + #[test] + fn a_local_segment_is_split_from_the_release() { + assert_eq!( + split_local_version("2.11.0+rocm7.13.0"), + ("2.11.0", Some("rocm7.13.0")) + ); + assert_eq!(split_local_version("2.11.0"), ("2.11.0", None)); + } } diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index 0ad0e4819..23f698673 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -6,8 +6,8 @@ use anyhow::{Context, Result, anyhow, bail}; use clap::{Parser, Subcommand}; use rocm_core::{ AppPaths, DEFAULT_LOCAL_PORT, DependencyViolation, check_dependencies, ensure_uv_binary, - format_http_base_url, openai_models_endpoint_has_model, require_nonempty, uv_command_env, - uv_pip_install_base, violations_requiring, + format_http_base_url, openai_models_endpoint_has_model, require_nonempty, split_local_version, + uv_command_env, uv_pip_install_base, violation_subject, violations_requiring, }; use rocm_engine_protocol::{ DEFAULT_LOG_TAIL_LINES, DetectRequest, DetectResponse, DevicePolicy, @@ -141,7 +141,7 @@ struct VllmRuntime { sdk_library_paths: Vec, } -#[derive(Debug, Clone, Deserialize)] +#[derive(Debug, Clone, Default, Deserialize)] struct TheRockRuntimeManifest { #[serde(default)] runtime_key: Option, @@ -151,11 +151,23 @@ struct TheRockRuntimeManifest { python_executable: Option, #[serde(default)] rocm_sdk: Option, + /// The SDK's own version, used only to reconstruct a build identifier for + /// manifests written before `sdk_torch` was recorded. + #[serde(default)] + version: Option, + /// The torch the SDK install wrote, e.g. `2.11.0+rocm7.13.0`. + /// + /// The CLI records it so a later engine install can be told apart from the SDK's + /// own work. Read here for the same reason in reverse: it is the only way this + /// engine can recognise a torch the CLI deliberately put back, as opposed to one + /// some other installer left behind. + #[serde(default)] + sdk_torch: Option, #[serde(default)] installed_at_unix_ms: Option, } -#[derive(Debug, Clone, Deserialize)] +#[derive(Debug, Clone, Default, Deserialize)] struct RocmSdkRuntimeProbe { #[serde(default)] import_ok: bool, @@ -167,6 +179,8 @@ struct RocmSdkRuntimeProbe { bin_paths: Vec, #[serde(default)] library_paths: Vec, + #[serde(default)] + rocm_sdk_version: Option, } #[derive(Debug, Clone)] @@ -534,19 +548,95 @@ fn assess_runtime_repair(runtime: &VllmRuntime) -> RepairAssessment { Err(error) => return unverified_repair(&error.to_string()), }; match check_dependencies(&paths, python) { - Ok(violations) => repair_from_violations(&violations), + Ok(violations) => repair_from_violations( + &violations, + recorded_sdk_torch_build(&runtime.runtime_id).as_deref(), + ), // An unusable `uv` or an offline host must not block an install that would // otherwise succeed; report that the check did not run and carry on as before. Err(error) => unverified_repair(&error.to_string()), } } +/// The package whose build the SDK and the engine both have an opinion about. +const TORCH_PACKAGE: &str = "torch"; + +/// Whether this violation is the torch divergence rocm-cli deliberately leaves behind. +/// +/// After an engine install, rocm-cli puts back the SDK's *build* of the torch release +/// the engine pins, because the engine's build cannot open a device against the +/// installed SDK libraries. The engine's metadata pins an exact version and cannot +/// express "same release, the SDK's build", so `uv pip check` reports the result as +/// unsatisfied forever. Treating that as a defect makes the two mechanisms fight: the +/// engine reinstalls torch to its own build, rocm-cli puts the SDK's back, and the +/// next invocation starts over — two full torch-stack flips a run, and a warning +/// claiming a repair that undid the intended state. +/// +/// Three conditions, all required, and they are exactly the rule rocm-cli applies: +/// take the *release* from the engine's pin and the *build* from the SDK. +/// +/// The violation must be about torch — any other unmet requirement is real. The +/// installed release must be the one the engine pins; an SDK torch of a *different* +/// release is the separate bug where the engine cannot accept what the SDK installed, +/// and a reinstall is the right answer there. And the installed build must be the one +/// the runtime's manifest records for the SDK; a torch from neither side is the +/// breakage this check exists to catch. Miss any one and the engine either fights the +/// alignment or silently accepts a runtime that cannot serve. +fn is_intended_torch_divergence(detail: &str, sdk_torch_build: Option<&str>) -> bool { + let Some(sdk_torch_build) = sdk_torch_build else { + return false; + }; + let Some(subject) = violation_subject(detail) else { + return false; + }; + if !subject.package.eq_ignore_ascii_case(TORCH_PACKAGE) { + return false; + } + let (Some(required), Some(installed)) = + (subject.required.as_deref(), subject.installed.as_deref()) + else { + return false; + }; + let (installed_release, Some(installed_build)) = split_local_version(installed) else { + return false; + }; + installed_build == sdk_torch_build && installed_release == split_local_version(required).0 +} + /// The repair decision for a set of violations found in the environment. -fn repair_from_violations(violations: &[DependencyViolation]) -> RepairAssessment { +/// +/// `sdk_torch_build` is the build identifier the runtime's manifest records for the +/// SDK's torch, or `None` when it cannot be determined — in which case nothing is +/// treated as intended and the previous behaviour stands. +fn repair_from_violations( + violations: &[DependencyViolation], + sdk_torch_build: Option<&str>, +) -> RepairAssessment { let owned = violations_requiring(violations, ENGINE_NAME); if owned.is_empty() { return RepairAssessment::default(); } + let (intended, defects): (Vec<&DependencyViolation>, Vec<&DependencyViolation>) = owned + .into_iter() + .partition(|violation| is_intended_torch_divergence(&violation.detail, sdk_torch_build)); + + if defects.is_empty() { + // Nothing to repair. Reinstalling here would replace the SDK's build with the + // engine's and hand back a runtime that cannot open a device. + let mut notes = vec![ + "the runtime holds the SDK's build of the torch vLLM pins; that divergence is intended and a reinstall would undo it".to_owned(), + ]; + notes.extend( + intended + .iter() + .map(|violation| format!("expected divergence: {}", violation.detail)), + ); + return RepairAssessment { + needed: false, + notes, + }; + } + let mut notes = vec![ "the runtime environment did not satisfy vLLM's pinned dependencies; vLLM was reinstalled to restore them".to_owned(), ]; @@ -556,13 +646,22 @@ fn repair_from_violations(violations: &[DependencyViolation]) -> RepairAssessmen // is unreadable in a terminal. Mirrors the per-finding `violation:` lines the // CLI-side renderer already emits. notes.extend( - owned + defects .iter() .map(|violation| format!("violation: {}", violation.detail)), ); - notes.push( - "if this recurs after `rocm install sdk`, the SDK torch stack is being written over vLLM's pinned torch".to_owned(), + // An intended divergence alongside a real one is still worth naming, so the reader + // is not left thinking the reinstall was about torch when it was not. + notes.extend( + intended + .iter() + .map(|violation| format!("expected divergence: {}", violation.detail)), ); + if sdk_torch_build.is_none() { + notes.push( + "if this recurs after `rocm install sdk`, the SDK torch stack is being written over vLLM's pinned torch".to_owned(), + ); + } RepairAssessment { needed: true, notes, @@ -1178,6 +1277,70 @@ struct ManagedRuntimeCandidate { sdk_library_paths: Vec, } +/// The runtime manifests matching `runtime_id`, most recently installed first. +fn load_runtime_manifests(runtime_id: Option<&str>) -> Result> { + let paths = AppPaths::discover()?; + let registry = paths.data_dir.join("runtimes").join("registry"); + if !registry.is_dir() { + return Ok(Vec::new()); + } + let mut manifests = Vec::new(); + for entry in + fs::read_dir(®istry).with_context(|| format!("failed to read {}", registry.display()))? + { + let path = entry?.path(); + if path.extension().and_then(|value| value.to_str()) != Some("json") { + continue; + } + let bytes = + fs::read(&path).with_context(|| format!("failed to read {}", path.display()))?; + let Ok(manifest) = serde_json::from_slice::(&bytes) else { + continue; + }; + if !runtime_matches(&manifest, runtime_id) { + continue; + } + manifests.push((manifest.installed_at_unix_ms.unwrap_or(0), manifest)); + } + manifests.sort_by_key(|(installed_at, _)| std::cmp::Reverse(*installed_at)); + Ok(manifests + .into_iter() + .map(|(_, manifest)| manifest) + .collect()) +} + +/// The torch build the SDK installed into this runtime, as its manifest records it. +/// +/// Read from the manifest, never from the environment. By the time this runs the +/// environment may already hold some other installer's build, and taking that for +/// the SDK's would conclude the runtime is correct and leave it wrong for good. +fn recorded_sdk_torch_build(runtime_id: &str) -> Option { + let manifest = load_runtime_manifests(Some(runtime_id)) + .ok()? + .into_iter() + .next()?; + sdk_torch_build_from_manifest(&manifest) +} + +/// The SDK's torch build identifier, with the fallback for older manifests. +/// +/// Manifests written before `sdk_torch` was recorded still name the SDK version, and +/// TheRock builds that into the local segment as `rocm`. Mirrors the CLI's +/// `sdk_torch_build_for_key` so both sides agree on what "the SDK's build" means. +fn sdk_torch_build_from_manifest(manifest: &TheRockRuntimeManifest) -> Option { + if let Some(recorded) = manifest.sdk_torch.as_deref() + && let Some(build) = split_local_version(recorded).1 + { + return Some(build.to_owned()); + } + let version = manifest + .rocm_sdk + .as_ref() + .and_then(|probe| probe.rocm_sdk_version.clone()) + .or_else(|| manifest.version.clone())?; + (!version.trim().is_empty()).then(|| format!("rocm{version}")) +} + /// Registered runtimes that matched the request but were passed over because the /// interpreter they record is not there, phrased for the end of an error message. /// @@ -1231,33 +1394,8 @@ fn describe_skipped_managed_runtimes(runtime_id: Option<&str>) -> Option fn collect_managed_runtime_candidates( runtime_id: Option<&str>, ) -> Result> { - let paths = AppPaths::discover()?; - let registry = paths.data_dir.join("runtimes").join("registry"); - if !registry.is_dir() { - return Ok(Vec::new()); - } - let mut manifests = Vec::new(); - for entry in - fs::read_dir(®istry).with_context(|| format!("failed to read {}", registry.display()))? - { - let path = entry?.path(); - if path.extension().and_then(|value| value.to_str()) != Some("json") { - continue; - } - let bytes = - fs::read(&path).with_context(|| format!("failed to read {}", path.display()))?; - let Ok(manifest) = serde_json::from_slice::(&bytes) else { - continue; - }; - if !runtime_matches(&manifest, runtime_id) { - continue; - } - manifests.push((manifest.installed_at_unix_ms.unwrap_or(0), manifest)); - } - manifests.sort_by_key(|(installed_at, _)| std::cmp::Reverse(*installed_at)); - let mut candidates = Vec::new(); - for (_, manifest) in manifests { + for manifest in load_runtime_manifests(runtime_id)? { let Some(python) = manifest .python_executable .clone() @@ -2728,19 +2866,30 @@ mod tests { ); } + /// The build identifier the SDK recorded in the runtimes used by these tests. + const SDK_BUILD: &str = "rocm7.13.0"; + #[test] fn a_consistent_environment_is_not_reinstalled() { - assert_eq!(repair_from_violations(&[]), RepairAssessment::default()); + assert_eq!( + repair_from_violations(&[], Some(SDK_BUILD)), + RepairAssessment::default() + ); } #[test] - fn a_replaced_pinned_torch_forces_a_reinstall() { - // A second `rocm install sdk` writes the SDK's torch over the build - // vLLM pins. vLLM still imports, so resolution alone cannot see the breakage. - let assessment = repair_from_violations(&[violation( - "vllm", - "The package `vllm` requires `torch==2.10.0+git8514f05`, but `2.9.1+rocm7.14.0a20260611` is installed", - )]); + fn a_torch_of_the_wrong_release_still_forces_a_reinstall() { + // The other direction of the same problem: the SDK installed a torch + // *release* the engine does not accept. The build is the SDK's, but the + // release is not the engine's, so this is not the intended divergence and + // the reinstall that restores the engine's release must still happen. + let assessment = repair_from_violations( + &[violation( + "vllm", + "The package `vllm` requires `torch==2.10.0+git8514f05`, but `2.9.1+rocm7.14.0a20260611` is installed", + )], + Some("rocm7.14.0a20260611"), + ); assert!(assessment.needed); assert!( @@ -2754,25 +2903,102 @@ mod tests { } #[test] - fn the_whole_replaced_torch_stack_is_reported_one_finding_per_line() { - // What the failure actually looks like on hardware: the SDK moves torch, - // torchvision and torchaudio together, so all three pins are violated at - // once. Joining them into a single note produced one ~380-character line; - // each finding gets its own so a terminal can show them. - let assessment = repair_from_violations(&[ - violation( + fn the_intended_torch_divergence_alone_does_not_force_a_reinstall() { + // The steady state this change exists to stop churning. rocm-cli put the + // SDK's build of the release vLLM pins back after the engine install; the + // engine's exact pin cannot express that, so `uv pip check` reports it + // forever. Reinstalling would replace it with the build that opens no + // device, and the next invocation would do the whole thing again. + let assessment = repair_from_violations( + &[violation( "vllm", "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed", - ), - violation( + )], + Some(SDK_BUILD), + ); + + assert!( + !assessment.needed, + "the intended divergence must not trigger a reinstall: {:?}", + assessment.notes + ); + assert!( + assessment + .notes + .iter() + .all(|note| !note.contains("was reinstalled")), + "no note may claim a repair that did not happen: {:?}", + assessment.notes + ); + } + + #[test] + fn a_torch_from_neither_side_still_forces_a_reinstall() { + // Same release the engine pins, but a build belonging to neither the SDK nor + // the engine — someone installed a torch by hand, or a resolver picked one + // off PyPI. Nothing about that is intended. + let assessment = repair_from_violations( + &[violation( "vllm", - "The package `vllm` requires `torchvision==0.24.1+d801a34`, but `0.26.0+rocm7.13.0` is installed", - ), - violation( + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+cpu` is installed", + )], + Some(SDK_BUILD), + ); + + assert!(assessment.needed); + } + + #[test] + fn an_unidentified_sdk_build_keeps_the_previous_behaviour() { + // Without a recorded build there is no way to tell the intended divergence + // from a defect, and guessing in the permissive direction would leave a + // genuinely broken runtime alone. Fall back to repairing. + let assessment = repair_from_violations( + &[violation( "vllm", - "The package `vllm` requires `torchaudio==2.9.0+eaa9e4e`, but `2.11.0+rocm7.13.0` is installed", - ), - ]); + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed", + )], + None, + ); + + assert!(assessment.needed); + assert!( + assessment + .notes + .iter() + .any(|note| note.contains("rocm install sdk")), + "the SDK-overwrite hint belongs to exactly this un-identifiable case: {:?}", + assessment.notes + ); + } + + #[test] + fn the_whole_replaced_torch_stack_is_reported_one_finding_per_line() { + // What the failure looks like on hardware right after `rocm install sdk`: + // the SDK moves torch, torchvision and torchaudio together, so all three + // pins are violated at once. Joining them into a single note produced one + // ~380-character line; each finding gets its own so a terminal can show it. + // + // Only torch is realigned, so only torch's divergence is intended. The other + // two are genuine and still drive the reinstall — which is what restores all + // three to the engine's builds before rocm-cli puts torch back. + let assessment = repair_from_violations( + &[ + violation( + "vllm", + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed", + ), + violation( + "vllm", + "The package `vllm` requires `torchvision==0.24.1+d801a34`, but `0.26.0+rocm7.13.0` is installed", + ), + violation( + "vllm", + "The package `vllm` requires `torchaudio==2.9.0+eaa9e4e`, but `2.11.0+rocm7.13.0` is installed", + ), + ], + Some(SDK_BUILD), + ); assert!(assessment.needed); let violation_notes: Vec<&String> = assessment @@ -2782,17 +3008,34 @@ mod tests { .collect(); assert_eq!( violation_notes.len(), - 3, - "every violated pin gets its own note: {:?}", + 2, + "every genuinely violated pin gets its own note: {:?}", assessment.notes ); - for package in ["torch==", "torchvision==", "torchaudio=="] { + for package in ["torchvision==", "torchaudio=="] { assert!( violation_notes.iter().any(|note| note.contains(package)), "{package} is missing from the reported notes: {:?}", assessment.notes ); } + assert!( + violation_notes + .iter() + .all(|note| !note.contains("torch==2.11.0")), + "the realigned torch is a divergence, not a violation: {:?}", + assessment.notes + ); + assert!( + assessment + .notes + .iter() + .any(|note| note.starts_with("expected divergence: ") + && note.contains("torch==2.11.0+gitd0c8b1f")), + "the intended divergence is still named, so the reader is not left \ + thinking the reinstall was about torch: {:?}", + assessment.notes + ); assert!( assessment.notes.iter().all(|note| note.len() < 200), "no note should be a wall of joined findings: {:?}", @@ -2804,20 +3047,62 @@ mod tests { fn unrelated_upstream_conflicts_do_not_force_a_reinstall() { // These environments routinely carry conflicts between third-party packages. // Reinstalling vLLM would not resolve them, so they must not trigger one. - let assessment = repair_from_violations(&[ - violation( - "tilelang", - "The package `tilelang` requires `cloudpickle>=3.0`, but `2.2.1` is installed", - ), - violation( - "torch", - "The package `torch` requires `sympy>=1.13`, but `1.12` is installed", - ), - ]); + let assessment = repair_from_violations( + &[ + violation( + "tilelang", + "The package `tilelang` requires `cloudpickle>=3.0`, but `2.2.1` is installed", + ), + violation( + "torch", + "The package `torch` requires `sympy>=1.13`, but `1.12` is installed", + ), + ], + Some(SDK_BUILD), + ); assert_eq!(assessment, RepairAssessment::default()); } + #[test] + fn a_recorded_sdk_torch_names_the_build() { + let manifest = TheRockRuntimeManifest { + sdk_torch: Some("2.11.0+rocm7.13.0".to_owned()), + ..TheRockRuntimeManifest::default() + }; + + assert_eq!( + sdk_torch_build_from_manifest(&manifest).as_deref(), + Some("rocm7.13.0") + ); + } + + #[test] + fn a_manifest_without_sdk_torch_reconstructs_the_build_from_the_sdk_version() { + // Written before `sdk_torch` was recorded. These are the runtimes already on + // real machines, so the fallback is what repairs them rather than a nicety. + let manifest = TheRockRuntimeManifest { + rocm_sdk: Some(RocmSdkRuntimeProbe { + rocm_sdk_version: Some("7.13.0".to_owned()), + ..RocmSdkRuntimeProbe::default() + }), + ..TheRockRuntimeManifest::default() + }; + + assert_eq!( + sdk_torch_build_from_manifest(&manifest).as_deref(), + Some("rocm7.13.0") + ); + } + + #[test] + fn a_manifest_that_identifies_no_sdk_build_says_so() { + assert_eq!( + sdk_torch_build_from_manifest(&TheRockRuntimeManifest::default()), + None + ); + } + #[test] fn an_unrunnable_check_reports_itself_without_forcing_a_reinstall() { let assessment = unverified_repair("uv binary is unavailable"); From 2492bfc219f7df3f6e83e5beaf4e00cf6f1dfec9 Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Mon, 31 Aug 2026 12:27:01 +0000 Subject: [PATCH 03/19] feat(install): let a user opt out of realigning torch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The alignment rewrites a package the user may have installed deliberately, and it runs on every path that installs an engine, so a hand-installed torch is replaced again by the next `engines install`, `install sdk`, or `engines shell`. There was no way to say no. That matters more than it would for a cosmetic correction, because the stack we resolve to — the SDK's build of the release the engine pins — is not validated against the supported matrix. "The SDK's build does not work on this machine" is a case that can happen rather than a hypothetical one, and until now its only exit was to stop using the CLI. Setting ROCM_CLI_DISABLE_TORCH_ALIGNMENT skips the rewrite. The check runs before the probe, so opting out means nothing ran, rather than running and being described differently; someone reaches for this precisely when their machine does not probe cleanly, so it cannot depend on a clean probe first. The dependency and device checks are untouched: this suppresses the correction, not the diagnosis, and a runtime that cannot open a device is still reported as one. The variable follows the existing convention for CLI switches — presence, not value, as with ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK. The two tests are a pair on purpose. Set, the call returns the opt-out reason even though it points at a Python that does not exist, which is what shows the gate precedes the probe. Unset, the same call has to fail at the probe instead, so the opt-out cannot appear to work for an unrelated reason. Raised in review by Eugene Volen. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 79 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index d82c83b6c..34be4d7b2 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -7627,6 +7627,24 @@ fn install_error_reports_version_unavailable(error: &str) -> bool { || error.contains("has no version") } +/// Whether the user has opted out of realigning torch. +/// +/// The alignment rewrites a package the user may have installed deliberately, and +/// it runs on every path that installs an engine, so a hand-installed torch is +/// otherwise replaced again by the next `engines install`, `install sdk`, or +/// `engines shell`. The stack this resolves to — the SDK's build of the release +/// the engine pins — is not validated against the supported matrix, so "the SDK's +/// build does not work on this machine" is a case that can happen rather than a +/// hypothetical one, and it needs an exit that is not "stop using the CLI". +/// +/// Checked before the probe runs, so opting out skips the rewrite rather than +/// performing it and describing it differently. The dependency and device checks +/// still run: this suppresses the correction, not the diagnosis, and a runtime +/// that cannot open a device is still reported as one. +fn torch_alignment_disabled() -> bool { + std::env::var_os("ROCM_CLI_DISABLE_TORCH_ALIGNMENT").is_some() +} + fn align_runtime_torch( paths: &AppPaths, python: &Path, @@ -7634,6 +7652,11 @@ fn align_runtime_torch( sdk_build: Option<&str>, engine: &str, ) -> TorchAlignment { + if torch_alignment_disabled() { + return TorchAlignment::NotApplicable( + "torch alignment is disabled by ROCM_CLI_DISABLE_TORCH_ALIGNMENT".to_owned(), + ); + } let probe = match therock::probe_torch_alignment(python, engine) { Ok(probe) => probe, Err(error) => return TorchAlignment::NotApplicable(error.to_string()), @@ -27104,6 +27127,62 @@ ID_LIKE="suse opensuse" assert_eq!(sdk_torch_build_from_manifest(&manifest), None); } + /// Opting out skips the rewrite without needing a runtime that answers. + /// + /// The gate has to precede the probe. Someone reaches for it precisely when the + /// SDK's build does not work on their machine, so it cannot depend on that + /// machine probing cleanly first. Pointing it at a Python that does not exist + /// would otherwise produce a probe failure; getting the opt-out reason back + /// instead is what shows nothing ran. + #[test] + fn disabling_alignment_skips_the_rewrite_before_anything_is_probed() { + let mut env = ScopedTestEnv::new(); + env.set("ROCM_CLI_DISABLE_TORCH_ALIGNMENT", "1"); + + let outcome = align_runtime_torch( + &test_app_paths(), + Path::new("/nonexistent/python"), + Some("https://example.invalid/simple"), + Some("rocm7.13.0"), + "vllm", + ); + + assert_eq!( + outcome, + TorchAlignment::NotApplicable( + "torch alignment is disabled by ROCM_CLI_DISABLE_TORCH_ALIGNMENT".to_owned() + ) + ); + } + + /// Unset, the rewrite is attempted as usual. + /// + /// Paired with the test above so the opt-out cannot appear to work for an + /// unrelated reason: the same call with the same unusable Python has to fail at + /// the probe rather than report itself disabled. + #[test] + fn alignment_runs_unless_the_variable_is_set() { + let mut env = ScopedTestEnv::new(); + env.clear("ROCM_CLI_DISABLE_TORCH_ALIGNMENT"); + + let outcome = align_runtime_torch( + &test_app_paths(), + Path::new("/nonexistent/python"), + Some("https://example.invalid/simple"), + Some("rocm7.13.0"), + "vllm", + ); + + assert!( + !matches!( + &outcome, + TorchAlignment::NotApplicable(reason) + if reason.contains("ROCM_CLI_DISABLE_TORCH_ALIGNMENT") + ), + "the opt-out must not fire when the variable is unset, got {outcome:?}" + ); + } + #[test] fn a_runtime_that_sees_no_devices_names_the_torch_that_cannot_use_the_sdk() { // The install succeeded and the engine's requirements are satisfied, so From 22fb6d8201107e7b005de1f62fe43a73af4eda90 Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Mon, 31 Aug 2026 12:27:12 +0000 Subject: [PATCH 04/19] docs(install): say why NotVerified is never fatal on its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review read `RuntimeDeviceCheck::NotVerified` as a case where an unimportable torch should fail the install, and the one-line doc ("never assume healthy") did not explain why it does not. The variant fires for benign reasons as well as real ones — a runtime whose Python could not be located, a probe that could not launch — so making it fatal would fail multi-gigabyte installs because a probe did not run. Only `NoDevices` is acted on, in `install_left_runtime_unusable`, and the cost of that choice is real and worth stating: a runtime whose torch is present but unimportable is reported rather than failed. Separating the two cases means splitting the variant, which is a change to behaviour rather than to a comment. Documenting the reasoning first so the next reader does not have to re-derive it. No functional change. Raised in review by Eugene Volen. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 34be4d7b2..d2be2a30a 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -7858,7 +7858,15 @@ enum RuntimeDeviceCheck { torch_version: String, hip_version: String, }, - /// The question could not be answered; never assume healthy. + /// The question could not be answered — including when torch does not import. + /// + /// Never a reason to assume healthy, but on its own never fatal either: see + /// `install_left_runtime_unusable`, which acts only on `NoDevices`. This + /// variant covers benign causes as well as real ones — a runtime whose Python + /// could not be located, or a probe that could not launch — and failing a + /// multi-gigabyte install because a probe did not run is worse than reporting + /// what was and was not seen. The cost is that a runtime whose torch is + /// present but unimportable is reported rather than failed. NotVerified(String), } From 01fff76a5cbd18865b61a4ed791f9b94e202ccee Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Mon, 31 Aug 2026 13:02:29 +0000 Subject: [PATCH 05/19] test(e2e): assert the torch alignment actually ran on the reinstall MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scenario 4 asserted the outcome that matters — the runtime can still open a GPU after a second `install sdk` — but nothing in the suite asserted that the alignment itself fired. Those are not the same check. A runtime whose torch was never touched can still open a device, so the device check passes whether `settle_engine_install` reached the engine or not. That leaves the gate in front of the settle step, the part most likely to be widened or narrowed by a later change, with no e2e coverage at all: silently skipping the alignment would look identical to doing it correctly. The new step reads the `torch_alignment:` block. It asserts the block is present, that it reached one of the two healthy outcomes, and — separately, negatively — that it reached none of the three unhealthy ones. The negative half is not redundant: the positive check would pass on a run that also printed a failed second block. `not_applicable` is the one that would otherwise go unnoticed, because it is what a manifest yielding no SDK build produces, which is exactly the repair path for every runtime installed before `sdk_torch` existed. Both healthy outcomes are accepted rather than pinning one. Whether the reinstall rewrites torch or finds it already correct depends on what the shared pre-warm tree held when the scenario started, and the rule held either way. The divergence assertion is conditional on purpose. A divergence is today's steady state — the engine pins an exact build, the SDK supplies a different one of the same release — but a future pair could agree, and then there is nothing to classify. Requiring it unconditionally would encode today's versions into the scenario. What must never happen is the CLI reporting a divergence and calling it a defect, and that is what is asserted. No expectations.toml entry: the scenario is expected to pass. Raised in review by Eugene Volen. Signed-off-by: Tomas Saaristola --- .../features/runtime_setup.feature | 7 +++ tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 50 +++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 44f0a84ab..61e246f2b 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -29,11 +29,18 @@ Feature: Runtime configuration # install, and a second SDK install, so it runs on the nightly GPU lane. # `@requires-engine:vllm` because only vLLM shares the runtime environment; # Lemonade manages its own. + # + # The second Then is not a restatement of the first. A runtime the alignment never + # touched can still open a device, so the device check alone cannot distinguish + # "settled correctly" from "skipped entirely" — and skipping is the regression the + # gate in front of the settle step would produce. Only the alignment block + # separates them, and it is the one part of this path with no other e2e coverage. @id:runtime-sdk-reinstall-keeps-engine-consistent @requires-gpu @requires-engine:vllm @nightly Scenario: 4 - Reinstalling the SDK leaves the installed engine able to use the GPU Given a managed runtime with an inference engine already installed When the user installs the SDK again Then the runtime can still use the GPU + And the torch alignment settled on the SDK's build # The GPU E2E lanes no longer install the shared runtime once and keep it # forever: `xtask e2e-prewarm` asks `rocm update` whether the channel index has diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index cc9ad08c3..ee9daf599 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -192,6 +192,56 @@ async fn assert_runtime_can_still_use_the_gpu(world: &mut E2eWorld) { ); } +/// The alignment ran, and settled on one of its two healthy outcomes. +/// +/// Separate from the device check above because the two can disagree: a runtime +/// whose torch was never touched at all can still open a device, so that check +/// passes whether or not the alignment fired. The `torch_alignment:` block is the +/// only evidence that `settle_engine_install` reached this engine, and the gate in +/// front of it is the part most likely to be widened or narrowed by a later change. +#[then("the torch alignment settled on the SDK's build")] +async fn assert_torch_alignment_settled(world: &mut E2eWorld) { + let output = world.cli_output.as_deref().expect("no install output"); + assert!( + output.contains("torch_alignment:"), + "no torch alignment block, so the runtime was never settled:\n{output}" + ); + // Both healthy outcomes are accepted rather than pinning one: whether this + // reinstall rewrote torch or found it already correct depends on what the shared + // tree held when the scenario started, and either way the rule held. + assert!( + output.contains("torch_alignment: realigned") + || output.contains("torch_alignment: already_aligned"), + "torch alignment reached no healthy outcome:\n{output}" + ); + // Asserted negatively as well, because the positive check above would pass on a + // second block that failed. `not_applicable` is the one that would otherwise go + // unnoticed: it is what a manifest yielding no SDK build produces, which is + // exactly the repair path for every runtime installed before `sdk_torch` was + // recorded. + for unhealthy in [ + "torch_alignment: unavailable", + "torch_alignment: install_failed", + "torch_alignment: not_applicable", + ] { + assert!( + !output.contains(unhealthy), + "torch alignment reported `{unhealthy}`:\n{output}" + ); + } + // Conditional on purpose. A divergence is today's steady state — the engine pins + // an exact build and the SDK supplies a different one of the same release — but + // a future pair could agree, and then there is nothing to classify. Pinning it + // unconditionally would encode today's versions into the scenario. What must + // never happen is the CLI reporting a divergence and calling it a defect. + if output.contains("divergence:") { + assert!( + output.contains("dependency_check: expected_divergence"), + "a divergence was reported without being classified as expected:\n{output}" + ); + } +} + /// The engine inventory reports a usable engine runtime. /// /// A precondition only. It deliberately has no Then counterpart: `engines list` From d616d14b87f0484855814b43944b0701a98007bb Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Fri, 28 Aug 2026 14:04:39 -0700 Subject: [PATCH 06/19] fix(install): validate torch kernels before alignment Signed-off-by: Michael Roy --- apps/rocm/src/main.rs | 953 ++++++++++++++++-- apps/rocm/src/therock.rs | 133 ++- engines/vllm/src/lib.rs | 274 ++++- .../features/runtime_setup.feature | 39 +- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 200 +++- 5 files changed, 1448 insertions(+), 151 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index d2be2a30a..48d2e162a 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -7543,13 +7543,16 @@ fn runtime_index_url_for_key(paths: &AppPaths, runtime_key: &str) -> Option bool { /// build does not work on this machine" is a case that can happen rather than a /// hypothetical one, and it needs an exit that is not "stop using the CLI". /// -/// Checked before the probe runs, so opting out skips the rewrite rather than -/// performing it and describing it differently. The dependency and device checks -/// still run: this suppresses the correction, not the diagnosis, and a runtime -/// that cannot open a device is still reported as one. +/// This suppresses the correction, not the diagnosis. The runtime is still asked +/// what it can do, the dependency check still runs, and a runtime that opens no +/// device or cannot run a kernel on one is still reported as such — and still +/// fails the install on a host where a GPU was found. fn torch_alignment_disabled() -> bool { std::env::var_os("ROCM_CLI_DISABLE_TORCH_ALIGNMENT").is_some() } +/// Install the SDK's build of the release the engine pins, when that is needed. +/// +/// `torch` is the metadata probe the caller already took: the retention decision +/// that runs first needs the engine's pin too, and asking the runtime the same +/// question twice would be a second interpreter launch for an answer in hand. +/// +/// The opt-out is read where the install would run, not at the top. That is the +/// only place it changes anything, and reading it there is what lets the result +/// name the replacement it declined to make: a runtime with no replacement due +/// reports what it is, rather than reporting an opt-out that skipped nothing. fn align_runtime_torch( paths: &AppPaths, python: &Path, index_url: Option<&str>, sdk_build: Option<&str>, engine: &str, + torch: Result<&therock::TorchAlignmentProbe, &anyhow::Error>, ) -> TorchAlignment { - if torch_alignment_disabled() { - return TorchAlignment::NotApplicable( - "torch alignment is disabled by ROCM_CLI_DISABLE_TORCH_ALIGNMENT".to_owned(), - ); - } - let probe = match therock::probe_torch_alignment(python, engine) { + let probe = match torch { Ok(probe) => probe, Err(error) => return TorchAlignment::NotApplicable(error.to_string()), }; @@ -7676,6 +7694,12 @@ fn align_runtime_torch( } TorchAlignmentPlan::Install { wanted, from } => (wanted, from), }; + // Read only now that a replacement is actually due, so the opt-out reports + // the install it stopped rather than standing in for a runtime that needed + // nothing. Everything after this point is the rewrite itself. + if torch_alignment_disabled() { + return TorchAlignment::Disabled { wanted, kept: from }; + } let Some(index_url) = index_url else { return TorchAlignment::NotApplicable( "the runtime manifest records no wheel index to install from".to_owned(), @@ -7756,6 +7780,15 @@ fn render_torch_alignment(outcome: &TorchAlignment, engine: &str) -> String { ); let _ = writeln!(output, " keeping {}", sanitize_log_value(kept)); } + TorchAlignment::Disabled { wanted, kept } => { + let _ = writeln!(output, " torch_alignment: disabled"); + let _ = writeln!( + output, + " ROCM_CLI_DISABLE_TORCH_ALIGNMENT is set; keeping {} rather than installing {}", + sanitize_log_value(kept), + sanitize_log_value(wanted) + ); + } TorchAlignment::NotApplicable(reason) => { let _ = writeln!( output, @@ -7767,21 +7800,29 @@ fn render_torch_alignment(outcome: &TorchAlignment, engine: &str) -> String { output } -/// Settle the runtime's torch, print the `torch_alignment:` block, and record it. +/// Repair the runtime's torch, print the `torch_alignment:` block, and record it. /// -/// The outcome is returned so the dependency and device checks that follow can be -/// read against it — `deliberately_diverged_package` turns it into the one package -/// whose divergence is intended, and `install_left_runtime_unusable` pairs it with -/// the device count. +/// Reached only when no torch in the runtime has proven it can run a GPU kernel. +/// The outcome is returned so the dependency check that follows can be read +/// against it: `deliberately_diverged_package` turns it into the one package +/// whose divergence is intended. fn report_torch_alignment( paths: &AppPaths, engine: &str, python: &Path, runtime_key: &str, sdk_build: Option<&str>, + torch: Result<&therock::TorchAlignmentProbe, &anyhow::Error>, ) -> TorchAlignment { let index_url = runtime_index_url_for_key(paths, runtime_key); - let outcome = align_runtime_torch(paths, python, index_url.as_deref(), sdk_build, engine); + let outcome = align_runtime_torch( + paths, + python, + index_url.as_deref(), + sdk_build, + engine, + torch, + ); print!("{}", render_torch_alignment(&outcome, engine)); let (level, message) = match &outcome { TorchAlignment::AlreadyAligned { version } => ( @@ -7812,6 +7853,15 @@ fn report_torch_alignment( "engine={engine} runtime_id={runtime_key} torch_alignment=install_failed wanted={wanted} kept={kept}: {error}" ), ), + // The user's own decision, carried out as asked, so it is not an error. + // The device check that follows is what says whether the torch they kept + // works, and that verdict is recorded — and enforced — on its own. + TorchAlignment::Disabled { wanted, kept } => ( + "info", + format!( + "engine={engine} runtime_id={runtime_key} torch_alignment=disabled kept={kept} wanted={wanted}" + ), + ), TorchAlignment::NotApplicable(reason) => ( "info", format!( @@ -7829,23 +7879,37 @@ fn report_torch_alignment( /// Both `Realigned` and `AlreadyAligned` diverge from the engine's exact pin — /// the second is a rerun over a runtime already put right, which is the normal /// state on every refresh after the first. +/// +/// `Disabled` counts for the same reason, from the other direction: the torch +/// that does not satisfy the pin is the one the user told us to leave alone, so +/// reporting it as a violation would answer their instruction with an error and +/// a `--reinstall` remedy that would undo it. It is only ever constructed over a +/// real mismatch, so this never suppresses a divergence that is not there. +/// +/// `Unavailable` and `InstallFailed` stay excluded. Nothing was replaced there +/// either, but nothing was intended either — the repair was attempted and did +/// not happen — so whatever the dependency check finds is a real finding. const fn deliberately_diverged_package(outcome: &TorchAlignment) -> Option<&'static str> { match outcome { - TorchAlignment::Realigned { .. } | TorchAlignment::AlreadyAligned { .. } => Some("torch"), + TorchAlignment::Realigned { .. } + | TorchAlignment::AlreadyAligned { .. } + | TorchAlignment::Disabled { .. } => Some("torch"), TorchAlignment::Unavailable { .. } | TorchAlignment::InstallFailed { .. } | TorchAlignment::NotApplicable(_) => None, } } -/// Whether the installed runtime can actually open a GPU. +/// Whether the installed runtime can actually run work on a GPU. /// /// The dependency check answers whether the engine's declared requirements are /// satisfied. That is a question about metadata, and it is not the same question -/// as whether the environment works: a torch built against a different ROCm -/// version than the installed SDK satisfies nothing yet loads cleanly, and then -/// reports no devices. vLLM turns that into `Failed to infer device type` at -/// first serve, long after the install reported success. +/// as whether the environment works. Two distinct answers matter here, because a +/// torch can fail at either step: one built against a different ROCm version than +/// the installed SDK loads cleanly and then reports no devices, which vLLM turns +/// into `Failed to infer device type` at first serve; one built without a kernel +/// image for this target reports a device and then dies on the first tensor +/// operation. Only a torch that gets past both has been shown to work. #[derive(Debug, Clone, PartialEq, Eq)] enum RuntimeDeviceCheck { /// torch imported and reported at least one device. @@ -7853,20 +7917,30 @@ enum RuntimeDeviceCheck { device_count: u32, torch_version: String, }, - /// torch imported but reported no devices — the failure this check exists for. + /// torch imported but reported no devices — a torch built for another SDK. NoDevices { torch_version: String, hip_version: String, }, + /// torch found a device and then could not execute a kernel on it. + /// + /// Distinct from `NoDevices` because the remedy is different: the runtime is + /// not looking at the wrong SDK, it is holding a build with no code for this + /// GPU. + KernelFailed { + torch_version: String, + error: String, + }, /// The question could not be answered — including when torch does not import. /// /// Never a reason to assume healthy, but on its own never fatal either: see - /// `install_left_runtime_unusable`, which acts only on `NoDevices`. This - /// variant covers benign causes as well as real ones — a runtime whose Python - /// could not be located, or a probe that could not launch — and failing a - /// multi-gigabyte install because a probe did not run is worse than reporting - /// what was and was not seen. The cost is that a runtime whose torch is - /// present but unimportable is reported rather than failed. + /// `install_left_runtime_unusable`, which acts only on the verdicts where the + /// runtime was asked and answered badly. This variant covers benign causes as + /// well as real ones — a runtime whose Python could not be located, or a probe + /// that could not launch — and failing a multi-gigabyte install because a + /// probe did not run is worse than reporting what was and was not seen. The + /// cost is that a runtime whose torch is present but unimportable is reported + /// rather than failed. NotVerified(String), } @@ -7876,10 +7950,14 @@ fn runtime_device_check(python: Option<&Path>, library_paths: &[PathBuf]) -> Run "the runtime's Python environment could not be located".to_owned(), ); }; - let probe = match therock::probe_runtime_devices(python, library_paths) { - Ok(probe) => probe, - Err(error) => return RuntimeDeviceCheck::NotVerified(error.to_string()), - }; + match therock::probe_runtime_devices(python, library_paths) { + Ok(probe) => classify_runtime_device_probe(probe), + Err(error) => RuntimeDeviceCheck::NotVerified(error.to_string()), + } +} + +/// Read one probe as a verdict, kept free of I/O so every state can be tested. +fn classify_runtime_device_probe(probe: therock::RuntimeDeviceProbe) -> RuntimeDeviceCheck { if !probe.import_ok { return RuntimeDeviceCheck::NotVerified( probe @@ -7888,6 +7966,15 @@ fn runtime_device_check(python: Option<&Path>, library_paths: &[PathBuf]) -> Run ); } let torch_version = probe.torch_version.unwrap_or_else(|| "unknown".to_owned()); + // A kernel that would not launch is definitive, so it is read before the + // count and never dropped: the probe only reaches that step after it has + // already enumerated a device, and the count alone would read as healthy. + if let Some(error) = probe.kernel_error { + return RuntimeDeviceCheck::KernelFailed { + torch_version, + error, + }; + } match probe.device_count { Some(0) => RuntimeDeviceCheck::NoDevices { torch_version, @@ -7950,18 +8037,38 @@ fn render_runtime_device_check(outcome: &RuntimeDeviceCheck) -> String { for a different ROCm version than the installed SDK" ); } + RuntimeDeviceCheck::KernelFailed { + torch_version, + error, + } => { + let _ = writeln!(output, " device_check: kernel_failed"); + let _ = writeln!( + output, + " torch {} found a GPU but could not run a kernel on it: {}", + sanitize_log_value(torch_version), + sanitize_log_value(error) + ); + let _ = writeln!( + output, + " serving will fail on the first tensor operation; this torch has no kernel \ + image for this GPU" + ); + } } output } +/// Print one device-check verdict and record it. +/// +/// The verdict is taken as an argument because the settle path probes the runtime +/// before it decides what to do with it, and the block is printed in its usual +/// place afterwards rather than where the probe happened to run. fn report_runtime_device_check( paths: &AppPaths, engine: &str, - python: Option<&Path>, runtime_key: &str, + outcome: RuntimeDeviceCheck, ) -> RuntimeDeviceCheck { - let library_paths = runtime_library_paths_for_key(paths, runtime_key); - let outcome = runtime_device_check(python, &library_paths); print!("{}", render_runtime_device_check(&outcome)); let (level, message) = match &outcome { RuntimeDeviceCheck::Usable { device_count, .. } => ( @@ -7979,6 +8086,15 @@ fn report_runtime_device_check( "engine={engine} runtime_id={runtime_key} device_check=no_devices torch={torch_version} hip={hip_version}" ), ), + RuntimeDeviceCheck::KernelFailed { + torch_version, + error, + } => ( + "error", + format!( + "engine={engine} runtime_id={runtime_key} device_check=kernel_failed torch={torch_version}: {error}" + ), + ), RuntimeDeviceCheck::NotVerified(reason) => ( "info", format!("engine={engine} runtime_id={runtime_key} device_check=not_verified: {reason}"), @@ -8029,24 +8145,199 @@ fn sdk_torch_build_from_manifest(manifest: &therock::InstalledRuntimeManifest) - (!version.trim().is_empty()).then(|| format!("rocm{version}")) } -/// Whether an install finished having produced a runtime that cannot serve. +/// Which torch a runtime keeps once one of them has run a GPU kernel. +/// +/// Exactly two builds are allowed to stand: the engine's own exact pin, and the +/// SDK's build of the release it pins. Both are states this tool produces on +/// purpose, and each is a fixed point — a rerun over either one keeps it and +/// installs nothing, which is what stops `--reinstall` from oscillating between +/// the two package sources. Anything else is repaired by `TorchAlignment`. +#[derive(Debug, Clone, PartialEq, Eq)] +enum TorchRetention { + /// The engine's exact pin ran a kernel. It satisfies the pin, so it diverges + /// from nothing and the dependency check is read straight. + EngineBuild { version: String }, + /// The SDK's build of the pinned release ran. The engine's pin stays + /// deliberately unsatisfied, which the dependency check is told to expect. + SdkBuild { version: String }, + /// Nothing here has been shown to work: align, then look again. + Realign, +} + +/// Decide retention from the torch that actually executed a kernel. /// -/// The conjunction is the point. An alignment that could not run may still leave -/// a working environment, and a device count of zero is expected wherever no GPU -/// is present — neither alone justifies failing a multi-gigabyte install. Both -/// together mean the runtime cannot open a device and we could not correct it. -const fn install_left_runtime_unusable( - alignment: &TorchAlignment, +/// The version compared is the one the running interpreter reported, not the one +/// distribution metadata claims: the build that ran is the build being kept. The +/// engine's pin is checked first, so a pin that already names the SDK's build is +/// reported as satisfied rather than as an intended divergence from itself. +fn classify_retained_torch( + sdk_build: Option<&str>, devices: &RuntimeDeviceCheck, -) -> bool { - let unsettled = matches!( - alignment, - TorchAlignment::Unavailable { .. } | TorchAlignment::InstallFailed { .. } + engine_requirement: Option<&str>, +) -> TorchRetention { + // Only a kernel that ran earns retention. Every other verdict — no devices, + // a failed launch, or no answer at all — goes to the repair path, which is + // also what this tool did before it could tell those apart. + let RuntimeDeviceCheck::Usable { torch_version, .. } = devices else { + return TorchRetention::Realign; + }; + let Some(pinned) = engine_requirement.and_then(therock::requirement_pinned_version) else { + return TorchRetention::Realign; + }; + if torch_version.as_str() == pinned { + return TorchRetention::EngineBuild { + version: torch_version.clone(), + }; + } + let Some(sdk_build) = sdk_build else { + return TorchRetention::Realign; + }; + // The same version `plan_torch_alignment` would install, so the state that + // repair produces is the state recognised here on the next run. + let sdk_torch = format!("{}+{sdk_build}", therock::split_local_version(pinned).0); + if torch_version.as_str() == sdk_torch { + return TorchRetention::SdkBuild { + version: torch_version.clone(), + }; + } + TorchRetention::Realign +} + +/// The package a retained torch deliberately diverges on, if any. +/// +/// The SDK's build does not satisfy the engine's exact pin and is kept anyway; +/// the engine's own build satisfies it, so claiming a divergence there would +/// suppress a violation that has not happened. +const fn retained_diverged_package(retention: &TorchRetention) -> Option<&'static str> { + match retention { + TorchRetention::SdkBuild { .. } => Some("torch"), + TorchRetention::EngineBuild { .. } | TorchRetention::Realign => None, + } +} + +/// Render a retention as the `torch_alignment:` block, in place of an alignment. +/// +/// `Realign` renders nothing: nothing was retained, and `render_torch_alignment` +/// prints the block for the repair that runs instead. +fn render_torch_retention(retention: &TorchRetention, engine: &str) -> String { + let mut output = String::new(); + match retention { + TorchRetention::EngineBuild { version } => { + let _ = writeln!( + output, + " torch_alignment: retained_engine_build ({})", + sanitize_log_value(version) + ); + let _ = writeln!( + output, + " the torch {} pins ran a GPU kernel with this SDK", + sanitize_log_value(engine) + ); + } + TorchRetention::SdkBuild { version } => { + let _ = writeln!( + output, + " torch_alignment: retained_sdk_build ({})", + sanitize_log_value(version) + ); + let _ = writeln!( + output, + " the SDK's build of the release {} pins ran a GPU kernel", + sanitize_log_value(engine) + ); + } + TorchRetention::Realign => {} + } + output +} + +/// Print the retention block and record it. `Realign` records nothing. +fn report_torch_retention( + paths: &AppPaths, + engine: &str, + runtime_key: &str, + retention: &TorchRetention, +) { + print!("{}", render_torch_retention(retention, engine)); + let (state, version) = match retention { + TorchRetention::EngineBuild { version } => ("retained_engine_build", version), + TorchRetention::SdkBuild { version } => ("retained_sdk_build", version), + TorchRetention::Realign => return, + }; + record_cli_audit_event( + paths, + "engine", + "torch_alignment", + "info", + format!( + "engine={engine} runtime_id={runtime_key} torch_alignment={state} version={version}" + ), + None, ); - unsettled && matches!(devices, RuntimeDeviceCheck::NoDevices { .. }) } -/// An engine install finished having left a runtime that cannot open a device. +/// Whether this host has a GPU at all, answered by inspecting the host itself. +/// +/// Deliberately independent of the runtime under test: the runtime reporting no +/// devices is the symptom being judged, so it cannot also be the evidence. The +/// third state is the one that matters — a host that could not be examined is not +/// a host without a GPU, and failing a multi-gigabyte install on that guess would +/// be worse than saying what was seen. +#[derive(Debug, Clone, PartialEq, Eq)] +enum HostGpu { + Detected, + Absent, + NotVerified(String), +} + +fn detect_host_gpu() -> HostGpu { + match ExamineSummary::gather() { + Ok(summary) if summary.detected_gfx_target.is_some() => HostGpu::Detected, + Ok(_) => HostGpu::Absent, + Err(error) => HostGpu::NotVerified(format!("{error:#}")), + } +} + +/// Say why a runtime that cannot serve did not fail the install. +fn report_unverified_host_gpu(paths: &AppPaths, engine: &str, runtime_key: &str, reason: &str) { + println!(" host_gpu: not_verified ({})", sanitize_log_value(reason)); + println!(" the install is not failed on a host whose GPUs could not be inspected"); + record_cli_audit_event( + paths, + "engine", + "host_gpu", + "info", + format!("engine={engine} runtime_id={runtime_key} host_gpu=not_verified: {reason}"), + None, + ); +} + +/// Whether the runtime this install produced is one that cannot serve. +/// +/// Both bad verdicts count: a runtime that opens no device and one that opens a +/// device it cannot run a kernel on both fail at first serve. Neither says +/// anything about the alignment that preceded it — a repair that could not run +/// may still leave a working environment, and one that ran may still not have +/// helped, so what the runtime does now is the only thing read. +const fn runtime_cannot_serve(devices: &RuntimeDeviceCheck) -> bool { + matches!( + devices, + RuntimeDeviceCheck::NoDevices { .. } | RuntimeDeviceCheck::KernelFailed { .. } + ) +} + +/// Whether an install finished having produced a runtime that cannot serve. +/// +/// Gated on the host, because the same verdict means different things on +/// different machines: no device is the correct answer on a machine with no GPU, +/// and a host that could not be examined has not told us which machine this is. +/// Only a GPU found independently of the runtime turns a runtime that cannot +/// serve into a failed install. +const fn install_left_runtime_unusable(devices: &RuntimeDeviceCheck, host_gpu: &HostGpu) -> bool { + matches!(host_gpu, HostGpu::Detected) && runtime_cannot_serve(devices) +} + +/// An engine install finished having left a runtime that cannot run GPU work. /// /// A distinct type rather than a plain message so `install sdk` can tell this /// apart from an ordinary engine-install failure. It deliberately tolerates the @@ -8067,10 +8358,9 @@ impl std::error::Error for UnusableRuntimeAfterInstall {} fn unusable_runtime_error(engine: &str, runtime_key: &str) -> anyhow::Error { anyhow::Error::new(UnusableRuntimeAfterInstall(format!( - "the {engine} install left runtime `{runtime_key}` on a torch it cannot use: no GPU could \ - be opened, and the matching build from the SDK index could not be installed. The ROCm \ - SDK itself is installed; re-run once the index is reachable, or install a torch matching \ - the SDK into the runtime environment." + "the {engine} install left runtime `{runtime_key}` on a torch that cannot run GPU work \ + with this SDK, on a host where a GPU was found. The ROCm SDK itself is installed; read \ + the device check above and install a torch build that suits both this SDK and this GPU." ))) } @@ -8151,10 +8441,16 @@ fn settles_runtime_torch(engine: &str, managed_env: Option) -> bool { /// /// Every path that installs an engine into a managed runtime must call this. /// Skipping it anywhere lets the engine's own torch win silently and can leave a -/// runtime that cannot open a device — including after an explicit +/// runtime that cannot serve — including after an explicit /// `rocm engines install --reinstall`, which is exactly what someone /// reaches for when a runtime already looks wrong. /// +/// The runtime is asked what it can do before anything is written to it, and a +/// torch that already runs a GPU kernel is kept, whichever of the two intended +/// builds it is. Only when nothing has proven itself does the alignment install +/// run, and then the runtime is asked again, because that answer is now about a +/// different torch. +/// /// See `settles_runtime_torch` for the environments this deliberately skips. fn settle_engine_install( paths: &AppPaths, @@ -8166,20 +8462,58 @@ fn settle_engine_install( return Ok(()); } let python = Path::new(&response.python_executable); + let host_gpu = detect_host_gpu(); + let library_paths = runtime_library_paths_for_key(paths, runtime_key); let sdk_build = sdk_torch_build_for_key(paths, runtime_key); - let alignment = - report_torch_alignment(paths, engine, python, runtime_key, sdk_build.as_deref()); - let diverged = deliberately_diverged_package(&alignment); + let torch = therock::probe_torch_alignment(python, engine); + let probed = runtime_device_check(Some(python), &library_paths); + + let retention = classify_retained_torch( + sdk_build.as_deref(), + &probed, + torch + .as_ref() + .ok() + .and_then(|probe| probe.engine_requires_torch.as_deref()), + ); + let (diverged, devices) = match retention { + TorchRetention::Realign => { + let alignment = report_torch_alignment( + paths, + engine, + python, + runtime_key, + sdk_build.as_deref(), + torch.as_ref(), + ); + // Only a realignment replaced torch. After every other outcome the + // environment is the one already probed, and asking it again would + // spend a second interpreter launch to be told the same thing. + let devices = match alignment { + TorchAlignment::Realigned { .. } => { + runtime_device_check(Some(python), &library_paths) + } + _ => probed, + }; + (deliberately_diverged_package(&alignment), devices) + } + retention => { + report_torch_retention(paths, engine, runtime_key, &retention); + (retained_diverged_package(&retention), probed) + } + }; report_engine_dependency_check(paths, engine, Some(python), runtime_key, diverged); - let devices = report_runtime_device_check(paths, engine, Some(python), runtime_key); - - // Fail only on the conjunction: torch could not be settled AND the runtime - // cannot open a device. Either alone is survivable — an alignment that could - // not run may still leave a working environment, and a device count of zero - // is expected where no GPU is present. Together they mean this install - // produced a runtime that cannot serve, and reporting success for that is the - // whole failure this change exists to end. - if install_left_runtime_unusable(&alignment, &devices) { + let devices = report_runtime_device_check(paths, engine, runtime_key, devices); + + // A runtime that cannot serve is only this install's failure where the host + // has a GPU to serve with. Where the host could not be examined, say so + // rather than failing a multi-gigabyte install on a guess. + if let HostGpu::NotVerified(reason) = &host_gpu + && runtime_cannot_serve(&devices) + { + report_unverified_host_gpu(paths, engine, runtime_key, reason); + } + if install_left_runtime_unusable(&devices, &host_gpu) { return Err(unusable_runtime_error(engine, runtime_key)); } Ok(()) @@ -8261,12 +8595,14 @@ fn render_engine_dependency_check(engine: &str, outcome: &EngineDependencyCheck) for detail in details { let _ = writeln!(output, " divergence: {}", sanitize_log_value(detail)); } - // Deliberately not the reinstall remedy. It does not apply: every - // path that installs an engine realigns torch afterwards, so a - // reinstall reproduces this same state rather than resolving it. + // Deliberately not the reinstall remedy. It does not apply to + // either state that reaches here: a reinstall realigns torch again + // and reproduces the SDK build, and where the user has opted out of + // that realignment it would undo the decision they made. Which of + // the two this is, the block above has already said. let _ = writeln!( output, - " action: none; the SDK's build is intended here (see torch_alignment above)" + " action: none; this torch is kept on purpose (see torch_alignment above)" ); } } @@ -26807,26 +27143,299 @@ ID_LIKE="suse opensuse" torch_version: "2.11.0+gitd0c8b1f".to_owned(), hip_version: "7.2.53211".to_owned(), }; + let kernel_failed = RuntimeDeviceCheck::KernelFailed { + torch_version: "2.11.0+rocm7.13.0".to_owned(), + error: "AcceleratorError: device kernel image is invalid".to_owned(), + }; let usable = RuntimeDeviceCheck::Usable { device_count: 8, torch_version: "2.11.0+rocm7.13.0".to_owned(), }; - let failed = TorchAlignment::InstallFailed { - wanted: "2.11.0+rocm7.13.0".to_owned(), - kept: "2.11.0+gitd0c8b1f".to_owned(), - error: "dns error".to_owned(), - }; - let realigned = TorchAlignment::Realigned { - from: "2.11.0+gitd0c8b1f".to_owned(), - to: "2.11.0+rocm7.13.0".to_owned(), + + // A machine with a GPU that this runtime cannot use, either way round. + assert!(install_left_runtime_unusable( + &no_devices, + &HostGpu::Detected + )); + assert!(install_left_runtime_unusable( + &kernel_failed, + &HostGpu::Detected + )); + // A machine with a GPU and a runtime that can use it. + assert!(!install_left_runtime_unusable(&usable, &HostGpu::Detected)); + // On a host with no GPU both verdicts are the expected answer, not a + // reason to throw away a multi-gigabyte install. + assert!(!install_left_runtime_unusable( + &no_devices, + &HostGpu::Absent + )); + assert!(!install_left_runtime_unusable( + &kernel_failed, + &HostGpu::Absent + )); + // A host we could not examine is not a host without a GPU, but it is not + // evidence of one either, so it never fails the install on its own. + let unknown = HostGpu::NotVerified("lspci is not installed".to_owned()); + assert!(!install_left_runtime_unusable(&no_devices, &unknown)); + assert!(!install_left_runtime_unusable(&kernel_failed, &unknown)); + } + + #[test] + fn a_kernel_launch_failure_is_not_reported_as_a_usable_device() { + // The probe enumerated a device and then failed to run anything on it. + // Reading only the count calls that healthy, which is the whole defect: + // the install passes and the first serve dies. + let outcome = classify_runtime_device_probe(therock::RuntimeDeviceProbe { + import_ok: true, + torch_version: Some("2.11.0+rocm7.14.0".to_owned()), + hip_version: Some("7.14.0".to_owned()), + device_count: Some(1), + error: None, + kernel_error: Some("AcceleratorError: device kernel image is invalid".to_owned()), + }); + + assert_eq!( + outcome, + RuntimeDeviceCheck::KernelFailed { + torch_version: "2.11.0+rocm7.14.0".to_owned(), + error: "AcceleratorError: device kernel image is invalid".to_owned(), + } + ); + } + + #[test] + fn an_enumeration_failure_stays_a_verdict_of_its_own() { + // `error` still carries the failures that happen before any kernel runs. + // Reporting one of those as a kernel failure would name the wrong remedy. + let import_failed = classify_runtime_device_probe(therock::RuntimeDeviceProbe { + import_ok: false, + torch_version: None, + hip_version: None, + device_count: None, + error: Some("ImportError: libamdhip64.so.7".to_owned()), + kernel_error: None, + }); + let count_raised = classify_runtime_device_probe(therock::RuntimeDeviceProbe { + import_ok: true, + torch_version: Some("2.11.0+rocm7.13.0".to_owned()), + hip_version: Some("7.13.0".to_owned()), + device_count: None, + error: Some("RuntimeError: HIP failed to initialize".to_owned()), + kernel_error: None, + }); + let no_devices = classify_runtime_device_probe(therock::RuntimeDeviceProbe { + import_ok: true, + torch_version: Some("2.11.0+gitd0c8b1f".to_owned()), + hip_version: Some("7.13.0".to_owned()), + device_count: Some(0), + error: None, + kernel_error: None, + }); + + assert_eq!( + import_failed, + RuntimeDeviceCheck::NotVerified("ImportError: libamdhip64.so.7".to_owned()) + ); + assert_eq!( + count_raised, + RuntimeDeviceCheck::NotVerified("RuntimeError: HIP failed to initialize".to_owned()) + ); + assert_eq!( + no_devices, + RuntimeDeviceCheck::NoDevices { + torch_version: "2.11.0+gitd0c8b1f".to_owned(), + hip_version: "7.13.0".to_owned(), + } + ); + } + + #[test] + fn a_kernel_failure_is_reported_as_a_failure_and_says_what_broke() { + let rendered = render_runtime_device_check(&RuntimeDeviceCheck::KernelFailed { + torch_version: "2.11.0+rocm7.14.0".to_owned(), + error: "AcceleratorError: device kernel image is invalid".to_owned(), + }); + + assert!(rendered.contains(" device_check: kernel_failed\n")); + assert!(rendered.contains("2.11.0+rocm7.14.0")); + assert!(rendered.contains("device kernel image is invalid")); + // The remedy differs from `no_devices`, so the text must not borrow its + // explanation about being built for a different ROCm version. + assert!(!rendered.contains("Failed to infer device type")); + assert!(rendered.contains("no kernel image for this GPU")); + } + + /// The engine's own pin runs: keep it, and claim no divergence from it. + #[test] + fn a_working_engine_torch_is_kept_instead_of_realigned() { + let retention = classify_retained_torch( + Some("rocm7.13.0"), + &RuntimeDeviceCheck::Usable { + device_count: 1, + torch_version: "2.11.0+gitd0c8b1f".to_owned(), + }, + Some("torch==2.11.0+gitd0c8b1f"), + ); + + assert_eq!( + retention, + TorchRetention::EngineBuild { + version: "2.11.0+gitd0c8b1f".to_owned(), + } + ); + assert_eq!(retained_diverged_package(&retention), None); + let rendered = render_torch_retention(&retention, "vllm"); + assert!( + rendered.contains(" torch_alignment: retained_engine_build (2.11.0+gitd0c8b1f)\n") + ); + assert!(rendered.contains("ran a GPU kernel with this SDK")); + } + + /// The SDK's build of the pinned release runs: keep it, and expect the pin to + /// stay unsatisfied so the dependency check does not report it as a violation. + #[test] + fn a_working_sdk_torch_is_kept_as_an_intended_divergence() { + let retention = classify_retained_torch( + Some("rocm7.13.0"), + &RuntimeDeviceCheck::Usable { + device_count: 8, + torch_version: "2.11.0+rocm7.13.0".to_owned(), + }, + Some("torch==2.11.0+gitd0c8b1f"), + ); + + assert_eq!( + retention, + TorchRetention::SdkBuild { + version: "2.11.0+rocm7.13.0".to_owned(), + } + ); + assert_eq!(retained_diverged_package(&retention), Some("torch")); + let rendered = render_torch_retention(&retention, "vllm"); + assert!(rendered.contains(" torch_alignment: retained_sdk_build (2.11.0+rocm7.13.0)\n")); + assert!(rendered.contains("the SDK's build of the release vllm pins ran a GPU kernel")); + } + + /// Both intended builds are fixed points, so a repeated install settles rather + /// than swapping torch back and forth between the two package sources. + #[test] + fn repairing_a_runtime_converges_on_one_of_the_two_intended_builds() { + let after_engine_install = classify_retained_torch( + Some("rocm7.13.0"), + &RuntimeDeviceCheck::Usable { + device_count: 1, + torch_version: "2.11.0+gitd0c8b1f".to_owned(), + }, + Some("torch==2.11.0+gitd0c8b1f"), + ); + // What the realignment installs, seen on the next run over the same runtime. + let after_realignment = classify_retained_torch( + Some("rocm7.13.0"), + &RuntimeDeviceCheck::Usable { + device_count: 1, + torch_version: "2.11.0+rocm7.13.0".to_owned(), + }, + Some("torch==2.11.0+gitd0c8b1f"), + ); + + assert!(!matches!(after_engine_install, TorchRetention::Realign)); + assert!(!matches!(after_realignment, TorchRetention::Realign)); + } + + /// A pin that already names the SDK's build satisfies itself. Calling that an + /// intended divergence would suppress a violation that has not happened. + #[test] + fn a_pin_that_already_names_the_sdk_build_diverges_from_nothing() { + let retention = classify_retained_torch( + Some("rocm7.13.0"), + &RuntimeDeviceCheck::Usable { + device_count: 1, + torch_version: "2.11.0+rocm7.13.0".to_owned(), + }, + Some("torch==2.11.0+rocm7.13.0"), + ); + + assert_eq!( + retention, + TorchRetention::EngineBuild { + version: "2.11.0+rocm7.13.0".to_owned(), + } + ); + assert_eq!(retained_diverged_package(&retention), None); + } + + /// Everything that has not been shown to work goes to the repair path — which + /// is what this tool did unconditionally before it could test a kernel. + #[test] + fn a_torch_that_has_not_been_shown_to_work_is_realigned() { + let usable = |version: &str| RuntimeDeviceCheck::Usable { + device_count: 1, + torch_version: version.to_owned(), }; + let pin = Some("torch==2.11.0+gitd0c8b1f"); - // Could not settle torch, and the runtime sees nothing: unusable. - assert!(install_left_runtime_unusable(&failed, &no_devices)); - // Could not settle torch, but the runtime works anyway: not our call to fail. - assert!(!install_left_runtime_unusable(&failed, &usable)); - // Settled fine; no devices just means no GPU on this host. - assert!(!install_left_runtime_unusable(&realigned, &no_devices)); + // A third build nobody here installed on purpose. + assert_eq!( + classify_retained_torch(Some("rocm7.13.0"), &usable("2.9.0+cpu"), pin), + TorchRetention::Realign + ); + // No devices at all, whatever build it is. + assert_eq!( + classify_retained_torch( + Some("rocm7.13.0"), + &RuntimeDeviceCheck::NoDevices { + torch_version: "2.11.0+gitd0c8b1f".to_owned(), + hip_version: "7.13.0".to_owned(), + }, + pin + ), + TorchRetention::Realign + ); + // A device that cannot run a kernel is never retained, not even when it + // holds exactly the build the engine pins. + assert_eq!( + classify_retained_torch( + Some("rocm7.13.0"), + &RuntimeDeviceCheck::KernelFailed { + torch_version: "2.11.0+gitd0c8b1f".to_owned(), + error: "device kernel image is invalid".to_owned(), + }, + pin + ), + TorchRetention::Realign + ); + // No answer is not an answer. + assert_eq!( + classify_retained_torch( + Some("rocm7.13.0"), + &RuntimeDeviceCheck::NotVerified("torch did not import".to_owned()), + pin + ), + TorchRetention::Realign + ); + // Nothing pins torch, so there is no release to hold either build of. + assert_eq!( + classify_retained_torch(Some("rocm7.13.0"), &usable("2.11.0+gitd0c8b1f"), None), + TorchRetention::Realign + ); + // A range is not an exact pin, so it cannot identify the engine's build. + assert_eq!( + classify_retained_torch( + Some("rocm7.13.0"), + &usable("2.11.0+gitd0c8b1f"), + Some("torch>=2.10") + ), + TorchRetention::Realign + ); + // The manifest does not say what the SDK's build is, so a torch that is + // not the engine's pin cannot be recognised as the other intended one. + assert_eq!( + classify_retained_torch(None, &usable("2.11.0+rocm7.13.0"), pin), + TorchRetention::Realign + ); + // Nothing is retained, so nothing is reported in place of the repair. + assert!(render_torch_retention(&TorchRetention::Realign, "vllm").is_empty()); + assert_eq!(retained_diverged_package(&TorchRetention::Realign), None); } fn sdk_install_finalization() -> SdkInstallFinalization { @@ -27135,17 +27744,21 @@ ID_LIKE="suse opensuse" assert_eq!(sdk_torch_build_from_manifest(&manifest), None); } - /// Opting out skips the rewrite without needing a runtime that answers. + /// A torch the user installed themselves is kept, and named as kept. /// - /// The gate has to precede the probe. Someone reaches for it precisely when the - /// SDK's build does not work on their machine, so it cannot depend on that - /// machine probing cleanly first. Pointing it at a Python that does not exist - /// would otherwise produce a probe failure; getting the opt-out reason back - /// instead is what shows nothing ran. + /// The Python does not exist and an index is supplied, so an alignment that + /// still ran would reach the install and come back `InstallFailed`. Getting + /// the divergence described instead — the build that was not installed, and + /// the one still there — is what shows the rewrite was skipped rather than + /// attempted and then reported differently. #[test] - fn disabling_alignment_skips_the_rewrite_before_anything_is_probed() { + fn disabling_alignment_keeps_the_installed_torch_and_says_what_it_declined() { let mut env = ScopedTestEnv::new(); env.set("ROCM_CLI_DISABLE_TORCH_ALIGNMENT", "1"); + let probe = therock::TorchAlignmentProbe { + installed_torch: Some("2.9.0+cpu".to_owned()), + engine_requires_torch: Some("torch==2.11.0+gitd0c8b1f".to_owned()), + }; let outcome = align_runtime_torch( &test_app_paths(), @@ -27153,25 +27766,64 @@ ID_LIKE="suse opensuse" Some("https://example.invalid/simple"), Some("rocm7.13.0"), "vllm", + Ok(&probe), ); assert_eq!( outcome, - TorchAlignment::NotApplicable( - "torch alignment is disabled by ROCM_CLI_DISABLE_TORCH_ALIGNMENT".to_owned() - ) + TorchAlignment::Disabled { + wanted: "2.11.0+rocm7.13.0".to_owned(), + kept: "2.9.0+cpu".to_owned(), + } ); } /// Unset, the rewrite is attempted as usual. /// /// Paired with the test above so the opt-out cannot appear to work for an - /// unrelated reason: the same call with the same unusable Python has to fail at - /// the probe rather than report itself disabled. + /// unrelated reason. The index is withheld here on purpose: it is the first + /// thing read after the gate, so the call stops there rather than reaching a + /// real install, and the outcome still tells the two paths apart. #[test] fn alignment_runs_unless_the_variable_is_set() { let mut env = ScopedTestEnv::new(); env.clear("ROCM_CLI_DISABLE_TORCH_ALIGNMENT"); + let probe = therock::TorchAlignmentProbe { + installed_torch: Some("2.9.0+cpu".to_owned()), + engine_requires_torch: Some("torch==2.11.0+gitd0c8b1f".to_owned()), + }; + + let outcome = align_runtime_torch( + &test_app_paths(), + Path::new("/nonexistent/python"), + None, + Some("rocm7.13.0"), + "vllm", + Ok(&probe), + ); + + assert_eq!( + outcome, + TorchAlignment::NotApplicable( + "the runtime manifest records no wheel index to install from".to_owned() + ), + "the opt-out must not fire when the variable is unset" + ); + } + + /// Opting out of a rewrite that was never due reports the runtime as it is. + /// + /// `Disabled` says a replacement was declined. On a runtime already holding + /// the SDK's build there was none to decline, and saying otherwise would send + /// the reader looking for a torch that was spared when nothing was. + #[test] + fn disabling_alignment_over_an_aligned_runtime_still_reports_it_aligned() { + let mut env = ScopedTestEnv::new(); + env.set("ROCM_CLI_DISABLE_TORCH_ALIGNMENT", "1"); + let probe = therock::TorchAlignmentProbe { + installed_torch: Some("2.11.0+rocm7.13.0".to_owned()), + engine_requires_torch: Some("torch==2.11.0+gitd0c8b1f".to_owned()), + }; let outcome = align_runtime_torch( &test_app_paths(), @@ -27179,18 +27831,105 @@ ID_LIKE="suse opensuse" Some("https://example.invalid/simple"), Some("rocm7.13.0"), "vllm", + Ok(&probe), + ); + + assert_eq!( + outcome, + TorchAlignment::AlreadyAligned { + version: "2.11.0+rocm7.13.0".to_owned(), + } ); + } + + /// The kept torch is a deliberate divergence, and the remedy is not reinstall. + /// + /// The engine's pin is unsatisfied on purpose here, so reporting a violation + /// would answer the user's instruction with an error — and the remedy that + /// comes with it, `--reinstall`, is the one action that would undo the very + /// decision they made. + #[test] + fn a_disabled_alignment_diverges_on_torch_without_offering_a_reinstall() { + let outcome = TorchAlignment::Disabled { + wanted: "2.11.0+rocm7.13.0".to_owned(), + kept: "2.9.0+cpu".to_owned(), + }; + assert_eq!(deliberately_diverged_package(&outcome), Some("torch")); + let rendered = render_torch_alignment(&outcome, "vllm"); assert!( - !matches!( - &outcome, - TorchAlignment::NotApplicable(reason) - if reason.contains("ROCM_CLI_DISABLE_TORCH_ALIGNMENT") + rendered.contains(" torch_alignment: disabled\n"), + "the block has to name the state, got {rendered:?}" + ); + assert!( + rendered.contains( + "ROCM_CLI_DISABLE_TORCH_ALIGNMENT is set; keeping 2.9.0+cpu rather than installing 2.11.0+rocm7.13.0" ), - "the opt-out must not fire when the variable is unset, got {outcome:?}" + "the block has to name the variable and both builds, got {rendered:?}" + ); + + let dependencies = classify_dependency_details( + vec![ + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.9.0+cpu` is installed".to_owned(), + ], + deliberately_diverged_package(&outcome), + ); + assert!(matches!( + dependencies, + EngineDependencyCheck::ExpectedDivergence(_) + )); + let rendered = render_engine_dependency_check("vllm", &dependencies); + assert!( + !rendered.contains("--reinstall"), + "the remedy would undo the opt-out, got {rendered:?}" ); } + /// Opting out suppresses the correction, not the diagnosis. + /// + /// The escape hatch exists because the SDK's build does not always work, so it + /// cannot also become a way to make a runtime that does not work pass. The + /// device check is taken over the torch the user kept and read exactly as it + /// would be otherwise: a host with a GPU the runtime cannot open, or can open + /// and then not run a kernel on, still fails the install. + #[test] + fn opting_out_still_fails_an_install_that_left_a_gpu_host_unable_to_serve() { + let no_devices = RuntimeDeviceCheck::NoDevices { + torch_version: "2.9.0+cpu".to_owned(), + hip_version: "7.2.53211".to_owned(), + }; + let kernel_failed = RuntimeDeviceCheck::KernelFailed { + torch_version: "2.9.0+cpu".to_owned(), + error: "AcceleratorError: device kernel image is invalid".to_owned(), + }; + + assert!(install_left_runtime_unusable( + &no_devices, + &HostGpu::Detected + )); + assert!(install_left_runtime_unusable( + &kernel_failed, + &HostGpu::Detected + )); + } + + /// On a machine with no GPU the same verdict is the correct answer. + /// + /// Someone running a CPU torch deliberately is the person this opt-out is for, + /// and a runtime reporting no device there has not failed at anything. + #[test] + fn opting_out_on_a_host_with_no_gpu_leaves_the_install_successful() { + let no_devices = RuntimeDeviceCheck::NoDevices { + torch_version: "2.9.0+cpu".to_owned(), + hip_version: "7.2.53211".to_owned(), + }; + + assert!(!install_left_runtime_unusable( + &no_devices, + &HostGpu::Absent + )); + } + #[test] fn a_runtime_that_sees_no_devices_names_the_torch_that_cannot_use_the_sdk() { // The install succeeded and the engine's requirements are satisfied, so diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 6a0bc3521..c520acf2f 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -2640,13 +2640,19 @@ fn parse_rocm_sdk_probe(output: &str) -> Result { serde_json::from_str(output.trim()).context("failed to parse rocm_sdk probe output") } -/// What the runtime's torch reports about the GPUs it can actually open. +/// What the runtime's torch reports about the GPUs it can actually open, and +/// whether those GPUs can actually run a kernel. /// /// [`validate_rocm_sdk_runtime_probe`] establishes that the SDK's libraries are /// present and resolvable. That is not the same question as whether the torch /// sharing the venv can enumerate a device: a torch built against a different /// HIP version loads happily against those libraries and then reports no /// devices at all. +/// +/// Enumeration succeeding is in turn not the same question as the device being +/// usable. A torch built against a different HIP version can enumerate the +/// GPUs and then fault on the first kernel it launches, so the two failures are +/// reported separately and must not be conflated. #[derive(Debug, Clone, Default, serde::Deserialize)] pub(crate) struct RuntimeDeviceProbe { pub import_ok: bool, @@ -2654,7 +2660,12 @@ pub(crate) struct RuntimeDeviceProbe { pub hip_version: Option, /// `None` when torch never imported, so "unknown" stays distinct from "zero". pub device_count: Option, + /// An import or enumeration failure. Never a kernel failure. pub error: Option, + /// A GPU kernel failure observed *after* devices enumerated successfully. + /// `None` when no kernel was attempted (no devices, or enumeration failed). + #[serde(default)] + pub kernel_error: Option, } /// Ask the runtime's own interpreter how many devices its torch can open. @@ -2698,7 +2709,11 @@ pub(crate) fn probe_runtime_devices( python_executable.display() ) })?; - serde_json::from_str(text.trim()).context("failed to parse runtime device probe output") + parse_runtime_device_probe(&text) +} + +fn parse_runtime_device_probe(output: &str) -> Result { + serde_json::from_str(output.trim()).context("failed to parse runtime device probe output") } /// The loader search-path variable used to expose the runtime's ROCm libraries. @@ -2718,6 +2733,7 @@ out = { "hip_version": None, "device_count": None, "error": None, + "kernel_error": None, } try: @@ -2730,6 +2746,19 @@ try: except Exception as exc: out["error"] = type(exc).__name__ + ": " + str(exc) +# Enumeration is not execution. A runtime whose torch and HIP disagree can +# report devices and then fault on the first kernel, so the kernel is launched +# under its own guard and its failure is recorded in its own field. Only run it +# once enumeration actually produced a device: with no devices there is nothing +# to execute on, and a failed import has already been described. +if out["error"] is None and (out["device_count"] or 0) > 0: + try: + probe = torch.ones(32, device="cuda") + probe.add_(1.0) + torch.cuda.synchronize() + except Exception as exc: + out["kernel_error"] = type(exc).__name__ + ": " + str(exc) + print(json.dumps(out)) "#; @@ -5642,4 +5671,104 @@ echo Python 3.12.10 "invalid calendar dates should not be displayed" ); } + + #[test] + fn runtime_device_probe_without_kernel_error_field_still_parses() { + // Output produced before the kernel probe existed must not become a parse + // failure: an older runtime's probe is still a valid "no kernel attempted". + let probe = parse_runtime_device_probe( + r#"{"import_ok":true,"torch_version":"2.11.0","hip_version":"7.13", + "device_count":8,"error":null}"#, + ) + .expect("probe without kernel_error should parse"); + + assert_eq!(probe.device_count, Some(8)); + assert_eq!(probe.error, None); + assert_eq!(probe.kernel_error, None); + } + + #[test] + fn runtime_device_probe_keeps_kernel_failures_out_of_the_enumeration_error() { + // The distinction the caller acts on: devices were found, so this is not a + // "no devices" runtime, but the GPU cannot run work. + let probe = parse_runtime_device_probe( + r#"{"import_ok":true,"torch_version":"2.11.0","hip_version":"7.13", + "device_count":8,"error":null, + "kernel_error":"RuntimeError: HIP error: invalid device function"}"#, + ) + .expect("probe with kernel_error should parse"); + + assert_eq!(probe.device_count, Some(8)); + assert_eq!(probe.error, None); + assert_eq!( + probe.kernel_error.as_deref(), + Some("RuntimeError: HIP error: invalid device function") + ); + } + + #[test] + fn runtime_device_probe_reports_import_failures_only_as_enumeration_errors() { + let probe = parse_runtime_device_probe( + r#"{"import_ok":false,"torch_version":null,"hip_version":null, + "device_count":null,"error":"ImportError: no module named torch", + "kernel_error":null}"#, + ) + .expect("failed-import probe should parse"); + + assert!(!probe.import_ok); + assert_eq!(probe.device_count, None); + assert_eq!(probe.kernel_error, None); + assert_eq!( + probe.error.as_deref(), + Some("ImportError: no module named torch") + ); + } + + #[test] + fn runtime_device_probe_script_guards_the_kernel_behind_successful_enumeration() { + // The script is the contract: a kernel must never be launched when the + // import or enumeration already failed, or when there is no device to launch + // it on. Getting this wrong turns a "no devices" runtime into a crash. + assert!( + RUNTIME_DEVICE_PROBE_SCRIPT + .contains(r#"if out["error"] is None and (out["device_count"] or 0) > 0:"#), + "the kernel attempt must be gated on a clean enumeration with devices" + ); + + let guard = RUNTIME_DEVICE_PROBE_SCRIPT + .split_once(r#"if out["error"] is None"#) + .expect("script should contain the kernel guard") + .0; + assert!( + !guard.contains("device=\"cuda\"") && !guard.contains("synchronize"), + "no kernel work may run before the guard" + ); + } + + #[test] + fn runtime_device_probe_script_records_kernel_failures_in_their_own_field() { + let kernel_section = RUNTIME_DEVICE_PROBE_SCRIPT + .split_once(r#"if out["error"] is None"#) + .expect("script should contain the kernel guard") + .1; + + // Allocate, mutate, and synchronize: an unusable GPU commonly survives the + // allocation and only faults once work is actually launched and awaited. + assert!(kernel_section.contains(r#"torch.ones(32, device="cuda")"#)); + assert!(kernel_section.contains("probe.add_(1.0)")); + assert!(kernel_section.contains("torch.cuda.synchronize()")); + + assert!( + kernel_section.contains(r#"out["kernel_error"] = type(exc).__name__"#), + "a kernel failure must be recorded in kernel_error" + ); + assert!( + !kernel_section.contains(r#"out["error"] ="#), + "the kernel attempt must never overwrite the enumeration error" + ); + assert!( + !kernel_section.contains(r#"out["device_count"] ="#), + "a kernel failure must preserve the enumerated device count" + ); + } } diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index 23f698673..27c16b051 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -551,6 +551,7 @@ fn assess_runtime_repair(runtime: &VllmRuntime) -> RepairAssessment { Ok(violations) => repair_from_violations( &violations, recorded_sdk_torch_build(&runtime.runtime_id).as_deref(), + torch_alignment_disabled(), ), // An unusable `uv` or an offline host must not block an install that would // otherwise succeed; report that the check did not run and carry on as before. @@ -561,6 +562,18 @@ fn assess_runtime_repair(runtime: &VllmRuntime) -> RepairAssessment { /// The package whose build the SDK and the engine both have an opinion about. const TORCH_PACKAGE: &str = "torch"; +/// Whether the user has opted out of rocm-cli choosing this runtime's torch. +/// +/// The same variable, read the same way, as the CLI's own opt-out: presence is the +/// signal, so any value — including the empty string — disables the alignment. The +/// engine cannot call into the CLI binary that owns the alignment itself, so the +/// contract is duplicated rather than shared; the two must not drift, or a runtime +/// the CLI deliberately left alone gets rewritten by the engine on the very next +/// `rocm engines install vllm`, which is the fight the opt-out exists to end. +fn torch_alignment_disabled() -> bool { + std::env::var_os("ROCM_CLI_DISABLE_TORCH_ALIGNMENT").is_some() +} + /// Whether this violation is the torch divergence rocm-cli deliberately leaves behind. /// /// After an engine install, rocm-cli puts back the SDK's *build* of the torch release @@ -572,8 +585,10 @@ const TORCH_PACKAGE: &str = "torch"; /// next invocation starts over — two full torch-stack flips a run, and a warning /// claiming a repair that undid the intended state. /// -/// Three conditions, all required, and they are exactly the rule rocm-cli applies: -/// take the *release* from the engine's pin and the *build* from the SDK. +/// With the alignment disabled the rule is simply "any torch pin": see below. +/// +/// Otherwise three conditions, all required, and they are exactly the rule rocm-cli +/// applies: take the *release* from the engine's pin and the *build* from the SDK. /// /// The violation must be about torch — any other unmet requirement is real. The /// installed release must be the one the engine pins; an SDK torch of a *different* @@ -582,16 +597,30 @@ const TORCH_PACKAGE: &str = "torch"; /// the runtime's manifest records for the SDK; a torch from neither side is the /// breakage this check exists to catch. Miss any one and the engine either fights the /// alignment or silently accepts a runtime that cannot serve. -fn is_intended_torch_divergence(detail: &str, sdk_torch_build: Option<&str>) -> bool { - let Some(sdk_torch_build) = sdk_torch_build else { - return false; - }; +fn is_intended_torch_divergence( + detail: &str, + sdk_torch_build: Option<&str>, + torch_alignment_disabled: bool, +) -> bool { let Some(subject) = violation_subject(detail) else { return false; }; if !subject.package.eq_ignore_ascii_case(TORCH_PACKAGE) { return false; } + // Opted out, so rocm-cli does not choose this runtime's torch and no build it + // holds can be wrong *here*: the pin is unmet because the user meant it to be. + // The build and release tests below are the aligned-case rule — asking a + // hand-installed torch to match the SDK's build would fail every time, and the + // reinstall that followed would install the engine's build over exactly the torch + // the opt-out exists to keep. Only torch is spared: the check above already + // rejected every other package, so an unrelated vLLM-owned defect still repairs. + if torch_alignment_disabled { + return true; + } + let Some(sdk_torch_build) = sdk_torch_build else { + return false; + }; let (Some(required), Some(installed)) = (subject.required.as_deref(), subject.installed.as_deref()) else { @@ -608,24 +637,43 @@ fn is_intended_torch_divergence(detail: &str, sdk_torch_build: Option<&str>) -> /// `sdk_torch_build` is the build identifier the runtime's manifest records for the /// SDK's torch, or `None` when it cannot be determined — in which case nothing is /// treated as intended and the previous behaviour stands. +/// +/// `torch_alignment_disabled` is the user's opt-out. It changes which violations count +/// as defects, never whether defects are acted on: a torch pin stops being one, and +/// everything else vLLM requires is assessed exactly as before. Returning early on the +/// opt-out instead would hide a broken torchvision behind an unrelated preference. fn repair_from_violations( violations: &[DependencyViolation], sdk_torch_build: Option<&str>, + torch_alignment_disabled: bool, ) -> RepairAssessment { let owned = violations_requiring(violations, ENGINE_NAME); if owned.is_empty() { return RepairAssessment::default(); } - let (intended, defects): (Vec<&DependencyViolation>, Vec<&DependencyViolation>) = owned - .into_iter() - .partition(|violation| is_intended_torch_divergence(&violation.detail, sdk_torch_build)); + let (intended, defects): (Vec<&DependencyViolation>, Vec<&DependencyViolation>) = + owned.into_iter().partition(|violation| { + is_intended_torch_divergence( + &violation.detail, + sdk_torch_build, + torch_alignment_disabled, + ) + }); if defects.is_empty() { - // Nothing to repair. Reinstalling here would replace the SDK's build with the - // engine's and hand back a runtime that cannot open a device. - let mut notes = vec![ - "the runtime holds the SDK's build of the torch vLLM pins; that divergence is intended and a reinstall would undo it".to_owned(), - ]; + // Nothing to repair. Reinstalling here would replace that torch with the + // engine's build and hand back a runtime nobody asked for. + // + // Which sentence is true depends on whose torch this is. Under the alignment it + // is the SDK's and rocm-cli put it there; under the opt-out it is the user's and + // rocm-cli never touched it. Reusing the first line for the second case would + // tell a user who hand-installed torch that the CLI had installed it for them. + let headline = if torch_alignment_disabled { + "torch alignment is disabled by ROCM_CLI_DISABLE_TORCH_ALIGNMENT; the torch this runtime holds is the user's and a reinstall would replace it" + } else { + "the runtime holds the SDK's build of the torch vLLM pins; that divergence is intended and a reinstall would undo it" + }; + let mut notes = vec![headline.to_owned()]; notes.extend( intended .iter() @@ -657,7 +705,11 @@ fn repair_from_violations( .iter() .map(|violation| format!("expected divergence: {}", violation.detail)), ); - if sdk_torch_build.is_none() { + // The hint blames `rocm install sdk` for writing the SDK torch stack over vLLM's + // pins, which is only a live theory while the alignment runs. Under the opt-out + // torch is never a defect here, so the hint would be pointing at the one package + // that cannot be the cause. + if sdk_torch_build.is_none() && !torch_alignment_disabled { notes.push( "if this recurs after `rocm install sdk`, the SDK torch stack is being written over vLLM's pinned torch".to_owned(), ); @@ -2869,10 +2921,19 @@ mod tests { /// The build identifier the SDK recorded in the runtimes used by these tests. const SDK_BUILD: &str = "rocm7.13.0"; + /// `repair_from_violations`'s opt-out argument, named at the call sites so a bare + /// `false`/`true` does not have to be decoded against the signature. + const ALIGNED: bool = false; + const OPTED_OUT: bool = true; + #[test] fn a_consistent_environment_is_not_reinstalled() { + // The other settled state: the SDK published no build of the release vLLM + // pins, so the runtime kept the engine's own build and the exact pin is + // satisfied. `uv pip check` reports nothing at all, and the recorded SDK + // build must not manufacture a finding out of that silence. assert_eq!( - repair_from_violations(&[], Some(SDK_BUILD)), + repair_from_violations(&[], Some(SDK_BUILD), ALIGNED), RepairAssessment::default() ); } @@ -2889,6 +2950,7 @@ mod tests { "The package `vllm` requires `torch==2.10.0+git8514f05`, but `2.9.1+rocm7.14.0a20260611` is installed", )], Some("rocm7.14.0a20260611"), + ALIGNED, ); assert!(assessment.needed); @@ -2915,6 +2977,7 @@ mod tests { "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed", )], Some(SDK_BUILD), + ALIGNED, ); assert!( @@ -2943,6 +3006,7 @@ mod tests { "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+cpu` is installed", )], Some(SDK_BUILD), + ALIGNED, ); assert!(assessment.needed); @@ -2959,6 +3023,7 @@ mod tests { "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed", )], None, + ALIGNED, ); assert!(assessment.needed); @@ -2998,6 +3063,7 @@ mod tests { ), ], Some(SDK_BUILD), + ALIGNED, ); assert!(assessment.needed); @@ -3043,6 +3109,30 @@ mod tests { ); } + #[test] + fn an_sdk_built_torchvision_is_still_a_violation() { + // The SDK writes the whole torch stack, so torchvision can carry the same + // build as torch and — when the releases happen to line up — look exactly + // like the intended divergence. rocm-cli realigns torch and nothing else, so + // this is a real violation the engine must repair. The stack test above + // cannot catch a regression here: its torchvision release differs too, so + // the release check alone would still reject it. + let assessment = repair_from_violations( + &[violation( + "vllm", + "The package `vllm` requires `torchvision==0.24.1+d801a34`, but `0.24.1+rocm7.13.0` is installed", + )], + Some(SDK_BUILD), + ALIGNED, + ); + + assert!( + assessment.needed, + "only torch is realigned; another package at the SDK's build is a genuine violation: {:?}", + assessment.notes + ); + } + #[test] fn unrelated_upstream_conflicts_do_not_force_a_reinstall() { // These environments routinely carry conflicts between third-party packages. @@ -3059,11 +3149,117 @@ mod tests { ), ], Some(SDK_BUILD), + ALIGNED, ); assert_eq!(assessment, RepairAssessment::default()); } + #[test] + fn an_opted_out_custom_torch_alone_does_not_force_a_reinstall() { + // The runtime the opt-out exists to produce: the user set + // ROCM_CLI_DISABLE_TORCH_ALIGNMENT, rocm-cli left their torch alone, and the + // engine's exact pin is therefore unmet. The build belongs to neither the SDK + // nor the engine — it is whatever the user chose — so the aligned-case rule + // would call it a defect and reinstall vLLM, which installs the engine's torch + // over the one the opt-out was set to keep. That is the CLI-side fight moved + // into the engine, and it would make the opt-out worthless on any managed + // runtime. + let assessment = repair_from_violations( + &[violation( + "vllm", + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.9.1+cu128` is installed", + )], + Some(SDK_BUILD), + OPTED_OUT, + ); + + assert!( + !assessment.needed, + "the opt-out must spare a hand-installed torch: {:?}", + assessment.notes + ); + assert!( + assessment + .notes + .iter() + .all(|note| !note.contains("was reinstalled")), + "no note may claim a repair that did not happen: {:?}", + assessment.notes + ); + assert!( + assessment + .notes + .iter() + .any(|note| note.contains("ROCM_CLI_DISABLE_TORCH_ALIGNMENT")), + "the reason given must be the opt-out, not a divergence rocm-cli produced: {:?}", + assessment.notes + ); + assert!( + assessment + .notes + .iter() + .all(|note| !note.contains("the runtime holds the SDK's build")), + "rocm-cli did not install this torch and must not say it did: {:?}", + assessment.notes + ); + } + + #[test] + fn an_opted_out_custom_torch_still_repairs_an_unrelated_defect() { + // The opt-out is about torch, not about the environment. A vLLM-owned pin that + // has nothing to do with torch is broken the same way it was before, and + // reinstalling vLLM is still what fixes it. Returning early on the opt-out + // would hide this defect behind a preference about a different package, and the + // runtime would stay unable to serve with nothing said about why. + let assessment = repair_from_violations( + &[ + violation( + "vllm", + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.9.1+cu128` is installed", + ), + violation( + "vllm", + "The package `vllm` requires `torchvision==0.24.1+d801a34`, but `0.20.0+cu128` is installed", + ), + ], + Some(SDK_BUILD), + OPTED_OUT, + ); + + assert!( + assessment.needed, + "an unrelated vLLM pin is still a defect under the opt-out: {:?}", + assessment.notes + ); + let violation_notes: Vec<&String> = assessment + .notes + .iter() + .filter(|note| note.starts_with("violation: ")) + .collect(); + assert_eq!( + violation_notes.len(), + 1, + "only the unrelated pin is a violation: {:?}", + assessment.notes + ); + assert!( + violation_notes[0].contains("torchvision=="), + "the defect named must be the unrelated one: {:?}", + assessment.notes + ); + assert!( + assessment + .notes + .iter() + .any(|note| note.starts_with("expected divergence: ") + && note.contains("torch==2.11.0+gitd0c8b1f")), + "the spared torch is still named, so the reader is not left thinking the \ + reinstall was about torch: {:?}", + assessment.notes + ); + } + #[test] fn a_recorded_sdk_torch_names_the_build() { let manifest = TheRockRuntimeManifest { @@ -3103,6 +3299,52 @@ mod tests { ); } + #[test] + fn a_settled_runtime_converges_on_the_build_its_own_manifest_records() { + // The convergence proof the tests above cannot give on their own. They hand + // the classification a build literal, so a change to what + // `sdk_torch_build_from_manifest` yields — `7.13.0` where the local segment + // reads `rocm7.13.0`, say — would leave every one of them passing while the + // real pipeline churned forever: the engine would call the realigned torch a + // defect, reinstall its own build, rocm-cli would put the SDK's back, and the + // next invocation would start over. Feeding the classification the value the + // manifest actually produces is what ties the two halves together. + // + // The SDK's own torch release is deliberately not the one vLLM pins, because + // that is the case realignment exists for: the release comes from the engine, + // only the build comes from the SDK. + let settled = violation( + "vllm", + "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed", + ); + let recorded = TheRockRuntimeManifest { + sdk_torch: Some("2.9.1+rocm7.13.0".to_owned()), + ..TheRockRuntimeManifest::default() + }; + // Written before `sdk_torch` was recorded. These runtimes are already on real + // machines, so they have to settle too rather than churn forever. + let reconstructed = TheRockRuntimeManifest { + rocm_sdk: Some(RocmSdkRuntimeProbe { + rocm_sdk_version: Some("7.13.0".to_owned()), + ..RocmSdkRuntimeProbe::default() + }), + ..TheRockRuntimeManifest::default() + }; + + for manifest in [recorded, reconstructed] { + let build = sdk_torch_build_from_manifest(&manifest) + .expect("both manifest generations identify the SDK's build"); + let assessment = + repair_from_violations(std::slice::from_ref(&settled), Some(&build), ALIGNED); + + assert!( + !assessment.needed, + "the state rocm-cli settles on must survive the engine's own check: {:?}", + assessment.notes + ); + } + } + #[test] fn an_unrunnable_check_reports_itself_without_forcing_a_reinstall() { let assessment = unverified_repair("uv binary is unavailable"); diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 61e246f2b..f05db5cfe 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -35,12 +35,49 @@ Feature: Runtime configuration # "settled correctly" from "skipped entirely" — and skipping is the regression the # gate in front of the settle step would produce. Only the alignment block # separates them, and it is the one part of this path with no other e2e coverage. + # It reads the block's verdict rather than one string, because a torch that has + # already run a GPU kernel with this SDK is kept instead of rewritten and reports + # a `retained_*` verdict — settled, with nothing installed. @id:runtime-sdk-reinstall-keeps-engine-consistent @requires-gpu @requires-engine:vllm @nightly Scenario: 4 - Reinstalling the SDK leaves the installed engine able to use the GPU Given a managed runtime with an inference engine already installed When the user installs the SDK again Then the runtime can still use the GPU - And the torch alignment settled on the SDK's build + And the torch alignment settled rather than being skipped + + # `ROCM_CLI_DISABLE_TORCH_ALIGNMENT` is the exit for the machine where the stack + # the alignment settles on — the SDK's build of the release the engine pins — + # does not work. That stack is not validated against the supported matrix, and + # the alignment runs on every path that installs an engine, so without the + # opt-out a torch the user installed deliberately is replaced again by the next + # command and the only remaining exit is to stop using the CLI. + # + # Nothing asserted it from the user's side. The unit tests reach the gate + # directly, and a gate that is honoured in isolation but bypassed by the install + # path around it looks identical to a working one from every surface a user can + # see. This is the same reinstall as scenario 4 with the opt-out set, so what + # differs between them is exactly the variable. + # + # Four claims across three Thens, because the opt-out is only coherent if all + # four hold: torch was not rewritten; the skip is reported as its own verdict + # rather than folded into the generic `not_applicable`, which would leave the + # user unable to tell whether the variable did anything; the divergence the + # opt-out deliberately leaves behind is not then sold back to that user as a + # runtime to repair by reinstalling the engine — an instruction that would undo + # what they asked for; and the checks the opt-out does not suppress still run, + # because it suppresses the correction, not the diagnosis. + # + # Same lane as scenario 4 and for the same reasons: a real SDK install and a + # real engine, on the serialized nightly GPU runners. `@requires-engine:vllm` + # because only vLLM shares the runtime environment the alignment writes into. + @id:runtime-torch-alignment-opt-out @requires-gpu @requires-engine:vllm @nightly + Scenario: 9 - Opting out of the torch alignment keeps the torch the user installed + Given a managed runtime with an inference engine already installed + And the user has opted out of realigning torch + When the user installs the SDK again + Then the torch alignment reports the opt-out instead of rewriting torch + And the install does not offer to reinstall the engine over the kept torch + And the runtime's device health is still reported # The GPU E2E lanes no longer install the shared runtime once and keep it # forever: `xtask e2e-prewarm` asks `rocm update` whether the channel index has diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index ee9daf599..1f79d4563 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -161,9 +161,34 @@ async fn setup_runtime_with_engine(world: &mut E2eWorld) { assert_engine_ready(world); } +/// Record the torch-alignment opt-out for this scenario's next `rocm` command. +/// +/// A behavioural precondition rather than a mechanism the feature file has to +/// name: the When step stays a plain "the user installs the SDK" and consumes +/// this on the way through. The CLI reads presence rather than value, so the +/// value is arbitrary. +#[given("the user has opted out of realigning torch")] +async fn setup_torch_alignment_opt_out(world: &mut E2eWorld) { + world + .command_env + .push(("ROCM_CLI_DISABLE_TORCH_ALIGNMENT", "1".into())); +} + #[when("the user installs the SDK")] async fn user_installs_sdk(world: &mut E2eWorld) { - let stdout = crate::run_rocm_ok(world, &["install", "sdk"]); + // Through `run_rocm_with_scenario_env` rather than `run_rocm_ok` so a Given + // can attach a behavioural fixture to this invocation — the torch-alignment + // opt-out is one — without the Gherkin naming an environment variable. The + // exit code is still asserted here, with the same diagnostic bundle + // `run_rocm_ok` prints: an install that failed leaves every Then behind it + // reading output that was never produced. + let args = ["install", "sdk"]; + let (stdout, stderr, rc) = crate::run_rocm_with_scenario_env(world, &args); + assert!( + rc == 0, + "{}", + e2e_cucumber::cli_failure_report(&args, rc, &stdout, &stderr) + ); world.cli_output = Some(stdout); } @@ -192,41 +217,75 @@ async fn assert_runtime_can_still_use_the_gpu(world: &mut E2eWorld) { ); } -/// The alignment ran, and settled on one of its two healthy outcomes. +/// The verdict on the ` torch_alignment: ` line, without the value some +/// verdicts carry after it. +/// +/// One line carries the whole outcome, and everything that can settle this +/// question prints through it — the alignment itself, and the retention that +/// stands in for it when a torch has already run a GPU kernel with this SDK — so +/// reading the verdict is what tells the settled states apart from each other and +/// from a skip. +fn torch_alignment_verdict(output: &str) -> &str { + output + .lines() + .find_map(|line| line.trim().strip_prefix("torch_alignment: ")) + .unwrap_or_else(|| { + panic!("no torch alignment block, so the runtime was never settled:\n{output}") + }) + .split_whitespace() + .next() + .unwrap_or_default() +} + +/// Verdicts that mean the runtime was settled and its torch left in a state this +/// tool produces on purpose. +/// +/// Four, not one. Whether a reinstall rewrites torch or finds it already correct +/// depends on what the shared pre-warm tree held when the scenario started, and a +/// torch that has already executed a GPU kernel with this SDK is kept exactly as +/// it is and reported as `retained_*` — settled without anything being installed. +const SETTLED_TORCH_ALIGNMENTS: [&str; 4] = [ + "realigned", + "already_aligned", + "retained_sdk_build", + "retained_engine_build", +]; + +/// Verdicts that mean the question was left unanswered. +/// +/// `not_applicable` is the one that would otherwise go unnoticed: it is what a +/// manifest yielding no SDK build produces, which is exactly the repair path for +/// every runtime installed before `sdk_torch` was recorded. `disabled` belongs +/// here for every scenario that did not ask for the opt-out — seeing it means the +/// opt-out was applied to a user who never set it. +const UNSETTLED_TORCH_ALIGNMENTS: [&str; 4] = [ + "torch_alignment: unavailable", + "torch_alignment: install_failed", + "torch_alignment: not_applicable", + "torch_alignment: disabled", +]; + +/// The alignment ran, and settled. /// /// Separate from the device check above because the two can disagree: a runtime /// whose torch was never touched at all can still open a device, so that check /// passes whether or not the alignment fired. The `torch_alignment:` block is the /// only evidence that `settle_engine_install` reached this engine, and the gate in /// front of it is the part most likely to be widened or narrowed by a later change. -#[then("the torch alignment settled on the SDK's build")] +#[then("the torch alignment settled rather than being skipped")] async fn assert_torch_alignment_settled(world: &mut E2eWorld) { let output = world.cli_output.as_deref().expect("no install output"); + let verdict = torch_alignment_verdict(output); assert!( - output.contains("torch_alignment:"), - "no torch alignment block, so the runtime was never settled:\n{output}" + SETTLED_TORCH_ALIGNMENTS.contains(&verdict), + "torch alignment reached no settled outcome (`{verdict}`):\n{output}" ); - // Both healthy outcomes are accepted rather than pinning one: whether this - // reinstall rewrote torch or found it already correct depends on what the shared - // tree held when the scenario started, and either way the rule held. - assert!( - output.contains("torch_alignment: realigned") - || output.contains("torch_alignment: already_aligned"), - "torch alignment reached no healthy outcome:\n{output}" - ); - // Asserted negatively as well, because the positive check above would pass on a - // second block that failed. `not_applicable` is the one that would otherwise go - // unnoticed: it is what a manifest yielding no SDK build produces, which is - // exactly the repair path for every runtime installed before `sdk_torch` was - // recorded. - for unhealthy in [ - "torch_alignment: unavailable", - "torch_alignment: install_failed", - "torch_alignment: not_applicable", - ] { + // Asserted negatively as well, because the check above reads only the first + // block and would pass on a second one that failed. + for unsettled in UNSETTLED_TORCH_ALIGNMENTS { assert!( - !output.contains(unhealthy), - "torch alignment reported `{unhealthy}`:\n{output}" + !output.contains(unsettled), + "torch alignment reported `{unsettled}`:\n{output}" ); } // Conditional on purpose. A divergence is today's steady state — the engine pins @@ -242,6 +301,97 @@ async fn assert_torch_alignment_settled(world: &mut E2eWorld) { } } +/// The opt-out was honoured, and said so in its own words. +/// +/// `realigned` is the one verdict that proves it was ignored, and it is rejected +/// unconditionally — that is this step's falsifiable half. `disabled` cannot be +/// demanded unconditionally alongside it: the CLI only has a rewrite to skip when +/// the runtime's torch is neither of the two builds it settles on, and whether +/// this reinstall leaves such a torch depends on whether the SDK's own torch +/// release is the release the engine pins — a property of the channel index on +/// the day, not of anything the scenario controls. Every other accepted verdict +/// is one where torch was kept as it was, which is what the user asked for. +/// +/// `not_applicable` is rejected for a reason of its own: it is the generic bucket +/// for "nothing to decide", and folding the opt-out into it leaves the user who +/// set the variable unable to tell whether it took effect. The reason line is +/// read too, so the block names the variable that caused the skip rather than +/// leaving the reader to guess which of several causes applied. +#[then("the torch alignment reports the opt-out instead of rewriting torch")] +async fn assert_torch_alignment_opted_out(world: &mut E2eWorld) { + let output = world.cli_output.as_deref().expect("no install output"); + let verdict = torch_alignment_verdict(output); + assert!( + verdict != "realigned", + "torch was realigned even though the user opted out:\n{output}" + ); + // Every settled state except the rewrite, plus the skip the opt-out produces. + let kept_torch = verdict == "disabled" || SETTLED_TORCH_ALIGNMENTS.contains(&verdict); + assert!( + kept_torch, + "the opt-out left torch in a state this tool does not produce (`{verdict}`):\n{output}" + ); + if verdict == "disabled" { + assert!( + output.contains("ROCM_CLI_DISABLE_TORCH_ALIGNMENT"), + "the skipped alignment does not name the variable that skipped it:\n{output}" + ); + } +} + +/// The kept torch is not sold to the user as a runtime to repair. +/// +/// Two surfaces print a repair for the same divergence and both have to be quiet +/// about this one: the CLI's own remedy line under a violated dependency check, +/// and the engine's built-in repair, which reports through the install's +/// `warning:` lines. A user who deliberately kept their torch and is then told to +/// reinstall the engine has been handed an instruction that undoes what they +/// asked for. +/// +/// The classification underneath is asserted as well rather than only its two +/// symptoms, because a remedy could be dropped from the renderer while the +/// divergence is still recorded as a defect — which is what every other reader of +/// that verdict, including `engines list`, would act on. +#[then("the install does not offer to reinstall the engine over the kept torch")] +async fn assert_no_reinstall_remedy(world: &mut E2eWorld) { + let output = world.cli_output.as_deref().expect("no install output"); + assert!( + !output.contains("action: rocm engines install vllm --reinstall"), + "the CLI told the user to reinstall vLLM over the torch they kept:\n{output}" + ); + assert!( + !output.contains("vLLM was reinstalled"), + "the engine's built-in repair replaced the torch the user kept:\n{output}" + ); + assert!( + !output.contains("dependency_check: violated"), + "the torch the user kept was reported as an unmet requirement:\n{output}" + ); +} + +/// Opting out of the correction did not opt out of the diagnosis. +/// +/// The install exited 0 — the When asserts that — and on a host with a GPU that +/// is only allowed for a runtime that can use it: a runtime that opens no device, +/// or opens one it cannot run a kernel on, fails the install. So the presence of +/// the block and the absence of both bad verdicts together say the health check +/// still ran and still had teeth, without pinning a device count this scenario +/// does not own. +#[then("the runtime's device health is still reported")] +async fn assert_device_health_reported(world: &mut E2eWorld) { + let output = world.cli_output.as_deref().expect("no install output"); + assert!( + output.contains("device_check:"), + "the opt-out suppressed the device check as well as the rewrite:\n{output}" + ); + for unusable in ["device_check: no_devices", "device_check: kernel_failed"] { + assert!( + !output.contains(unusable), + "the install reported `{unusable}` and exited 0 anyway:\n{output}" + ); + } +} + /// The engine inventory reports a usable engine runtime. /// /// A precondition only. It deliberately has no Then counterpart: `engines list` From 74a157a60735cfe92a36b8469397442f689f1ab1 Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Mon, 31 Aug 2026 11:46:42 -0700 Subject: [PATCH 07/19] fix(e2e): ensure reused runtimes have an engine Signed-off-by: Michael Roy --- xtask/src/e2e_prewarm.rs | 137 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 136 insertions(+), 1 deletion(-) diff --git a/xtask/src/e2e_prewarm.rs b/xtask/src/e2e_prewarm.rs index a8d9f3eef..db3aabf98 100644 --- a/xtask/src/e2e_prewarm.rs +++ b/xtask/src/e2e_prewarm.rs @@ -360,10 +360,25 @@ pub fn run(channel: &str, keep: usize, prewarm_dir: &Path) -> Result<()> { } Decision::Reuse { reason } => { println!("pre-warm: reusing the shared {channel} runtime ({reason})"); - return Ok(()); } } + // Unconditional, and deliberately BEFORE the reuse early return below. The + // runtime and the serving engine are installed separately: `install sdk` + // lays down the ROCm runtime, `engines install` builds the engine venv + // against it. `decide` only ever reasons about the runtime, so a tree whose + // runtime is current but whose engine was never installed — or was left + // behind with an older runtime — resolves to `Reuse`, which used to return + // here having done nothing. The shared tree then served every GPU scenario a + // runtime with no engine, which is the one thing those lanes exist to + // exercise. Re-checking a warm tree is cheap: without `--reinstall`, + // `engines install` on a ready engine installs nothing. + ensure_default_engine(&rocm, prewarm_dir)?; + + if !runtime_changed(&decision) { + return Ok(()); + } + // An install/update that exits 0 without leaving a registry behind is the // confusing case the lanes used to call out by hand: every scenario then falls // back to installing its own runtime and the job quietly blows its time cap. @@ -394,6 +409,59 @@ pub fn run(channel: &str, keep: usize, prewarm_dir: &Path) -> Result<()> { Ok(()) } +/// Whether `decision` put a new runtime in the tree, and so whether the registry +/// check and the retention prune at the end of [`run`] have anything to do. +/// +/// Read AFTER the engine check, never inside the decision's own match arm: the +/// engine is installed separately from the runtime, so every decision — reuse +/// most of all, since it is the one a warm runner takes every time — has to +/// reach that check before this can end the pre-warm early. +const fn runtime_changed(decision: &Decision) -> bool { + !matches!(decision, Decision::Reuse { .. }) +} + +/// Install the engine the active runtime would serve on, so the shared tree has +/// one before a scenario asks it to serve. +/// +/// Which engine that is comes from the CLI rather than from a constant here: +/// `rocm engines list` marks the engine `serve` picks for the detected GPU with +/// `* ` (vLLM on Instinct, Lemonade on Strix), and the pre-warm must agree with +/// `serve` on every runner without this file learning the hardware map. +/// +/// Fatal on failure, like [`repair_poisoned_runtimes`] and unlike the freshness +/// path: reusing a stale-but-working runtime keeps a lane meaningful, whereas +/// serving with no engine fails every GPU scenario later and for reasons that +/// name none of this. +fn ensure_default_engine(rocm: &Path, prewarm_dir: &Path) -> Result<()> { + let output = rocm_command(rocm, prewarm_dir) + .args(["engines", "list"]) + .output() + .context("failed to run `rocm engines list`")?; + if !output.status.success() { + bail!("`rocm engines list` exited with {}", output.status); + } + let inventory = String::from_utf8_lossy(&output.stdout); + let engine = default_engine_from_inventory(&inventory) + .context("`rocm engines list` did not identify a default engine")?; + println!("pre-warm: ensuring the {engine} engine is installed"); + rocm_command(rocm, prewarm_dir) + .args(["engines", "install", engine, "--yes"]) + .status_ok("rocm engines install") +} + +/// The engine `rocm engines list` marks as the default for this host, if any. +/// +/// The inventory renders one line per engine as `{marker} {name:10} {note}` with +/// the marker in column 0, then indents that engine's detail lines (` adapter: +/// …`, ` runtime: …`) beneath it. Matching `* ` at the start of the line +/// unindented is therefore what separates the default engine's own line from +/// everything else the report prints. +fn default_engine_from_inventory(inventory: &str) -> Option<&str> { + inventory + .lines() + .find_map(|line| line.strip_prefix("* ")?.split_whitespace().next()) +} + /// Drop any managed runtime in the shared tree that records an install root /// outside it, so the pre-warm reinstalls instead of serving a dead one. /// @@ -731,6 +799,73 @@ mode=managed status=ready\n install_root: /tmp/rocm-e2e-XXXX/data/runtimes/ assert_eq!(poisoned[0].format, "tarball"); } + /// A real `rocm engines list` on an Instinct host, captured verbatim: the + /// default engine's line carries the `* ` marker in column 0, and its own + /// detail lines are indented beneath it. + const ENGINES_READY: &str = "\ +Local model engines + Built-in engines are included with rocm-cli. External plugins are optional. + ROCm GPU execution is required. + Plugin folders: + 1. /w/e2e-prewarm/data/engines/plugins (primary) + lemonade default embedded Lemonade server with ROCm llama.cpp backend + adapter: built-in + runtime: not found +* vllm Linux/WSL ROCm GPU serving engine through external vLLM + adapter: built-in + runtime: /w/e2e-prewarm/data/runtimes/wheel/release-wheel-gfx94x-dcgpu-7-15-0 + protocol: 0.1.0 +"; + + #[test] + fn the_marked_engine_is_the_one_pre_warmed() { + // Which engine to install comes from the CLI's own host detection, not + // from a hardware map duplicated here. + assert_eq!(default_engine_from_inventory(ENGINES_READY), Some("vllm")); + } + + #[test] + fn an_indented_detail_line_is_not_read_as_the_default() { + // Every engine's detail lines are indented under it, and a note may well + // start with a bullet. Only the marker in column 0 names the default. + let inventory = "\ +Local model engines +* lemonade default embedded Lemonade server with ROCm llama.cpp backend + adapter: built-in + * not a marker +"; + assert_eq!(default_engine_from_inventory(inventory), Some("lemonade")); + } + + #[test] + fn an_inventory_without_a_default_engine_names_none() { + // `ensure_default_engine` turns this into an error rather than guessing an + // engine: installing the wrong one costs a multi-GiB build and still + // leaves the lane with nothing to serve on. + assert_eq!(default_engine_from_inventory(""), None); + assert_eq!( + default_engine_from_inventory("Local model engines\n lemonade embedded\n"), + None + ); + } + + #[test] + fn reusing_the_shared_runtime_still_reaches_the_engine_check() { + // The regression this guards: `Reuse` — the decision EVERY warm runner + // takes, run after run — used to end `run` with a `return` inside its own + // match arm, before anything looked at the engine. The early return is now + // this predicate, read only AFTER `ensure_default_engine`, so reuse cannot + // skip the engine. Reuse must still install and update NOTHING, which is + // what keeps it cheap enough to re-check the engine on every run. + assert!(!runtime_changed(&Decision::Reuse { + reason: "up to date".to_owned() + })); + assert!(runtime_changed(&Decision::Install)); + assert!(runtime_changed(&Decision::Update { + runtime_key: "release-wheel-gfx94x-dcgpu-7-13-0".to_owned() + })); + } + #[test] fn no_managed_runtime_installs() { assert_eq!(decide(EMPTY, "release"), Decision::Install); From e57e5e4bed1b0c7e80e4c949f777b407faf222aa Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 08:02:41 +0000 Subject: [PATCH 08/19] test(e2e): assert the settled runtime is not called a violation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The divergence check in this step could not fail. `divergence:` is emitted only by the `expected_divergence` render arm, which prints that verdict one line above it, so requiring the verdict whenever the lines appear asserted that a single arm prints both of its own halves. The property the step was reaching for is the neighbouring one: a torch this tool put here on purpose must never be reported as a defect. `violated` is the only rendering that would say so, and it is now rejected outright. Unconditional, where the old form was guarded on today's versions being different. That guard is no longer needed: should a future engine pin and SDK build agree, there is no divergence to classify — and still no violation to report, so the assertion holds unchanged rather than silently going quiet. Signed-off-by: Tomas Saaristola --- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 1f79d4563..ea88af310 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -288,17 +288,17 @@ async fn assert_torch_alignment_settled(world: &mut E2eWorld) { "torch alignment reported `{unsettled}`:\n{output}" ); } - // Conditional on purpose. A divergence is today's steady state — the engine pins - // an exact build and the SDK supplies a different one of the same release — but - // a future pair could agree, and then there is nothing to classify. Pinning it - // unconditionally would encode today's versions into the scenario. What must - // never happen is the CLI reporting a divergence and calling it a defect. - if output.contains("divergence:") { - assert!( - output.contains("dependency_check: expected_divergence"), - "a divergence was reported without being classified as expected:\n{output}" - ); - } + // Asserted on the verdict, not on the divergence lines. Requiring + // `expected_divergence` whenever `divergence:` appears cannot fail: one render + // arm emits both, the verdict first. The property worth protecting is the other + // one — that a torch this tool put here on purpose is never called a defect — + // and `violated` is the only rendering that would say so. Unconditional because + // it stays true if a future engine pin and SDK build happen to agree: then there + // is no divergence to classify, and still no violation to report. + assert!( + !output.contains("dependency_check: violated"), + "the dependency check called a settled runtime a violation:\n{output}" + ); } /// The opt-out was honoured, and said so in its own words. From 3ee2e4f7a94896473c0d2b0948d838e9b0a4e700 Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 08:02:50 +0000 Subject: [PATCH 09/19] test(install): pin the dated-alpha manifest fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every runtime installed before `sdk_torch` was recorded derives its SDK torch build from the recorded SDK version, and nightly SDKs version as `7.14.0a20260812`. The date belongs to the build — TheRock's torch for that SDK is `+rocm7.14.0a20260812` — but the existing cases covered only a plain release, an absent version, and a blank one, so nothing held the whole string in place. That leaves the obvious tidy-up unguarded: normalising the version to its `7.14.0` release before interpolating looks like a cleanup and instead names a build belonging to a different SDK, so the alignment would install a torch built against libraries the runtime does not have. On nightly runtimes only, which is where it would be found last. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 48d2e162a..47c0d53ba 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -27734,6 +27734,34 @@ ID_LIKE="suse opensuse" ); } + /// A dated alpha carries its date into the build, and must not be trimmed. + /// + /// Nightly SDKs version as `7.14.0a20260812`, and TheRock's torch for one is + /// `+rocm7.14.0a20260812` — the date is part of the build, not decoration on + /// the version. Shortening it to the `7.14.0` release would name a build that + /// exists for a different SDK, so the alignment would install a torch built + /// against libraries the runtime does not have. The fallback is a whole-string + /// interpolation today; this pins that, because the tempting "clean up the + /// version first" refactor is the one that breaks every nightly runtime. + #[test] + fn a_dated_alpha_sdk_keeps_its_date_in_the_derived_build() { + let manifest = test_runtime_manifest_for_update( + "wheel-gfx94x", + "gfx94x", + "gfx94x-dcgpu", + "7.14.0a20260812", + ); + assert!( + manifest.sdk_torch.is_none(), + "this test is about the pre-change manifest shape" + ); + + assert_eq!( + sdk_torch_build_from_manifest(&manifest).as_deref(), + Some("rocm7.14.0a20260812") + ); + } + /// A manifest that names no version at all must not invent a build. #[test] fn a_manifest_with_no_version_identifies_no_build() { From da2ea4cdadc352397a11886953abe37c8c7070da Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 10:29:33 +0000 Subject: [PATCH 10/19] fix(install): settle the torch of the runtime that owns the env `settle_engine_install` derived the SDK torch build and the device-probe library paths from a caller-supplied selector, but installed into `response.python_executable`. Two of its three callers pass a `runtime_id`, which is shared by every side-by-side install of one channel and family, so the selector can name two runtimes at once. `runtime_manifest_for_selector` correctly declines to guess, both lookups come back empty, and the settle runs blind. Where the selector does resolve it can still name the *active* runtime while the engine was installed into an older one, and then the wrong tree's torch is settled. Resolve the runtime from the interpreter instead: an install root contains one runtime by construction, so the environment names itself. The caller's selector remains the fallback for interpreters outside every install root (external and self-managed environments). Paths are compared verbatim and canonicalized, because the CLI writes `install_root` canonicalized while an engine adapter reports back whatever path it was handed; comparing one form only would make ownership fail silently on a symlinked runtimes directory. Roots can nest, so the longest containing root wins. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 148 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 1 deletion(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 47c0d53ba..3dc4ca3de 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -3941,6 +3941,51 @@ fn runtime_manifest_for_selector<'a>( }) } +/// The `runtime_key` of the runtime whose install root contains `python`. +fn runtime_key_for_python(paths: &AppPaths, python: &Path) -> Option { + let manifests = therock::load_runtime_manifests(paths).ok()?; + runtime_key_owning_python(&manifests, python).map(str::to_owned) +} + +/// Which runtime owns an interpreter, decided by install root. +/// +/// Split from the registry read so the decision can be tested without a +/// registry on disk, matching `sdk_torch_build_from_manifest`. +/// +/// `runtime_id` cannot answer this: it is shared by every side-by-side install +/// of one channel and family, which is exactly the situation an engine install +/// has to be attributed in. An install root contains one runtime by +/// construction, so the interpreter's path settles it. +/// +/// Both sides are compared verbatim *and* canonicalized. The CLI writes +/// `install_root` canonicalized while an engine adapter reports back whatever +/// path it was handed, and comparing a single form makes ownership fail +/// silently on a symlinked runtimes directory. Roots can nest, so the longest +/// containing root wins. +fn runtime_key_owning_python<'a>( + manifests: &'a [therock::InstalledRuntimeManifest], + python: &Path, +) -> Option<&'a str> { + fn both_forms(path: &Path) -> Vec { + let verbatim = path.to_path_buf(); + match path.canonicalize() { + Ok(resolved) if resolved != verbatim => vec![verbatim, resolved], + _ => vec![verbatim], + } + } + + let pythons = both_forms(python); + manifests + .iter() + .filter(|manifest| { + both_forms(&manifest.install_root) + .iter() + .any(|root| pythons.iter().any(|python| python.starts_with(root))) + }) + .max_by_key(|manifest| manifest.install_root.as_os_str().len()) + .map(|manifest| manifest.runtime_key.as_str()) +} + fn env_root_for_service( paths: &AppPaths, engine: &str, @@ -8455,13 +8500,23 @@ fn settles_runtime_torch(engine: &str, managed_env: Option) -> bool { fn settle_engine_install( paths: &AppPaths, engine: &str, - runtime_key: &str, + selector: &str, response: &InstallResponse, ) -> Result<()> { if !settles_runtime_torch(engine, response.managed_env) { return Ok(()); } let python = Path::new(&response.python_executable); + // Which runtime is being settled has to be asked of the environment, not of + // the caller. Two of the three callers pass a `runtime_id`, and side-by-side + // installs of one channel and family share it, so the selector can name two + // runtimes at once; `runtime_manifest_for_selector` then correctly declines + // to guess and every lookup below silently comes back empty. It can also + // name the *active* runtime while the engine was installed into an older + // one, which is worse than empty — it settles the wrong tree's torch. + // The interpreter is unambiguous, so let it name its own runtime. + let owned = runtime_key_for_python(paths, python); + let runtime_key = owned.as_deref().unwrap_or(selector); let host_gpu = detect_host_gpu(); let library_paths = runtime_library_paths_for_key(paths, runtime_key); let sdk_build = sdk_torch_build_for_key(paths, runtime_key); @@ -27772,6 +27827,97 @@ ID_LIKE="suse opensuse" assert_eq!(sdk_torch_build_from_manifest(&manifest), None); } + /// Two runtimes installed side by side, as a pre-warmed CI tree holds them. + /// + /// They differ in `runtime_key`, `version` and install root, and share one + /// `runtime_id` — that is what the field means, so this is not a corrupt + /// registry. + fn side_by_side_runtimes() -> Vec { + let mut older = test_runtime_manifest_for_update( + "release-wheel-gfx94x-dcgpu-7-13-0", + "therock-release:gfx94X-dcgpu", + "gfx94X-dcgpu", + "7.13.0", + ); + older.install_root = PathBuf::from("/runtimes/release-wheel-gfx94x-dcgpu-7-13-0"); + let mut newer = test_runtime_manifest_for_update( + "release-wheel-gfx94x-dcgpu-7-14-0", + "therock-release:gfx94X-dcgpu", + "gfx94X-dcgpu", + "7.14.0", + ); + newer.install_root = PathBuf::from("/runtimes/release-wheel-gfx94x-dcgpu-7-14-0"); + vec![older, newer] + } + + /// The interpreter names its runtime where the shared `runtime_id` cannot. + /// + /// This is the cross-wiring that settled the active runtime's torch into an + /// older runtime's environment: the engine's env id drops the version, so + /// the environment belongs to 7.13.0 while the caller's selector says only + /// "release, gfx94X-dcgpu". Resolving by install root has to pick 7.13.0. + #[test] + fn the_runtime_is_resolved_by_its_interpreter_not_the_shared_runtime_id() { + let manifests = side_by_side_runtimes(); + + assert_eq!( + runtime_manifest_for_selector(&manifests, "therock-release:gfx94X-dcgpu") + .map(|manifest| manifest.runtime_key.as_str()), + None, + "the shared runtime_id names two runtimes, so a selector cannot resolve it" + ); + assert_eq!( + runtime_key_owning_python( + &manifests, + Path::new("/runtimes/release-wheel-gfx94x-dcgpu-7-13-0/bin/python3"), + ), + Some("release-wheel-gfx94x-dcgpu-7-13-0"), + "the interpreter's install root names the runtime being settled" + ); + } + + /// An interpreter outside every install root leaves the caller's selector alone. + /// + /// External and self-managed environments live outside the registry, and + /// inventing an owner for them would settle a runtime nobody asked about. + #[test] + fn an_interpreter_outside_every_install_root_owns_nothing() { + assert_eq!( + runtime_key_owning_python( + &side_by_side_runtimes(), + Path::new("/opt/somewhere-else/bin/python3"), + ), + None + ); + } + + /// A prefix match alone is ambiguous once roots nest, so the longest wins. + #[test] + fn the_longest_containing_install_root_owns_the_interpreter() { + let mut outer = test_runtime_manifest_for_update( + "outer", + "therock-release:gfx94X-dcgpu", + "gfx94X-dcgpu", + "7.13.0", + ); + outer.install_root = PathBuf::from("/runtimes"); + let mut inner = test_runtime_manifest_for_update( + "inner", + "therock-release:gfx94X-dcgpu", + "gfx94X-dcgpu", + "7.14.0", + ); + inner.install_root = PathBuf::from("/runtimes/release-wheel-gfx94x-dcgpu-7-14-0"); + + assert_eq!( + runtime_key_owning_python( + &[outer, inner], + Path::new("/runtimes/release-wheel-gfx94x-dcgpu-7-14-0/bin/python3"), + ), + Some("inner") + ); + } + /// A torch the user installed themselves is kept, and named as kept. /// /// The Python does not exist and an index is supplied, so an alignment that From 336709ca7225a4199b8107baddb725eb57f6332d Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 13:30:37 +0000 Subject: [PATCH 11/19] refactor(core): share one torch-alignment opt-out with the engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ROCM_CLI_DISABLE_TORCH_ALIGNMENT` was read in two places — the CLI that owns the alignment and the vLLM engine that repairs what it finds — with the variable name and the presence-is-the-signal rule spelled out separately in each. Nothing held the two together, and the failure mode of drift is not a warning: a runtime the CLI deliberately left alone would be rewritten by the engine on the very next `rocm engines install vllm`, which is the fight the opt-out exists to end. The engine cannot call into the CLI binary, so the read moves to `rocm-core`, which both already depend on, and both sides now make the same call rather than matching implementations. The reasoning that has to stay true for either reader moves with it; the callers keep only what is local to them. No behaviour change: same variable, same presence test. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 11 ++++++----- crates/rocm-core/src/lib.rs | 22 ++++++++++++++++++++++ engines/vllm/src/lib.rs | 11 ++++------- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 3dc4ca3de..735861bf6 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -7694,12 +7694,13 @@ fn install_error_reports_version_unavailable(error: &str) -> bool { /// build does not work on this machine" is a case that can happen rather than a /// hypothetical one, and it needs an exit that is not "stop using the CLI". /// -/// This suppresses the correction, not the diagnosis. The runtime is still asked -/// what it can do, the dependency check still runs, and a runtime that opens no -/// device or cannot run a kernel on one is still reported as such — and still -/// fails the install on a host where a GPU was found. +/// The engine reads the same variable through the same helper, so the two sides +/// cannot drift; [`rocm_core::torch_alignment_disabled`] carries why that matters. +/// Suppressing the correction does not suppress the diagnosis: a runtime that +/// opens no device or cannot run a kernel on one is still reported as such — and +/// still fails the install on a host where a GPU was found. fn torch_alignment_disabled() -> bool { - std::env::var_os("ROCM_CLI_DISABLE_TORCH_ALIGNMENT").is_some() + rocm_core::torch_alignment_disabled() } /// Install the SDK's build of the release the engine pins, when that is needed. diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index e6b495578..8471f98d0 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -78,6 +78,28 @@ pub use uv::{ pub const DEFAULT_LOCAL_PORT: u16 = 11_435; pub const DEFAULT_LOCAL_HOST: &str = "127.0.0.1"; + +/// The variable that opts a machine out of rocm-cli choosing its runtime's torch. +pub const TORCH_ALIGNMENT_DISABLED_ENV: &str = "ROCM_CLI_DISABLE_TORCH_ALIGNMENT"; + +/// Whether the user has opted out of rocm-cli choosing this runtime's torch. +/// +/// Presence is the signal, so any value — including the empty string — disables the +/// alignment; that keeps `ROCM_CLI_DISABLE_TORCH_ALIGNMENT=` from reading as "off" +/// to one side and "on" to the other. +/// +/// The CLI and the vLLM engine both consult this: the engine cannot call into the +/// binary that owns the alignment, and a duplicated read is a contract that drifts. +/// If the two ever disagreed, a runtime the CLI deliberately left alone would be +/// rewritten by the engine on the very next `rocm engines install vllm` — the fight +/// the opt-out exists to end. +/// +/// This suppresses the correction, not the diagnosis. The runtime is still asked +/// what it can do, the dependency check still runs, and a runtime that opens no +/// device or cannot run a kernel on one is still reported as such. +pub fn torch_alignment_disabled() -> bool { + std::env::var_os(TORCH_ALIGNMENT_DISABLED_ENV).is_some() +} const OPTIONAL_COMMAND_TIMEOUT: Duration = Duration::from_millis(1_500); const WINDOWS_INVENTORY_QUERY_TIMEOUT: Duration = Duration::from_secs(5); const WINDOWS_VIDEO_CONTROLLER_INVENTORY_SCRIPT: &str = r#"$gpus = Get-CimInstance -ClassName Win32_VideoController -Property Name,DriverVersion,PNPDeviceID,AdapterCompatibility | Where-Object { $_.PNPDeviceID -match 'VEN_1002' -or $_.AdapterCompatibility -match 'AMD|Advanced Micro Devices' -or $_.Name -match 'AMD|Radeon|Instinct' }; foreach ($gpu in $gpus) { "GPU`t$($gpu.Name)`t$($gpu.DriverVersion)`t$($gpu.PNPDeviceID)" }"#; diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index 27c16b051..a910ba3ea 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -564,14 +564,11 @@ const TORCH_PACKAGE: &str = "torch"; /// Whether the user has opted out of rocm-cli choosing this runtime's torch. /// -/// The same variable, read the same way, as the CLI's own opt-out: presence is the -/// signal, so any value — including the empty string — disables the alignment. The -/// engine cannot call into the CLI binary that owns the alignment itself, so the -/// contract is duplicated rather than shared; the two must not drift, or a runtime -/// the CLI deliberately left alone gets rewritten by the engine on the very next -/// `rocm engines install vllm`, which is the fight the opt-out exists to end. +/// The CLI's own opt-out is the same call, not a matching one: the engine cannot +/// call into the binary that owns the alignment, and a duplicated read is a +/// contract that drifts. [`rocm_core::torch_alignment_disabled`] carries the rest. fn torch_alignment_disabled() -> bool { - std::env::var_os("ROCM_CLI_DISABLE_TORCH_ALIGNMENT").is_some() + rocm_core::torch_alignment_disabled() } /// Whether this violation is the torch divergence rocm-cli deliberately leaves behind. From c21179204a76dd1c0deb6a2d53e7c33cf91dd49f Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 13:30:44 +0000 Subject: [PATCH 12/19] fix(vllm): keep the SDK-stack hint under the torch-alignment opt-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trailing hint — that a recurring defect after `rocm install sdk` means the SDK torch stack is being written over vLLM's pins — was suppressed whenever the opt-out was set, on the reasoning that torch is never a defect there so the hint would name the one package that cannot be the cause. That conflated the package `torch` with the SDK torch stack. `is_intended_torch_divergence` spares only the package literally named `torch`, so a `torchvision` or `torchaudio` defect still reaches this point under the opt-out — and those are the same SDK stack written over the same pins. The opt-out has switched off the step that would have corrected them, which makes the hint more use there, not less. The remaining gate is the one that was load-bearing all along: once the manifest names the SDK's build, the alignment identifies that stack and settles it, so a defect surviving to here is something else and the hint would misdirect. Signed-off-by: Tomas Saaristola --- engines/vllm/src/lib.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index a910ba3ea..b7f3ac138 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -703,10 +703,13 @@ fn repair_from_violations( .map(|violation| format!("expected divergence: {}", violation.detail)), ); // The hint blames `rocm install sdk` for writing the SDK torch stack over vLLM's - // pins, which is only a live theory while the alignment runs. Under the opt-out - // torch is never a defect here, so the hint would be pointing at the one package - // that cannot be the cause. - if sdk_torch_build.is_none() && !torch_alignment_disabled { + // pins, which stops being a live theory once the manifest names the SDK's build: + // the alignment then identifies that stack and settles it, so a defect surviving + // to here is something else. It stays on under the opt-out, which spares only the + // package named `torch` — a `torchvision` or `torchaudio` defect is still the SDK + // stack written over vLLM's pins, and the opt-out has turned off the step that + // would have corrected it, so the hint is more use there rather than less. + if sdk_torch_build.is_none() { notes.push( "if this recurs after `rocm install sdk`, the SDK torch stack is being written over vLLM's pinned torch".to_owned(), ); From 7b6f99f90514259a7e8823fbc6da96d6b13ba68c Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 13:31:20 +0000 Subject: [PATCH 13/19] fix(install): judge each dependency violation on its own subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `classify_dependency_details` demoted the whole run to `Violated` unless *every* line named the realigned package. One unrelated violation — say numpy — took the deliberate torch divergence down with it, and the block then advised `rocm engines install --reinstall`: a remedy that repairs numpy and reinstates the engine's own build of torch, leaving a runtime that cannot open a device. Trading one defect for a worse one, on the strength of a defect that had nothing to do with the alignment. Each line is now judged alone, and `Violated` carries both sets. The subject comes from `rocm_core::violation_subject` rather than a local ```{package}==``` substring test — one parser for the shape `uv pip check` emits, so the CLI and the engine cannot disagree about what a line is about. A line whose subject does not parse stays a violation: an unrecognised shape is not evidence that a divergence was intended. Where both sets are populated the reinstall is no longer named at all. The block prints the violations, prints the divergences under their own `divergence:` key, and says to repair the former without reinstalling. With no divergence in play the old advice is unchanged. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 131 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 108 insertions(+), 23 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 735861bf6..5b29d214a 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -7485,7 +7485,16 @@ enum EngineDependencyCheck { Satisfied, /// The engine declares requirements the environment does not meet, one line each, /// as the resolver reported them. - Violated(Vec), + /// + /// `expected` carries any divergence the install made on purpose that shares the + /// run with a genuine violation. It is reported separately rather than folded in + /// because the remedy differs: reinstalling the engine repairs `violations` and + /// destroys `expected`, so naming that remedy is only safe while `expected` is + /// empty. + Violated { + violations: Vec, + expected: Vec, + }, /// The only unmet requirements are ones the install deliberately diverged from. /// /// Torch alignment leaves the runtime holding the SDK's build of the release @@ -7517,11 +7526,19 @@ fn report_engine_dependency_check( "info", format!("engine={engine} runtime_id={runtime_key} dependency_check=satisfied"), ), - EngineDependencyCheck::Violated(details) => ( + EngineDependencyCheck::Violated { + violations, + expected, + } => ( "error", format!( - "engine={engine} runtime_id={runtime_key} dependency_check=violated: {}", - details.join("; ") + "engine={engine} runtime_id={runtime_key} dependency_check=violated: {}{}", + violations.join("; "), + if expected.is_empty() { + String::new() + } else { + format!(" (expected_divergence: {})", expected.join("; ")) + } ), ), EngineDependencyCheck::ExpectedDivergence(details) => ( @@ -8601,8 +8618,11 @@ fn engine_dependency_check( /// Separate a deliberate divergence from a genuine violation. /// -/// Only a divergence about the realigned package is expected; anything else in -/// the same run is still a violation and must keep saying so. +/// Each line is judged on its own subject: a genuine violation elsewhere in the +/// same run says nothing about the package the install deliberately diverged on, +/// and must not drag it along. Lumping the two together loses the distinction +/// exactly where it matters most — the remedy for the genuine violation would +/// reinstall the engine and undo the alignment. fn classify_dependency_details( details: Vec, realigned_package: Option<&str>, @@ -8611,15 +8631,25 @@ fn classify_dependency_details( return EngineDependencyCheck::Satisfied; } let Some(package) = realigned_package else { - return EngineDependencyCheck::Violated(details); + return EngineDependencyCheck::Violated { + violations: details, + expected: Vec::new(), + }; }; - // `uv pip check` phrases the requirement as ``requires `torch==…` ``, so the - // package name followed by a specifier is what identifies the subject. - let marker = format!("`{package}=="); - if details.iter().all(|detail| detail.contains(&marker)) { - EngineDependencyCheck::ExpectedDivergence(details) + // A line whose subject cannot be parsed is not evidence of a divergence, so it + // stays a violation: the conservative side keeps saying something is wrong. + let (expected, violations): (Vec, Vec) = + details.into_iter().partition(|detail| { + rocm_core::violation_subject(detail) + .is_some_and(|subject| subject.package.eq_ignore_ascii_case(package)) + }); + if violations.is_empty() { + EngineDependencyCheck::ExpectedDivergence(expected) } else { - EngineDependencyCheck::Violated(details) + EngineDependencyCheck::Violated { + violations, + expected, + } } } @@ -8636,15 +8666,32 @@ fn render_engine_dependency_check(engine: &str, outcome: &EngineDependencyCheck) sanitize_log_value(reason) ); } - EngineDependencyCheck::Violated(details) => { + EngineDependencyCheck::Violated { + violations, + expected, + } => { let _ = writeln!(output, " dependency_check: violated"); - for detail in details { + for detail in violations { let _ = writeln!(output, " violation: {}", sanitize_log_value(detail)); } - let _ = writeln!( - output, - " action: rocm engines install {engine} --reinstall" - ); + for detail in expected { + let _ = writeln!(output, " divergence: {}", sanitize_log_value(detail)); + } + if expected.is_empty() { + let _ = writeln!( + output, + " action: rocm engines install {engine} --reinstall" + ); + } else { + // The reinstall would repair the violations above and reinstate the + // engine's own build of the diverged package, restoring a runtime + // that cannot open a device. Naming a remedy that trades one defect + // for a worse one is not worth the convenience. + let _ = writeln!( + output, + " action: repair the violations above without reinstalling {engine}; a reinstall would undo the divergence kept on purpose (see torch_alignment above)" + ); + } } EngineDependencyCheck::ExpectedDivergence(details) => { let _ = writeln!(output, " dependency_check: expected_divergence"); @@ -27098,9 +27145,12 @@ ID_LIKE="suse opensuse" // succeeded, so the only signal the user gets is this block. let rendered = render_engine_dependency_check( "vllm", - &EngineDependencyCheck::Violated(vec![ - "The package `vllm` requires `torch==2.10.0+git8514f05`, but `2.9.1+rocm7.14.0a20260611` is installed".to_owned(), - ]), + &EngineDependencyCheck::Violated { + violations: vec![ + "The package `vllm` requires `torch==2.10.0+git8514f05`, but `2.9.1+rocm7.14.0a20260611` is installed".to_owned(), + ], + expected: Vec::new(), + }, ); assert!(rendered.contains(" dependency_check: violated\n")); @@ -27135,6 +27185,10 @@ ID_LIKE="suse opensuse" #[test] fn a_divergence_on_any_other_package_is_still_a_violation() { + // The unrelated violation is real and must keep saying so, but it does not + // make the deliberate torch divergence one too — and the reinstall that + // would repair numpy is exactly what must not be advised while the + // alignment stands. let outcome = classify_dependency_details( vec![ "The package `vllm` requires `torch==2.11.0+gitd0c8b1f`, but `2.11.0+rocm7.13.0` is installed".to_owned(), @@ -27143,7 +27197,38 @@ ID_LIKE="suse opensuse" Some("torch"), ); - assert!(matches!(outcome, EngineDependencyCheck::Violated(_))); + let EngineDependencyCheck::Violated { + violations, + expected, + } = &outcome + else { + panic!("an unrelated violation is still a violation: {outcome:?}"); + }; + assert_eq!(violations.len(), 1, "only numpy violates: {violations:?}"); + assert!(violations[0].contains("numpy")); + assert_eq!(expected.len(), 1, "torch diverged on purpose: {expected:?}"); + assert!(expected[0].contains("torch")); + + let rendered = render_engine_dependency_check("vllm", &outcome); + assert!(rendered.contains(" dependency_check: violated\n")); + assert!(rendered.contains(" violation: The package `vllm` requires `numpy")); + assert!(rendered.contains(" divergence: The package `vllm` requires `torch")); + assert!( + !rendered.contains("action: rocm engines install vllm --reinstall"), + "the reinstall would undo the alignment while repairing numpy: {rendered}" + ); + } + + #[test] + fn a_line_whose_subject_cannot_be_parsed_stays_a_violation() { + // Conservative by construction: an unrecognised shape is not evidence that + // the divergence was intended, so it must not be quietly excused. + let outcome = classify_dependency_details( + vec!["something uv said that this parser does not recognise".to_owned()], + Some("torch"), + ); + + assert!(matches!(outcome, EngineDependencyCheck::Violated { .. })); } /// The engine's build enumerates no devices against the installed SDK. From 19f2da0cb0455fe2a3d0fffa4a14c43539268d04 Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 13:31:39 +0000 Subject: [PATCH 14/19] feat(install): report what the runtime could do before a realignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `device_check:` block prints the runtime as it is *after* the install settles its torch. On a realignment that is the only state reported, so an install that ends in a runtime which cannot open a device does not say whether the alignment broke something that worked or found something already broken. Those need opposite responses, and telling them apart afterwards means going to the machine — by which time the torch that produced the earlier verdict is gone. The probe that answered the question is already in hand at the call site, so `torch_alignment: realigned` now carries it, in the printed block and in the audit line both. The log matters as much as the console here: it is read long after the install, when the runtime on disk can no longer be asked what it was like beforehand. Only the realigned arm prints it. Every other outcome left the runtime alone, so the device check below already concerns the same torch this block names, and a before/after pair there would invite reading a change into an outcome that made none. The quoted form is `device_check_verdict`, not the full block: it keeps the verdict name and the torch it judged, and drops the explanation and the suggested consequence. Those are right where the verdict is the answer and wrong where it is context for something else — two full blocks in a row read as two competing diagnoses rather than as one pair. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 120 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 116 insertions(+), 4 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 5b29d214a..f2ebd807f 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -7800,7 +7800,20 @@ fn align_runtime_torch( /// reads `... the release vllm pins` rather than an anonymous "the engine". It /// arrives from the engine selection and is sanitized like every other /// interpolated value. -fn render_torch_alignment(outcome: &TorchAlignment, engine: &str) -> String { +/// +/// `before` is the verdict the runtime gave *prior* to the realignment. Only the +/// realigned outcome replaced anything, so only that arm prints it — and it must, +/// because the `device_check:` block further down reports the runtime as it is +/// now. Without the before verdict beside it, a realignment that ends in a +/// failing runtime does not say whether the alignment broke something that +/// worked or found something already broken, and answering that afterwards means +/// going to the machine to look. The value is already in hand at the call site; +/// dropping it only moves the cost onto whoever reads the output. +fn render_torch_alignment( + outcome: &TorchAlignment, + engine: &str, + before: Option<&RuntimeDeviceCheck>, +) -> String { let mut output = String::new(); match outcome { TorchAlignment::AlreadyAligned { version } => { @@ -7819,6 +7832,13 @@ fn render_torch_alignment(outcome: &TorchAlignment, engine: &str) -> String { sanitize_log_value(to), sanitize_log_value(engine) ); + if let Some(before) = before { + let _ = writeln!( + output, + " before this replacement: {} (device_check below is after it)", + sanitize_log_value(&device_check_verdict(before)) + ); + } } TorchAlignment::Unavailable { wanted, kept } => { let _ = writeln!(output, " torch_alignment: unavailable"); @@ -7876,6 +7896,7 @@ fn report_torch_alignment( runtime_key: &str, sdk_build: Option<&str>, torch: Result<&therock::TorchAlignmentProbe, &anyhow::Error>, + before: &RuntimeDeviceCheck, ) -> TorchAlignment { let index_url = runtime_index_url_for_key(paths, runtime_key); let outcome = align_runtime_torch( @@ -7886,7 +7907,7 @@ fn report_torch_alignment( engine, torch, ); - print!("{}", render_torch_alignment(&outcome, engine)); + print!("{}", render_torch_alignment(&outcome, engine, Some(before))); let (level, message) = match &outcome { TorchAlignment::AlreadyAligned { version } => ( "info", @@ -7894,10 +7915,14 @@ fn report_torch_alignment( "engine={engine} runtime_id={runtime_key} torch_alignment=already_aligned version={version}" ), ), + // The before verdict rides along in the audit line too: the log is read + // long after the install, when the runtime on disk can no longer answer + // what it was like beforehand. TorchAlignment::Realigned { from, to } => ( "info", format!( - "engine={engine} runtime_id={runtime_key} torch_alignment=realigned from={from} to={to}" + "engine={engine} runtime_id={runtime_key} torch_alignment=realigned from={from} to={to} before={}", + device_check_verdict(before) ), ), TorchAlignment::Unavailable { wanted, kept } => ( @@ -8060,6 +8085,29 @@ fn classify_runtime_device_probe(probe: therock::RuntimeDeviceProbe) -> RuntimeD } } +/// One device-check verdict on one line, for quoting inside another block. +/// +/// The full block explains the verdict and names a consequence, which is right +/// where it is the answer and wrong where it is context for something else. This +/// keeps the part that identifies the verdict — the name and the torch it was +/// asked about — so a before/after pair reads as a pair rather than as two +/// competing diagnoses. +fn device_check_verdict(outcome: &RuntimeDeviceCheck) -> String { + match outcome { + RuntimeDeviceCheck::Usable { + device_count, + torch_version, + } => format!("usable ({device_count} device(s), torch {torch_version})"), + RuntimeDeviceCheck::NoDevices { torch_version, .. } => { + format!("no_devices (torch {torch_version})") + } + RuntimeDeviceCheck::KernelFailed { torch_version, .. } => { + format!("kernel_failed (torch {torch_version})") + } + RuntimeDeviceCheck::NotVerified(reason) => format!("not_verified ({reason})"), + } +} + fn render_runtime_device_check(outcome: &RuntimeDeviceCheck) -> String { let mut output = String::new(); match outcome { @@ -8558,6 +8606,7 @@ fn settle_engine_install( runtime_key, sdk_build.as_deref(), torch.as_ref(), + &probed, ); // Only a realignment replaced torch. After every other outcome the // environment is the one already probed, and asking it again would @@ -27706,6 +27755,7 @@ ID_LIKE="suse opensuse" kept: "2.10.0+git8514f05".to_owned(), }, "vllm", + None, ); assert!(rendered.contains(" torch_alignment: unavailable\n")); @@ -27721,6 +27771,7 @@ ID_LIKE="suse opensuse" error: "failed to launch uv: Permission denied".to_owned(), }, "vllm", + None, ); assert!(rendered.contains(" torch_alignment: install_failed\n")); @@ -27741,6 +27792,7 @@ ID_LIKE="suse opensuse" to: "2.11.0+rocm7.13.0".to_owned(), }, "vllm", + None, ); assert!(rendered.contains(" torch_alignment: realigned\n")); @@ -27751,6 +27803,65 @@ ID_LIKE="suse opensuse" ); } + #[test] + fn a_realignment_reports_what_the_runtime_could_do_before_it() { + // The whole point of the line: the device_check printed afterwards is the + // after state, so without this one a realignment that ends badly does not + // say whether it broke a working runtime or repaired a broken one. Here it + // repaired one, and the output says so without anyone visiting the machine. + let rendered = render_torch_alignment( + &TorchAlignment::Realigned { + from: "2.11.0+rocm7.14.0".to_owned(), + to: "2.11.0+rocm7.13.0".to_owned(), + }, + "vllm", + Some(&RuntimeDeviceCheck::KernelFailed { + torch_version: "2.11.0+rocm7.14.0".to_owned(), + error: "HIP error: hipErrorInvalidImage".to_owned(), + }), + ); + + assert!( + rendered.contains("before this replacement: kernel_failed (torch 2.11.0+rocm7.14.0)"), + "the before verdict names itself and the torch it judged: {rendered}" + ); + assert!( + rendered.contains("device_check below is after it"), + "the reader is told which of the two blocks is which: {rendered}" + ); + } + + #[test] + fn only_a_realignment_reports_a_before_verdict() { + // Every other outcome left the runtime as it was, so the device_check + // below is already about the same torch this block names. A before/after + // pair there would invite reading a change into an outcome that made none. + let before = RuntimeDeviceCheck::Usable { + device_count: 1, + torch_version: "2.11.0+rocm7.13.0".to_owned(), + }; + + for outcome in [ + TorchAlignment::AlreadyAligned { + version: "2.11.0+rocm7.13.0".to_owned(), + }, + TorchAlignment::Disabled { + wanted: "2.11.0+rocm7.13.0".to_owned(), + kept: "2.9.0+cpu".to_owned(), + }, + TorchAlignment::Unavailable { + wanted: "2.11.0+rocm7.13.0".to_owned(), + kept: "2.11.0+gitd0c8b1f".to_owned(), + }, + ] { + let rendered = render_torch_alignment(&outcome, "vllm", Some(&before)); + assert!( + !rendered.contains("before this replacement"), + "{outcome:?} replaced nothing: {rendered}" + ); + } + } + #[test] fn an_already_aligned_runtime_reports_the_version_it_kept() { // Every refresh after the first lands here, so this is the most frequently @@ -27760,6 +27871,7 @@ ID_LIKE="suse opensuse" version: "2.11.0+rocm7.13.0".to_owned(), }, "vllm", + None, ); assert!(rendered.contains(" torch_alignment: already_aligned (2.11.0+rocm7.13.0)\n")); @@ -28116,7 +28228,7 @@ ID_LIKE="suse opensuse" }; assert_eq!(deliberately_diverged_package(&outcome), Some("torch")); - let rendered = render_torch_alignment(&outcome, "vllm"); + let rendered = render_torch_alignment(&outcome, "vllm", None); assert!( rendered.contains(" torch_alignment: disabled\n"), "the block has to name the state, got {rendered:?}" From 4e38b21b461f794f586d56e3bfe0b5d5a4fcb96c Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 13:31:49 +0000 Subject: [PATCH 15/19] perf(install): probe the host only when the runtime cannot serve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `settle_engine_install` ran `detect_host_gpu` unconditionally while gathering facts, but both readers of the result additionally required `runtime_cannot_serve`. On a healthy install — the overwhelmingly common case — the whole host scan was paid for an answer nothing went on to read. The probe moves behind an early return on that same predicate, so it now runs only where its answer decides something: whether a runtime that cannot serve is this install's failure, or a host with no GPU to serve with. Behaviour is unchanged. The early return sits after both reports are printed, so a healthy install still emits the same dependency-check and device-check output; only the two host-conditional branches are skipped, and neither could have fired in that state anyway. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index f2ebd807f..e7bbd26f7 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -8583,7 +8583,6 @@ fn settle_engine_install( // The interpreter is unambiguous, so let it name its own runtime. let owned = runtime_key_for_python(paths, python); let runtime_key = owned.as_deref().unwrap_or(selector); - let host_gpu = detect_host_gpu(); let library_paths = runtime_library_paths_for_key(paths, runtime_key); let sdk_build = sdk_torch_build_for_key(paths, runtime_key); let torch = therock::probe_torch_alignment(python, engine); @@ -8627,12 +8626,19 @@ fn settle_engine_install( report_engine_dependency_check(paths, engine, Some(python), runtime_key, diverged); let devices = report_runtime_device_check(paths, engine, runtime_key, devices); + // Only a runtime that cannot serve raises the question the host answers, so + // the host is inspected only then. Both readers below already require this, + // so asking earlier would spend a full host scan on every healthy install to + // reach an answer nothing goes on to read. + if !runtime_cannot_serve(&devices) { + return Ok(()); + } + // A runtime that cannot serve is only this install's failure where the host // has a GPU to serve with. Where the host could not be examined, say so // rather than failing a multi-gigabyte install on a guess. - if let HostGpu::NotVerified(reason) = &host_gpu - && runtime_cannot_serve(&devices) - { + let host_gpu = detect_host_gpu(); + if let HostGpu::NotVerified(reason) = &host_gpu { report_unverified_host_gpu(paths, engine, runtime_key, reason); } if install_left_runtime_unusable(&devices, &host_gpu) { From c0c13679bebdf5042f5c36fb0eae08b5abbd0282 Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 13:34:53 +0000 Subject: [PATCH 16/19] test(install): make the release-correction case actually test the release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `the_sdks_release_is_corrected_to_the_one_the_engine_pins` passed the same release on both sides, so the only difference the plan could act on was the build — which is what the test directly above it already covers. It asserted the right outcome for the wrong reason and would have kept passing had the release half of the rule been dropped. The arguments now disagree on the release and agree on the build, the inverse of the neighbouring case: installed `2.11.0+rocm7.14.0a20260611` against a pin of `2.10.0+git8514f05`. The expectation follows the rule — the engine's release, the SDK's build — so a regression that took the release from the installed torch instead of the pin now fails here. Signed-off-by: Tomas Saaristola --- apps/rocm/src/main.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index e7bbd26f7..91fc32220 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -27317,9 +27317,13 @@ ID_LIKE="suse opensuse" /// engine's build nor the SDK's release selection wins outright. #[test] fn the_sdks_release_is_corrected_to_the_one_the_engine_pins() { + // The installed torch already carries the SDK's build, so the build is not + // what is wrong here — the release is. Pinning it needs the two sides to + // disagree on the release and agree on the build, which is the opposite of + // the case above; identical arguments to it would only restate that one. let plan = plan_torch_alignment( Some("rocm7.14.0a20260611"), - Some("2.10.0+git8514f05"), + Some("2.11.0+rocm7.14.0a20260611"), Some("torch==2.10.0+git8514f05"), "vllm", ); @@ -27328,8 +27332,9 @@ ID_LIKE="suse opensuse" plan, TorchAlignmentPlan::Install { wanted: "2.10.0+rocm7.14.0a20260611".to_owned(), - from: "2.10.0+git8514f05".to_owned(), - } + from: "2.11.0+rocm7.14.0a20260611".to_owned(), + }, + "the engine's release must win while the SDK's build is kept" ); } From 83586fb0a2cfb1faf51ef3b3587f5c71c29ed783 Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 13:34:53 +0000 Subject: [PATCH 17/19] fix(e2e): repair a dangling active-runtime pointer in the pre-warm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pre-warm whose cached tree names a runtime that no longer exists on disk died nine seconds in, before any test ran: the engine install reads the active runtime key, resolves it to a directory, and finds nothing there. The cache is shared across runs, so one interrupted or hand-edited tree kept failing every subsequent job on that runner until someone repaired it by hand. The pre-warm now checks the pointer against the runtimes actually present and, where it dangles, repoints it at an installed runtime rather than failing. Both pointers are rewritten — the `active.json` marker and `config.json`'s `active_runtime_key` — because they are read by different callers and repairing only one leaves the engine install still following the dead key. A pointer that already resolves is left alone, and a tree with no runtimes at all is left to fail as before: there is nothing to repoint it at, and inventing one would hide an install that produced nothing. Signed-off-by: Tomas Saaristola --- xtask/src/e2e_prewarm.rs | 226 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 226 insertions(+) diff --git a/xtask/src/e2e_prewarm.rs b/xtask/src/e2e_prewarm.rs index db3aabf98..ebcb130e0 100644 --- a/xtask/src/e2e_prewarm.rs +++ b/xtask/src/e2e_prewarm.rs @@ -269,6 +269,69 @@ impl RuntimeEntry { } } +/// The active-runtime selector the config still records after the runtime it +/// names has gone, if the report says so. +/// +/// This is the OTHER way the shared tree goes stale, and it is the one that kills +/// the lane outright. [`assess`] finds a runtime the registry has but the tree +/// does not; this finds a pointer the config has but the registry does not. The +/// two are independent: removing a runtime through +/// `rocm runtimes uninstall` clears the config pointers with it, so a dangling +/// pointer means something removed the tree WITHOUT the CLI — a hand cleanup on +/// the runner, or a scenario that deleted a folder it had symlinked in. +/// +/// `rocm engines install` resolves the runtime to build against as +/// `--runtime` → `config.active_runtime_key` → `config.default_runtime_id`, so a +/// dangling pointer in either field fails that resolution before anything else +/// runs: +/// +/// ```text +/// pre-warm: ensuring the vllm engine is installed +/// Error: runtime selector `release-wheel-gfx94x-dcgpu-7-14-0` from engine install +/// runtime selection is not an exact usable runtime: installed runtime not found +/// ``` +/// +/// Nine seconds in, before a single scenario, and the message names neither the +/// pre-warm nor the tree it is about. Detecting it is free because +/// `render_runtimes_text` already reports it, and reporting it is all the CLI +/// does — there is no verb that clears a pointer, so the repair has to be +/// activating something that exists. +/// +/// Only `config.json` is read this way. `engines..last_installed_runtime_id` +/// can dangle too, but nothing resolves through it — it is displayed, not +/// followed — so healing it would be motion without a failure behind it. +#[must_use] +pub fn dangling_active_runtime(runtimes_list_report: &str) -> Option<&str> { + runtimes_list_report.lines().find_map(|line| { + let rest = line + .trim() + .strip_prefix("active_status: missing manifest for ")?; + // Whichever field held the dead name — the key when one was activated, the + // id when only a default was ever set. Both are selectors the engine + // install would resolve, and both fail it the same way. + let (_, selector) = rest.split_once('=')?; + let selector = selector.trim(); + (!selector.is_empty()).then_some(selector) + }) +} + +/// Managed runtimes the tree could be pointed at instead, in report order. +/// +/// Read-only entries are excluded on purpose. `runtimes adopt` records a folder +/// the pre-warm does not own, and quietly making somebody's external ROCm install +/// the default for every GPU scenario on the runner is a larger decision than +/// repairing a pointer. If the tree holds nothing else, the caller leaves the +/// pointer dangling and the install path replaces it. +#[must_use] +pub fn activation_candidates(runtimes_list_report: &str) -> Vec { + runtimes_list_report + .lines() + .filter_map(RuntimeEntry::parse) + .filter(|entry| !entry.read_only) + .map(|entry| entry.runtime_key) + .collect() +} + /// One ` runtime format=… channel=… … status=…` line from `rocm update`. /// /// Both shapes that renderer emits are handled: the full report line, and the @@ -320,6 +383,11 @@ pub fn run(channel: &str, keep: usize, prewarm_dir: &Path) -> Result<()> { // Repairing first means `decide` reads a registry with nothing dead in it. repair_poisoned_runtimes(&rocm, prewarm_dir)?; + // And point the tree at something that exists before anything resolves the + // active runtime. Must run after the repair above, which can itself remove + // the runtime the pointer names. + repair_dangling_active_runtime(&rocm, prewarm_dir)?; + let decision = match probe(&rocm, prewarm_dir) { Ok(report) => decide(&report, channel), Err(error) => { @@ -534,6 +602,82 @@ fn repair_poisoned_runtimes(rocm: &Path, prewarm_dir: &Path) -> Result<()> { Ok(()) } +/// Re-point the shared tree's active runtime when it names one that is gone, so +/// the engine install resolves instead of dying nine seconds into the lane. +/// +/// See [`dangling_active_runtime`] for how the pointer goes stale and why this is +/// worth healing here. The repair is `rocm runtimes activate`: the CLI has no verb +/// that clears a pointer, so the only way out is to name something that exists. +/// Candidates are tried in report order and the first that takes it wins — which +/// may be an older runtime than the tree would ideally serve, and that is fine. +/// `decide` runs next, and the `Update` arm re-activates the newest as part of +/// updating to it. +/// +/// Doing nothing is correct when the tree holds no managed runtime at all: the +/// install path activates whatever it installs, and it runs before +/// `ensure_default_engine` — which is the only thing that would have tripped over +/// the pointer. The case that has to be handled here is the one that self-healing +/// misses: a pointer dangling while OTHER runtimes are sitting right there, where +/// `decide` says `Reuse`, nothing installs, and nothing re-activates. +fn repair_dangling_active_runtime(rocm: &Path, prewarm_dir: &Path) -> Result<()> { + if !prewarm_dir.join("data").join("runtimes").is_dir() { + return Ok(()); + } + + // Deliberately a second listing rather than one shared with the repair above: + // that repair uninstalls, which rewrites exactly the pointers read here. + let listing = match list_runtimes(rocm, prewarm_dir) { + Ok(listing) => listing, + Err(error) => { + println!( + "pre-warm: could not list runtimes ({error:#}); leaving the active runtime as it is" + ); + return Ok(()); + } + }; + + let Some(dangling) = dangling_active_runtime(&listing) else { + return Ok(()); + }; + + let candidates = activation_candidates(&listing); + if candidates.is_empty() { + println!( + "pre-warm: the active runtime {dangling} is gone and nothing is installed to take \ + its place; the install below will set it" + ); + return Ok(()); + } + + println!( + "::warning::pre-warm: the shared tree still points at the runtime {dangling}, which is \ + no longer installed — something removed it without going through \ + `rocm runtimes uninstall`. Engine installs resolve through that pointer, so re-pointing \ + it at an installed runtime. See rocm-cli#314." + ); + + for candidate in &candidates { + let activated = rocm_command(rocm, prewarm_dir) + .args(["runtimes", "activate", candidate]) + .status() + .with_context(|| format!("failed to run `rocm runtimes activate {candidate}`"))?; + if activated.success() { + println!("pre-warm: active runtime is now {candidate}"); + return Ok(()); + } + // An installed runtime can still be refused — `activate` validates the + // manifest. Try the next one rather than taking the whole lane down for + // one bad entry. + println!("pre-warm: could not activate {candidate}; trying the next runtime"); + } + + bail!( + "the shared pre-warm tree points at the runtime `{dangling}`, which is not installed, and \ + none of its {} installed runtime(s) could be activated in its place", + candidates.len() + ) +} + /// Ask the CLI what it has registered. Read-only. fn list_runtimes(rocm: &Path, prewarm_dir: &Path) -> Result { let output = rocm_command(rocm, prewarm_dir) @@ -698,6 +842,29 @@ registered ROCm runtimes next step: rocm install sdk --channel release --format wheel "; + /// A real `rocm runtimes list` on the tree that killed the lane: the config + /// still names the 7.14.0 runtime that was removed out of band, while two + /// runtimes it could be pointed at sit right there. + /// + /// Note there is no `*` marker on any entry — the renderer only marks the + /// active one by matching `active_runtime_key` against a manifest, and that is + /// exactly the match that fails here. The `active_status:` line is the only + /// thing in the document that says so. + const DANGLING_ACTIVE: &str = "\ +registered ROCm runtimes + active_runtime_id: therock-release:gfx94X-dcgpu + active_runtime_key: release-wheel-gfx94x-dcgpu-7-14-0 + previous_runtime_key: release-wheel-gfx94x-dcgpu-7-13-0 + registry: /w/e2e-prewarm/data/runtimes/registry + marker: /w/e2e-prewarm/data/runtimes/active.json + active_status: missing manifest for active_runtime_key=release-wheel-gfx94x-dcgpu-7-14-0 + installed: + adopted-external-env runtime_id=external-adopted version=7.14.0 format=wheel family=gfx94X-dcgpu mode=read-only status=usable + install_root: /opt/external-rocm + release-wheel-gfx94x-dcgpu-7-13-0 runtime_id=therock-release-gfx94x-dcgpu version=7.13.0 format=wheel family=gfx94X-dcgpu mode=managed status=usable + install_root: /w/e2e-prewarm/data/runtimes/wheel/release-wheel-gfx94x-dcgpu-7-13-0 +"; + fn prewarm_runtimes_dir() -> &'static Path { Path::new("/w/e2e-prewarm/data/runtimes") } @@ -747,6 +914,65 @@ registered ROCm runtimes assert!(assess(NO_RUNTIMES, prewarm_runtimes_dir()).is_empty()); } + #[test] + fn an_active_runtime_that_is_gone_is_reported_by_name() { + assert_eq!( + dangling_active_runtime(DANGLING_ACTIVE), + Some("release-wheel-gfx94x-dcgpu-7-14-0") + ); + } + + #[test] + fn a_default_runtime_id_that_is_gone_dangles_the_same_way() { + // The other field the engine install resolves through, reported by the + // renderer under the same key when no runtime_key was ever activated. + let text = "registered ROCm runtimes\n active_runtime_id: therock-release:gfx94X-dcgpu\n \ +active_runtime_key: \n \ +active_status: missing manifest for active_runtime_id=therock-release:gfx94X-dcgpu\n"; + assert_eq!( + dangling_active_runtime(text), + Some("therock-release:gfx94X-dcgpu") + ); + } + + #[test] + fn a_tree_whose_active_runtime_is_installed_is_left_alone() { + // No active_status line at all is the healthy shape — MIXED has a dead + // runtime in it, but nothing points AT the dead one. + assert_eq!(dangling_active_runtime(MIXED), None); + assert_eq!(dangling_active_runtime(NO_RUNTIMES), None); + assert_eq!(dangling_active_runtime(""), None); + } + + #[test] + fn an_ambiguous_runtime_id_is_not_a_dangling_pointer() { + // Same `active_status:` prefix, entirely different condition: the runtimes + // are all there, one runtime_id just names several of them. Activating + // something would be a guess, and the engine install resolves it fine. + let text = "registered ROCm runtimes\n \ +active_status: ambiguous runtime_id=therock-release:gfx94X-dcgpu; activate one runtime_key: a, b\n"; + assert_eq!(dangling_active_runtime(text), None); + } + + #[test] + fn only_managed_runtimes_are_offered_as_replacements() { + // The read-only adopted entry is installed and usable, and still excluded: + // making somebody's external ROCm the default for every GPU scenario is a + // bigger decision than repairing a pointer. + assert_eq!( + activation_candidates(DANGLING_ACTIVE), + vec!["release-wheel-gfx94x-dcgpu-7-13-0".to_owned()] + ); + } + + #[test] + fn a_tree_with_nothing_installed_offers_no_replacement() { + // The caller leaves the pointer dangling here rather than failing: the + // install that follows activates whatever it installs. + assert!(activation_candidates(NO_RUNTIMES).is_empty()); + assert!(activation_candidates("").is_empty()); + } + #[test] fn a_report_that_cannot_be_read_removes_nothing() { // The conservative floor: never delete on a shape this does not recognise. From 05fb20bd8be2cdcb4e87e26136c2ab7755d0d6fc Mon Sep 17 00:00:00 2001 From: Tomas Saaristola Date: Tue, 1 Sep 2026 13:34:53 +0000 Subject: [PATCH 18/19] docs(vllm): document the torch alignment and its opt-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Engine install can now replace the torch in a managed runtime, which is a surprising thing for an install to do if the only place it is explained is the line it prints while doing it. The adapter doc gains the rule — a kernel-proven torch is kept, otherwise the runtime moves to the SDK's build of the release the engine pins — and says where each half of that version comes from and why. It also documents `ROCM_CLI_DISABLE_TORCH_ALIGNMENT` as the escape hatch for a locally built wheel or a stack pinned for a reproduction, including that any value works because presence is the signal, that the device check still runs under it, and that the result is not validated against the supported matrix. Signed-off-by: Tomas Saaristola --- docs/vllm.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/docs/vllm.md b/docs/vllm.md index 04fbd71ca..aef979e05 100644 --- a/docs/vllm.md +++ b/docs/vllm.md @@ -18,6 +18,41 @@ the existing TheRock PyTorch stack. A prebuilt vLLM ROCm wheel can replace the TheRock torch packages or target a different ROCm soname set; that is not a valid no-fallback setup for rocm-cli GPU serving. +## Torch alignment on engine install + +Installing an engine into a managed TheRock runtime can change the torch in that +runtime. Two installers write torch into the same environment — the SDK install +writes TheRock's build, and the engine install then writes the build from its own +index — so `rocm engines install` settles which one stays and prints the result +as a `torch_alignment:` line. + +A torch that already executes a GPU kernel against the installed SDK is kept +exactly as it is, whichever installer put it there. Otherwise the runtime is +moved to the SDK's *build* of the torch *release* the engine pins: the release +comes from the engine, which was built against it, and the build comes from the +SDK, whose libraries it has to load. A `device_check:` line reports what the +result can actually do, and a realignment also reports what the runtime could do +before it. + +Set `ROCM_CLI_DISABLE_TORCH_ALIGNMENT` to keep whatever torch is installed and +skip the replacement: + +```bash +ROCM_CLI_DISABLE_TORCH_ALIGNMENT=1 rocm engines install vllm --yes +``` + +Any value works, including an empty one — the variable being set is the signal. +The install then reports `torch_alignment: disabled`, naming both the build it +would have installed and the one it kept. The device check still runs, so an +opt-out that leaves the runtime unable to serve says so rather than failing later +during serving. + +Use it when you are deliberately running a torch the alignment would replace — a +locally built wheel, a version under test, a stack pinned for a reproduction. It +is an escape hatch, not a supported configuration: the resulting combination is +not validated against the supported matrix, and a runtime that cannot execute a +kernel will fail at serving time. + Supported discovery paths: - `ROCM_CLI_VLLM_COMMAND=/path/to/vllm` From fd552ba09dcc91c8a0714bda7751fbf00ab3784b Mon Sep 17 00:00:00 2001 From: Michael Roy Date: Tue, 1 Sep 2026 09:21:56 -0700 Subject: [PATCH 19/19] fix(security): keep provider credentials opaque Signed-off-by: Michael Roy --- apps/rocm/src/dash.rs | 4 +- apps/rocm/src/main.rs | 4 +- apps/rocm/src/provider_keys.rs | 113 +++++++++++++++++---------------- apps/rocm/src/providers.rs | 25 ++++---- 4 files changed, 75 insertions(+), 71 deletions(-) diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index 88b63864d..4f404b61d 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -94,9 +94,9 @@ fn chat_api_key_from_env() -> Option { /// unavailable store yields `None` (the dash still launches; switching to the /// Anthropic provider then surfaces an actionable error turn). fn anthropic_api_key_for_dash() -> Option { - crate::provider_keys::resolve_provider_api_key("anthropic", "ANTHROPIC_API_KEY") + crate::provider_keys::provider_credential("anthropic", "ANTHROPIC_API_KEY") .ok() - .map(|k| k.value) + .map(crate::provider_keys::ProviderCredential::into_value) } /// Adapt the built-in `rocm-core` model recipes into the TUI-local summaries the diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 91fc32220..75a75ffc3 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -9494,7 +9494,7 @@ fn config(command: ConfigCommand) -> Result<()> { bail!("local provider does not use a cloud API key"); } let key = read_provider_key_from_user(provider)?; - let status = provider_keys::set_provider_api_key(provider, &key)?; + let status = provider_keys::store_provider_credential(provider, &key)?; println!("{provider} API key saved"); println!( " key: {}", @@ -9517,7 +9517,7 @@ fn config(command: ConfigCommand) -> Result<()> { if provider == "local" { bail!("local provider does not use a cloud API key"); } - let status = provider_keys::clear_provider_api_key(provider)?; + let status = provider_keys::remove_provider_credential(provider)?; println!("{provider} API key cleared"); println!( " key: {}", diff --git a/apps/rocm/src/provider_keys.rs b/apps/rocm/src/provider_keys.rs index 356cc9eac..d05222134 100644 --- a/apps/rocm/src/provider_keys.rs +++ b/apps/rocm/src/provider_keys.rs @@ -21,17 +21,23 @@ pub(crate) struct ProviderKeyStatus { pub source: String, } -#[derive(Debug, Clone, Eq, PartialEq)] -pub(crate) struct ProviderApiKey { - pub value: String, - pub source: String, +pub(crate) struct ProviderCredential(String); + +impl ProviderCredential { + pub(crate) fn as_str(&self) -> &str { + &self.0 + } + + pub(crate) fn into_value(self) -> String { + self.0 + } } pub(crate) trait ProviderKeyStore: Send + Sync { fn label(&self) -> &'static str; - fn get_secret(&self, provider: &str) -> Result>>; - fn set_secret(&self, provider: &str, secret: &[u8]) -> Result<()>; - fn clear_secret(&self, provider: &str) -> Result<()>; + fn get_entry(&self, provider: &str) -> Result>>; + fn store_entry(&self, provider: &str, value: &[u8]) -> Result<()>; + fn remove_entry(&self, provider: &str) -> Result<()>; } #[derive(Debug, Clone, Copy, Default)] @@ -49,9 +55,9 @@ pub(crate) fn provider_key_status(provider: &str, env_name: &str) -> ProviderKey ) } -pub(crate) fn resolve_provider_api_key(provider: &str, env_name: &str) -> Result { +pub(crate) fn provider_credential(provider: &str, env_name: &str) -> Result { let store = NativeProviderKeyStore; - resolve_provider_api_key_with_store( + provider_credential_with_store( &store, provider, env_name, @@ -61,12 +67,12 @@ pub(crate) fn resolve_provider_api_key(provider: &str, env_name: &str) -> Result ) } -pub(crate) fn set_provider_api_key(provider: &str, value: &str) -> Result { +pub(crate) fn store_provider_credential(provider: &str, value: &str) -> Result { let store = NativeProviderKeyStore; - set_provider_api_key_with_store(&store, provider, value) + store_provider_credential_with_store(&store, provider, value) } -pub(crate) fn set_provider_api_key_with_store( +pub(crate) fn store_provider_credential_with_store( store: &dyn ProviderKeyStore, provider: &str, value: &str, @@ -77,7 +83,7 @@ pub(crate) fn set_provider_api_key_with_store( } ensure_cloud_provider(provider)?; store - .set_secret(provider, trimmed.as_bytes()) + .store_entry(provider, trimmed.as_bytes()) .with_context(|| format!("failed to save {provider} API key in secure storage"))?; Ok(ProviderKeyStatus { state: ProviderKeyState::Configured, @@ -85,18 +91,18 @@ pub(crate) fn set_provider_api_key_with_store( }) } -pub(crate) fn clear_provider_api_key(provider: &str) -> Result { +pub(crate) fn remove_provider_credential(provider: &str) -> Result { let store = NativeProviderKeyStore; - clear_provider_api_key_with_store(&store, provider) + remove_provider_credential_with_store(&store, provider) } -pub(crate) fn clear_provider_api_key_with_store( +pub(crate) fn remove_provider_credential_with_store( store: &dyn ProviderKeyStore, provider: &str, ) -> Result { ensure_cloud_provider(provider)?; store - .clear_secret(provider) + .remove_entry(provider) .with_context(|| format!("failed to clear {provider} API key from secure storage"))?; Ok(ProviderKeyStatus { state: ProviderKeyState::Missing, @@ -138,8 +144,8 @@ fn provider_key_status_with_store( source: format!("env:{env_name}"), }; } - match store.get_secret(provider) { - Ok(Some(secret)) if !secret.is_empty() => ProviderKeyStatus { + match store.get_entry(provider) { + Ok(Some(value)) if !value.is_empty() => ProviderKeyStatus { state: ProviderKeyState::Configured, source: secure_source_label(store.label()), }, @@ -154,32 +160,26 @@ fn provider_key_status_with_store( } } -fn resolve_provider_api_key_with_store( +fn provider_credential_with_store( store: &dyn ProviderKeyStore, provider: &str, env_name: &str, env_value: Option, -) -> Result { +) -> Result { ensure_cloud_provider(provider)?; if let Some(value) = env_value { - return Ok(ProviderApiKey { - value, - source: format!("env:{env_name}"), - }); + return Ok(ProviderCredential(value)); } - match store.get_secret(provider) { - Ok(Some(secret)) if !secret.is_empty() => { - let value = String::from_utf8(secret) + match store.get_entry(provider) { + Ok(Some(value)) if !value.is_empty() => { + let value = String::from_utf8(value) .context("stored provider API key was not valid UTF-8")? .trim() .to_owned(); if value.is_empty() { bail!("{provider} API key in secure storage is empty"); } - Ok(ProviderApiKey { - value, - source: secure_source_label(store.label()), - }) + Ok(ProviderCredential(value)) } Ok(_) => bail!( "{provider} provider requires a saved API key; run `rocm config set-provider-key {provider}` or set {env_name} for this session" @@ -207,21 +207,21 @@ impl ProviderKeyStore for NativeProviderKeyStore { native_store_label() } - fn get_secret(&self, provider: &str) -> Result>> { + fn get_entry(&self, provider: &str) -> Result>> { with_native_entry(provider, |entry| match entry.get_secret() { - Ok(secret) => Ok(Some(secret)), + Ok(value) => Ok(Some(value)), Err(KeyringError::NoEntry) => Ok(None), Err(error) => Err(keyring_anyhow(error)), }) } - fn set_secret(&self, provider: &str, secret: &[u8]) -> Result<()> { + fn store_entry(&self, provider: &str, value: &[u8]) -> Result<()> { with_native_entry(provider, |entry| { - entry.set_secret(secret).map_err(keyring_anyhow) + entry.set_secret(value).map_err(keyring_anyhow) }) } - fn clear_secret(&self, provider: &str) -> Result<()> { + fn remove_entry(&self, provider: &str) -> Result<()> { with_native_entry(provider, |entry| match entry.delete_credential() { Ok(()) | Err(KeyringError::NoEntry) => Ok(()), Err(error) => Err(keyring_anyhow(error)), @@ -236,7 +236,7 @@ impl ProviderKeyStore for NativeProviderKeyStore { /// runtime *context* is already entered on the calling thread, that nested /// `block_on` panics with "Cannot start a runtime from within a runtime". The /// dash resolves keys off-runtime, but this is the single chokepoint for every -/// store op (get/set/clear) and for `resolve_provider_api_key` / +/// store op (get/store/remove) and for `provider_credential` / /// `provider_key_status`, so guard the whole class here: when a runtime is /// active, run the entry build *and* the action on a fresh OS thread that has no /// runtime entered. @@ -348,22 +348,22 @@ mod tests { "test keychain" } - fn get_secret(&self, provider: &str) -> Result>> { + fn get_entry(&self, provider: &str) -> Result>> { if let Some(fail) = self.fail { bail!("{fail}"); } Ok(self.secrets.lock().unwrap().get(provider).cloned()) } - fn set_secret(&self, provider: &str, secret: &[u8]) -> Result<()> { + fn store_entry(&self, provider: &str, value: &[u8]) -> Result<()> { self.secrets .lock() .unwrap() - .insert(provider.to_owned(), secret.to_vec()); + .insert(provider.to_owned(), value.to_vec()); Ok(()) } - fn clear_secret(&self, provider: &str) -> Result<()> { + fn remove_entry(&self, provider: &str) -> Result<()> { self.secrets.lock().unwrap().remove(provider); Ok(()) } @@ -392,19 +392,20 @@ mod tests { } #[test] - fn provider_key_store_round_trips_without_exposing_value_in_status() -> Result<()> { + fn provider_credential_round_trip_keeps_secret_out_of_status() -> Result<()> { let store = MemoryKeyStore::default(); - store.set_secret("openai", b"sk-secret-sentinel")?; + store_provider_credential_with_store(&store, "openai", "sk-secret-sentinel")?; let status = provider_key_status_with_store(&store, "openai", "OPENAI_API_KEY", None); - let resolved = - resolve_provider_api_key_with_store(&store, "openai", "OPENAI_API_KEY", None)?; + let credential = provider_credential_with_store(&store, "openai", "OPENAI_API_KEY", None)?; assert_eq!(status.state, ProviderKeyState::Configured); assert_eq!(status.source, "secure:test keychain"); assert!(!provider_key_status_label(&status).contains("sk-secret")); - assert_eq!(resolved.value, "sk-secret-sentinel"); - assert_eq!(resolved.source, "secure:test keychain"); + assert_eq!(credential.as_str(), "sk-secret-sentinel"); + + remove_provider_credential_with_store(&store, "openai")?; + assert!(store.get_entry("openai")?.is_none()); Ok(()) } @@ -415,9 +416,10 @@ mod tests { ..MemoryKeyStore::default() }; - let error = resolve_provider_api_key_with_store(&store, "openai", "OPENAI_API_KEY", None) - .unwrap_err() - .to_string(); + let error = match provider_credential_with_store(&store, "openai", "OPENAI_API_KEY", None) { + Ok(_) => panic!("credential resolution should fail when storage is unavailable"), + Err(error) => error.to_string(), + }; assert!(error.contains("secure API-key storage is unavailable")); assert!(error.contains("no plaintext fallback was used")); @@ -428,9 +430,10 @@ mod tests { let store = MemoryKeyStore::default(); let error = - resolve_provider_api_key_with_store(&store, "anthropic", "ANTHROPIC_API_KEY", None) - .unwrap_err() - .to_string(); + match provider_credential_with_store(&store, "anthropic", "ANTHROPIC_API_KEY", None) { + Ok(_) => panic!("credential resolution should fail when no credential is stored"), + Err(error) => error.to_string(), + }; assert!(error.contains("requires a saved API key")); assert!(error.contains("rocm config set-provider-key anthropic")); @@ -455,7 +458,7 @@ mod tests { let outcome = rt.block_on(async { std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { // Read-only: never writes to a real keychain on dev machines. - NativeProviderKeyStore.get_secret("anthropic") + NativeProviderKeyStore.get_entry("anthropic") })) }); assert!( diff --git a/apps/rocm/src/providers.rs b/apps/rocm/src/providers.rs index 665074554..af6e8609b 100644 --- a/apps/rocm/src/providers.rs +++ b/apps/rocm/src/providers.rs @@ -114,7 +114,7 @@ struct LocalProvider<'a> { struct RemoteProvider { provider: &'static str, - api_key_env: &'static str, + credential_env: &'static str, model_env: &'static str, endpoint_env: &'static str, default_endpoint: &'static str, @@ -261,14 +261,14 @@ fn provider_adapter<'a>( "local" => Box::new(LocalProvider { paths }), "openai" => Box::new(RemoteProvider { provider: "openai", - api_key_env: "OPENAI_API_KEY", + credential_env: "OPENAI_API_KEY", model_env: "ROCM_CLI_OPENAI_MODEL", endpoint_env: "OPENAI_BASE_URL", default_endpoint: "https://api.openai.com/v1/chat/completions", }), "anthropic" => Box::new(RemoteProvider { provider: "anthropic", - api_key_env: "ANTHROPIC_API_KEY", + credential_env: "ANTHROPIC_API_KEY", model_env: "ROCM_CLI_ANTHROPIC_MODEL", endpoint_env: "ANTHROPIC_BASE_URL", default_endpoint: "https://api.anthropic.com/v1/messages", @@ -366,7 +366,8 @@ impl ProviderAdapter for RemoteProvider { .filter(|value| !value.trim().is_empty()) .into_iter() .collect::>(); - let key_status = crate::provider_keys::provider_key_status(self.provider, self.api_key_env); + let key_status = + crate::provider_keys::provider_key_status(self.provider, self.credential_env); Ok(ProviderStatus { provider: self.provider.to_owned(), auth_status: crate::provider_keys::provider_key_status_label(&key_status), @@ -376,8 +377,8 @@ impl ProviderAdapter for RemoteProvider { } fn chat(&self, request: &ChatRequest) -> Result { - let api_key = - crate::provider_keys::resolve_provider_api_key(self.provider, self.api_key_env)?; + let credential = + crate::provider_keys::provider_credential(self.provider, self.credential_env)?; let model = resolve_remote_model(self.provider, self.model_env, request.model.as_deref())?; let endpoint = remote_endpoint(self.endpoint_env, self.default_endpoint); let (content, tool_calls) = match self.provider { @@ -386,7 +387,7 @@ impl ProviderAdapter for RemoteProvider { let json = post_json_with_headers( &endpoint, &[ - ("Authorization", format!("Bearer {}", api_key.value)), + ("Authorization", format!("Bearer {}", credential.as_str())), ("Content-Type", "application/json".to_owned()), ], &body, @@ -398,7 +399,7 @@ impl ProviderAdapter for RemoteProvider { let json = post_json_with_headers( &endpoint, &[ - ("x-api-key", api_key.value), + ("x-api-key", credential.into_value()), ("anthropic-version", "2023-06-01".to_owned()), ("Content-Type", "application/json".to_owned()), ], @@ -421,8 +422,8 @@ impl ProviderAdapter for RemoteProvider { request: &ChatRequest, on_event: &mut dyn FnMut(ProviderStreamEvent) -> Result<()>, ) -> Result { - let api_key = - crate::provider_keys::resolve_provider_api_key(self.provider, self.api_key_env)?; + let credential = + crate::provider_keys::provider_credential(self.provider, self.credential_env)?; let model = resolve_remote_model(self.provider, self.model_env, request.model.as_deref())?; let endpoint = remote_endpoint(self.endpoint_env, self.default_endpoint); match self.provider { @@ -432,7 +433,7 @@ impl ProviderAdapter for RemoteProvider { stream_json_with_headers( &endpoint, &[ - ("Authorization", format!("Bearer {}", api_key.value)), + ("Authorization", format!("Bearer {}", credential.as_str())), ("Content-Type", "application/json".to_owned()), ("Accept", "text/event-stream".to_owned()), ], @@ -447,7 +448,7 @@ impl ProviderAdapter for RemoteProvider { stream_json_with_headers( &endpoint, &[ - ("x-api-key", api_key.value), + ("x-api-key", credential.into_value()), ("anthropic-version", "2023-06-01".to_owned()), ("Content-Type", "application/json".to_owned()), ("Accept", "text/event-stream".to_owned()),