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..7de1e927d 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,18 @@ 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. `--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 family or channel you are installing: a `--family` or `--channel` you have never @@ -381,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. @@ -402,12 +420,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/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..1c3120fa0 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")] @@ -621,6 +622,18 @@ 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, 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. #[arg(long)] dry_run: bool, @@ -770,12 +783,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. @@ -2882,6 +2898,7 @@ fn install(target: InstallTarget) -> Result<()> { channel, format, prefix, + devel, version, build_date, family, @@ -2904,13 +2921,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, + sdk_install_request( + &channel, + format_name, + prefix, + version_selector, + family.as_deref(), + dry_run, + devel, + consents.replace_active_default, + ), ) { Ok(result) => { let therock::SdkInstallResult { output, mutated } = result; @@ -8632,16 +8652,21 @@ pub(crate) fn render_runtimes_text(paths: &AppPaths, config: &RocmCliConfig) -> } else { "managed" }; + // 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={}", + " {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, @@ -11223,6 +11248,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. @@ -11439,6 +11467,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<()> { @@ -15269,6 +15311,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)] +const 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"); @@ -15276,21 +15347,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 = 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 // 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) } @@ -15974,6 +16049,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 { @@ -18096,6 +18181,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 +18204,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, )?; @@ -28570,6 +28659,101 @@ 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:?}"), + } + } + + /// 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 [ @@ -32087,6 +32271,58 @@ ID_LIKE="suse opensuse" Ok(()) } + /// 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"); + 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"); @@ -32398,6 +32634,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)?; @@ -32414,6 +32657,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"); @@ -35144,6 +35465,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"); @@ -35694,6 +36071,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 +36113,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..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() { @@ -980,10 +997,32 @@ mod tests { wheel_composition: None, read_only: false, imported_from: None, + devel: true, installed_at_unix_ms, } } + /// 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("..") @@ -1066,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/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index b3dec8595..d8c236eae 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. @@ -736,10 +743,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 +989,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 +1059,7 @@ pub(crate) fn install_sdk( }, version_selector.as_ref(), dry_run, + include_devel, consent, ), "tarball" => { @@ -1042,6 +1111,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 +1133,7 @@ pub(crate) fn install_sdk_for_update( }, None, dry_run, + include_devel, consent, ), "tarball" => install_tarball_runtime( @@ -1434,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 @@ -1455,9 +1529,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)) - } + 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( @@ -1640,6 +1716,7 @@ fn install_wheel_runtime( source_override: InstallSourceOverride<'_>, version_selector: Option<&RuntimeVersionSelector>, dry_run: bool, + include_devel: bool, consent: SdkInstallConsent, ) -> Result { let InstallSourceOverride { @@ -1688,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(), @@ -1704,7 +1782,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 @@ -1783,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: {}", @@ -1954,7 +2041,14 @@ fn install_wheel_runtime( .map(String::as_str) .collect::>() .as_slice(), - "install TheRock devel SDK, torch stack, and resolved dependencies", + // 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" + }, )?; progress_line("Checking the installed ROCm SDK..."); @@ -1987,6 +2081,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 +2115,74 @@ fn install_wheel_runtime( Ok(SdkInstallResult::installed(output)) } -fn therock_pip_package_specs( - package_versions: &TheRockPipPackageVersions, +/// 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. +/// +/// 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" + } else { + "libraries" + } +} + +/// 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[libraries,devel,{device_extra}]=={}", - package_versions.rocm + "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 +/// 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 = format!("{},{device_extra}", therock_sdk_extras(include_devel)); + vec![ + format!("rocm[{rocm_extras}]=={}", package_versions.rocm), format!("torch[{device_extra}]=={}", package_versions.torch), format!( "torchvision[{device_extra}]=={}", @@ -2045,18 +2198,41 @@ 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 +2909,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)?; @@ -2749,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, @@ -2757,6 +2936,7 @@ fn resolve_pip_runtime( wheel_compatibility, version_selector, layout_override, + include_devel, None, ) } @@ -2771,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)?; @@ -2822,6 +3003,7 @@ fn resolve_pip_runtime_with_timeout( &source, wheel_compatibility, version_selector, + include_devel, download_timeout_secs, ) .with_context(|| { @@ -2834,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, @@ -2841,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) @@ -2880,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, @@ -2946,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(), @@ -3333,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 { @@ -3345,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", @@ -7701,7 +7891,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 +8130,317 @@ 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:?}" + ); + } + + 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, + version_resolution_specs: 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 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] + 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 + /// 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 { + 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, + "saving a manifest must replace devel, not merge with the prior one" + ); + + let _ = fs::remove_dir_all(&root); + Ok(()) + } + + /// 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 +9820,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 +10675,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..73503b9a0 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: @@ -176,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 e8c87b344..a2083d021 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -193,7 +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`), 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 @@ -966,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/docs/wsl.md b/docs/wsl.md index b22f1228d..da9974fe7 100644 --- a/docs/wsl.md +++ b/docs/wsl.md @@ -152,9 +152,11 @@ 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(...)`. +`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/scripts/therock_sdk_install_test.py b/scripts/therock_sdk_install_test.py index abe911fa7..e61ea6c54 100644 --- a/scripts/therock_sdk_install_test.py +++ b/scripts/therock_sdk_install_test.py @@ -26,14 +26,23 @@ 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 +375,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 +445,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 +506,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 +538,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 +609,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", diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 347324aa7..b9d74b2ac 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -1,5 +1,15 @@ 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. + # + # 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 @@ -7,6 +17,8 @@ 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 + 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/` @@ -345,3 +357,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/features/therock_next_generation.feature b/tests/e2e-cucumber/features/therock_next_generation.feature index e739e8c9b..fe4362cb5 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,32 @@ 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. + # + # 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. + # + # 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 + 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.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", diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 60e5e9d10..b429444ef 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}" ); @@ -930,6 +930,135 @@ 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}" + ); + + // 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. + // + // `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) + .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) + .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) + .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" + ); +} + +/// 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. 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 diff --git a/tests/e2e-cucumber/tests/e2e/therock_steps.rs b/tests/e2e-cucumber/tests/e2e/therock_steps.rs index cbf87d58b..64f3a01c3 100644 --- a/tests/e2e-cucumber/tests/e2e/therock_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/therock_steps.rs @@ -373,6 +373,44 @@ 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"); + assert_version_resolution_extras(world, true); +} + /// 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,12 +604,13 @@ 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}" ); assert_contains(world, &expected, "device extras"); + assert_version_resolution_extras(world, false); assert_contains( world, &format!("device_target: {RAW_ARCH}"), @@ -579,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); 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