From 596c5be488a77109c2c88c315c89d520a45b3900 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Fri, 14 Aug 2026 14:21:24 +0000 Subject: [PATCH 1/2] fix(install): restore the engine's pinned deps after an SDK install The managed runtime venv has two owners. `rocm install sdk` writes TheRock's torch stack into it, and the vLLM engine installs into the same interpreter, pinning a torch build from its own index that no TheRock build satisfies. A second `install sdk` therefore wrote the SDK's torch back over the engine's pin. The auto-install that runs afterwards should have restored it, but its short-circuit tested whether vLLM resolved rather than whether vLLM's requirements were met. Only torch had moved, so vLLM still resolved and the pin was never re-asserted. Nothing downstream noticed: runtime validation checks paths and the rocm_sdk probe, so every surface kept reporting the runtime ready, and the first symptom was a serve failure whose traceback named neither torch nor the SDK. Gate the short-circuit on the engine's own `Requires-Dist` instead, via a `uv pip check` primitive in rocm-core, so a violated pin falls through to an install that restores it. The repair targets the interpreter that was assessed, which is not always the one the other resolver would pick. The check runs with colour disabled because uv colourises its findings under FORCE_COLOR even into a pipe, and an escape prefix would read as "could not verify" and silently restore the old behaviour. `install sdk` now states the outcome as `dependency_check:` and records it in the audit log, on every auto-install outcome including a failed one, where a violated pin is most likely. It stays exit-0 when a violation survives: the SDK installed correctly, and the warning names the one command that clears it. Signed-off-by: Eugene Volen --- apps/rocm/src/main.rs | 209 ++++++++++++++- crates/rocm-core/src/lib.rs | 7 +- crates/rocm-core/src/uv.rs | 251 ++++++++++++++++++ engines/vllm/src/lib.rs | 226 +++++++++++++++- .../features/runtime_setup.feature | 14 + tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 46 ++++ 6 files changed, 736 insertions(+), 17 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 2de7389a2..c33241bc3 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -7193,7 +7193,7 @@ fn maybe_auto_install_sdk_preferred_engine( let mut config = RocmCliConfig::load(paths)?; let env_root = env_root_for_engine_install(paths, &config, engine, &finalized.runtime_key)?; - let response = engine_request_with_env_root::<_, InstallResponse>( + let response = match engine_request_with_env_root::<_, InstallResponse>( Some(paths), engine, EngineMethod::Install, @@ -7204,7 +7204,23 @@ fn maybe_auto_install_sdk_preferred_engine( env_root: env_root.clone(), }, env_root.as_deref(), - )?; + ) { + Ok(response) => response, + Err(error) => { + // A failed engine install is the case most likely to leave the SDK's torch + // sitting over the engine's pin, because the install is what would have + // restored it. Reporting the check only on success would print nothing here + // — and `install sdk` still exits 0 — which is exactly the silent broken + // runtime this block exists to prevent. + report_engine_dependency_check( + paths, + engine, + runtime_python_for_key(paths, &finalized.runtime_key).as_deref(), + &finalized.runtime_key, + ); + return Err(error); + } + }; println!(" reinstall: false"); println!(" env_id: {}", response.env_id); println!(" env_path: {}", response.env_path); @@ -7223,6 +7239,17 @@ fn maybe_auto_install_sdk_preferred_engine( engine_config.preferred_env_id = Some(response.env_id.clone()); } 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, + ); } record_cli_audit_event( @@ -7239,6 +7266,125 @@ fn maybe_auto_install_sdk_preferred_engine( Ok(()) } +/// Whether an installed engine's declared requirements are met in the environment it +/// shares with the ROCm SDK. +#[derive(Debug, Clone, PartialEq, Eq)] +enum EngineDependencyCheck { + /// Every requirement the engine declares is satisfied. + Satisfied, + /// The engine declares requirements the environment does not meet, one line each, + /// as the resolver reported them. + Violated(Vec), + /// The check itself could not run (no usable `uv`, unreadable environment). + NotVerified(String), +} + +/// Print the dependency check for `engine` and record it in the CLI audit log. +/// +/// A surviving violation does not fail the install: the SDK itself installed correctly, +/// and abandoning a multi-gigabyte install would cost the user more than the warning and +/// the one-line remedy it names. +fn report_engine_dependency_check( + paths: &AppPaths, + engine: &str, + python: Option<&Path>, + runtime_key: &str, +) { + let outcome = engine_dependency_check(paths, engine, python); + print!("{}", render_engine_dependency_check(engine, &outcome)); + let (level, message) = match &outcome { + EngineDependencyCheck::Satisfied => ( + "info", + format!("engine={engine} runtime_id={runtime_key} dependency_check=satisfied"), + ), + EngineDependencyCheck::Violated(details) => ( + "error", + format!( + "engine={engine} runtime_id={runtime_key} dependency_check=violated: {}", + details.join("; ") + ), + ), + EngineDependencyCheck::NotVerified(reason) => ( + "info", + format!( + "engine={engine} runtime_id={runtime_key} dependency_check=not_verified: {reason}" + ), + ), + }; + record_cli_audit_event( + paths, + "engine", + "engine_dependency_check", + level, + message, + None, + ); +} + +/// The Python interpreter of the managed runtime `runtime_key`, when its manifest names +/// one. Used to check an environment whose engine install did not get far enough to +/// report its own interpreter. +fn runtime_python_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.python_executable.as_deref().map(PathBuf::from) +} + +fn engine_dependency_check( + paths: &AppPaths, + engine: &str, + python: Option<&Path>, +) -> EngineDependencyCheck { + let Some(python) = python else { + return EngineDependencyCheck::NotVerified( + "the runtime's Python environment could not be located".to_owned(), + ); + }; + 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(), + ) + } + } + Err(error) => EngineDependencyCheck::NotVerified(error.to_string()), + } +} + +fn render_engine_dependency_check(engine: &str, outcome: &EngineDependencyCheck) -> String { + let mut output = String::new(); + match outcome { + EngineDependencyCheck::Satisfied => { + let _ = writeln!(output, " dependency_check: satisfied"); + } + EngineDependencyCheck::NotVerified(reason) => { + let _ = writeln!( + output, + " dependency_check: not_verified ({})", + sanitize_log_value(reason) + ); + } + EngineDependencyCheck::Violated(details) => { + let _ = writeln!(output, " dependency_check: violated"); + for detail in details { + let _ = writeln!(output, " violation: {}", sanitize_log_value(detail)); + } + let _ = writeln!( + output, + " action: rocm engines install {engine} --reinstall" + ); + } + } + output +} + fn render_sdk_install_success(finalized: &SdkInstallFinalization) -> String { format!( "ROCm SDK installed successfully.\n install folder: {}\n active runtime: {}\n next step: run `rocm help` to see how to use rocm-cli.\n", @@ -25432,6 +25578,65 @@ ID_LIKE="suse opensuse" Ok(()) } + #[test] + fn a_satisfied_dependency_check_is_stated_plainly() { + let rendered = render_engine_dependency_check("vllm", &EngineDependencyCheck::Satisfied); + + assert_eq!(rendered, " dependency_check: satisfied\n"); + } + + #[test] + fn a_violated_dependency_check_names_the_pin_and_the_remedy() { + // The SDK torch stack replaced the build vLLM pins. The install + // 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(), + ]), + ); + + assert!(rendered.contains(" dependency_check: violated\n")); + assert!(rendered.contains("torch==2.10.0+git8514f05")); + assert!(rendered.contains("2.9.1+rocm7.14.0a20260611")); + assert!(rendered.contains(" action: rocm engines install vllm --reinstall\n")); + } + + #[test] + fn an_unlocatable_environment_is_reported_not_assumed_healthy() { + // The engine install can fail before it reports its own interpreter. That path + // still prints a block, and with no interpreter to check it must say so rather + // than fall through to `satisfied`. + let (root, paths) = test_paths("engine-dependency-no-python"); + + let outcome = engine_dependency_check(&paths, "vllm", None); + let _ = fs::remove_dir_all(root); + + assert_eq!( + outcome, + EngineDependencyCheck::NotVerified( + "the runtime's Python environment could not be located".to_owned() + ) + ); + assert!( + render_engine_dependency_check("vllm", &outcome).contains("not_verified"), + "the failure path still prints a dependency_check block" + ); + } + + #[test] + fn a_dependency_check_that_could_not_run_says_so_instead_of_claiming_health() { + let rendered = render_engine_dependency_check( + "vllm", + &EngineDependencyCheck::NotVerified("uv binary is unavailable\nofflinehost".to_owned()), + ); + + assert!(rendered.contains("not_verified")); + assert!(!rendered.contains("satisfied")); + // A multi-line failure must stay on one reported line. + assert_eq!(rendered.lines().count(), 1, "{rendered}"); + } + #[test] fn render_engine_inventory_text_surfaces_external_plugin_policy() { let (root, paths) = test_paths("engine-plugin-policy"); diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 1846e0131..060926eb1 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -70,9 +70,10 @@ pub use runtime::{ runtime_rocm_library_filename, shell_command_for_host, user_runtime_dir, }; pub use uv::{ - DEFAULT_UV_TIMEOUT_SECS, UV_CACHE_DIR_ENV, UV_CACHE_DIR_OVERRIDE_ENV, UvCacheSource, - ensure_uv_binary, uv_binary_name, uv_cache_source, uv_command_env, uv_http_timeout_secs, - uv_pip_freeze_args, uv_pip_install_base, uv_venv_args, + 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, }; 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 b9ab379e8..ef99cf503 100644 --- a/crates/rocm-core/src/uv.rs +++ b/crates/rocm-core/src/uv.rs @@ -201,6 +201,124 @@ pub fn uv_pip_freeze_args(venv_python: &Path) -> Vec { ] } +/// Arguments for `uv pip check` targeting the interpreter `venv_python`. +/// +/// `--color never` is not cosmetic: this output is parsed, and `uv` colorizes its +/// findings whenever `FORCE_COLOR` / `CLICOLOR_FORCE` is exported — even when stderr is +/// a pipe rather than a terminal. The escape prefix would push every finding past the +/// parser, which reads as "the check could not run" and silently restores the very +/// behavior the check exists to catch. +pub fn uv_pip_check_args(venv_python: &Path) -> Vec { + vec![ + "pip".to_owned(), + "check".to_owned(), + "--color".to_owned(), + "never".to_owned(), + "--python".to_owned(), + venv_python.to_string_lossy().into_owned(), + ] +} + +/// One unsatisfied `Requires-Dist` in an environment, as reported by `uv pip check`. +/// +/// The managed runtime venv has more than one owner: `rocm install sdk` writes the +/// TheRock torch stack into it, and engines installed against that runtime write their +/// own pinned dependencies over the top. Either side can leave the other's requirements +/// unmet without removing any package, so "the distribution is importable" is not +/// evidence that it can run. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DependencyViolation { + /// Distribution whose `Requires-Dist` is unsatisfied (the requirer, not the + /// package at the wrong version). + pub requiring: String, + /// The line `uv` reported, kept verbatim so the user sees the requirement and the + /// installed version exactly as the resolver saw them. + pub detail: String, +} + +/// Opening frame of every line `uv pip check` emits about a distribution. +const UV_CHECK_VIOLATION_PREFIX: &str = "The package `"; + +/// What follows the requirer's name on an unsatisfied-requirement line specifically. +/// +/// The trailing backtick is the whole point: `uv` reports several other conditions under +/// the same opening frame — a missing `WHEEL`/`METADATA` file, an unsatisfied +/// `Requires-Python`, a package with multiple installed distributions — and none of them +/// is a `Requires-Dist` a reinstall of the requirer can resolve. Only a requirement +/// `uv` quotes is one. +const UV_CHECK_REQUIREMENT_INFIX: &str = " requires `"; + +/// Report the unsatisfied `Requires-Dist` entries in the environment at `venv_python`. +/// +/// An empty vector means every installed distribution's requirements are met. `uv pip +/// check` writes its findings to stderr and exits non-zero when it finds any, so a +/// non-zero exit with parsed findings is a successful check — but a non-zero exit with +/// nothing parsed is a failed invocation (missing interpreter, unusable `uv`) and is +/// surfaced as an error rather than mistaken for a clean environment. +pub fn check_dependencies( + paths: &AppPaths, + venv_python: &Path, +) -> Result> { + let uv = ensure_uv_binary(paths).context("failed to acquire uv binary for dependency check")?; + let output = Command::new(&uv) + .args(uv_pip_check_args(venv_python)) + .envs(uv_command_env(paths)) + .output() + .with_context(|| format!("failed to run {} pip check", uv.display()))?; + let stderr = String::from_utf8_lossy(&output.stderr); + let violations = parse_dependency_violations(&stderr); + if output.status.success() { + return Ok(Vec::new()); + } + if violations.is_empty() { + bail!( + "`uv pip check` failed for {}: {}", + venv_python.display(), + stderr.trim() + ); + } + Ok(violations) +} + +/// The violations whose requirer is `distribution`, matched case-insensitively. +/// +/// Environments that host an inference engine routinely carry unrelated upstream +/// conflicts between third-party packages. Callers filter to the distribution they own +/// so those do not bury the one finding they can act on. +pub fn violations_requiring<'a>( + violations: &'a [DependencyViolation], + distribution: &str, +) -> Vec<&'a DependencyViolation> { + violations + .iter() + .filter(|violation| violation.requiring.eq_ignore_ascii_case(distribution)) + .collect() +} + +/// Pull the unsatisfied-requirement lines out of a `uv pip check` stderr body. +/// +/// `uv` frames one as ``The package `` requires ``, but `` is +/// installed`` (or `` but it's not installed``), among progress lines (`Using Python +/// ...`, `Checked N packages ...`, `Found N incompatibilities`) and reports of other +/// conditions that share the opening frame. Both halves of the frame are required, so +/// neither a change in the surrounding chatter nor a differently-shaped `uv` diagnostic +/// can invent a violation. +fn parse_dependency_violations(stderr: &str) -> Vec { + stderr + .lines() + .map(str::trim) + .filter_map(|line| { + let requirer = line.strip_prefix(UV_CHECK_VIOLATION_PREFIX)?; + let (requirer, rest) = requirer.split_once('`')?; + rest.starts_with(UV_CHECK_REQUIREMENT_INFIX) + .then(|| DependencyViolation { + requiring: requirer.to_owned(), + detail: line.to_owned(), + }) + }) + .collect() +} + /// Ensure a usable `uv` binary is available, downloading and caching one if needed. /// Returns the path to the executable. pub fn ensure_uv_binary(paths: &AppPaths) -> Result { @@ -461,6 +579,139 @@ mod tests { ); } + #[test] + fn pip_check_args_target_venv_python_and_disable_color() { + let args = uv_pip_check_args(Path::new("/envs/run/bin/python")); + assert_eq!( + args, + vec![ + "pip", + "check", + "--color", + "never", + "--python", + "/envs/run/bin/python" + ], + "the output is parsed, so color must be off even under FORCE_COLOR" + ); + } + + #[test] + fn a_colorized_finding_is_never_silently_dropped() { + // `--color never` is what keeps this out of the real output, but the parser must + // not be the only thing standing between a colorized line and a false all-clear: + // an escape-prefixed finding must not parse as a clean environment. + let stderr = "\u{1b}[1mThe package `vllm` requires `torch==2.10.0+git8514f05`, but `2.9.1` is installed\u{1b}[0m\n"; + + assert!( + parse_dependency_violations(stderr).is_empty(), + "a colorized line does not parse — check_dependencies reports the non-zero \ + exit as a failed invocation rather than a clean environment" + ); + } + + #[test] + fn an_absent_requirement_is_a_violation() { + // Verbatim `uv pip check` output for a dependency that is missing outright + // rather than present at the wrong version — also an unsatisfied Requires-Dist. + let stderr = + "The package `vllm` requires `torch==2.10.0+git8514f05`, but it's not installed\n"; + + let violations = parse_dependency_violations(stderr); + + assert_eq!(violations.len(), 1, "{violations:?}"); + assert_eq!(violations[0].requiring, "vllm"); + } + + #[test] + fn other_uv_diagnostics_sharing_the_frame_are_not_violations() { + // Verbatim shapes `uv pip check` emits under the same `The package `x`` opening. + // None is a Requires-Dist a reinstall of the requirer would resolve, so treating + // them as violations would force a futile multi-gigabyte reinstall and print a + // remedy that cannot clear the condition. + let stderr = "\ +The package `vllm` is broken or incomplete (unable to read `WHEEL` file). Consider recreating the virtualenv, or removing the package directory at: /rocm/site-packages/vllm-0.26.0.dist-info. +The package `vllm` is broken or incomplete (unable to read `METADATA`). Consider recreating the virtualenv, or removing the package directory at: /rocm/site-packages/vllm-0.26.0.dist-info. +The package `vllm` requires Python >=3.99, but `3.12.9` is installed +The package `vllm` has multiple installed distributions: /rocm/site-packages/vllm-0.26.0.dist-info +"; + + assert!( + parse_dependency_violations(stderr).is_empty(), + "{:?}", + parse_dependency_violations(stderr) + ); + } + + #[test] + fn dependency_violations_come_only_from_framed_findings() { + // Verbatim `uv pip check` stderr for an environment where the SDK torch stack + // was written over the engine's pinned torch, plus an unrelated + // upstream conflict of the kind these environments routinely carry. + let stderr = "\ +Using Python 3.12.9 environment at: /rocm/runtimes/wheel/nightly-gfx94x +Checked 251 packages in 12ms +Found 2 incompatibilities +The package `vllm` requires `torch==2.10.0+git8514f05`, but `2.9.1+rocm7.14.0a20260611` is installed +The package `tilelang` requires `cloudpickle>=3.0`, but `2.2.1` is installed +"; + + let violations = parse_dependency_violations(stderr); + + assert_eq!(violations.len(), 2, "{violations:?}"); + assert_eq!(violations[0].requiring, "vllm"); + assert!( + violations[0].detail.contains("torch==2.10.0+git8514f05"), + "the requirement and the installed version are kept verbatim: {}", + violations[0].detail + ); + assert_eq!(violations[1].requiring, "tilelang"); + } + + #[test] + fn a_compatible_environment_reports_no_violations() { + let stderr = "\ +Using Python 3.12.9 environment at: /rocm/runtimes/wheel/nightly-gfx94x +Checked 251 packages in 12ms +All installed packages are compatible +"; + + assert!(parse_dependency_violations(stderr).is_empty()); + assert!(parse_dependency_violations("").is_empty()); + } + + #[test] + fn unframed_output_never_invents_a_violation() { + // An invocation failure (bad interpreter) must not read as a finding; the + // caller distinguishes it from a clean environment by the empty result. + let stderr = "error: Failed to inspect Python interpreter from /nope/bin/python\n"; + + assert!(parse_dependency_violations(stderr).is_empty()); + } + + #[test] + fn violations_are_filtered_to_the_distribution_that_owns_them() { + let violations = vec![ + DependencyViolation { + requiring: "vLLM".to_owned(), + detail: "The package `vLLM` requires `torch==2.10.0`, but `2.9.1` is installed" + .to_owned(), + }, + DependencyViolation { + requiring: "tilelang".to_owned(), + detail: + "The package `tilelang` requires `cloudpickle>=3.0`, but `2.2.1` is installed" + .to_owned(), + }, + ]; + + let owned = violations_requiring(&violations, "vllm"); + + assert_eq!(owned.len(), 1, "match is case-insensitive: {owned:?}"); + assert_eq!(owned[0].requiring, "vLLM"); + assert!(violations_requiring(&violations, "torch").is_empty()); + } + #[test] fn pip_install_base_targets_venv_python() { let args = uv_pip_install_base(Path::new("/envs/run/bin/python")); diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index 25d61c32c..d4a330c70 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -5,8 +5,9 @@ use anyhow::{Context, Result, anyhow, bail}; use clap::{Parser, Subcommand}; use rocm_core::{ - AppPaths, DEFAULT_LOCAL_PORT, ensure_uv_binary, format_http_base_url, - openai_models_endpoint_has_model, require_nonempty, uv_command_env, uv_pip_install_base, + 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, }; use rocm_engine_protocol::{ DEFAULT_LOG_TAIL_LINES, DetectRequest, DetectResponse, DevicePolicy, @@ -406,22 +407,47 @@ fn capabilities() -> EngineCapabilities { } fn install_response(request: InstallRequest) -> Result { - let already_installed = if request.reinstall { + let resolved = if request.reinstall { None } else { resolve_vllm_runtime(Some(&request.runtime_id)).ok() }; + // A resolvable vLLM is not necessarily a usable one. `rocm install sdk` writes the + // TheRock torch stack into the same environment vLLM lives in, so a second run + // replaces the torch build vLLM pins without touching vLLM itself. + // Short-circuiting on "vllm resolves" left that environment unrepaired; short-circuit + // on "vllm's own requirements are met" instead, so the install below restores them. + let (already_installed, repair, assessed) = match resolved { + Some(runtime) => { + let repair = assess_runtime_repair(&runtime); + if repair.needed { + (None, repair, assessed_python_for_repair(&runtime)) + } else { + (Some(runtime), repair, None) + } + } + None => (None, RepairAssessment::default(), None), + }; let runtime = if let Some(runtime) = already_installed { runtime } else { - let managed = resolve_managed_runtime_python(Some(&request.runtime_id))?.with_context( - || { - format!( - "runtime `{}` did not resolve to a managed TheRock Python environment for automatic vLLM install", - request.runtime_id - ) - }, - )?; + // A repair installs into the environment that was *assessed*. The two resolvers + // do not agree: `resolve_vllm_runtime` walks the candidates until one actually + // has vLLM beside it, while `resolve_managed_runtime_python` takes the first + // candidate unconditionally — and `runtime_id` matches by prefix, so several + // candidates routinely qualify. Installing into a different interpreter than the + // one found broken would leave the broken one broken and report it fixed. + let managed = match assessed { + Some(assessed) => assessed, + None => resolve_managed_runtime_python(Some(&request.runtime_id))?.with_context( + || { + format!( + "runtime `{}` did not resolve to a managed TheRock Python environment for automatic vLLM install", + request.runtime_id + ) + }, + )?, + }; install_vllm_with_uv(&managed.python_executable, request.reinstall)?; resolve_vllm_runtime(Some(&managed.runtime_id)).with_context(|| { format!( @@ -457,10 +483,90 @@ fn install_response(request: InstallRequest) -> Result { )], capabilities: capabilities(), lock_hash: runtime_lock_hash(&runtime), - warnings: vllm_runtime_warnings(&runtime), + warnings: repair + .notes + .into_iter() + .chain(vllm_runtime_warnings(&runtime)) + .collect(), }) } +/// Whether a resolvable vLLM environment still needs an install pass, and what to tell +/// the user about why. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +struct RepairAssessment { + /// The environment holds vLLM but not the dependencies vLLM declares, so the + /// install must run even though `reinstall` was not requested. + needed: bool, + /// Findings to surface with the install response. + notes: Vec, +} + +/// The environment a repair must target: the one that was assessed, named by its own +/// runtime id rather than by the (possibly prefix-matching) id the caller requested. +fn assessed_python_for_repair(runtime: &VllmRuntime) -> Option { + Some(ManagedRuntimePython { + runtime_id: runtime.runtime_id.clone(), + python_executable: runtime.python_executable.clone()?, + }) +} + +/// Decide whether an already-resolvable vLLM environment must be reinstalled. +/// +/// Only managed environments are assessed. An external environment belongs to the user: +/// rocm-cli reports on it but does not rewrite its packages, which is the very failure +/// mode this check exists to catch. +fn assess_runtime_repair(runtime: &VllmRuntime) -> RepairAssessment { + if !runtime_is_managed(runtime) { + return RepairAssessment::default(); + } + let Some(python) = runtime.python_executable.as_ref() else { + return RepairAssessment::default(); + }; + let paths = match AppPaths::discover() { + Ok(paths) => paths, + Err(error) => return unverified_repair(&error.to_string()), + }; + match check_dependencies(&paths, python) { + Ok(violations) => repair_from_violations(&violations), + // 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 repair decision for a set of violations found in the environment. +fn repair_from_violations(violations: &[DependencyViolation]) -> RepairAssessment { + let owned = violations_requiring(violations, ENGINE_NAME); + if owned.is_empty() { + return RepairAssessment::default(); + } + let mut notes = vec![format!( + "the runtime environment did not satisfy vLLM's pinned dependencies; vLLM was reinstalled to restore them ({})", + owned + .iter() + .map(|violation| violation.detail.as_str()) + .collect::>() + .join("; ") + )]; + 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, + } +} + +fn unverified_repair(reason: &str) -> RepairAssessment { + RepairAssessment { + needed: false, + notes: vec![format!( + "vLLM's pinned dependencies could not be verified in this environment: {reason}" + )], + } +} + fn runtime_is_managed(runtime: &VllmRuntime) -> bool { runtime.source.starts_with("managed_runtime_manifest") } @@ -2403,6 +2509,102 @@ mod tests { resolve_vllm_install_target(index.map(ToOwned::to_owned)) } + fn violation(requiring: &str, detail: &str) -> DependencyViolation { + DependencyViolation { + requiring: requiring.to_owned(), + detail: detail.to_owned(), + } + } + + #[test] + fn a_repair_targets_the_environment_that_was_assessed() { + // `resolve_managed_runtime_python` returns the first candidate unconditionally, + // while the assessment walked on to the candidate that actually has vLLM. Both + // match when `runtime_id` matching is by prefix, so the repair must follow the + // assessed environment or it fixes an interpreter nobody found broken. + let assessed = VllmRuntime { + runtime_id: "nightly-wheel-gfx94x-dcgpu-7-14-0a20260611".to_owned(), + env_id: "external-vllm-therock".to_owned(), + command: PathBuf::from("/rocm/runtimes/wheel/nightly-gfx94x/bin/vllm"), + python_executable: Some(PathBuf::from( + "/rocm/runtimes/wheel/nightly-gfx94x/bin/python", + )), + version: Some("0.26.0".to_owned()), + source: "managed_runtime_manifest:nightly-wheel-gfx94x-dcgpu".to_owned(), + sdk_root: None, + sdk_bin: None, + sdk_bin_paths: Vec::new(), + sdk_library_paths: Vec::new(), + }; + + let target = assessed_python_for_repair(&assessed).expect("a managed runtime has a python"); + + assert_eq!( + target.python_executable, + PathBuf::from("/rocm/runtimes/wheel/nightly-gfx94x/bin/python") + ); + assert_eq!( + target.runtime_id, "nightly-wheel-gfx94x-dcgpu-7-14-0a20260611", + "the assessed runtime's own id, not the prefix the caller asked for" + ); + } + + #[test] + fn a_consistent_environment_is_not_reinstalled() { + assert_eq!(repair_from_violations(&[]), 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", + )]); + + assert!(assessment.needed); + assert!( + assessment + .notes + .iter() + .any(|note| note.contains("2.10.0+git8514f05")), + "the reported note names the pin that was violated: {:?}", + assessment.notes + ); + } + + #[test] + 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", + ), + ]); + + assert_eq!(assessment, RepairAssessment::default()); + } + + #[test] + fn an_unrunnable_check_reports_itself_without_forcing_a_reinstall() { + let assessment = unverified_repair("uv binary is unavailable"); + + assert!(!assessment.needed); + assert_eq!(assessment.notes.len(), 1); + assert!( + assessment.notes[0].contains("could not be verified"), + "{:?}", + assessment.notes + ); + } + #[test] fn vllm_extra_index_url_defaults_to_const() { assert_eq!( diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index e81eb8fac..643787e62 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -19,6 +19,20 @@ 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. + @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 + 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 + And the engine is still ready to serve + # Linux-only: the step adopts a standard `/opt/rocm` install with a Unix python # path. On Windows those paths don't exist (the CLI resolves `/usr/bin/python3` # to a bogus `C:/usr/bin/python3` and errors on the missing path before it can diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index cf678b565..a2ba669a3 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -47,12 +47,58 @@ async fn setup_active_runtime(world: &mut E2eWorld) { ); } +#[given("a managed runtime with an inference engine already installed")] +async fn setup_runtime_with_engine(world: &mut E2eWorld) { + // Share the runtimes tree for the same reason `a managed runtime is active` does: + // the first scenario to find it empty pays for the multi-GiB SDK pull, the rest + // reuse it. `install sdk` auto-installs the family's preferred engine, so one + // install satisfies both halves of this precondition. + world.use_shared_runtimes(); + let (stdout, _, _) = crate::run_rocm(world, &["runtimes", "list"]); + if stdout.contains("installed: none") { + crate::run_rocm_ok(world, &["install", "sdk"]); + } + assert_engine_ready(world); +} + #[when("the user installs the SDK")] async fn user_installs_sdk(world: &mut E2eWorld) { let stdout = crate::run_rocm_ok(world, &["install", "sdk"]); world.cli_output = Some(stdout); } +#[when("the user installs the SDK again")] +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) { + let output = world.cli_output.as_deref().expect("no install output"); + assert!( + !output.contains("dependency_check: violated"), + "the reinstall left the engine's declared requirements unmet:\n{output}" + ); + assert!( + output.contains("dependency_check: satisfied"), + "the install did not report on the engine's requirements at all:\n{output}" + ); +} + +#[then("the engine is still ready to serve")] +async fn assert_engine_still_ready(world: &mut E2eWorld) { + assert_engine_ready(world); +} + +/// The engine inventory reports a usable engine runtime. +fn assert_engine_ready(world: &mut E2eWorld) { + let (stdout, _, _) = crate::run_rocm(world, &["engines", "list"]); + assert!( + stdout.contains("runtime: ready"), + "no engine runtime is ready:\n{stdout}" + ); +} + #[when("the user tries to adopt the existing install")] async fn user_tries_adopt(world: &mut E2eWorld) { let (stdout, stderr, rc) = crate::run_rocm( From 62f6d736fe04d80ed9614ee134bc59a4cea1b937 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Mon, 24 Aug 2026 11:25:36 +0000 Subject: [PATCH 2/2] fix(vllm): target the assessed environment on a forced reinstall MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups, all three from the same reading of the repair path. A forced reinstall skipped resolution entirely, so no environment was ever assessed and the install fell back to the first prefix-matching candidate. On the multi-candidate hosts that fallback exists for, `--reinstall` could rebuild a healthy environment and report success while the broken one stayed broken — the exact failure the assessed-interpreter comment warns about, reachable through the remedy the tool prints. Resolution now always runs; only the short-circuit is gated on `reinstall`. Report one finding per violated pin. The real failure moves torch, torchvision and torchaudio together, so joining them produced a single ~380-character line no terminal shows usefully. This matches the per-finding lines the CLI-side renderer already emits, and the new test uses a real three-package body rather than the single-pin fixture. Drop the "engine is still ready to serve" assertion. It checked that `engines list` reports ready — the surface that stays green while the runtime is broken, which is what this scenario exists to catch — so it would have passed before the fix as readily as after. The remaining dependency-check assertion is the falsifiable one; the precondition helper keeps its use and now records why it has no Then counterpart. Signed-off-by: Eugene Volen --- engines/vllm/src/lib.rs | 78 ++++++++++++++++--- .../features/runtime_setup.feature | 1 - tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 13 ++-- 3 files changed, 74 insertions(+), 18 deletions(-) diff --git a/engines/vllm/src/lib.rs b/engines/vllm/src/lib.rs index d4a330c70..7971d2b4f 100644 --- a/engines/vllm/src/lib.rs +++ b/engines/vllm/src/lib.rs @@ -407,11 +407,13 @@ fn capabilities() -> EngineCapabilities { } fn install_response(request: InstallRequest) -> Result { - let resolved = if request.reinstall { - None - } else { - resolve_vllm_runtime(Some(&request.runtime_id)).ok() - }; + // Resolve regardless of `reinstall`. Resolution answers *which* interpreter holds + // vLLM, and a forced reinstall needs that answer just as much as a repair does — + // gating it on `reinstall` left `--reinstall` with no assessed environment, so it + // fell back to `resolve_managed_runtime_python` (first prefix-matching candidate) + // and could reinstall a healthy environment while leaving the broken one broken. + // Only the short-circuit below is gated on `reinstall`. + let resolved = resolve_vllm_runtime(Some(&request.runtime_id)).ok(); // A resolvable vLLM is not necessarily a usable one. `rocm install sdk` writes the // TheRock torch stack into the same environment vLLM lives in, so a second run // replaces the torch build vLLM pins without touching vLLM itself. @@ -420,7 +422,7 @@ fn install_response(request: InstallRequest) -> Result { let (already_installed, repair, assessed) = match resolved { Some(runtime) => { let repair = assess_runtime_repair(&runtime); - if repair.needed { + if request.reinstall || repair.needed { (None, repair, assessed_python_for_repair(&runtime)) } else { (Some(runtime), repair, None) @@ -541,14 +543,19 @@ fn repair_from_violations(violations: &[DependencyViolation]) -> RepairAssessmen if owned.is_empty() { return RepairAssessment::default(); } - let mut notes = vec![format!( - "the runtime environment did not satisfy vLLM's pinned dependencies; vLLM was reinstalled to restore them ({})", + let mut notes = vec![ + "the runtime environment did not satisfy vLLM's pinned dependencies; vLLM was reinstalled to restore them".to_owned(), + ]; + // One note per violation rather than one joined line. The real failure is the whole + // torch stack — torch, torchvision and torchaudio move together when the SDK writes + // over the engine's pins — so joining them produced a single ~380-character line that + // is unreadable in a terminal. Mirrors the per-finding `violation:` lines the + // CLI-side renderer already emits. + notes.extend( owned .iter() - .map(|violation| violation.detail.as_str()) - .collect::>() - .join("; ") - )]; + .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(), ); @@ -2574,6 +2581,53 @@ 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( + "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", + ), + ]); + + assert!(assessment.needed); + let violation_notes: Vec<&String> = assessment + .notes + .iter() + .filter(|note| note.starts_with("violation: ")) + .collect(); + assert_eq!( + violation_notes.len(), + 3, + "every violated pin gets its own note: {:?}", + assessment.notes + ); + for package in ["torch==", "torchvision==", "torchaudio=="] { + assert!( + violation_notes.iter().any(|note| note.contains(package)), + "{package} is missing from the reported notes: {:?}", + assessment.notes + ); + } + assert!( + assessment.notes.iter().all(|note| note.len() < 200), + "no note should be a wall of joined findings: {:?}", + assessment.notes + ); + } + #[test] fn unrelated_upstream_conflicts_do_not_force_a_reinstall() { // These environments routinely carry conflicts between third-party packages. diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 643787e62..61d837de7 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -31,7 +31,6 @@ Feature: Runtime configuration 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 - And the engine is still ready to serve # Linux-only: the step adopts a standard `/opt/rocm` install with a Unix python # path. On Windows those paths don't exist (the CLI resolves `/usr/bin/python3` diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index a2ba669a3..6f22a8c66 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -85,12 +85,15 @@ async fn assert_engine_requirements_satisfied(world: &mut E2eWorld) { ); } -#[then("the engine is still ready to serve")] -async fn assert_engine_still_ready(world: &mut E2eWorld) { - assert_engine_ready(world); -} - /// The engine inventory reports a usable engine runtime. +/// +/// A precondition only. It deliberately has no Then counterpart: `engines list` +/// reports `runtime: ready` even while the engine's pinned dependencies are +/// 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. fn assert_engine_ready(world: &mut E2eWorld) { let (stdout, _, _) = crate::run_rocm(world, &["engines", "list"]); assert!(