From 31d9e835c618aefdef99b92ae823195cc82995bf Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Mon, 3 Aug 2026 11:42:43 +0000 Subject: [PATCH 01/17] feat(install): make the ROCm compiler toolchain opt-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every wheel SDK install pulled in the devel extra — headers, static libraries, and the full LLVM toolchain — whether or not the user would ever build GPU code. It is roughly half the download (1.2-1.5 GiB compressed, depending on target) and nothing in this repository needs it at run time: the SDK probe already tolerates its absence and has a test covering that path. Default to rocm[libraries] and add --devel for people who do build against ROCm. Record the choice in the runtime manifest so an update reinstalls what the user picked rather than silently adding or dropping the toolchain. Manifests written before this change have no such field and were all toolchain installs, so a missing field reads back as present. Group the install arguments into SdkInstallRequest, which keeps the argument count within the clippy threshold and makes the call sites readable. Closes #164 Signed-off-by: Roman Inflianskas --- apps/rocm/src/comfyui.rs | 3 + apps/rocm/src/main.rs | 52 ++++++--- apps/rocm/src/storage.rs | 1 + apps/rocm/src/therock.rs | 224 +++++++++++++++++++++++++++++++++------ docs/manual-testing.md | 12 ++- docs/testing.md | 3 +- docs/wsl.md | 2 +- 7 files changed, 244 insertions(+), 53 deletions(-) diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 2f45da87f..c2ba98100 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -2353,6 +2353,7 @@ mod tests { wheel_composition: None, read_only: false, imported_from: None, + devel: true, installed_at_unix_ms: 100, }; @@ -2818,6 +2819,7 @@ mod tests { wheel_composition: None, read_only: false, imported_from: None, + devel: true, installed_at_unix_ms: 100, }; @@ -2917,6 +2919,7 @@ mod tests { wheel_composition: None, read_only: false, imported_from: None, + devel: true, installed_at_unix_ms: 100, }) } diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 3377ee51c..215f762e8 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -621,6 +621,9 @@ rocm install sdk --family gfx110X-all --dry-run")] /// TheRock GPU package family to install, such as gfx110X-all. #[arg(long)] family: Option, + /// Also install the ROCm compiler and headers, for building GPU code. + #[arg(long)] + devel: bool, /// Resolve the install plan without changing files. #[arg(long)] dry_run: bool, @@ -2882,6 +2885,7 @@ fn install(target: InstallTarget) -> Result<()> { channel, format, prefix, + devel, version, build_date, family, @@ -2904,13 +2908,16 @@ fn install(target: InstallTarget) -> Result<()> { .map_or_else(|| "".to_owned(), |path| path.display().to_string()); match therock::install_sdk( &paths, - &channel, - format_name, - prefix, - version_selector, - family.as_deref(), - dry_run, - consents.replace_active_default, + therock::SdkInstallRequest { + channel: &channel, + format: format_name, + prefix, + version_selector, + family_override: family.as_deref(), + dry_run, + include_devel: devel, + consent: consents.replace_active_default, + }, ) { Ok(result) => { let therock::SdkInstallResult { output, mutated } = result; @@ -11223,6 +11230,9 @@ fn adopt_runtime_from_probe( python_launcher: None, python_executable: Some(python_executable.display().to_string()), pip_cache_dir: None, + // The probe only reports a CMake path when the `devel` packages are + // present, so it tells us what this pre-existing environment has. + devel: probe.cmake_path.is_some(), rocm_sdk: Some(probe), // Adoption does not install torch, so the build is derived from the SDK // version instead. @@ -15276,21 +15286,25 @@ fn render_install_sdk_dry_run_for_args(paths: &AppPaths, args: &[String]) -> Res let version = chat_cli_arg_value(args, "--version").map(str::to_owned); let build_date = chat_cli_arg_value(args, "--build-date").map(str::to_owned); let selector = therock_install_version_selector(version, build_date)?; + let devel = args.iter().any(|arg| arg == "--devel"); // Dry run, so nothing is displaced and the consent gate is never reached; // the narrow consent is what this chat surface would pass for a real // install, and passing `--yes`'s source here would be a lie waiting to be // printed if the preview ever grew a gate. Ok(therock::install_sdk( paths, - channel, - format, - prefix, - selector, - None, - true, - therock::SdkInstallConsent::Preapproved( - therock::SdkInstallApprovalSource::ApproveReplacingActiveDefault, - ), + therock::SdkInstallRequest { + channel, + format, + prefix, + version_selector: selector, + dry_run: true, + include_devel: devel, + consent: therock::SdkInstallConsent::Preapproved( + therock::SdkInstallApprovalSource::ApproveReplacingActiveDefault, + ), + ..therock::SdkInstallRequest::default() + }, )? .output) } @@ -18096,6 +18110,7 @@ fn apply_runtime_update( &source.family, plan.device_target.as_deref(), plan.source_layout_generation.as_deref(), + source.includes_devel(), true, activate, )?; @@ -18118,6 +18133,9 @@ fn apply_runtime_update( &source.family, plan.device_target.as_deref(), plan.source_layout_generation.as_deref(), + // Reinstall what the user originally chose rather than silently + // adding or dropping the compiler toolchain on update. + source.includes_devel(), false, activate, )?; @@ -35694,6 +35712,7 @@ ID_LIKE="suse opensuse" wheel_composition: None, read_only: false, imported_from: None, + devel: true, installed_at_unix_ms, }; fs::create_dir_all(runtime_registry_dir(paths))?; @@ -35735,6 +35754,7 @@ ID_LIKE="suse opensuse" wheel_composition: None, read_only: false, imported_from: None, + devel: true, installed_at_unix_ms: 1, } } diff --git a/apps/rocm/src/storage.rs b/apps/rocm/src/storage.rs index c70096c1f..075e062b7 100644 --- a/apps/rocm/src/storage.rs +++ b/apps/rocm/src/storage.rs @@ -980,6 +980,7 @@ mod tests { wheel_composition: None, read_only: false, imported_from: None, + devel: true, installed_at_unix_ms, } } diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index b3dec8595..e9ceff931 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -736,10 +736,33 @@ pub(crate) struct InstalledRuntimeManifest { pub read_only: bool, #[serde(default)] pub imported_from: Option, + /// Whether the compiler toolchain (`devel`) is part of this install, so an + /// update reinstalls the same thing the user originally chose. + #[serde(default = "devel_default_for_legacy_manifest")] + pub devel: bool, pub installed_at_unix_ms: u128, } +/// Manifests written before `devel` became opt-in always included the +/// toolchain, so a missing field means it is present. Defaulting to `false` +/// here would silently strip it on the next update. +const fn devel_default_for_legacy_manifest() -> bool { + true +} + impl InstalledRuntimeManifest { + /// Whether this runtime has the compiler toolchain. + /// + /// Prefers the recorded `wheel_composition`, because those specs are what + /// `uv` was actually given — so the answer cannot drift from the install + /// the way a separately-written flag can. `devel` is the fallback for + /// tarball installs and for manifests written before compositions were + /// recorded; on a manifest older still it defaults to `true`, since every + /// install predating the flag shipped the toolchain. + pub(crate) fn includes_devel(&self) -> bool { + wheel_composition_includes_devel(self.wheel_composition.as_ref()).unwrap_or(self.devel) + } + fn normalize_host_paths(mut self) -> Self { self.install_root = normalize_manifest_path(self.install_root); self.python_launcher = self @@ -959,25 +982,63 @@ struct InstallSourceOverride<'a> { layout: Option, } -/// Install a TheRock SDK runtime. +/// What to install, as resolved from the `rocm install sdk` arguments. /// -/// `consent` carries the *source* of any up-front approval rather than a bare -/// bool, because the progress line names it: `--yes` and -/// `--approve-replacing-active-default` both clear this gate, but only the -/// former also approves a `sudo` system-package install, so collapsing them -/// would print "Approved by --yes" on every install ROCm CLI's own -/// terminal-less surfaces make. -#[allow(clippy::too_many_arguments)] +/// A struct rather than a parameter list: with `include_devel` added this is +/// past the point where positional `bool`s at a call site say anything about +/// what they mean. +#[derive(Debug)] +pub(crate) struct SdkInstallRequest<'a> { + pub channel: &'a str, + pub format: &'a str, + pub prefix: Option, + pub version_selector: Option, + pub family_override: Option<&'a str>, + pub dry_run: bool, + /// Install the compiler and headers alongside the runtime libraries. + pub include_devel: bool, + /// The *source* of any up-front approval rather than a bare bool, because + /// the progress line names it: `--yes` and + /// `--approve-replacing-active-default` both clear this gate, but only the + /// former also approves a `sudo` system-package install, so collapsing them + /// would print "Approved by --yes" on every install ROCm CLI's own + /// terminal-less surfaces make. + pub consent: SdkInstallConsent, +} + +impl Default for SdkInstallRequest<'_> { + fn default() -> Self { + Self { + channel: "", + format: "", + prefix: None, + version_selector: None, + family_override: None, + dry_run: false, + include_devel: false, + // Withholding consent is the safe default: a caller that leaves it + // unset has granted nothing, so the gate prompts or refuses rather + // than silently displacing the active default runtime. + consent: SdkInstallConsent::Ask, + } + } +} + +/// Install a TheRock SDK runtime. pub(crate) fn install_sdk( paths: &AppPaths, - channel: &str, - format: &str, - prefix: Option, - version_selector: Option, - family_override: Option<&str>, - dry_run: bool, - consent: SdkInstallConsent, + request: SdkInstallRequest<'_>, ) -> Result { + let SdkInstallRequest { + channel, + format, + prefix, + version_selector, + family_override, + dry_run, + include_devel, + consent, + } = request; let channel = TheRockChannel::parse(channel)?; ensure_install_format_supported(format)?; match format { @@ -991,6 +1052,7 @@ pub(crate) fn install_sdk( }, version_selector.as_ref(), dry_run, + include_devel, consent, ), "tarball" => { @@ -1042,6 +1104,7 @@ pub(crate) fn install_sdk_for_update( family: &str, device_target: Option<&str>, source_layout_generation: Option<&str>, + include_devel: bool, dry_run: bool, activate_after_install: bool, ) -> Result { @@ -1063,6 +1126,7 @@ pub(crate) fn install_sdk_for_update( }, None, dry_run, + include_devel, consent, ), "tarball" => install_tarball_runtime( @@ -1456,7 +1520,11 @@ fn resolve_latest_for_manifest( // this host could not perform. let wheel_composition = match &device_target { AggregateDeviceTarget::Exact(_) => { - Some(wheel_runtime_composition(&resolution, &device_target)) + Some(wheel_runtime_composition( + &resolution, + &device_target, + manifest.includes_devel(), + )) } AggregateDeviceTarget::Undetermined(_) => None, }; @@ -1640,6 +1708,7 @@ fn install_wheel_runtime( source_override: InstallSourceOverride<'_>, version_selector: Option<&RuntimeVersionSelector>, dry_run: bool, + include_devel: bool, consent: SdkInstallConsent, ) -> Result { let InstallSourceOverride { @@ -1704,7 +1773,7 @@ fn install_wheel_runtime( // usable target still composes a key here — from the `` extras // — which no real install can ever produce, and the refusal below stops it // from reaching a manifest. - let wheel_composition = wheel_runtime_composition(&resolution, &device_target); + let wheel_composition = wheel_runtime_composition(&resolution, &device_target, include_devel); progress_line(format!( "Found canonical TheRock aggregate version {} with a matching PyTorch stack for target family {}.", resolution.latest_version, resolution.family @@ -1791,6 +1860,7 @@ fn install_wheel_runtime( let _ = writeln!( output, " package_policy: resolve the pinned target-complete rocm, torch, torchvision, and torchaudio plan from published package metadata, then install it in one uv transaction" + ); let no_wheel_warning = repo_version_without_wheels( resolution.newest_repo_version.as_deref(), @@ -1954,7 +2024,11 @@ fn install_wheel_runtime( .map(String::as_str) .collect::>() .as_slice(), - "install TheRock devel SDK, torch stack, and resolved dependencies", + if include_devel { + "install TheRock SDK with the compiler toolchain, torch stack, and resolved dependencies" + } else { + "install TheRock SDK, torch stack, and resolved dependencies" + }, )?; progress_line("Checking the installed ROCm SDK..."); @@ -1987,6 +2061,7 @@ fn install_wheel_runtime( wheel_composition: Some(wheel_composition), read_only: false, imported_from: None, + devel: include_devel, installed_at_unix_ms: unix_time_millis(), }; save_runtime_manifest(paths, &manifest)?; @@ -2020,16 +2095,27 @@ fn install_wheel_runtime( Ok(SdkInstallResult::installed(output)) } +/// Package specs for a wheel SDK install. +/// +/// `devel` adds the compiler, headers, and static libraries — roughly doubling +/// the download — and is only needed to *build* GPU code. Running models needs +/// `libraries` alone, so the toolchain is installed only when asked for. +/// +/// The `device-` extra is separate and always present: it selects which +/// GPU payload the wheels carry, not whether the toolchain comes with them. fn therock_pip_package_specs( package_versions: &TheRockPipPackageVersions, device_target: &str, + include_devel: bool, ) -> Vec { let device_extra = format!("device-{device_target}"); + let rocm_extras = if include_devel { + format!("libraries,devel,{device_extra}") + } else { + format!("libraries,{device_extra}") + }; vec![ - format!( - "rocm[libraries,devel,{device_extra}]=={}", - package_versions.rocm - ), + format!("rocm[{rocm_extras}]=={}", package_versions.rocm), format!("torch[{device_extra}]=={}", package_versions.torch), format!( "torchvision[{device_extra}]=={}", @@ -2045,18 +2131,36 @@ fn therock_pip_package_specs( fn wheel_runtime_composition( resolution: &PipRuntimeResolution, device_target: &AggregateDeviceTarget, + include_devel: bool, ) -> WheelRuntimeComposition { WheelRuntimeComposition { source_layout_generation: resolution.layout.generation().to_owned(), package_specs: therock_pip_package_specs( &resolution.package_versions, device_target.as_str(), + include_devel, ), rocm_sdk_target: matches!(device_target, AggregateDeviceTarget::Exact(_)) .then(|| device_target.as_str().to_owned()), } } +/// Whether a recorded composition installed the compiler toolchain, read back +/// out of its `rocm[...]` extras. +/// +/// Same reasoning as [`wheel_composition_device_target`]: the specs are stored +/// verbatim, so the answer is derivable from what was actually installed and +/// cannot drift from a second field. `None` for a manifest with no recorded +/// composition — see [`InstalledRuntimeManifest::includes_devel`] for how that +/// legacy case is resolved. +fn wheel_composition_includes_devel(composition: Option<&WheelRuntimeComposition>) -> Option { + let composition = composition?; + composition.package_specs.iter().find_map(|spec| { + let extras = spec.strip_prefix("rocm[")?.split_once(']')?.0; + Some(extras.split(',').map(str::trim).any(|extra| extra == "devel")) + }) +} + /// The GFX target a recorded composition installed, read back out of its /// `rocm[...,device-]` requirement. /// @@ -2733,6 +2837,8 @@ fn install_tarball_runtime( wheel_composition: None, read_only: false, imported_from: None, + // Tarball artifacts ship the whole SDK; the extra is a wheel concept. + devel: true, installed_at_unix_ms: unix_time_millis(), }; save_runtime_manifest(paths, &manifest)?; @@ -7701,7 +7807,7 @@ mod tests { torchaudio: "2.10.0+rocm7.13.0a20260513".to_owned(), compatibility_key: "7.13.0a20260513".to_owned(), }; - let package_specs = therock_pip_package_specs(&package_versions, "gfx942"); + let package_specs = therock_pip_package_specs(&package_versions, "gfx942", true); assert_eq!( package_specs, @@ -7940,6 +8046,63 @@ mod tests { Ok(()) } + /// The default install skips the compiler toolchain, which is roughly half + /// the download and is only needed to build GPU code. The torch stack is + /// unaffected. + #[test] + fn pip_runtime_omits_devel_extra_by_default() { + let package_versions = TheRockPipPackageVersions { + rocm: "7.13.0a20260513".to_owned(), + torch: "2.10.0+rocm7.13.0a20260513".to_owned(), + torchvision: "0.25.0+rocm7.13.0a20260513".to_owned(), + torchaudio: "2.10.0+rocm7.13.0a20260513".to_owned(), + compatibility_key: "7.13.0a20260513".to_owned(), + }; + + let package_specs = therock_pip_package_specs(&package_versions, "gfx942", false); + + assert_eq!( + package_specs, + vec![ + "rocm[libraries,device-gfx942]==7.13.0a20260513".to_owned(), + "torch[device-gfx942]==2.10.0+rocm7.13.0a20260513".to_owned(), + "torchvision[device-gfx942]==0.25.0+rocm7.13.0a20260513".to_owned(), + "torchaudio==2.10.0+rocm7.13.0a20260513".to_owned(), + ] + ); + assert!( + !package_specs[0].contains("devel"), + "default install must not request the toolchain: {package_specs:?}" + ); + } + + /// A manifest written before `devel` became opt-in has no such field, and + /// those installs all had the toolchain. Reading one back must not claim + /// otherwise, or the next update would silently strip it. + #[test] + fn legacy_manifest_without_devel_field_is_treated_as_having_it() { + let json = r#"{ + "runtime_key": "release-wheel-gfx110X-all-7.13.0", + "runtime_id": "therock-release:gfx110X-all", + "channel": "release", + "format": "wheel", + "family": "gfx110X-all", + "family_source": "detected", + "version": "7.13.0", + "install_root": "/tmp/rocm-runtime", + "selected_artifact_url": "https://example.invalid/simple", + "installed_at_unix_ms": 1 + }"#; + + let manifest: InstalledRuntimeManifest = + serde_json::from_str(json).expect("legacy manifest should still parse"); + + assert!( + manifest.devel, + "a manifest predating the flag must be treated as a toolchain install" + ); + } + #[test] fn pip_runtime_selects_latest_common_rocm_suffix_not_latest_rocm_package() { let rocm_versions = vec![ @@ -9319,13 +9482,13 @@ echo Python 3.12.10 let error = install_sdk( &paths, - "release", - "tarball", - None, - None, - None, - true, - SdkInstallConsent::Preapproved(SdkInstallApprovalSource::AssumeYes), + SdkInstallRequest { + channel: "release", + format: "tarball", + dry_run: true, + consent: SdkInstallConsent::Preapproved(SdkInstallApprovalSource::AssumeYes), + ..SdkInstallRequest::default() + }, ) .unwrap_err() .to_string(); @@ -10174,6 +10337,7 @@ echo Python 3.12.10 wheel_composition: None, read_only: false, imported_from: None, + devel: true, installed_at_unix_ms, } } diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 6056696ac..52648b7e6 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -157,16 +157,18 @@ Expected result: - rocm-cli creates or reuses a rocm-cli managed Python venv. - pip installs pinned `rocm`, `torch`, and `torchvision` requirements with exactly one `device-` extra (`rocm` also requests - `libraries,devel`), alongside pinned `torchaudio` from the TheRock index. On a host - with no detectable AMD GPU the preview reports `device_target: undetermined` - and a real install refuses rather than pulling every published device payload. + `libraries`, and `devel` only when `--devel` is passed), alongside pinned + `torchaudio` from the TheRock index. On a host with no detectable AMD GPU the + preview reports `device_target: undetermined` and a real install refuses + rather than pulling every published device payload. - rocm-cli chooses the newest exact ROCm build suffix common to the SDK package and the PyTorch stack for the current Python/platform wheel tags, then pins all four packages in one pip transaction. - The install does not ask for an external Python venv. -- Runtime validation uses TheRock's runtime/devel package roots and +- Runtime validation uses TheRock's runtime package roots and `rocm_sdk.find_libraries`; `rocm-sdk path --root` is expected after the - pinned `rocm[libraries,devel,device-…]` install succeeds. + pinned `rocm[libraries,device-…]` install succeeds. The compiler toolchain is + not required for validation to pass; `--devel` adds `devel` to those extras. - `rocm examine` reports the active runtime as ready. Developer-only deterministic override: diff --git a/docs/testing.md b/docs/testing.md index e8c87b344..51082c52e 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -193,7 +193,8 @@ Then it verifies: pip creates it inside the ROCm folder when packages are downloaded - a single TheRock-index pip install plan for pinned `rocm`, `torch`, and `torchvision` requirements with exactly one `device-` - extra (`rocm` also requests `libraries,devel`), plus pinned `torchaudio` + extra (`rocm` also requests `libraries`; `--devel` adds `devel` to them), + plus pinned `torchaudio` - on a host with no detectable AMD GPU the preview reports `device_target: undetermined` and renders the device extra as a placeholder; a real install refuses rather than falling back to every published device payload diff --git a/docs/wsl.md b/docs/wsl.md index b22f1228d..d1f3cd6ab 100644 --- a/docs/wsl.md +++ b/docs/wsl.md @@ -152,7 +152,7 @@ created venv such as `D:\ROCm\venv`. The WSL activation environment for HIP applications that do not preload ROCm the way PyTorch does must include the managed TheRock runtime package paths and -WSL DXCore path. With the managed `rocm[libraries,devel]` install, these paths +WSL DXCore path. With the managed `rocm[libraries]` install, these paths come from the rocm-cli runtime manifest, `rocm-sdk path --root`, and `rocm_sdk.find_libraries(...)`. From f7c20ebcd3613a6d4abf9fe9c33c94c17dd23fae Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Wed, 12 Aug 2026 15:00:35 +0000 Subject: [PATCH 02/17] fix(install): name only the extras the install asked for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolution failures always read `rocm[libraries,devel]`, so anyone who never passed `--devel` was told a compiler toolchain could not be resolved — sending them after a problem they do not have. Thread the requested extras through the resolver and share one helper with the install plan and the progress text, so the three cannot drift again. The documented manual acceptance test asserted the devel spec while installing without `--devel`, so it failed on every run. Derive the expected spec from a new `--devel` passthrough, which also gives the opt-in path its own coverage. Building vLLM from source compiles HIP sources and so needs the toolchain, which docs/vllm.md recommended without mentioning the flag that now installs it. Signed-off-by: Roman Inflianskas --- apps/rocm/src/main.rs | 85 +++++++++++++++++++++++++++++ apps/rocm/src/therock.rs | 72 ++++++++++++++++++++---- docs/manual-testing.md | 3 +- docs/testing.md | 9 ++- docs/vllm.md | 5 ++ scripts/therock_sdk_install_test.py | 44 +++++++++++++-- 6 files changed, 199 insertions(+), 19 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 215f762e8..50f80836d 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -32416,6 +32416,13 @@ ID_LIKE="suse opensuse" ); assert!(!external_root.join(".rocm-cli-runtime.json").exists()); assert!(runtime_manifest_path(&paths, &adopted.runtime_key).is_file()); + // No CMake path in the probe means no compiler toolchain in that + // environment. `rocm update` reinstalls from this field, so recording + // it wrongly would add a toolchain the user never had. + assert!( + !adopted.devel, + "a probe without a CMake path must not claim the toolchain" + ); let mut config = RocmCliConfig::default(); activate_runtime(&paths, &mut config, &adopted.runtime_key)?; @@ -32432,6 +32439,84 @@ ID_LIKE="suse opensuse" Ok(()) } + /// The other half of the same derivation: an adopted environment that does + /// expose a CMake path has the toolchain, and the manifest must say so or + /// the next `rocm update` would quietly reinstall without it. + #[test] + fn runtime_adopt_records_devel_when_the_probe_finds_cmake() -> Result<()> { + let (root, paths) = test_paths("runtime-adopt-devel"); + let external_root = root.join("external-therock-venv"); + let scripts_dir = external_root.join(if cfg!(windows) { "Scripts" } else { "bin" }); + let python_executable = scripts_dir.join(if cfg!(windows) { + "python.exe" + } else { + "python" + }); + let sdk_root = external_root + .join("Lib") + .join("site-packages") + .join("rocm_sdk"); + let sdk_bin = sdk_root.join("bin"); + let cmake_path = sdk_root.join("lib").join("cmake"); + fs::create_dir_all(&scripts_dir)?; + fs::create_dir_all(&sdk_bin)?; + fs::create_dir_all(&cmake_path)?; + let amdhip = sdk_bin.join(if cfg!(windows) { + "amdhip64_7.dll" + } else { + "libamdhip64.so" + }); + let hipblas = sdk_bin.join(if cfg!(windows) { + "hipblas.dll" + } else { + "libhipblas.so" + }); + fs::write(&python_executable, "python")?; + fs::write(&amdhip, "amdhip")?; + fs::write(&hipblas, "hipblas")?; + + let adopted = adopt_runtime_from_probe( + &paths, + AdoptRuntimeRequest { + python_executable, + install_root: external_root, + runtime_id: "therock-release:gfx120X-all".to_owned(), + runtime_key: "adopted-release-pip-gfx120x-all-7-13-0".to_owned(), + replace: false, + }, + therock::RocmSdkPythonProbe { + import_ok: true, + rocm_sdk_version: Some("7.13.0".to_owned()), + root_path: Some(sdk_root.clone()), + bin_path: Some(sdk_bin.clone()), + cmake_path: Some(cmake_path), + runtime_roots: vec![sdk_root], + bin_paths: vec![sdk_bin.clone()], + library_paths: vec![sdk_bin], + resolved_libraries: vec![ + therock::RocmSdkLibraryProbe { + shortname: "amdhip64".to_owned(), + paths: vec![amdhip], + }, + therock::RocmSdkLibraryProbe { + shortname: "hipblas".to_owned(), + paths: vec![hipblas], + }, + ], + resolved_target_family: Some("gfx120X-all".to_owned()), + ..therock::RocmSdkPythonProbe::default() + }, + )?; + + assert!( + adopted.devel, + "a probe reporting a CMake path must record the toolchain" + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + #[test] fn runtime_adopt_request_infers_ids_from_probe() -> Result<()> { let (root, _) = test_paths("runtime-adopt-infer"); diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index e9ceff931..b4385cd6d 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -2095,25 +2095,49 @@ fn install_wheel_runtime( Ok(SdkInstallResult::installed(output)) } -/// Package specs for a wheel SDK install. +/// The `rocm` wheel extras a given install asks for, excluding the device payload. /// /// `devel` adds the compiler, headers, and static libraries — roughly doubling /// the download — and is only needed to *build* GPU code. Running models needs /// `libraries` alone, so the toolchain is installed only when asked for. /// -/// The `device-` extra is separate and always present: it selects which -/// GPU payload the wheels carry, not whether the toolchain comes with them. +/// Shared so the install plan, the progress text, and the resolution failure all +/// name the same extras instead of drifting apart. +const fn therock_sdk_extras(include_devel: bool) -> &'static str { + if include_devel { + "libraries,devel" + } else { + "libraries" + } +} + +/// What the user sees when no index has a mutually compatible package set. +/// +/// Names only the extras that were actually asked for: someone who never passed +/// `--devel` should not be told a compiler toolchain could not be resolved. +fn no_compatible_pip_versions_message( + include_devel: bool, + requested: &str, + index_url: &str, +) -> String { + format!( + "no mutually compatible TheRock rocm[{}], torch, torchvision, and torchaudio versions were found for {requested} in {index_url}", + therock_sdk_extras(include_devel) + ) +} + +/// Package specs for a wheel SDK install. +/// +/// The `device-` extra is separate from [`therock_sdk_extras`] and +/// always present: it selects which GPU payload the wheels carry, not whether +/// the toolchain comes with them. fn therock_pip_package_specs( package_versions: &TheRockPipPackageVersions, device_target: &str, include_devel: bool, ) -> Vec { let device_extra = format!("device-{device_target}"); - let rocm_extras = if include_devel { - format!("libraries,devel,{device_extra}") - } else { - format!("libraries,{device_extra}") - }; + let rocm_extras = format!("{},{device_extra}", therock_sdk_extras(include_devel)); vec![ format!("rocm[{rocm_extras}]=={}", package_versions.rocm), format!("torch[{device_extra}]=={}", package_versions.torch), @@ -2872,7 +2896,6 @@ fn resolve_pip_runtime( #[allow(clippy::too_many_arguments)] fn resolve_pip_runtime_with_timeout( paths: &AppPaths, - channel: TheRockChannel, family_override: Option<&str>, wheel_compatibility: &WheelCompatibility, version_selector: Option<&RuntimeVersionSelector>, @@ -2942,7 +2965,6 @@ fn resolve_pip_runtime_with_timeout( fn resolve_pip_runtime_from_index( paths: &AppPaths, - channel: TheRockChannel, family_resolution: &FamilyResolution, source: &ResolvedAggregateWheelSource, wheel_compatibility: &WheelCompatibility, @@ -8076,6 +8098,36 @@ mod tests { ); } + /// A resolution failure must describe the install that was actually asked + /// for. Naming `devel` to someone who never passed `--devel` sends them + /// looking for a toolchain problem they do not have. + #[test] + fn no_compatible_versions_message_names_only_the_requested_extras() { + let without_devel = no_compatible_pip_versions_message( + false, + "latest compatible version", + "https://example.invalid/simple/", + ); + assert!( + !without_devel.contains("devel"), + "default install failure must not mention the toolchain: {without_devel}" + ); + assert!( + without_devel.contains("rocm[libraries],"), + "default install failure should name the runtime extras: {without_devel}" + ); + + let with_devel = no_compatible_pip_versions_message( + true, + "latest compatible version", + "https://example.invalid/simple/", + ); + assert!( + with_devel.contains("rocm[libraries,devel],"), + "--devel failure should name the toolchain: {with_devel}" + ); + } + /// A manifest written before `devel` became opt-in has no such field, and /// those installs all had the toolchain. Reading one back must not claim /// otherwise, or the next update would silently strip it. diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 52648b7e6..73503b9a0 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -178,7 +178,8 @@ python scripts\therock_sdk_install_test.py --dry-run --family gfx120X-all ``` Use `--family` only when a test needs a fixed package family. Do not use it for -normal user setup. +normal user setup. The script checks the default install; add `--devel` to check +the opt-in compiler toolchain path instead. ## 3. Lemonade GPU Verification diff --git a/docs/testing.md b/docs/testing.md index 51082c52e..a2083d021 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -193,8 +193,9 @@ Then it verifies: pip creates it inside the ROCm folder when packages are downloaded - a single TheRock-index pip install plan for pinned `rocm`, `torch`, and `torchvision` requirements with exactly one `device-` - extra (`rocm` also requests `libraries`; `--devel` adds `devel` to them), - plus pinned `torchaudio` + extra (`rocm` also requests `libraries`), plus pinned `torchaudio`, and that + the toolchain is not planned for unless asked (pass the script `--devel` to + check the opt-in path instead, which adds `devel` to the `rocm` extras) - on a host with no detectable AMD GPU the preview reports `device_target: undetermined` and renders the device extra as a placeholder; a real install refuses rather than falling back to every published device payload @@ -967,7 +968,9 @@ it must be an exact runtime key or an unambiguous runtime id. It requires TheRock SDK wheel directories. For TheRock 7.13, patch vLLM's GPTQ ROCm compatibility guard to include HIP 7.13 before building from source; otherwise `q_gemm.hip` can fail on missing -`half`/`half2` `atomicAdd` overloads. +`half`/`half2` `atomicAdd` overloads. Building from source needs the compiler +toolchain, so install the runtime with `rocm install sdk --devel` (see +[vllm.md](vllm.md)). On native Windows this script prints a JSON skip result; run it from WSL/Linux for live ROCm GPU acceptance. diff --git a/docs/vllm.md b/docs/vllm.md index 71d66a07d..75c32059b 100644 --- a/docs/vllm.md +++ b/docs/vllm.md @@ -18,6 +18,11 @@ 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. +Building from source compiles HIP sources, so it needs the ROCm compiler +toolchain. That is opt-in: install the runtime with `rocm install sdk --devel`, +or the build fails on a missing compiler. A runtime installed without `--devel` +can still *serve* an already-built vLLM. + ## Torch alignment on engine install Installing an engine into a managed TheRock runtime can change the torch in that diff --git a/scripts/therock_sdk_install_test.py b/scripts/therock_sdk_install_test.py index abe911fa7..5e9d11b62 100644 --- a/scripts/therock_sdk_install_test.py +++ b/scripts/therock_sdk_install_test.py @@ -26,14 +26,24 @@ from pathlib import Path from typing import Any -# A prefix, not the whole spec: the canonical index always appends a third extra -# naming the device payload (`rocm[libraries,devel,device-gfx1201]`), so the -# closing bracket is no longer in a fixed place. -THEROCK_SDK_PACKAGE_SPEC = "rocm[libraries,devel" + THEROCK_TORCH_PACKAGES = ["torch", "torchvision", "torchaudio"] THEROCK_RUNTIME_PACKAGES = ["rocm", "rocm-sdk-core"] +def therock_sdk_package_spec(include_devel: bool) -> str: + """The `rocm` wheel spec prefix `rocm install sdk` is expected to plan for. + + The compiler toolchain is opt-in, so a default install asks for the runtime + libraries alone and only `--devel` adds `devel`. + + A prefix, not the whole spec: the canonical index always appends a further + extra naming the device payload (`rocm[libraries,devel,device-gfx1201]`), so + the closing bracket is not in a fixed place. + """ + return "rocm[libraries,devel," if include_devel else "rocm[libraries,device-" + + def fail(message: str) -> None: print(f"therock-sdk-install failed: {message}", file=sys.stderr) raise SystemExit(1) @@ -366,6 +376,19 @@ def verify_rocm_manifest_packages(manifest: dict[str, Any]) -> None: fail("manifest rocm_sdk packages missed the family-specific ROCm library wheel") +def verify_manifest_devel(manifest: dict[str, Any], expected: bool) -> None: + """The manifest must record whether the toolchain was installed. + + `rocm update` reinstalls from this field, so a wrong value silently adds or + drops the compiler toolchain on the next update. + """ + recorded = manifest.get("devel") + if not isinstance(recorded, bool): + fail(f"manifest did not record a boolean devel flag: {recorded!r}") + if recorded != expected: + fail(f"manifest recorded devel={recorded}, expected {expected}") + + def verify_rocm_runtime_libraries( python: Path, env: dict[str, str], @@ -423,6 +446,11 @@ def main() -> int: "--prefix", type=Path, help="explicit ROCm runtime folder to pass to rocm-cli" ) parser.add_argument("--check-windows-tools", action="store_true") + parser.add_argument( + "--devel", + action="store_true", + help="install the compiler toolchain too, as `rocm install sdk --devel` does", + ) parser.add_argument( "--dry-run", action="store_true", @@ -479,6 +507,8 @@ def main() -> int: ] if args.prefix is not None: install_argv.extend(["--prefix", str(args.prefix)]) + if args.devel: + install_argv.append("--devel") if args.dry_run: install_argv.append("--dry-run") else: @@ -509,7 +539,10 @@ def main() -> int: assert_contains(install_output, "python_wheel_tag:", "sdk install") assert_contains(install_output, "platform_wheel_tags:", "sdk install") assert_contains(install_output, "package_specs:", "sdk install") - assert_contains(install_output, THEROCK_SDK_PACKAGE_SPEC, "sdk install") + assert_contains(install_output, therock_sdk_package_spec(args.devel), "sdk install") + if not args.devel: + # The toolchain is opt-in: a plain install must not plan for it. + assert_not_contains(install_output, "rocm[libraries,devel", "sdk install") for package in THEROCK_TORCH_PACKAGES: assert_contains(install_output, f"{package}==", "sdk install") assert_not_contains(install_output, "rocm[devel]", "sdk install") @@ -577,6 +610,7 @@ def main() -> int: runtime_python, THEROCK_RUNTIME_PACKAGES, env ) verify_rocm_manifest_packages(manifest) + verify_manifest_devel(manifest, args.devel) vllm_detect = run( "vLLM adapter detects the managed TheRock runtime", From a354fb0e27733822c4e514188c59c296cb645223 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 18 Aug 2026 09:47:19 +0000 Subject: [PATCH 03/17] test(e2e): prove runtime-only SDK serves with vLLM Signed-off-by: Roman Inflianskas --- apps/rocm/src/main.rs | 4 +- .../features/runtime_setup.feature | 19 ++++++ tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 64 +++++++++++++++++++ tests/e2e-cucumber/tests/e2e/serving_steps.rs | 13 ++++ 4 files changed, 99 insertions(+), 1 deletion(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 50f80836d..c7a842269 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -621,7 +621,9 @@ rocm install sdk --family gfx110X-all --dry-run")] /// TheRock GPU package family to install, such as gfx110X-all. #[arg(long)] family: Option, - /// Also install the ROCm compiler and headers, for building GPU code. + /// Also install the ROCm compiler, headers, and static libraries for + /// building GPU code. Roughly doubles the wheel download; already + /// included by `--format tarball`. #[arg(long)] devel: bool, /// Resolve the install plan without changing files. diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 347324aa7..a14c56c64 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -1,5 +1,10 @@ Feature: Runtime configuration + # The acceptance criterion for the runtime-only default, kept engine-agnostic + # on purpose: every GPU lane must check that a fresh install registers, + # activates, still carries an inference engine, and omits the compiler + # toolchain. Pinning this to one engine would drop that check on the lanes + # where that engine is not the effective one. @id:runtime-install-sdk-active @requires-gpu @nightly Scenario: runtime-01 - Installing the SDK makes it the active runtime Given a machine with no CLI-managed runtimes @@ -7,6 +12,7 @@ Feature: Runtime configuration Then a runtime is registered And the runtime is set as active And the runtime includes an inference engine + And the runtime excludes the compiler toolchain # Dogfooding #17: re-provisioning was observed writing inside the previous # runtime, producing a recursively nested `runtimes/wheel/.../runtimes/wheel/` @@ -345,3 +351,16 @@ Feature: Runtime configuration When the user reinstalls the lemonade engine Then the CLI reports that Lemonade's backend alignment was skipped by the opt-out And the packaged pin survives the install + + # The other half: that a runtime installed without the toolchain can actually + # serve. vLLM compiles Triton kernels at runtime, which is the case most + # likely to need `devel`, so it is the one worth proving end to end. + @id:runtime-install-sdk-serves-without-toolchain @requires-gpu @requires-engine:vllm @nightly + Scenario: runtime-18 - A runtime-only SDK install serves vLLM inference + Given a machine with no CLI-managed runtimes + When the user installs the SDK + Then the runtime excludes the compiler toolchain + When the user serves a model on GPU from the installed runtime + And the user sends a chat completion request + Then the response contains a model reply + And the response identifies the correct model diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 60e5e9d10..c46575ffe 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -930,6 +930,70 @@ async fn assert_runtime_has_stack(world: &mut E2eWorld) { ); } +#[then("the runtime excludes the compiler toolchain")] +async fn assert_runtime_excludes_devel(world: &mut E2eWorld) { + let root = world + .isolated_root + .as_ref() + .expect("scenario has no isolated state root") + .path(); + let registry = root.join("data/runtimes/registry"); + let entries = std::fs::read_dir(®istry).unwrap_or_else(|error| { + panic!( + "failed to read runtime registry {}: {error}", + registry.display() + ) + }); + let manifests = entries + .filter_map(Result::ok) + .filter(|entry| entry.path().extension().is_some_and(|ext| ext == "json")) + .map(|entry| entry.path()) + .collect::>(); + assert_eq!( + manifests.len(), + 1, + "expected one freshly installed runtime manifest in {}: {manifests:?}", + registry.display() + ); + + let manifest_path = &manifests[0]; + let manifest: serde_json::Value = serde_json::from_slice( + &std::fs::read(manifest_path) + .unwrap_or_else(|error| panic!("failed to read {}: {error}", manifest_path.display())), + ) + .unwrap_or_else(|error| panic!("failed to parse {}: {error}", manifest_path.display())); + assert_eq!( + manifest.get("devel").and_then(serde_json::Value::as_bool), + Some(false), + "default SDK install recorded the compiler toolchain as present: {manifest}" + ); + + let python = manifest + .get("python_executable") + .and_then(serde_json::Value::as_str) + .expect("wheel runtime manifest has no python_executable"); + let output = std::process::Command::new(python) + .args([ + "-c", + "import importlib.metadata as m; print('present' if any(d.metadata['Name'].lower() == 'rocm-sdk-devel' for d in m.distributions()) else 'absent')", + ]) + .output() + .unwrap_or_else(|error| { + panic!("failed to inspect the installed runtime with {python}: {error}") + }); + assert!( + output.status.success(), + "failed to enumerate installed runtime packages:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim(), + "absent", + "default SDK install pulled rocm-sdk-devel back transitively" + ); +} + #[then("the managed runtime folder path is not recursively nested")] async fn assert_runtime_path_not_nested(world: &mut E2eWorld) { // `rocm examine` prints `Folder: ` for the active runtime. diff --git a/tests/e2e-cucumber/tests/e2e/serving_steps.rs b/tests/e2e-cucumber/tests/e2e/serving_steps.rs index e1515f24c..438cb752c 100644 --- a/tests/e2e-cucumber/tests/e2e/serving_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/serving_steps.rs @@ -489,6 +489,19 @@ fn claim_relaunch() -> bool { #[given("a model is being served on GPU")] async fn setup_gpu_model(world: &mut E2eWorld) { + serve_gpu_model(world).await; +} + +#[when("the user serves a model on GPU from the installed runtime")] +async fn user_serves_gpu_model_from_installed_runtime(world: &mut E2eWorld) { + // Unlike `setup_active_runtime`, this scenario intentionally keeps the + // runtime it just installed in its isolated World. Do not opt into the + // persistent E2E_SHARED_RUNTIMES_DIR here: the acceptance criterion is that + // vLLM starts from this fresh runtime-only install, not a pre-warmed SDK. + serve_gpu_model(world).await; +} + +async fn serve_gpu_model(world: &mut E2eWorld) { // Serve by the canonical HuggingFace ID (not the `qwen2.5` alias) with an // explicit engine matching this host. This step is a *precondition* for // scenarios that test inference/chat behavior, so it must not fail for From 18a1992210aa3495b92e2a989991f7cba9bc17a2 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 20 Aug 2026 10:25:48 +0000 Subject: [PATCH 04/17] fix(install): close the remaining --devel non-blocking gaps - README and docs/wsl.md now mention --devel where relevant - fix stale CI comments still describing devel as the default - render_install_sdk_dry_run_for_args now uses chat_cli_has_flag, matching every other flag parse in the file - add a CLI-parse test pinning --devel's default and wiring - add a test pinning that a plain reinstall overwrites devel in the manifest, since nothing asserted that behaviour before Signed-off-by: Roman Inflianskas --- .github/workflows/e2e-selfhosted.yml | 5 +-- README.md | 6 ++-- apps/rocm/src/main.rs | 26 +++++++++++++-- apps/rocm/src/therock.rs | 50 ++++++++++++++++++++++++++++ docs/wsl.md | 4 ++- tests/e2e-cucumber/tests/e2e.rs | 7 ++-- 6 files changed, 88 insertions(+), 10 deletions(-) diff --git a/.github/workflows/e2e-selfhosted.yml b/.github/workflows/e2e-selfhosted.yml index 0e490d7ef..5397bb028 100644 --- a/.github/workflows/e2e-selfhosted.yml +++ b/.github/workflows/e2e-selfhosted.yml @@ -294,8 +294,9 @@ jobs: export E2E_SHARED_UV_CACHE_DIR="$runner_root/uv-cache" # Share ONE installed managed runtime across the serve/chat scenarios so # `rocm install sdk` runs once per runner, not once per scenario (the - # per-scenario install count — each a multi-GiB TheRock SDK whose probe - # unpacks an ~8.8 GiB devel tarball — is what blew the time cap). The + # per-scenario install count — each a multi-GiB TheRock SDK, plus an + # additional ~8.8 GiB compiler/headers tarball on any scenario passing + # `--devel` — is what blew the time cap). The # "a managed runtime is active" precondition symlinks each scenario's # data/runtimes here (see use_shared_runtimes); clean-slate scenarios # stay isolated. Persisted across runs on RUNNER_WORKSPACE and refreshed diff --git a/README.md b/README.md index cbad76471..1440615f8 100644 --- a/README.md +++ b/README.md @@ -300,7 +300,7 @@ sometimes because it also needs sudo or a reboot). ``` rocm install sdk [--channel release|nightly] [--format wheel|tarball] [--version x.y.z | --build-date YYYY-MM-DD] - [--family gfx110X-all] [--prefix PATH] [--dry-run] + [--family gfx110X-all] [--prefix PATH] [--devel] [--dry-run] [--approve-replacing-active-default] [--yes] rocm install driver [--dkms] [--yes] [--dry-run] [--reconcile] @@ -310,7 +310,9 @@ rocm update [--apply] [--runtime KEY] [--activate] [--dry-run] ``` `install sdk` downloads TheRock ROCm wheels into a Python environment managed -by rocm-cli. An install with no active default runtime never prompts, but once a +by rocm-cli; pass `--devel` to also install the compiler and headers needed to +build GPU code, roughly doubling the download. +An install with no active default runtime never prompts, but once a managed runtime is the active default every `install sdk` asks first, because the new install takes over as the active default. That gate is not scoped to the family or channel you are installing: a `--family` or `--channel` you have never diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index c7a842269..a009fcdf8 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -601,7 +601,8 @@ enum InstallTarget { #[command(after_help = "EXAMPLES:\n \ rocm install sdk\n \ rocm install sdk --channel nightly --build-date 2025-01-15\n \ -rocm install sdk --family gfx110X-all --dry-run")] +rocm install sdk --family gfx110X-all --dry-run\n \ +rocm install sdk --devel")] Sdk { /// Package channel to install, such as release or nightly. #[arg(long, default_value = "release")] @@ -15288,7 +15289,7 @@ fn render_install_sdk_dry_run_for_args(paths: &AppPaths, args: &[String]) -> Res let version = chat_cli_arg_value(args, "--version").map(str::to_owned); let build_date = chat_cli_arg_value(args, "--build-date").map(str::to_owned); let selector = therock_install_version_selector(version, build_date)?; - let devel = args.iter().any(|arg| arg == "--devel"); + let devel = chat_cli_has_flag(args, "--devel"); // Dry run, so nothing is displaced and the consent gate is never reached; // the narrow consent is what this chat surface would pass for a real // install, and passing `--yes`'s source here would be a lie waiting to be @@ -28590,6 +28591,27 @@ install therock"; .expect("install sdk should accept a TheRock family override"); } + #[test] + fn install_sdk_devel_flag_defaults_off_and_wires_through_when_passed() { + let cli = Cli::try_parse_from(["rocm", "install", "sdk"]) + .expect("install sdk should parse with no flags"); + match cli.command { + Some(Command::Install { + target: InstallTarget::Sdk { devel, .. }, + }) => assert!(!devel, "--devel should default to false"), + other => panic!("expected an install sdk target, got {other:?}"), + } + + let cli = Cli::try_parse_from(["rocm", "install", "sdk", "--devel"]) + .expect("install sdk should accept --devel"); + match cli.command { + Some(Command::Install { + target: InstallTarget::Sdk { devel, .. }, + }) => assert!(devel, "--devel should set the flag to true"), + other => panic!("expected an install sdk target, got {other:?}"), + } + } + #[test] fn top_level_cli_commands_are_not_treated_as_freeform() { for command in [ diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index b4385cd6d..036024f95 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -8098,6 +8098,56 @@ mod tests { ); } + /// A plain reinstall (`rocm install sdk`, no `--devel`) overwrites the + /// manifest unconditionally, `devel` included — there is no merge with the + /// prior manifest. This pins that behaviour: a user who installed with + /// `--devel` and later reinstalls the same runtime key without it keeps + /// `hipcc`/headers on disk, but the manifest silently records `devel: + /// false`, and `apply_runtime_update` reinstalls from that field on the + /// next `rocm update`. Documented as-is rather than fixed here — the flag + /// reflects what the most recent install actually asked for, which is + /// arguably correct; the drift risk is that neither `rocm runtimes list` + /// nor `rocm examine` surfaces `devel`, so the state is invisible. + #[test] + fn reinstall_without_devel_overwrites_manifest_devel_flag() -> Result<()> { + let (root, paths) = test_paths("reinstall-overwrites-devel"); + let install_root = root.join("install-root"); + fs::create_dir_all(&install_root)?; + let with_devel = InstalledRuntimeManifest { + install_root, + ..test_runtime_manifest( + "release-wheel-gfx110X-all-7.13.0", + "therock-release:gfx110X-all", + 1, + ) + }; + assert!( + with_devel.devel, + "test fixture should start with devel: true" + ); + save_runtime_manifest(&paths, &with_devel)?; + + let without_devel = InstalledRuntimeManifest { + devel: false, + installed_at_unix_ms: 2, + ..with_devel.clone() + }; + save_runtime_manifest(&paths, &without_devel)?; + + let manifests = load_runtime_manifests(&paths)?; + let reloaded = manifests + .iter() + .find(|manifest| manifest.runtime_key == with_devel.runtime_key) + .expect("manifest should still be registered under the same runtime_key"); + assert!( + !reloaded.devel, + "a plain reinstall must overwrite devel, not merge with the prior manifest" + ); + + let _ = fs::remove_dir_all(&root); + Ok(()) + } + /// A resolution failure must describe the install that was actually asked /// for. Naming `devel` to someone who never passed `--devel` sends them /// looking for a toolchain problem they do not have. diff --git a/docs/wsl.md b/docs/wsl.md index d1f3cd6ab..da9974fe7 100644 --- a/docs/wsl.md +++ b/docs/wsl.md @@ -154,7 +154,9 @@ The WSL activation environment for HIP applications that do not preload ROCm the way PyTorch does must include the managed TheRock runtime package paths and WSL DXCore path. With the managed `rocm[libraries]` install, these paths come from the rocm-cli runtime manifest, `rocm-sdk path --root`, and -`rocm_sdk.find_libraries(...)`. +`rocm_sdk.find_libraries(...)`. `rocm[libraries]` is enough to run a pre-built +HIP application; compiling one from source under WSL needs the compiler and +headers, so run `rocm install sdk --devel` instead. ```bash export ROCM_ROOT="" diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 628128a20..57367a85e 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -170,9 +170,10 @@ fn shared_uv_cache_dir() -> Option { /// persistent disk; unset for local runs, where every scenario installs its own. /// /// Why opt-in and not global: a cold `rocm install sdk` installs a multi-GiB -/// TheRock runtime (and its post-install probe unpacks an ~8.8 GiB devel tarball), -/// and each scenario's isolated data dir made it re-run per scenario — the GPU job -/// then exceeds its time cap. Scenarios that just need a runtime present +/// TheRock runtime (`--devel`, when passed, unpacks an additional ~8.8 GiB +/// compiler/headers tarball), and each scenario's isolated data dir made it +/// re-run per scenario — the GPU job then exceeds its time cap. Scenarios that +/// just need a runtime present /// ("a managed runtime is active") point their `data/runtimes` at this shared tree /// (see [`E2eWorld::use_shared_runtimes`]) so the install happens once per runner. /// Scenarios that ASSERT a clean slate ("a machine with no CLI-managed runtimes", From 979295c77adc4608c5ea6c74a8e06d937c4d2e0b Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 20 Aug 2026 10:58:29 +0000 Subject: [PATCH 05/17] test(install): narrow the manifest devel test to what it proves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test never reached install_sdk, so it did not cover the write site that threads include_devel into the manifest — hardcoding devel: true there passes it and the rest of the suite. Renamed and re-scoped to the save_runtime_manifest replace-vs-merge behaviour it actually pins, with the uncovered path stated in the doc comment. Signed-off-by: Roman Inflianskas --- apps/rocm/src/therock.rs | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 036024f95..68460a6d9 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -8098,19 +8098,25 @@ mod tests { ); } - /// A plain reinstall (`rocm install sdk`, no `--devel`) overwrites the - /// manifest unconditionally, `devel` included — there is no merge with the - /// prior manifest. This pins that behaviour: a user who installed with - /// `--devel` and later reinstalls the same runtime key without it keeps - /// `hipcc`/headers on disk, but the manifest silently records `devel: - /// false`, and `apply_runtime_update` reinstalls from that field on the - /// next `rocm update`. Documented as-is rather than fixed here — the flag - /// reflects what the most recent install actually asked for, which is - /// arguably correct; the drift risk is that neither `rocm runtimes list` - /// nor `rocm examine` surfaces `devel`, so the state is invisible. - #[test] - fn reinstall_without_devel_overwrites_manifest_devel_flag() -> Result<()> { - let (root, paths) = test_paths("reinstall-overwrites-devel"); + /// Writing a manifest over an existing one for the same `runtime_key` + /// REPLACES it — `devel` included — rather than merging with what was on + /// disk. That is the storage half of the reinstall-drift case: a user who + /// installed with `--devel` and later reinstalls without it keeps + /// `hipcc`/headers on disk while the manifest records `devel: false`, and + /// `apply_runtime_update` reinstalls from that field on the next `rocm + /// update`. Left as-is rather than fixed — the flag reflects what the most + /// recent install asked for, which is arguably correct; the drift risk is + /// that neither `rocm runtimes list` nor `rocm examine` surfaces `devel`, + /// so the state is invisible. + /// + /// LIMIT: this covers `save_runtime_manifest` only. It does NOT reach + /// `install_sdk`, which is what actually threads `include_devel` into the + /// manifest — that call needs `uv`, a live index, and a real `rocm_sdk` + /// probe, so it has no unit coverage here. Hardcoding `devel: true` at that + /// write site passes this test and the rest of the suite. + #[test] + fn save_runtime_manifest_replaces_devel_rather_than_merging() -> Result<()> { + let (root, paths) = test_paths("manifest-replaces-devel"); let install_root = root.join("install-root"); fs::create_dir_all(&install_root)?; let with_devel = InstalledRuntimeManifest { @@ -8141,7 +8147,7 @@ mod tests { .expect("manifest should still be registered under the same runtime_key"); assert!( !reloaded.devel, - "a plain reinstall must overwrite devel, not merge with the prior manifest" + "saving a manifest must replace devel, not merge with the prior one" ); let _ = fs::remove_dir_all(&root); From d5e36e38a7b780663a120e964b9062b8cadd9d85 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 17 Sep 2026 11:16:44 +0000 Subject: [PATCH 06/17] fix(install): reconcile --devel with the ROCm 10 layout landed on main The rebase needed real decisions, not a mechanical resolve, because main reworked the same code from a different direction. - `therock_pip_package_specs` now composes both axes: main's mandatory `device-` extra (which payload the wheels carry) and this branch's optional `devel` (whether the toolchain comes with them). - `wheel_runtime_composition` takes `include_devel`, so the specs that go to `uv` and the specs recorded in the manifest are one value, not two that can disagree. - `InstalledRuntimeManifest::includes_devel()` reads the answer back out of those recorded specs, mirroring `wheel_composition_device_target`, and falls back to the `devel` field and then to `true` for manifests written before either existed. - The update path keeps main's `install_sdk_for_update`, which preserves the family, device target and source layout the runtime was installed with; the branch's version routed through `install_sdk` defaults and would have dropped all three. - `PipRuntimeQuery` and `no_compatible_pip_versions_message` are dropped: main fixed that message by naming no extras at all, which cannot drift, and its resolution path no longer iterates candidate indexes. The e2e scenario is split so the engine-agnostic acceptance criterion -- installs, activates, has an engine, has no toolchain -- runs on every GPU lane again, with the vLLM serve half as its own scenario. Restores the `the runtime includes an inference engine` step this branch had deleted. Signed-off-by: Roman Inflianskas --- apps/rocm/src/therock.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 68460a6d9..65ed7c53c 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -1519,13 +1519,11 @@ fn resolve_latest_for_manifest( // falls back to the version comparison rather than demanding a repair // this host could not perform. let wheel_composition = match &device_target { - AggregateDeviceTarget::Exact(_) => { - Some(wheel_runtime_composition( - &resolution, - &device_target, - manifest.includes_devel(), - )) - } + AggregateDeviceTarget::Exact(_) => Some(wheel_runtime_composition( + &resolution, + &device_target, + manifest.includes_devel(), + )), AggregateDeviceTarget::Undetermined(_) => None, }; let target_runtime_key = wheel_composition.as_ref().map_or_else( @@ -1860,7 +1858,6 @@ fn install_wheel_runtime( let _ = writeln!( output, " package_policy: resolve the pinned target-complete rocm, torch, torchvision, and torchaudio plan from published package metadata, then install it in one uv transaction" - ); let no_wheel_warning = repo_version_without_wheels( resolution.newest_repo_version.as_deref(), @@ -2181,7 +2178,12 @@ fn wheel_composition_includes_devel(composition: Option<&WheelRuntimeComposition let composition = composition?; composition.package_specs.iter().find_map(|spec| { let extras = spec.strip_prefix("rocm[")?.split_once(']')?.0; - Some(extras.split(',').map(str::trim).any(|extra| extra == "devel")) + Some( + extras + .split(',') + .map(str::trim) + .any(|extra| extra == "devel"), + ) }) } @@ -2896,6 +2898,7 @@ fn resolve_pip_runtime( #[allow(clippy::too_many_arguments)] fn resolve_pip_runtime_with_timeout( paths: &AppPaths, + channel: TheRockChannel, family_override: Option<&str>, wheel_compatibility: &WheelCompatibility, version_selector: Option<&RuntimeVersionSelector>, @@ -2965,6 +2968,7 @@ fn resolve_pip_runtime_with_timeout( fn resolve_pip_runtime_from_index( paths: &AppPaths, + channel: TheRockChannel, family_resolution: &FamilyResolution, source: &ResolvedAggregateWheelSource, wheel_compatibility: &WheelCompatibility, From ede904f0995287b45fd444b7e1ea99386fb707ba Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 17 Sep 2026 11:25:47 +0000 Subject: [PATCH 07/17] test(install): guard the seam --devel travels through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flag reached the package specs and the manifest through `install_wheel_runtime`, which has one caller, no test callers, and needs uv plus a live index plus a real probe to run — so nothing in a blocking lane noticed if the value stopped being forwarded. Two seams now have tests, both polarities each: - `wheel_runtime_composition`, the pure function that produces the specs handed to uv AND the specs recorded in the manifest. Hardcoding either polarity inside it, or inside `therock_pip_package_specs`, now fails. The device extra is asserted alongside so a fix to one axis cannot quietly drop the other. - the CLI-to-request mapping, extracted from the `install sdk` arm as `sdk_install_request`. The existing test stopped at clap parsing, so hardcoding the request field passed the whole suite; it now fails, as does a parse-through-to-request test across both flag states. Also asserts the recorded `package_specs` in the e2e toolchain check. The `devel` field alone could agree while the specs handed to uv did not, which is the drift `includes_devel` reading from the specs exists to stop. Remaining gap, stated rather than papered over: the single line inside `install_wheel_runtime` that passes `include_devel` into the composition is still only covered by the GPU nightly scenario. Hardcoding it there passes every unit test. Closing that needs the real install path. Signed-off-by: Roman Inflianskas --- apps/rocm/src/main.rs | 117 ++++++++++++++++- apps/rocm/src/therock.rs | 120 ++++++++++++++++++ tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 27 ++++ 3 files changed, 257 insertions(+), 7 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index a009fcdf8..c5f9ca0a6 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -2911,16 +2911,16 @@ fn install(target: InstallTarget) -> Result<()> { .map_or_else(|| "".to_owned(), |path| path.display().to_string()); match therock::install_sdk( &paths, - therock::SdkInstallRequest { - channel: &channel, - format: format_name, + sdk_install_request( + &channel, + format_name, prefix, version_selector, - family_override: family.as_deref(), + family.as_deref(), dry_run, - include_devel: devel, - consent: consents.replace_active_default, - }, + devel, + consents.replace_active_default, + ), ) { Ok(result) => { let therock::SdkInstallResult { output, mutated } = result; @@ -15282,6 +15282,35 @@ fn parse_optional_lines(args: &[String]) -> Result { Ok(DEFAULT_LOG_TAIL_LINES) } +/// Map the parsed `install sdk` arguments onto the install request. +/// +/// Extracted from the command arm so this mapping has a seam. `include_devel` +/// is the field that most needs one: nothing downstream re-derives it, so if +/// the flag stopped being forwarded here the install would silently go back to +/// pulling the compiler toolchain and every other assertion would still pass. +#[allow(clippy::too_many_arguments)] +fn sdk_install_request<'a>( + channel: &'a str, + format: &'a str, + prefix: Option, + version_selector: Option, + family_override: Option<&'a str>, + dry_run: bool, + devel: bool, + consent: therock::SdkInstallConsent, +) -> therock::SdkInstallRequest<'a> { + therock::SdkInstallRequest { + channel, + format, + prefix, + version_selector, + family_override, + dry_run, + include_devel: devel, + consent, + } +} + fn render_install_sdk_dry_run_for_args(paths: &AppPaths, args: &[String]) -> Result { let channel = chat_cli_arg_value(args, "--channel").unwrap_or("release"); let format = chat_cli_arg_value(args, "--format").unwrap_or("wheel"); @@ -28612,6 +28641,80 @@ install therock"; } } + /// The half the parse test above cannot reach: that the parsed flag is what + /// the install request carries. + /// + /// `install()` is not callable here — it needs `uv`, a live index and a real + /// probe — so the mapping is extracted into `sdk_install_request` and pinned + /// directly. Hardcoding `include_devel` at that mapping is the change this + /// catches and the clap test does not. + #[test] + fn install_sdk_request_forwards_the_parsed_devel_flag() { + for devel in [false, true] { + let request = sdk_install_request( + "release", + "wheel", + None, + None, + None, + false, + devel, + therock::SdkInstallConsent::Ask, + ); + assert_eq!( + request.include_devel, devel, + "the parsed --devel flag must reach the install request" + ); + } + + // And the flag must not be confused with the neighbouring bool. + let dry_run_only = sdk_install_request( + "release", + "wheel", + None, + None, + None, + true, + false, + therock::SdkInstallConsent::Ask, + ); + assert!(dry_run_only.dry_run); + assert!(!dry_run_only.include_devel); + } + + /// End to end across the two seams a `rocm install sdk --devel` traverses: + /// clap parse, then the request mapping. Neither alone proves the flag + /// survives the trip. + #[test] + fn parsed_install_sdk_arguments_reach_the_request_with_devel_intact() { + for (args, expected) in [ + (vec!["rocm", "install", "sdk"], false), + (vec!["rocm", "install", "sdk", "--devel"], true), + ] { + let cli = Cli::try_parse_from(&args).expect("install sdk should parse"); + let Some(Command::Install { + target: InstallTarget::Sdk { devel, .. }, + }) = cli.command + else { + panic!("expected an install sdk target for {args:?}"); + }; + let request = sdk_install_request( + "release", + "wheel", + None, + None, + None, + false, + devel, + therock::SdkInstallConsent::Ask, + ); + assert_eq!( + request.include_devel, expected, + "--devel did not survive parse -> request for {args:?}" + ); + } + } + #[test] fn top_level_cli_commands_are_not_treated_as_freeform() { for command in [ diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 65ed7c53c..14be93d8f 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -8102,6 +8102,126 @@ mod tests { ); } + fn devel_test_resolution() -> PipRuntimeResolution { + PipRuntimeResolution { + family: "gfx94X-dcgpu".to_owned(), + family_source: "detected".to_owned(), + index_url: "https://example.invalid/simple".to_owned(), + layout: SourceLayout::Canonical, + latest_version: "7.13.0".to_owned(), + newest_repo_version: None, + package_versions: TheRockPipPackageVersions { + rocm: "7.13.0".to_owned(), + torch: "2.11.0+rocm7.13.0".to_owned(), + torchvision: "0.26.0+rocm7.13.0".to_owned(), + torchaudio: "2.11.0+rocm7.13.0".to_owned(), + compatibility_key: "7.13.0".to_owned(), + }, + device_target: AggregateDeviceTarget::Exact("gfx942".to_owned()), + published_device_targets: vec!["gfx942".to_owned()], + } + } + + /// The seam the `--devel` flag actually travels through. + /// + /// `wheel_runtime_composition` produces the specs handed to `uv` AND the + /// specs recorded in the manifest, so hardcoding either polarity here is + /// the single change that would silently restore the old behaviour. Both + /// directions are pinned, and the device extra is asserted alongside so a + /// fix to one axis cannot quietly drop the other. + #[test] + fn wheel_composition_requests_the_toolchain_only_when_asked() { + let resolution = devel_test_resolution(); + let target = AggregateDeviceTarget::Exact("gfx942".to_owned()); + + let runtime_only = wheel_runtime_composition(&resolution, &target, false); + assert_eq!( + runtime_only.package_specs[0], "rocm[libraries,device-gfx942]==7.13.0", + "a default install must not request the toolchain: {:?}", + runtime_only.package_specs + ); + + let with_devel = wheel_runtime_composition(&resolution, &target, true); + assert_eq!( + with_devel.package_specs[0], "rocm[libraries,devel,device-gfx942]==7.13.0", + "--devel must reach the specs: {:?}", + with_devel.package_specs + ); + + // The two axes are independent: opting into the toolchain must not + // disturb the device payload, and vice versa. + assert_eq!( + runtime_only.package_specs[1..], + with_devel.package_specs[1..], + "devel must only affect the rocm spec" + ); + assert_eq!(runtime_only.rocm_sdk_target, with_devel.rocm_sdk_target); + } + + /// The manifest answer is derived from the specs that were installed, so a + /// recorded composition and the `devel` field can never disagree. + #[test] + fn manifest_reads_devel_back_out_of_the_recorded_composition() { + let resolution = devel_test_resolution(); + let target = AggregateDeviceTarget::Exact("gfx942".to_owned()); + + for include_devel in [false, true] { + let composition = wheel_runtime_composition(&resolution, &target, include_devel); + assert_eq!( + wheel_composition_includes_devel(Some(&composition)), + Some(include_devel), + "composition round-trip failed for include_devel={include_devel}" + ); + + // Even when the `devel` field contradicts the specs, the specs win: + // they are what `uv` was given. + let manifest = InstalledRuntimeManifest { + wheel_composition: Some(composition), + devel: !include_devel, + ..test_runtime_manifest( + "release-wheel-gfx94X-dcgpu-7.13.0", + "therock-release:gfx94X-dcgpu", + 1, + ) + }; + assert_eq!( + manifest.includes_devel(), + include_devel, + "the recorded specs must outrank a stale devel field" + ); + } + } + + /// A manifest from before compositions were recorded has only the field, + /// and one older still has neither — those installs all had the toolchain. + #[test] + fn manifest_without_a_composition_falls_back_to_the_devel_field() { + let without_composition = InstalledRuntimeManifest { + wheel_composition: None, + devel: false, + ..test_runtime_manifest( + "release-wheel-gfx94X-dcgpu-7.13.0", + "therock-release:gfx94X-dcgpu", + 1, + ) + }; + assert!(!without_composition.includes_devel()); + + let legacy = InstalledRuntimeManifest { + wheel_composition: None, + devel: devel_default_for_legacy_manifest(), + ..test_runtime_manifest( + "release-wheel-gfx94X-dcgpu-7.13.0", + "therock-release:gfx94X-dcgpu", + 1, + ) + }; + assert!( + legacy.includes_devel(), + "a manifest predating the flag must be treated as a toolchain install" + ); + } + /// Writing a manifest over an existing one for the same `runtime_key` /// REPLACES it — `devel` included — rather than merging with what was on /// disk. That is the storage half of the reinstall-drift case: a user who diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index c46575ffe..489927537 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -968,6 +968,33 @@ async fn assert_runtime_excludes_devel(world: &mut E2eWorld) { "default SDK install recorded the compiler toolchain as present: {manifest}" ); + // The specs are the authoritative record — they are what `uv` was handed, + // and `InstalledRuntimeManifest::includes_devel` reads the answer back out + // of them. Asserting the `devel` field alone would miss the install args + // and the manifest disagreeing, which is the drift this guards. + let specs = manifest + .get("wheel_composition") + .and_then(|composition| composition.get("package_specs")) + .and_then(serde_json::Value::as_array) + .expect("wheel runtime manifest has no recorded package_specs"); + let rocm_spec = specs + .iter() + .filter_map(serde_json::Value::as_str) + .find(|spec| spec.starts_with("rocm[")) + .expect("no rocm requirement in the recorded package_specs"); + let extras = rocm_spec + .strip_prefix("rocm[") + .and_then(|rest| rest.split_once(']')) + .map(|(extras, _)| extras) + .expect("malformed rocm requirement in the recorded package_specs"); + assert!( + !extras + .split(',') + .map(str::trim) + .any(|extra| extra == "devel"), + "default SDK install requested the toolchain: {rocm_spec}" + ); + let python = manifest .get("python_executable") .and_then(serde_json::Value::as_str) From e784b2d5c526a20b883a629b88104e48f4a574a6 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 17 Sep 2026 11:28:22 +0000 Subject: [PATCH 08/17] refactor(install): drop the extras helper main superseded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `no_compatible_pip_versions_message` existed so a user who never passed `--devel` would not be told a compiler toolchain could not be resolved. Main fixed the same complaint from the other side: its message names no extras at all, which cannot go stale as the extras change. Keeping both left the helper unreachable, so it and its test go. `therock_sdk_extras` stays — the package specs still need it. Signed-off-by: Roman Inflianskas --- apps/rocm/src/main.rs | 2 +- apps/rocm/src/therock.rs | 45 ---------------------------------------- 2 files changed, 1 insertion(+), 46 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index c5f9ca0a6..ecc85ac2e 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -15289,7 +15289,7 @@ fn parse_optional_lines(args: &[String]) -> Result { /// the flag stopped being forwarded here the install would silently go back to /// pulling the compiler toolchain and every other assertion would still pass. #[allow(clippy::too_many_arguments)] -fn sdk_install_request<'a>( +const fn sdk_install_request<'a>( channel: &'a str, format: &'a str, prefix: Option, diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 14be93d8f..908ac0d19 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -2108,21 +2108,6 @@ const fn therock_sdk_extras(include_devel: bool) -> &'static str { } } -/// What the user sees when no index has a mutually compatible package set. -/// -/// Names only the extras that were actually asked for: someone who never passed -/// `--devel` should not be told a compiler toolchain could not be resolved. -fn no_compatible_pip_versions_message( - include_devel: bool, - requested: &str, - index_url: &str, -) -> String { - format!( - "no mutually compatible TheRock rocm[{}], torch, torchvision, and torchaudio versions were found for {requested} in {index_url}", - therock_sdk_extras(include_devel) - ) -} - /// Package specs for a wheel SDK install. /// /// The `device-` extra is separate from [`therock_sdk_extras`] and @@ -8278,36 +8263,6 @@ mod tests { Ok(()) } - /// A resolution failure must describe the install that was actually asked - /// for. Naming `devel` to someone who never passed `--devel` sends them - /// looking for a toolchain problem they do not have. - #[test] - fn no_compatible_versions_message_names_only_the_requested_extras() { - let without_devel = no_compatible_pip_versions_message( - false, - "latest compatible version", - "https://example.invalid/simple/", - ); - assert!( - !without_devel.contains("devel"), - "default install failure must not mention the toolchain: {without_devel}" - ); - assert!( - without_devel.contains("rocm[libraries],"), - "default install failure should name the runtime extras: {without_devel}" - ); - - let with_devel = no_compatible_pip_versions_message( - true, - "latest compatible version", - "https://example.invalid/simple/", - ); - assert!( - with_devel.contains("rocm[libraries,devel],"), - "--devel failure should name the toolchain: {with_devel}" - ); - } - /// A manifest written before `devel` became opt-in has no such field, and /// those installs all had the toolchain. Reading one back must not claim /// otherwise, or the next update would silently strip it. From 098d599cdb1eaf51a7fb9710284a8022cf4e73f2 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 17 Sep 2026 11:41:55 +0000 Subject: [PATCH 09/17] test(e2e): prove the toolchain default on a lane that runs per PR The preview scenarios assert the whole `package_specs` line, so making the toolchain opt-in changed what two of them expect. Updating them is what turns them into the guard this change was missing. `therock-next-02` previews a default install and now expects `rocm[libraries,device-gfx1200]`. Hardcoding `include_devel` at the one line inside `install_wheel_runtime` that no unit test can reach fails exactly this scenario -- verified by mutation -- and it carries no `@requires-gpu`, so it runs on the required E2E lane rather than the GPU nightly one. `therock-next-09` is its opt-in counterpart: same preview with `--devel`, expecting `devel` added to the rocm extras and the device payload untouched on all four requirements. `therock-next-08` previews an update to a runtime whose recorded specs include the toolchain, so it now expects the toolchain too. That is the assertion for `includes_devel`: an update reinstalls what was installed, not whatever the current default happens to be. `rocm runtimes list` also reports `toolchain=included|excluded`. It was the one runtime property with no CLI surface, while `rocm update` silently reinstalls from it. Signed-off-by: Roman Inflianskas --- apps/rocm/src/main.rs | 64 ++++++++++++++++++- .../features/therock_next_generation.feature | 18 +++++- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 2 +- tests/e2e-cucumber/tests/e2e/therock_steps.rs | 39 ++++++++++- 4 files changed, 118 insertions(+), 5 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index ecc85ac2e..5aa649e81 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -8642,16 +8642,25 @@ pub(crate) fn render_runtimes_text(paths: &AppPaths, config: &RocmCliConfig) -> } else { "managed" }; + // `toolchain` is the only runtime property a user cannot otherwise + // see: the compiler is opt-in, and `rocm update` reinstalls whatever + // this says, so an install missing it should not be silent about that. + let toolchain = if manifest.includes_devel() { + "included" + } else { + "excluded" + }; let _ = writeln!( output, - " {marker} {} runtime_id={} version={} format={} family={} mode={} status={}", + " {marker} {} runtime_id={} version={} format={} family={} mode={} status={} toolchain={}", manifest.runtime_key, manifest.runtime_id, therock::runtime_version_display(&manifest.version), manifest.format, manifest.family, mode, - status + status, + toolchain ); let _ = writeln!( output, @@ -32232,6 +32241,57 @@ ID_LIKE="suse opensuse" Ok(()) } + /// Whether a runtime carries the compiler toolchain is otherwise invisible: + /// nothing else in the CLI reports it, and `rocm update` reinstalls whatever + /// the runtime recorded — so a user who installed without it has no way to + /// see that, or to understand why a later build step fails. + #[test] + fn runtime_list_reports_whether_the_toolchain_is_installed() -> Result<()> { + let (root, paths) = test_paths("runtime-list-toolchain"); + let manifest = write_test_pip_runtime( + &paths, + "release-pip-gfx120x-all-7-13-0", + "therock-release:gfx120X-all", + "7.13.0", + 10, + )?; + let config = RocmCliConfig { + active_runtime_key: Some(manifest.runtime_key.clone()), + ..RocmCliConfig::default() + }; + + // The fixture records `devel: true` with no composition. + let rendered = render_runtimes_text(&paths, &config)?; + assert!( + rendered.contains("toolchain=included"), + "a toolchain install must say so:\n{rendered}" + ); + + // A runtime-only install reports the other way. Written through the + // recorded specs, which is what `includes_devel` actually reads. + let runtime_only = therock::InstalledRuntimeManifest { + devel: false, + wheel_composition: Some(therock::WheelRuntimeComposition { + source_layout_generation: "canonical".to_owned(), + package_specs: vec!["rocm[libraries,device-gfx1201]==7.13.0".to_owned()], + rocm_sdk_target: Some("gfx1201".to_owned()), + }), + ..manifest + }; + fs::write( + runtime_manifest_path(&paths, &runtime_only.runtime_key), + serde_json::to_vec_pretty(&runtime_only)?, + )?; + let rendered = render_runtimes_text(&paths, &config)?; + assert!( + rendered.contains("toolchain=excluded"), + "a runtime-only install must say so:\n{rendered}" + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + #[test] fn runtime_lists_display_build_date_from_version_string() -> Result<()> { let (root, paths) = test_paths("runtime-build-date-display"); diff --git a/tests/e2e-cucumber/features/therock_next_generation.feature b/tests/e2e-cucumber/features/therock_next_generation.feature index e739e8c9b..ef112c5af 100644 --- a/tests/e2e-cucumber/features/therock_next_generation.feature +++ b/tests/e2e-cucumber/features/therock_next_generation.feature @@ -121,7 +121,9 @@ Feature: TheRock "next" ROCm 10 install layout Given a canonical release pip index fixture and a ROCm 10 pip index fixture And a registered ROCm 10 wheel runtime with a grouped family When the user previews applying the pending update to that runtime - Then the preview requests the gfx1200 device extras + # With the toolchain because that runtime's recorded specs have it: an + # update must reinstall what was installed, not the current default. + Then the preview requests the gfx1200 device extras with the toolchain # Proves the vLLM ROCm 10.x wheel discovery route rather than the fixed pin # table other SDK versions use: AMD publishes vllm, flash-attn, and @@ -142,3 +144,17 @@ Feature: TheRock "next" ROCm 10 install layout And the runtime includes an inference engine When the user reinstalls vllm Then the install reports the vLLM ROCm 10.x discovery pins + + # The opt-in half of therock-next-02. Both polarities run here, on the mock + # lane, because this is the only place the flag's effect on the real install + # plan is observable without a GPU and a multi-GiB download. + # + # The `@id:` still reads `-09-`: this scenario was written as therock-next-09 + # and the display index moved when main landed one ahead of it. The id is the + # stable identifier and is deliberately not renumbered with the index. + @id:therock-next-09-wheel-devel-adds-the-toolchain + Scenario: therock-next-10 - A pinned ROCm 10 wheel install adds the toolchain when asked + Given a canonical release pip index fixture and a ROCm 10 pip index fixture + When the user previews a wheel SDK install for arch gfx1200 pinned to ROCm 10.0.0 with the toolchain + Then the preview resolves the ROCm 10 pip index + And the preview requests the gfx1200 device extras with the toolchain diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 489927537..d16806d98 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -849,7 +849,7 @@ async fn assert_release_device_payload(world: &mut E2eWorld) { ); assert_eq!( requested_rocm_extras(preview_rocm_spec(output)), - format!("libraries,devel,device-{detected}"), + format!("libraries,device-{detected}"), "the install does not request exactly this host's device payload:\n{output}" ); diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index cbf87d58b..00f56dbdd 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -373,6 +373,43 @@ async fn preview_pinned_wheel_install(world: &mut E2eWorld) { ); } +#[when( + "the user previews a wheel SDK install for arch gfx1200 pinned to ROCm 10.0.0 with the toolchain" +)] +async fn preview_pinned_wheel_install_with_devel(world: &mut E2eWorld) { + preview_ok( + world, + &[ + "install", + "sdk", + "--channel", + "release", + "--format", + "wheel", + "--family", + RAW_ARCH, + "--version", + NEXT_ROCM_VERSION, + "--devel", + "--dry-run", + ], + ); +} + +#[then("the preview requests the gfx1200 device extras with the toolchain")] +async fn preview_requests_device_extras_with_devel(world: &mut E2eWorld) { + // The opt-in half of the same assertion. Whole line for the same reason: + // `devel` has to be added to the rocm extras without disturbing the device + // payload on any of the four requirements. + let expected = format!( + "package_specs: rocm[libraries,devel,device-{RAW_ARCH}]=={NEXT_ROCM_VERSION} \ + torch[device-{RAW_ARCH}]=={NEXT_TORCH_VERSION} \ + torchvision[device-{RAW_ARCH}]=={NEXT_TORCHVISION_VERSION} \ + torchaudio=={NEXT_TORCHAUDIO_VERSION}" + ); + assert_contains(world, &expected, "device extras with devel"); +} + /// Distinct from [`preview_pinned_wheel_install`]: this scenario's whole point /// is that the override is ignored and resolution falls through to the real /// `DEFAULT_NEXT_RELEASE_PIP_BASE`, so the pin has to be a version that source @@ -566,7 +603,7 @@ async fn preview_requests_device_extras(world: &mut E2eWorld) { // and torchvision and none for torchaudio, which only the whole spec list // shows. A group-bucket or `device-all` regression still matches any subset. let expected = format!( - "package_specs: rocm[libraries,devel,device-{RAW_ARCH}]=={NEXT_ROCM_VERSION} \ + "package_specs: rocm[libraries,device-{RAW_ARCH}]=={NEXT_ROCM_VERSION} \ torch[device-{RAW_ARCH}]=={NEXT_TORCH_VERSION} \ torchvision[device-{RAW_ARCH}]=={NEXT_TORCHVISION_VERSION} \ torchaudio=={NEXT_TORCHAUDIO_VERSION}" From 1d6b2d69b3faf63a66583426d4352a2abdad5d02 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 17 Sep 2026 11:55:22 +0000 Subject: [PATCH 10/17] style(scripts): drop the blank line left by removing the spec constant ruff-check fixes this automatically, which fails the prek gate. Signed-off-by: Roman Inflianskas --- scripts/therock_sdk_install_test.py | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/therock_sdk_install_test.py b/scripts/therock_sdk_install_test.py index 5e9d11b62..e61ea6c54 100644 --- a/scripts/therock_sdk_install_test.py +++ b/scripts/therock_sdk_install_test.py @@ -26,7 +26,6 @@ from pathlib import Path from typing import Any - THEROCK_TORCH_PACKAGES = ["torch", "torchvision", "torchaudio"] THEROCK_RUNTIME_PACKAGES = ["rocm", "rocm-sdk-core"] From bd02e0d824d188641c19889fc1f4c567ab8b4aab Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 17 Sep 2026 12:39:36 +0000 Subject: [PATCH 11/17] test(e2e): move the --devel preview to the nightly lane Adding it to the no-GPU mock lane made `dash-gen-tps-held-after-scrape-failure` and `dash-gen-tps-expiry-boundary` fail 2/2 CI runs. That is the effect runtime-06 already documents and takes the same remedy for: those two assert on a validity window and are fragile under the lane's concurrency, so one more index-resolving scenario is enough to tip them. Not a coverage loss where it counts. therock-next-02 still pins the default on the mock lane -- the direction a regression would actually take, and the one that catches hardcoding `include_devel` inside `install_wheel_runtime` -- and the opt-in direction is pinned per PR by `wheel_composition_requests_the_toolchain_only_when_asked`. Signed-off-by: Roman Inflianskas --- .../features/therock_next_generation.feature | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/e2e-cucumber/features/therock_next_generation.feature b/tests/e2e-cucumber/features/therock_next_generation.feature index ef112c5af..4096672b9 100644 --- a/tests/e2e-cucumber/features/therock_next_generation.feature +++ b/tests/e2e-cucumber/features/therock_next_generation.feature @@ -145,14 +145,24 @@ Feature: TheRock "next" ROCm 10 install layout When the user reinstalls vllm Then the install reports the vLLM ROCm 10.x discovery pins - # The opt-in half of therock-next-02. Both polarities run here, on the mock - # lane, because this is the only place the flag's effect on the real install - # plan is observable without a GPU and a multi-GiB download. + # The opt-in half of therock-next-02, which covers the default. + # + # `@nightly` for the reason runtime-06 documents: one more index-resolving + # scenario on the no-GPU mock lane is enough to push + # `dash-gen-tps-held-after-scrape-failure` and `dash-gen-tps-expiry-boundary` + # past the validity window they assert on. Measured here too — both failed + # 2/2 mock-lane runs with this scenario ungated, and the lane is green with + # it on the nightly one. + # + # The per-PR guard that matters is unaffected: therock-next-02 still pins the + # DEFAULT on the mock lane, which is the direction a regression would take, + # and `wheel_composition_requests_the_toolchain_only_when_asked` pins both + # polarities as a unit test on every PR. # # The `@id:` still reads `-09-`: this scenario was written as therock-next-09 # and the display index moved when main landed one ahead of it. The id is the # stable identifier and is deliberately not renumbered with the index. - @id:therock-next-09-wheel-devel-adds-the-toolchain + @id:therock-next-09-wheel-devel-adds-the-toolchain @nightly Scenario: therock-next-10 - A pinned ROCm 10 wheel install adds the toolchain when asked Given a canonical release pip index fixture and a ROCm 10 pip index fixture When the user previews a wheel SDK install for arch gfx1200 pinned to ROCm 10.0.0 with the toolchain From 07c4df315c10436d5cba2985ad2cf21ee3f82d2e Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 22 Sep 2026 09:46:40 +0000 Subject: [PATCH 12/17] fix(install): resolve versions against the extras that were asked for `uv pip compile` was handed `rocm[libraries,devel,device-]` no matter what the install requested, so `rocm install sdk --version 10.x` without `--devel` still constrained the version choice by the resolvability of a toolchain the user declined -- and could fail outright with a toolchain-resolution error on a default install. The plan the CLI then printed named `rocm[libraries,device-...]`, disagreeing with what resolution had actually asked for. Version resolution and install composition are separate code paths: resolution runs first and decides which versions exist, composition runs second and decides what is installed. `include_devel` was threaded through the second only. It now reaches the first as well, and both build their `rocm` extras from `therock_sdk_extras`. The requirement lines are composed once, in `resolve_pip_runtime_from_index`, and the same value is both sent to `uv` and carried back on the resolution, so what the preview reports is what was sent rather than a re-derivation that could drift. The preview prints it as `version_resolution_specs:` beside `package_specs:`, which is what lets an acceptance scenario hold the two to the same extras. The progress line now reads the toolchain back out of the composed specs instead of branching on the flag a second time, and the doc comment on `therock_sdk_extras` names its two real callers rather than one that was deleted and one that never used it. Signed-off-by: Roman Inflianskas --- apps/rocm/src/therock.rs | 100 ++++++++++++++++++++++++++++++++++----- 1 file changed, 87 insertions(+), 13 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 908ac0d19..2f8f5e48c 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -511,6 +511,13 @@ struct PipRuntimeResolution { /// installed (no wheels) and we warn about it. `None` when a specific version /// was requested (the "latest" concept does not apply). newest_repo_version: Option, + /// The exact requirement lines handed to `uv pip compile` to choose this + /// version set. This is the value that was sent, not a re-derivation of it, + /// so the preview can show what resolution actually asked for and an + /// acceptance test can hold it to the same extras the install plan names. + /// `None` on the canonical layout, which picks versions by scraping each + /// package's simple index rather than by resolving a requirement set. + version_resolution_specs: Option>, package_versions: TheRockPipPackageVersions, /// The device payload the resolved source must supply for this host, /// decided against the targets that source actually publishes. @@ -1498,6 +1505,9 @@ fn resolve_latest_for_manifest( &wheel_compatibility, None, Some(layout), + // An update re-resolves for the install it will reinstall, so it + // must resolve against the extras that runtime already has. + manifest.includes_devel(), download_timeout_secs, )?; // Prefer the device payload this runtime was actually built with over @@ -1755,6 +1765,7 @@ fn install_wheel_runtime( &wheel_compatibility, version_selector, layout_override, + include_devel, )?; let device_target = device_target_override.map_or_else( || resolution.device_target.clone(), @@ -1850,6 +1861,15 @@ fn install_wheel_runtime( " platform_wheel_tags: {}", wheel_compatibility.platform_tags.join(",") ); + // What resolution asked for, printed beside what the install will ask for. + // The two are produced by different code paths from the same `include_devel` + // and must name the same extras; showing only the second would hide a + // resolve that constrained the version choice by a toolchain the user + // declined. Absent on the canonical layout, which resolves by scraping each + // package's index instead of compiling a requirement set. + if let Some(specs) = resolution.version_resolution_specs.as_ref() { + let _ = writeln!(output, " version_resolution_specs: {}", specs.join(" ")); + } let _ = writeln!( output, " package_specs: {}", @@ -2021,7 +2041,10 @@ fn install_wheel_runtime( .map(String::as_str) .collect::>() .as_slice(), - if include_devel { + // Read back out of the specs `uv` is being handed on this very call + // rather than off `include_devel` a second time, so the line a user + // watches cannot name a toolchain the install is not requesting. + if wheel_composition_includes_devel(Some(&wheel_composition)).unwrap_or(include_devel) { "install TheRock SDK with the compiler toolchain, torch stack, and resolved dependencies" } else { "install TheRock SDK, torch stack, and resolved dependencies" @@ -2098,8 +2121,20 @@ fn install_wheel_runtime( /// the download — and is only needed to *build* GPU code. Running models needs /// `libraries` alone, so the toolchain is installed only when asked for. /// -/// Shared so the install plan, the progress text, and the resolution failure all -/// name the same extras instead of drifting apart. +/// Two callers, and they are the two that must agree: +/// +/// - [`therock_pip_package_specs`], which produces the specs `uv` installs and +/// the specs recorded in the manifest, and +/// - [`published_pip_requirements`], the requirement set `uv pip compile` is +/// asked to resolve versions against on the ROCm 10 (`next`) layout. +/// +/// Version resolution and install composition are separate code paths — +/// resolution runs first and decides which versions exist, composition runs +/// second and decides what is installed — so a flag threaded correctly through +/// one says nothing about the other. Both go through this helper for that +/// reason. Everything else that names the toolchain (the install progress line, +/// `runtimes list`, `InstalledRuntimeManifest::includes_devel`) reads it back +/// out of the composed specs rather than re-deciding it. const fn therock_sdk_extras(include_devel: bool) -> &'static str { if include_devel { "libraries,devel" @@ -2108,6 +2143,32 @@ const fn therock_sdk_extras(include_devel: bool) -> &'static str { } } +/// The requirement lines handed to `uv pip compile` to pick a mutually +/// installable version set on the ROCm 10 (`next`) layout. +/// +/// Unversioned for torch/torchvision/torchaudio on purpose: choosing those +/// versions is what the resolve is for. The `rocm` extras, though, must match +/// the ones [`therock_pip_package_specs`] will install — resolving against +/// `devel` for someone who did not ask for it constrains the chosen versions by +/// a toolchain they declined, and can fail the whole install with a +/// toolchain-resolution error on a default `rocm install sdk`. +fn published_pip_requirements( + rocm_version: &str, + device_target: &str, + include_devel: bool, +) -> Vec { + let device_extra = format!("device-{device_target}"); + vec![ + format!( + "rocm[{},{device_extra}]=={rocm_version}", + therock_sdk_extras(include_devel) + ), + format!("torch[{device_extra}]"), + format!("torchvision[{device_extra}]"), + "torchaudio".to_owned(), + ] +} + /// Package specs for a wheel SDK install. /// /// The `device-` extra is separate from [`therock_sdk_extras`] and @@ -2866,6 +2927,7 @@ fn resolve_pip_runtime( wheel_compatibility: &WheelCompatibility, version_selector: Option<&RuntimeVersionSelector>, layout_override: Option, + include_devel: bool, ) -> Result { resolve_pip_runtime_with_timeout( paths, @@ -2874,6 +2936,7 @@ fn resolve_pip_runtime( wheel_compatibility, version_selector, layout_override, + include_devel, None, ) } @@ -2888,6 +2951,7 @@ fn resolve_pip_runtime_with_timeout( wheel_compatibility: &WheelCompatibility, version_selector: Option<&RuntimeVersionSelector>, layout_override: Option, + include_devel: bool, download_timeout_secs: Option, ) -> Result { let family_resolution = resolve_family(paths, family_override)?; @@ -2939,6 +3003,7 @@ fn resolve_pip_runtime_with_timeout( &source, wheel_compatibility, version_selector, + include_devel, download_timeout_secs, ) .with_context(|| { @@ -2951,6 +3016,10 @@ fn resolve_pip_runtime_with_timeout( }) } +/// `include_devel` reaches this far because version resolution, not only install +/// composition, has to ask for the extras the caller actually requested — see +/// [`published_pip_requirements`]. +#[allow(clippy::too_many_arguments)] fn resolve_pip_runtime_from_index( paths: &AppPaths, channel: TheRockChannel, @@ -2958,9 +3027,11 @@ fn resolve_pip_runtime_from_index( source: &ResolvedAggregateWheelSource, wheel_compatibility: &WheelCompatibility, version_selector: Option<&RuntimeVersionSelector>, + include_devel: bool, download_timeout_secs: Option, ) -> Result { let index_url = source.index_url.as_str(); + let mut version_resolution_specs = None; let rocm_versions = load_simple_index_versions(paths, index_url, "rocm", None, download_timeout_secs)?; if matches!(channel, TheRockChannel::Release) @@ -2997,14 +3068,16 @@ fn resolve_pip_runtime_from_index( // budget, not the single-fetch one — reusing the bare per-fetch value // here would make a startup check that budgets 2s per fetch reliably // time out a call doing 4 fetches' worth of work. - resolve_published_pip_package_versions( + let requirements = published_pip_requirements(&rocm_version, device_target, include_devel); + let versions = resolve_published_pip_package_versions( paths, index_url, - &rocm_version, - device_target, + &requirements, wheel_compatibility, download_timeout_secs.map(|secs| secs.saturating_mul(4)), - )? + )?; + version_resolution_specs = Some(requirements); + versions } else { let torch_versions = load_simple_index_versions( paths, @@ -3063,6 +3136,7 @@ fn resolve_pip_runtime_from_index( layout: source.layout, latest_version, newest_repo_version, + version_resolution_specs, package_versions, device_target: source.device_target.clone(), published_device_targets: source.published_device_targets.clone(), @@ -3450,11 +3524,13 @@ fn wait_with_output_bounded(mut child: Child, timeout: Option) -> Resu } } +/// `requirements` is passed in rather than composed here so that the lines sent +/// to `uv` and the lines the resolution reports back (and the preview prints) +/// are one value, not two that can disagree. fn resolve_published_pip_package_versions( paths: &AppPaths, index_url: &str, - rocm_version: &str, - device_target: &str, + requirements: &[String], compatibility: &WheelCompatibility, download_timeout_secs: Option, ) -> Result { @@ -3462,10 +3538,7 @@ fn resolve_published_pip_package_versions( ensure_uv_binary(paths).context("failed to acquire uv for ROCm X metadata resolution")?; let python_version = uv_python_version(compatibility)?; let python_platform = uv_python_platform(compatibility)?; - let device_extra = format!("device-{device_target}"); - let requirements = format!( - "rocm[libraries,devel,{device_extra}]=={rocm_version}\ntorch[{device_extra}]\ntorchvision[{device_extra}]\ntorchaudio\n" - ); + let requirements = format!("{}\n", requirements.join("\n")); let mut child = Command::new(&uv) .args([ "pip", @@ -8095,6 +8168,7 @@ mod tests { layout: SourceLayout::Canonical, latest_version: "7.13.0".to_owned(), newest_repo_version: None, + version_resolution_specs: None, package_versions: TheRockPipPackageVersions { rocm: "7.13.0".to_owned(), torch: "2.11.0+rocm7.13.0".to_owned(), From 3f249abd5dd9a294fe2c290ebbc15dff401f2014 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 22 Sep 2026 09:53:50 +0000 Subject: [PATCH 13/17] test(install): pin what version resolution asks uv for The existing coverage could not see the resolution path at all. The unit tests pass `include_devel` literally, so they pin the helpers rather than what `install_wheel_runtime` passes them, and the preview scenarios only asserted `package_specs` -- which is composed after resolution has already chosen the versions. Three assertions now close that: - `version_resolution_requests_the_toolchain_only_when_asked` pins both polarities of the requirement set handed to `uv pip compile`. - `resolution_and_install_name_the_same_rocm_extras` pins the two paths against each other, which is the actual failure mode: each was self-consistent while they disagreed with one another. - the preview scenarios assert `version_resolution_specs:` alongside `package_specs:`, so the wiring is covered and not only the helpers. Verified by injecting each regression and watching the right test go red: hardcoding the requirements to `rocm[libraries,devel,...]` fails both new unit tests; hardcoding only the resolution argument inside `install_wheel_runtime` fails therock-next-02, which previously passed with that exact bug present. Signed-off-by: Roman Inflianskas --- apps/rocm/src/therock.rs | 77 +++++++++++++++++++ tests/e2e-cucumber/tests/e2e/therock_steps.rs | 23 ++++++ 2 files changed, 100 insertions(+) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 2f8f5e48c..d8c236eae 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -8217,6 +8217,83 @@ mod tests { assert_eq!(runtime_only.rocm_sdk_target, with_devel.rocm_sdk_target); } + /// The other half of the same claim, on the *resolution* path. + /// + /// `uv pip compile` decides which versions exist before anything is + /// composed, so asking it for `devel` on a default install constrains the + /// version choice by a toolchain the user declined — and fails the install + /// outright when no version can satisfy it. That is a separate code path + /// from `wheel_runtime_composition` above, and the two must name the same + /// extras. + #[test] + fn version_resolution_requests_the_toolchain_only_when_asked() { + let runtime_only = published_pip_requirements("10.0.0", "gfx1200", false); + assert_eq!( + runtime_only[0], "rocm[libraries,device-gfx1200]==10.0.0", + "a default install must not resolve against the toolchain: {runtime_only:?}" + ); + + let with_devel = published_pip_requirements("10.0.0", "gfx1200", true); + assert_eq!( + with_devel[0], "rocm[libraries,devel,device-gfx1200]==10.0.0", + "--devel must reach the resolved requirements: {with_devel:?}" + ); + + // Only the `rocm` requirement carries the toolchain axis; the torch + // stack is unversioned here because choosing those versions is what the + // resolve is for. + assert_eq!( + runtime_only[1..], + with_devel[1..], + "devel must only affect the rocm requirement" + ); + assert_eq!( + runtime_only[1..], + [ + "torch[device-gfx1200]".to_owned(), + "torchvision[device-gfx1200]".to_owned(), + "torchaudio".to_owned(), + ] + ); + } + + /// Resolution and composition are two code paths reading one flag, and the + /// bug this pins is them disagreeing: the requirements handed to `uv` were + /// hardcoded to `devel` while the plan printed and installed `libraries`. + /// Asserting each in isolation cannot catch that; asserting they agree can. + #[test] + fn resolution_and_install_name_the_same_rocm_extras() { + let resolution = devel_test_resolution(); + let target = AggregateDeviceTarget::Exact("gfx942".to_owned()); + + for include_devel in [false, true] { + let requirements = published_pip_requirements( + &resolution.package_versions.rocm, + "gfx942", + include_devel, + ); + let composition = wheel_runtime_composition(&resolution, &target, include_devel); + assert_eq!( + rocm_requirement_extras(&requirements[0]), + rocm_requirement_extras(&composition.package_specs[0]), + "resolution and install must request the same rocm extras for \ + include_devel={include_devel}: {requirements:?} vs {:?}", + composition.package_specs + ); + } + } + + /// The `...` of a `rocm[...]==version` requirement. + fn rocm_requirement_extras(requirement: &str) -> &str { + requirement + .strip_prefix("rocm[") + .and_then(|rest| rest.split_once(']')) + .map_or_else( + || panic!("not a rocm extras requirement: {requirement}"), + |(extras, _)| extras, + ) + } + /// The manifest answer is derived from the specs that were installed, so a /// recorded composition and the `devel` field can never disagree. #[test] diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index 00f56dbdd..64f3a01c3 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -408,6 +408,7 @@ async fn preview_requests_device_extras_with_devel(world: &mut E2eWorld) { torchaudio=={NEXT_TORCHAUDIO_VERSION}" ); assert_contains(world, &expected, "device extras with devel"); + assert_version_resolution_extras(world, true); } /// Distinct from [`preview_pinned_wheel_install`]: this scenario's whole point @@ -609,6 +610,7 @@ async fn preview_requests_device_extras(world: &mut E2eWorld) { torchaudio=={NEXT_TORCHAUDIO_VERSION}" ); assert_contains(world, &expected, "device extras"); + assert_version_resolution_extras(world, false); assert_contains( world, &format!("device_target: {RAW_ARCH}"), @@ -616,6 +618,27 @@ async fn preview_requests_device_extras(world: &mut E2eWorld) { ); } +/// What version resolution asked `uv pip compile` for, as opposed to what the +/// install plan says it will install. +/// +/// These are produced by two different code paths from one `include_devel`, and +/// only this assertion covers the first. `package_specs` alone passed while the +/// requirements handed to `uv` were hardcoded to `rocm[libraries,devel,...]`, +/// which constrained the chosen versions by a toolchain a default install never +/// asked for. +fn assert_version_resolution_extras(world: &mut E2eWorld, include_devel: bool) { + let extras = if include_devel { + "libraries,devel" + } else { + "libraries" + }; + let expected = format!( + "version_resolution_specs: rocm[{extras},device-{RAW_ARCH}]=={NEXT_ROCM_VERSION} \ + torch[device-{RAW_ARCH}] torchvision[device-{RAW_ARCH}] torchaudio" + ); + assert_contains(world, &expected, "version resolution extras"); +} + #[then("the preview resolves the ROCm 10 tarball catalog")] async fn preview_resolves_next_tarball_catalog(world: &mut E2eWorld) { let base = next_tarball_base(world); From ef1b527d03d3ba3637250336f1472d5e0abafbc1 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 22 Sep 2026 09:53:59 +0000 Subject: [PATCH 14/17] Revert "test(e2e): move the --devel preview to the nightly lane" This reverts commit 2f23310f76e2c183616f16259392457e4e331ccb. The two dash scenarios that gating was meant to protect -- `dash-gen-tps-held-after-scrape-failure` and `dash-gen-tps-expiry-boundary` -- now hold their observation clock instead of racing the host (#412, on main). That is the layer that was actually broken, so displacing real coverage to work around it is no longer warranted. Gating this scenario made the guard asymmetric: hardcoding `include_devel = true` inside `install_wheel_runtime` still fails therock-next-02, but hardcoding it to FALSE -- making `--devel` a silent no-op on every real install -- passed every blocking check. Verified by injecting exactly that: with therock-next-09 back on the mock lane, it fails. The comment no longer claims a per-PR unit-test guard for this seam. `wheel_composition_requests_the_toolchain_only_when_asked` supplies the flag literally, so it cannot observe what `install_wheel_runtime` passes. Signed-off-by: Roman Inflianskas --- .../features/therock_next_generation.feature | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/tests/e2e-cucumber/features/therock_next_generation.feature b/tests/e2e-cucumber/features/therock_next_generation.feature index 4096672b9..fe4362cb5 100644 --- a/tests/e2e-cucumber/features/therock_next_generation.feature +++ b/tests/e2e-cucumber/features/therock_next_generation.feature @@ -145,24 +145,29 @@ Feature: TheRock "next" ROCm 10 install layout When the user reinstalls vllm Then the install reports the vLLM ROCm 10.x discovery pins - # The opt-in half of therock-next-02, which covers the default. + # The opt-in half of therock-next-02. Both polarities run here, on the mock + # lane, because this is the only place the flag's effect on the real install + # plan is observable without a GPU and a multi-GiB download. # - # `@nightly` for the reason runtime-06 documents: one more index-resolving - # scenario on the no-GPU mock lane is enough to push - # `dash-gen-tps-held-after-scrape-failure` and `dash-gen-tps-expiry-boundary` - # past the validity window they assert on. Measured here too — both failed - # 2/2 mock-lane runs with this scenario ungated, and the lane is green with - # it on the nightly one. + # Deliberately not `@nightly`. Gating it would make the asymmetry that + # therock-next-02 alone cannot cover: hardcoding `include_devel = true` inside + # `install_wheel_runtime` fails therock-next-02, but hardcoding it to FALSE — + # making `--devel` a silent no-op on every real install — would pass every + # blocking check with this scenario off the lane. The unit tests cannot close + # that gap: they pass the flag literally, so they pin the helpers, not what + # `install_wheel_runtime` passes them. # - # The per-PR guard that matters is unaffected: therock-next-02 still pins the - # DEFAULT on the mock lane, which is the direction a regression would take, - # and `wheel_composition_requests_the_toolchain_only_when_asked` pins both - # polarities as a unit test on every PR. + # This scenario was briefly moved to the nightly lane because the extra + # index-resolving work tipped `dash-gen-tps-held-after-scrape-failure` and + # `dash-gen-tps-expiry-boundary` past the validity window they assert on. + # Those two now hold their observation clock instead of racing the host + # (rocm-cli#412), which is the layer that was actually broken, so the reason + # to displace this coverage is gone. # # The `@id:` still reads `-09-`: this scenario was written as therock-next-09 # and the display index moved when main landed one ahead of it. The id is the # stable identifier and is deliberately not renumbered with the index. - @id:therock-next-09-wheel-devel-adds-the-toolchain @nightly + @id:therock-next-09-wheel-devel-adds-the-toolchain Scenario: therock-next-10 - A pinned ROCm 10 wheel install adds the toolchain when asked Given a canonical release pip index fixture and a ROCm 10 pip index fixture When the user previews a wheel SDK install for arch gfx1200 pinned to ROCm 10.0.0 with the toolchain From c9207a14c3da384684c00ff61e148fcff92c3c73 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Wed, 23 Sep 2026 09:55:16 +0000 Subject: [PATCH 15/17] fix(storage): keep the toolchain out of the runtime-only prune bucket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making the compiler opt-in turned `rocm install sdk` and `rocm install sdk --devel` at one version into two separate runtimes, because the runtime key hashes the requested package specs. `remove-old-installs` grouped by channel, format and family only, so those two competed for the same `--keep` slots and recency decided which survived: a later runtime-only install could evict a multi-gigabyte toolchain the user had explicitly asked for, with nothing in the output naming it as a choice. They are not newer and older versions of the same thing, so they no longer share a bucket. The active, default and rollback runtimes were already held unconditionally, so only a non-active devel runtime was ever exposed — which is exactly the case nothing else protected. Signed-off-by: Roman Inflianskas --- README.md | 15 +++--- apps/rocm/src/main.rs | 9 ++-- apps/rocm/src/storage.rs | 105 +++++++++++++++++++++++++++++++++++---- xtask/src/main.rs | 4 +- 4 files changed, 113 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 1440615f8..583c9832c 100644 --- a/README.md +++ b/README.md @@ -404,12 +404,15 @@ rocm storage remove-downloads [--dry-run] [--yes] ``` `remove-old-installs` keeps the two most recent installs for each channel, -format, and GPU family, and never touches the install in use, the rollback -target, or a folder rocm-cli did not create. "Most recent" means most recently -installed rather than highest version, so after a deliberate downgrade the -older version counts as the newer install. Because the count applies per -channel, format, and GPU family, a machine that has tried several channels -keeps `--keep` installs for each of them. Anything it declines to remove is +format, GPU family, and toolchain choice, and never touches the install in use, +the rollback target, or a folder rocm-cli did not create. "Most recent" means +most recently installed rather than highest version, so after a deliberate +downgrade the older version counts as the newer install. Because the count +applies per channel, format, GPU family, and toolchain choice, a machine that +has tried several channels keeps `--keep` installs for each of them — and a +runtime-only install never evicts a `--devel` one, since the two are separate +runtimes serving different purposes rather than newer and older versions of the +same thing. Anything it declines to remove is listed with the reason, and `--dry-run` shows the whole plan without changing anything. `remove-downloads` clears cached archives that rocm-cli can download again; a cache folder that is a link to somewhere else is left alone rather diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 5aa649e81..c1470c232 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -776,12 +776,15 @@ enum StorageCommand { /// Remove older ROCm installs, keeping the most recent ones. #[command(name = "remove-old-installs", alias = "remove-old-runtimes")] RemoveOldInstalls { - /// How many recent installs to keep for each channel, format, and GPU family. + /// How many recent installs to keep for each channel, format, GPU + /// family, and toolchain choice. /// /// "Recent" means most recently installed, not highest version, so /// after a deliberate downgrade the older version counts as the newer - /// install. The one in use and the rollback target are always kept on - /// top of this count, whatever it is set to. + /// install. A `--devel` install and a plain one are separate runtimes + /// and get separate counts, so a newer runtime-only install never + /// evicts the toolchain. The one in use and the rollback target are + /// always kept on top of this count, whatever it is set to. #[arg(long, default_value_t = storage::DEFAULT_KEEP)] keep: usize, /// Show what would happen without changing files. diff --git a/apps/rocm/src/storage.rs b/apps/rocm/src/storage.rs index 075e062b7..3e196ed86 100644 --- a/apps/rocm/src/storage.rs +++ b/apps/rocm/src/storage.rs @@ -32,9 +32,10 @@ use crate::{ should_remove_runtime_install_root, therock, }; -/// Recent installs kept per channel/format/family by default: the one in use -/// plus one rollback target. This is the natural floor rather than a tuned -/// value — see the maintainer question in the pull request description. +/// Recent installs kept per retention bucket by default (see +/// [`retention_group`]): the one in use plus one rollback target. This is the +/// natural floor rather than a tuned value — see the maintainer question in the +/// pull request description. pub(crate) const DEFAULT_KEEP: usize = 2; // --------------------------------------------------------------------------- @@ -174,7 +175,9 @@ impl HoldReason { Self::Default => "the configured default", Self::Marker => "named by the active install marker", Self::NotOwned => "added with adopt or import, so ROCm CLI does not own the folder", - Self::WithinKeepLimit => "one of the most recent installs kept for this GPU family", + Self::WithinKeepLimit => { + "one of the most recent installs kept for this GPU family and toolchain choice" + } } } } @@ -268,12 +271,23 @@ pub(crate) fn unconditional_hold( } /// Retention group: a multi-GPU machine legitimately keeps one install per -/// family, so recency is only ever compared inside a channel/format/family. -fn retention_group(manifest: &therock::InstalledRuntimeManifest) -> (String, String, String) { +/// family, so recency is only ever compared inside a +/// channel/format/family/toolchain bucket. +/// +/// The toolchain axis exists because the compiler is opt-in: `rocm install sdk` +/// and `rocm install sdk --devel` at the same version produce two *separate* +/// runtimes, since `wheel_runtime_key` hashes the requested package specs. +/// Without this axis they compete for the same `--keep` slots, so prune can +/// retain the newer runtime-only install and delete the toolchain one — a +/// multi-gigabyte download the user explicitly asked for, gone without ever +/// being named as a choice. They are not substitutes for each other, so they do +/// not compete. +fn retention_group(manifest: &therock::InstalledRuntimeManifest) -> (String, String, String, bool) { ( manifest.channel.to_ascii_lowercase(), manifest.format.to_ascii_lowercase(), manifest.family.to_ascii_lowercase(), + manifest.includes_devel(), ) } @@ -289,8 +303,10 @@ pub(crate) fn select_runtimes_to_remove( keep: usize, ) -> (Vec, Vec<(String, HoldReason)>) { let mut held: Vec<(String, HoldReason)> = Vec::new(); - let mut groups: BTreeMap<(String, String, String), Vec<&therock::InstalledRuntimeManifest>> = - BTreeMap::new(); + let mut groups: BTreeMap< + (String, String, String, bool), + Vec<&therock::InstalledRuntimeManifest>, + > = BTreeMap::new(); let default_key = resolved_default_runtime_key(manifests, inputs); for manifest in manifests { @@ -660,7 +676,8 @@ pub(crate) fn render_prune_plan(plan: &PrunePlan, keep: usize, dry_run: bool) -> let _ = writeln!(output); let _ = writeln!( output, - "Keeping the {keep} most recent install(s) for each channel, format, and GPU family." + "Keeping the {keep} most recent install(s) for each channel, format, GPU family, and \ + toolchain choice." ); let _ = writeln!(output); if plan.remove.is_empty() { @@ -985,6 +1002,27 @@ mod tests { } } + /// The same manifest with the compiler toolchain left out, recorded the way + /// a real `rocm install sdk` (no `--devel`) records it: the answer lives in + /// the specs `uv` was handed, and `includes_devel` reads it back out of + /// them. Writing the `devel` field alone would test a fallback rather than + /// the path every wheel install actually takes. + fn runtime_only( + mut record: therock::InstalledRuntimeManifest, + device_target: &str, + ) -> therock::InstalledRuntimeManifest { + record.devel = false; + record.wheel_composition = Some(therock::WheelRuntimeComposition { + source_layout_generation: "canonical".to_owned(), + package_specs: vec![format!( + "rocm[libraries,device-{device_target}]=={}", + record.version + )], + rocm_sdk_target: Some(device_target.to_owned()), + }); + record + } + fn test_paths(name: &str) -> (PathBuf, AppPaths) { let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("..") @@ -1067,6 +1105,55 @@ mod tests { ); } + /// A toolchain install and a runtime-only install of the same channel, + /// format and family are two separate runtimes, because `wheel_runtime_key` + /// hashes the requested specs. If they shared a retention bucket, prune + /// would rank them by recency alone and delete the multi-gigabyte toolchain + /// the user explicitly asked for — silently, since the failure only shows + /// up much later as a missing compiler. Only a *non-active* devel runtime + /// is exposed (the active and default ones are held unconditionally), which + /// is exactly the case nothing else protects. + #[test] + fn a_runtime_only_install_never_evicts_the_toolchain_install_it_sits_beside() { + let manifests = vec![ + manifest("release-wheel-gfx120x-devel-1", "gfx120X-all", "7.13.0", 10), + manifest("release-wheel-gfx120x-devel-2", "gfx120X-all", "7.14.0", 20), + runtime_only( + manifest("release-wheel-gfx120x-only-1", "gfx120X-all", "7.13.0", 30), + "gfx1201", + ), + runtime_only( + manifest("release-wheel-gfx120x-only-2", "gfx120X-all", "7.14.0", 40), + "gfx1201", + ), + ]; + + // `keep = 1` so each bucket has something to give up: the axis must + // separate the two toolchain choices without also making `--keep` inert + // inside either of them. + let (removable, held) = + select_runtimes_to_remove(&manifests, &RetentionInputs::default(), 1); + + assert_eq!( + removable, + vec![ + "release-wheel-gfx120x-devel-1".to_owned(), + "release-wheel-gfx120x-only-1".to_owned(), + ], + "prune must drop the older install of each toolchain choice, not the \ + older toolchain choice" + ); + let kept: Vec<&str> = held.iter().map(|(key, _)| key.as_str()).collect(); + assert_eq!( + kept, + vec![ + "release-wheel-gfx120x-devel-2", + "release-wheel-gfx120x-only-2", + ], + "the newest install of each toolchain choice must survive" + ); + } + #[test] fn never_selects_the_active_previous_default_or_marked_install() { let manifests = vec![ diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 11bf032f5..5b5fbeb64 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -189,8 +189,8 @@ enum Command { /// TheRock package channel the shared runtime should track. #[arg(long, default_value = "release")] channel: String, - /// Recent installs to keep per channel, format, and GPU family when - /// pruning after an install or update. + /// Recent installs to keep per channel, format, GPU family, and + /// toolchain choice when pruning after an install or update. #[arg(long, default_value_t = 2)] keep: usize, /// Pre-warm root holding `config/`, `data/`, and `cache/`. Its From a36821af9dfafef254f24ac546066eced961b867 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Wed, 23 Sep 2026 10:00:41 +0000 Subject: [PATCH 16/17] feat(examine): report whether the active runtime has the toolchain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rocm runtimes list` reports `toolchain=included|excluded` per runtime, but `rocm examine` — the command a user runs first when something ROCm is wrong — said nothing about it. With the compiler now opt-in, "my build cannot find hipcc" is a state the CLI can produce by design, and the primary diagnostic could not answer it. `runtime-01` gains the user-visible half of the assertion it already made against the manifest, so a recorded-correctly/reported-silently split fails. Also stop unwrapping `wheel_composition` in the toolchain-exclusion step. Absent is a reachable manifest state — tarball installs and adopted runtimes have none — so reusing the step outside a wheel install reported a legitimate precondition mismatch as a missing-field crash. Signed-off-by: Roman Inflianskas --- apps/rocm/src/main.rs | 101 +++++++++++++++--- .../features/runtime_setup.feature | 6 ++ tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 40 ++++++- 3 files changed, 134 insertions(+), 13 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index c1470c232..f461585f1 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -8645,14 +8645,10 @@ pub(crate) fn render_runtimes_text(paths: &AppPaths, config: &RocmCliConfig) -> } else { "managed" }; - // `toolchain` is the only runtime property a user cannot otherwise - // see: the compiler is opt-in, and `rocm update` reinstalls whatever - // this says, so an install missing it should not be silent about that. - let toolchain = if manifest.includes_devel() { - "included" - } else { - "excluded" - }; + // The compiler is opt-in and `rocm update` reinstalls whatever this + // says, so an install missing it should not be silent about that. + // `rocm examine` reports the same thing for the active runtime. + let toolchain = toolchain_state_text(manifest.includes_devel()); let _ = writeln!( output, " {marker} {} runtime_id={} version={} format={} family={} mode={} status={} toolchain={}", @@ -11464,6 +11460,20 @@ pub(crate) fn runtime_usability_status(manifest: &therock::InstalledRuntimeManif } } +/// How the CLI names the toolchain state of a runtime. +/// +/// `rocm runtimes list` reports it per runtime and `rocm examine` reports it +/// for the active one; sharing the vocabulary here keeps the two from drifting +/// into different words for the same fact, which is the kind of difference a +/// user reads as a difference in meaning. +pub(crate) const fn toolchain_state_text(includes_devel: bool) -> &'static str { + if includes_devel { + "included" + } else { + "excluded" + } +} + fn validate_runtime_manifest_for_activation( manifest: &therock::InstalledRuntimeManifest, ) -> Result<()> { @@ -16032,6 +16042,16 @@ fn append_examine_runtime_state( therock::runtime_version_display(&manifest.version) ); let _ = writeln!(output, " active_runtime_family: {}", manifest.family); + // The same fact `rocm runtimes list` reports as `toolchain=`, for the + // one runtime that is actually in use. `examine` is where a user looks + // when a build fails on a missing `hipcc`, and without this line the + // command that exists to answer "what is my ROCm state" could not say + // whether the active runtime has a compiler at all. + let _ = writeln!( + output, + " active_runtime_toolchain: {}", + toolchain_state_text(manifest.includes_devel()) + ); let mode = if manifest.read_only { "read-only" } else { @@ -32244,10 +32264,11 @@ ID_LIKE="suse opensuse" Ok(()) } - /// Whether a runtime carries the compiler toolchain is otherwise invisible: - /// nothing else in the CLI reports it, and `rocm update` reinstalls whatever - /// the runtime recorded — so a user who installed without it has no way to - /// see that, or to understand why a later build step fails. + /// Whether a runtime carries the compiler toolchain is otherwise invisible, + /// and `rocm update` reinstalls whatever the runtime recorded — so a user + /// who installed without it has no way to see that, or to understand why a + /// later build step fails. This is the per-runtime half; `rocm examine` + /// reports the same fact for the active one. #[test] fn runtime_list_reports_whether_the_toolchain_is_installed() -> Result<()> { let (root, paths) = test_paths("runtime-list-toolchain"); @@ -35437,6 +35458,62 @@ ID_LIKE="suse opensuse" Ok(()) } + /// `rocm examine` is the first command a user runs when something ROCm is + /// wrong, and "my build cannot find `hipcc`" is now a reachable state by + /// design. Reporting the toolchain only from `rocm runtimes list` leaves + /// the primary diagnostic unable to answer the question the opt-in + /// created. Both polarities are pinned, because a field hardcoded to either + /// word reads as working from a single run. + #[test] + fn examine_runtime_state_reports_whether_the_active_runtime_has_the_toolchain() -> Result<()> { + let (root, paths) = test_paths("examine-runtime-toolchain"); + let manifest = write_test_pip_runtime( + &paths, + "release-pip-gfx120x-all-7-13-0", + "therock-release:gfx120X-all", + "7.13.0", + 10, + )?; + let config = RocmCliConfig { + active_runtime_key: Some(manifest.runtime_key.clone()), + ..RocmCliConfig::default() + }; + + // The fixture records `devel: true` with no composition. + let mut output = String::new(); + append_examine_runtime_state(&mut output, &paths, &config)?; + assert!( + output.contains("active_runtime_toolchain: included"), + "a toolchain runtime must say so:\n{output}" + ); + + // A runtime-only install reports the other way. Written through the + // recorded specs, which is what `includes_devel` actually reads, so + // this exercises the same path a real `rocm install sdk` produces. + let runtime_only = therock::InstalledRuntimeManifest { + devel: false, + wheel_composition: Some(therock::WheelRuntimeComposition { + source_layout_generation: "canonical".to_owned(), + package_specs: vec!["rocm[libraries,device-gfx1201]==7.13.0".to_owned()], + rocm_sdk_target: Some("gfx1201".to_owned()), + }), + ..manifest + }; + fs::write( + runtime_manifest_path(&paths, &runtime_only.runtime_key), + serde_json::to_vec_pretty(&runtime_only)?, + )?; + output.clear(); + append_examine_runtime_state(&mut output, &paths, &config)?; + assert!( + output.contains("active_runtime_toolchain: excluded"), + "a runtime-only runtime must say so:\n{output}" + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + #[test] fn examine_runtime_state_reports_ambiguous_default_runtime_id() -> Result<()> { let (root, paths) = test_paths("examine-runtime-ambiguous"); diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index a14c56c64..b9d74b2ac 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -5,6 +5,11 @@ Feature: Runtime configuration # activates, still carries an inference engine, and omits the compiler # toolchain. Pinning this to one engine would drop that check on the lanes # where that engine is not the effective one. + # + # The last two Thens are the same fact from the two surfaces that can + # disagree: what the install recorded, and what the diagnostic tells the user + # it recorded. The toolchain is now something a user can end up without, so + # `rocm examine` staying silent about it is its own defect. @id:runtime-install-sdk-active @requires-gpu @nightly Scenario: runtime-01 - Installing the SDK makes it the active runtime Given a machine with no CLI-managed runtimes @@ -13,6 +18,7 @@ Feature: Runtime configuration And the runtime is set as active And the runtime includes an inference engine And the runtime excludes the compiler toolchain + And the inspection reports the active runtime has no compiler toolchain # Dogfooding #17: re-provisioning was observed writing inside the previous # runtime, producing a recursively nested `runtimes/wheel/.../runtimes/wheel/` diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index d16806d98..b429444ef 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -972,11 +972,30 @@ async fn assert_runtime_excludes_devel(world: &mut E2eWorld) { // and `InstalledRuntimeManifest::includes_devel` reads the answer back out // of them. Asserting the `devel` field alone would miss the install args // and the manifest disagreeing, which is the drift this guards. + // + // `wheel_composition` is legitimately absent on a tarball install and on an + // adopted runtime, so an unwrap here would report a reachable manifest + // state as a missing-field crash the moment this step is reused outside a + // wheel install. Name the precondition instead: the failure a future + // scenario author needs to read is "this step only applies to wheel + // installs", not "Option::unwrap on a None value". + let format = manifest + .get("format") + .and_then(serde_json::Value::as_str) + .unwrap_or(""); let specs = manifest .get("wheel_composition") .and_then(|composition| composition.get("package_specs")) .and_then(serde_json::Value::as_array) - .expect("wheel runtime manifest has no recorded package_specs"); + .unwrap_or_else(|| { + panic!( + "this step reads the extras a wheel install requested, but runtime {} \ + (format={format}) records no wheel_composition.package_specs. Tarball \ + installs and adopted runtimes have none — use this step only on a \ + wheel-format install.\n{manifest}", + manifest_path.display() + ) + }); let rocm_spec = specs .iter() .filter_map(serde_json::Value::as_str) @@ -1021,6 +1040,25 @@ async fn assert_runtime_excludes_devel(world: &mut E2eWorld) { ); } +/// The manifest half of this is `the runtime excludes the compiler toolchain`, +/// which reads the recorded specs directly. This is the half a user can see: +/// `rocm examine` is where someone looks when a build cannot find `hipcc`, and +/// a manifest that records the right thing while the diagnostic stays silent +/// about it is indistinguishable, from the outside, from one that does not. +#[then("the inspection reports the active runtime has no compiler toolchain")] +async fn assert_examine_reports_no_toolchain(world: &mut E2eWorld) { + let examine = crate::run_rocm_ok(world, &["examine"]); + let reported = super::examine_steps::field_value(&examine, "active_runtime_toolchain") + .unwrap_or_else(|| { + panic!("`rocm examine` reported no toolchain state for the active runtime:\n{examine}") + }); + assert_eq!( + reported, "excluded", + "the SDK was installed without --devel, but `rocm examine` reports the active \ + runtime's toolchain as `{reported}`:\n{examine}" + ); +} + #[then("the managed runtime folder path is not recursively nested")] async fn assert_runtime_path_not_nested(world: &mut E2eWorld) { // `rocm examine` prints `Folder: ` for the active runtime. From 92f363fa625b388857964317358c3184f04018ce Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Wed, 23 Sep 2026 10:02:21 +0000 Subject: [PATCH 17/17] docs: say that --devel creates a second runtime, not a bigger one A runtime is keyed by the packages it was installed from, so `rocm install sdk` followed by `rocm install sdk --devel` leaves two side-by-side runtimes and activates the second. Nothing said so: a user expecting the toolchain to be added to the runtime they already had gets a second multi-gigabyte install instead, and only finds out from the disk usage. Stated where the flag is introduced (README and its --help) and where side-by-side runtimes are the subject (runtime management), with the commands that show which runtime is which and how to remove the one that is not wanted. Signed-off-by: Roman Inflianskas --- README.md | 18 +++++++++++++++++- apps/rocm/src/main.rs | 7 +++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 583c9832c..7de1e927d 100644 --- a/README.md +++ b/README.md @@ -311,7 +311,16 @@ rocm update [--apply] [--runtime KEY] [--activate] [--dry-run] `install sdk` downloads TheRock ROCm wheels into a Python environment managed by rocm-cli; pass `--devel` to also install the compiler and headers needed to -build GPU code, roughly doubling the download. +build GPU code, roughly doubling the download. `--devel` is not an addition to +an existing runtime: a runtime is identified by the packages it was installed +from, so running `rocm install sdk` and later `rocm install sdk --devel` at the +same version leaves you with **two** side-by-side runtimes — the second is a +fresh full install, not a toolchain bolted onto the first — and the second one +becomes active. `rocm runtimes list` marks each one `toolchain=included` or +`toolchain=excluded`, `rocm examine` reports the active runtime's as +`active_runtime_toolchain`, and `rocm storage remove-old-installs` counts the +two kinds separately so neither evicts the other. To reclaim the space, uninstall +the one you do not want with `rocm runtimes uninstall `. An install with no active default runtime never prompts, but once a managed runtime is the active default every `install sdk` asks first, because the new install takes over as the active default. That gate is not scoped to the @@ -383,6 +392,13 @@ rocm runtimes adopt --python [--root ] [--runtime-id ID] [--runtime-key KEY] [--channel LABEL] [--replace] ``` +`list` shows each runtime's version, GPU family, mode, and whether it carries +the compiler toolchain (`toolchain=included|excluded`). Runtimes are told apart +by the packages they were installed from, not by version alone, so one version +can appear more than once: a `--devel` install and a plain one of the same +version are two separate runtimes, as are two installs that resolved different +GPU device payloads. + `uninstall` prompts for confirmation unless `--yes` is passed; outside an interactive terminal `--yes` is required. `--dry-run` prints the plan and exits without prompting or making changes. diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index f461585f1..1c3120fa0 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -625,6 +625,13 @@ rocm install sdk --devel")] /// Also install the ROCm compiler, headers, and static libraries for /// building GPU code. Roughly doubles the wheel download; already /// included by `--format tarball`. + /// + /// This does not add the toolchain to a runtime you already have: a + /// runtime is identified by the packages it was installed from, so + /// passing --devel over an existing plain install of the same version + /// creates a SECOND side-by-side runtime and activates it. Remove the + /// one you do not want with `rocm runtimes uninstall `; + /// `rocm runtimes list` shows which is which. #[arg(long)] devel: bool, /// Resolve the install plan without changing files.