diff --git a/README.md b/README.md index 2e9c2ede3..ab0c4b1ce 100644 --- a/README.md +++ b/README.md @@ -601,7 +601,8 @@ Log in with `huggingface-cli login` or set `HF_TOKEN` in your environment before serving gated models. `--gpu` selects which AMD GPU the server runs on. `auto` (the default) probes -per-GPU VRAM with `amd-smi` and picks the lowest-numbered GPU that is idle and +per-GPU VRAM (via `amd-smi`, or the amdgpu DRM sysfs counters when `amd-smi` is +not installed) and picks the lowest-numbered GPU that is idle and not already used by another rocm-cli server (managed or foreground), falling back to the GPU with the most free memory. Pass a single index (`--gpu 1`) to pin a specific device. The diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 499620bfe..d742cc711 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -6697,6 +6697,9 @@ fn serve(args: ServeArgs) -> Result<()> { host_gpu_summary.as_ref(), ) { println!(" {warning}"); + if engine_serves_vllm { + println!(" note: {}", rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT); + } } } if let Some(engine_recipe) = &resolve.engine_recipe { @@ -6785,6 +6788,7 @@ fn serve(args: ServeArgs) -> Result<()> { gpu_vram.as_deref(), gpu_memory_utilization_note.as_deref(), host_gpu_summary.as_ref(), + engine_serves_vllm, ); let summary = serve_summary::DeploymentSummary { engine: selected_engine.clone(), @@ -6824,6 +6828,7 @@ fn serve(args: ServeArgs) -> Result<()> { /// GPU/device warnings folded into the interactive deployment summary. Mirrors the /// inline warnings printed in the plain serve plan, in the same order. +#[allow(clippy::too_many_arguments)] fn collect_serve_notes( cpu_only: bool, gpu_selection: &GpuSelection, @@ -6832,6 +6837,7 @@ fn collect_serve_notes( gpu_vram: Option<&[GpuVramUsage]>, engine_flag_note: Option<&str>, host_gpu_summary: Option<&rocm_core::HostGpuSummary>, + engine_is_vllm: bool, ) -> Vec { let mut notes = Vec::new(); // An engine-scoped flag the selected engine cannot honor must be reported here @@ -6859,6 +6865,11 @@ fn collect_serve_notes( if let Some(warning) = serve_gpu_low_memory_warning(gpu_indices, gpu_vram, host_gpu_summary) { notes.push(warning); + // vLLM's total-VRAM reservation is what turns a busy card into an OOM; + // pair the generic warning with the concrete knob that avoids it. + if engine_is_vllm { + notes.push(rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT.to_owned()); + } } } notes @@ -21876,6 +21887,20 @@ fn validate_pinned_gpu_index( Ok(vec![index]) } +/// The GPU count to rank `--gpu auto` candidates over. Prefers the `amd-smi` +/// device count, but only when that count is usable — `None` (amd-smi +/// unavailable) and `Some(0)` (amd-smi ran and saw no devices) both fall through +/// to the DRM sysfs VRAM fallback's row count, so auto-selection can still +/// consider devices amd-smi did not enumerate. Used only to decide whether +/// `--gpu auto` has anything at all to rank, never as a `--gpu ` +/// validation bound — see [`validate_pinned_gpu_index`]. `None` when neither +/// source yields a non-zero count. +fn effective_gpu_count(detected: Option, vram: Option<&[GpuVramUsage]>) -> Option { + detected + .filter(|&count| count > 0) + .or_else(|| vram.map(<[GpuVramUsage]>::len).filter(|&count| count > 0)) +} + /// A GPU's local VRAM occupancy as reported by `amd-smi metric --json`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] struct GpuVramUsage { @@ -21909,10 +21934,10 @@ const AUTO_FREE_VRAM_FRACTION: f64 = 0.90; /// Pick a GPU ordinal for `--gpu auto`. Prefers the lowest-numbered GPU that is /// idle (free VRAM at or above [`AUTO_FREE_VRAM_FRACTION`]) and not pinned by a /// running rocm-cli service; otherwise falls back to the non-busy GPU with the -/// most free VRAM, then to the first non-busy GPU. When the GPU count is unknown -/// (amd-smi unavailable or zero devices) it returns no selection rather than -/// assuming device 0 — the engine's device probe then pins the first present GPU -/// or fails fast under the GPU-required policy. +/// most free VRAM, then to the first non-busy GPU. When neither the amd-smi +/// device count nor the DRM sysfs VRAM fallback yields any device it returns no +/// selection rather than assuming device 0 — the engine's device probe then pins +/// the first present GPU or fails fast under the GPU-required policy. /// /// Selection is mask-aware: candidates are restricted to `visible`, the HIP /// ordinals still usable after the active visibility mask (see @@ -21953,30 +21978,102 @@ fn select_auto_gpu_index( busy: &[u32], vram: Option<&[GpuVramUsage]>, ) -> Vec { - // Candidate ordinals: the detected device range, narrowed to those still - // visible under the active mask so auto-select never targets a masked-out GPU. - // `visible` is in HIP space (a ROCR mask's survivors are re-indexed to `0..N`), - // which is the space the selection is exported through, so the retained - // ordinals are directly selectable. When the host is unprobeable (`visible` is - // `None`) the range is used unrestricted (mask-unaware, as before). - let count = detected.unwrap_or(0); - let mut all: Vec = (0..count as u32).collect(); + // The amd-smi `list` device count is the primary source, but on a shared + // node without amd-smi it is `None` even though the DRM sysfs fallback probe + // may still have populated `vram`. Derive the count from those rows in that + // case so `--gpu auto` can rank GPUs by free VRAM instead of returning no + // selection and landing on a busy device 0. + let count = effective_gpu_count(detected, vram).unwrap_or(0); + if count == 0 { + // No GPU count from amd-smi and no VRAM telemetry (both unavailable, or + // genuinely zero devices). Do not assume device 0 exists: return no + // selection and let the engine's device probe pin the first present GPU + // or fail fast under the GPU-required policy (no GPU-0 fallback). + return Vec::new(); + } + // Ordinals this selection may hand out, lowest first. When VRAM telemetry is + // present, drive the scan from the *actual* row indices it reports rather + // than a synthetic `0..count` range: amd-smi metric (and the DRM sysfs + // fallback) can report non-contiguous ordinals when devices are hidden by a + // visibility mask, so `0..count` would look up rows that do not exist and + // miss the real ones. Without telemetry, fall back to the dense detected + // range so service-state-only selection still has candidates. + // + // Either way the set is then narrowed to the ordinals still visible under + // the active mask, so auto-select never targets a masked-out GPU. `visible` + // is in HIP space (a ROCR mask's survivors are re-indexed to `0..N`), which + // is the space the selection is exported through, so the retained ordinals + // are directly selectable. When the host is unprobeable (`visible` is + // `None`) the set is left unrestricted (mask-unaware, as before). + // + // A short row set is not always a mask, though: `parse_gpu_vram_usage` + // drops any device entry missing `/mem_usage/used_vram/value` or + // `/mem_usage/total_vram/value`, so a device the lighter `list` enumeration + // counts can simply have no row. Taking rows-only there would shrink the + // candidate set for a reason that has nothing to do with visibility, and if + // every *reported* device is busy the terminal fallback would hand back a + // busy GPU while an idle, merely untelemetried one went unconsidered — + // exactly the "serve pinned to an occupied GPU" fault this selection exists + // to avoid. So the `0..count` range is unioned back in — but only where it + // is evidence rather than invention, which is decided by the rows, not by + // `visible`: + // + // * `detected` must be `Some(n > 0)`, so `count` is the `list` device + // count — an independent source asserting those ordinals exist. When it + // is absent `effective_gpu_count` falls back to `rows.len()`, which is + // only the rows restating their own size and confirms nothing they omit. + // * every row index must be below `count`. A row at or above it is the one + // available proof that the ordinal space is re-indexed or sparse rather + // than dense from 0 (`[2, 3]` with `count == 2`), and that is exactly the + // masked shape the rows-only construction was introduced for, so there + // the rows stay authoritative. Below the count the rows are a subset of a + // dense range the count already asserts, and `0..count` is no stronger an + // assumption than the telemetry-less arm below already makes. + // + // Gating on `visible.is_some()` instead would have left the bug standing + // wherever the mask is unknown, which is not a corner: `usable_amd_gpu_indices` + // is `None` unconditionally off Linux, so `visible` is always `None` on + // Windows. The retain below still runs on top whenever a mask *is* known, so + // no synthesised ordinal survives that the mask contradicts. + let mut reported: Vec = match vram { + Some(rows) => { + let mut indices: Vec = rows.iter().map(|row| row.index).collect(); + let count_is_independently_sourced = detected.is_some_and(|detected| detected > 0); + let rows_are_a_dense_range_subset = + indices.iter().all(|&index| (index as usize) < count); + if count_is_independently_sourced && rows_are_a_dense_range_subset { + indices.extend(0..count as u32); + } + indices + } + None => (0..count as u32).collect(), + }; + reported.sort_unstable(); + // The union above can repeat an ordinal that both sources name. + reported.dedup(); if let Some(visible) = visible { - all.retain(|index| visible.contains(index)); + reported.retain(|index| visible.contains(index)); } - if all.is_empty() { - // No visible device (amd-smi unavailable, genuinely zero devices, or every - // detected device masked out). Do not assume device 0 exists: return no - // selection and let the engine's device probe pin the first present GPU or - // fail fast under the GPU-required policy (no GPU-0 fallback). + if reported.is_empty() { + // Every reported device is masked out (or telemetry reported no rows at + // all). Do not assume device 0 exists: return no selection and let the + // engine's device probe pin the first present GPU or fail fast under the + // GPU-required policy (no GPU-0 fallback). return Vec::new(); } - let candidates = || all.iter().copied().filter(|index| !busy.contains(index)); + // The subset still free to claim. Kept separate from `reported` so the + // terminal all-busy fallback below can still name a device that was actually + // reported instead of fabricating one. + let candidate_indices: Vec = reported + .iter() + .copied() + .filter(|index| !busy.contains(index)) + .collect(); let usage_for = |index: u32| vram.and_then(|rows| rows.iter().find(|row| row.index == index)); if vram.is_some() { // Pass 1: lowest-index idle GPU. - for index in candidates() { + for &index in &candidate_indices { if let Some(free) = usage_for(index).and_then(|usage| usage.free_fraction()) && free >= AUTO_FREE_VRAM_FRACTION { @@ -21985,36 +22082,93 @@ fn select_auto_gpu_index( } // Pass 2: the non-busy GPU with the most free VRAM in absolute terms // (not free percentage, which can favor a smaller GPU on heterogeneous - // VRAM systems). - if let Some(index) = candidates().max_by(|left, right| { - let left_free = usage_for(*left).map(|usage| usage.free_mb()); - let right_free = usage_for(*right).map(|usage| usage.free_mb()); + // VRAM systems). `usage_for` is only *partial* over `candidate_indices` — + // the union above puts back ordinals the `list` count confirms but + // telemetry never reported — yet no "does this ordinal have a row?" guard + // is needed, because the comparator already orders them last: the keys are + // `Option`, and `None` sorts below every `Some`, including `Some(0)`. + // So a rowless ordinal can only be the maximum when *no* candidate has a + // row; then every key is `None`, every comparison falls through to the + // index tie-break, and the winner is the lowest candidate index — which is + // precisely what Pass 3 would return from an ascending `reported`. Adding + // the guard back would therefore change no outcome, only skip a pass that + // already agrees. + if let Some(&index) = candidate_indices.iter().max_by(|left, right| { + let left_free = usage_for(**left).map(|usage| usage.free_mb()); + let right_free = usage_for(**right).map(|usage| usage.free_mb()); left_free .cmp(&right_free) // Break ties toward the lowest index. .then(right.cmp(left)) - }) && usage_for(index).is_some() - { + }) { return vec![index]; } } // Pass 3: no VRAM telemetry; first GPU not pinned by a managed service. - if let Some(index) = candidates().next() { + if let Some(&index) = candidate_indices.first() { return vec![index]; } - // Every visible GPU is pinned by a running service. Return the lowest visible - // ordinal (no CPU fallback is ever used); the caller surfaces a low-memory - // warning so the user can free a device or pick another `--gpu`. Using the - // lowest *visible* ordinal rather than a hardcoded 0 keeps this correct when - // device 0 is masked out. - vec![all[0]] + // Every reported, visible GPU is pinned by a running service. Return the + // lowest of them (no CPU fallback is ever used); the caller surfaces a + // low-memory warning so the user can free a device or pick another `--gpu`. + // It must come from `reported`, not a hardcoded 0: once candidates are + // sourced from the VRAM rows those ordinals can be sparse (a visibility mask + // can leave only `[2, 3]`), so a literal 0 would pin a device nothing ever + // reported — exported straight into `HIP_VISIBLE_DEVICES` as a silently + // wrong choice. `reported` is non-empty here, guaranteed by the check above. + vec![reported[0]] +} + +/// Best-effort per-GPU VRAM occupancy. Prefers `amd-smi metric --json`; when +/// amd-smi is not installed (e.g. a shared node or container that ships no +/// amd-smi — this never tries `rocm-smi`) it falls back to the amdgpu DRM sysfs +/// VRAM counters so `--gpu auto` selection and the low-VRAM warning still have +/// telemetry to work with on a single-GPU host (see +/// [`read_drm_vram_usage`] for why the sysfs fallback withholds telemetry on a +/// multi-GPU one). Returns `None` only when neither source is readable +/// (callers then fall back to service-state-only auto-selection). +fn gpu_vram_usage() -> Option> { + #[cfg(feature = "e2e-test-hooks")] + if let Some(forced) = simulated_low_vram_usage() { + return Some(forced); + } + gpu_vram_usage_amd_smi().or_else(gpu_vram_usage_sysfs) } -/// Best-effort per-GPU VRAM occupancy via `amd-smi metric --json`. Returns -/// `None` when amd-smi is unavailable or its output cannot be parsed (callers -/// then fall back to service-state-only auto-selection). -fn gpu_vram_usage() -> Option> { +/// E2E hook: when `ROCM_E2E_FORCE_LOW_VRAM` names a GPU ordinal, report that GPU +/// as almost entirely occupied, overriding the real telemetry. +/// +/// The serve-plan low-VRAM OOM warning (and, for vLLM, its +/// `--gpu-memory-utilization` note) only fires when a *selected* GPU is below +/// [`AUTO_FREE_VRAM_FRACTION`] free. The GPU lane's real cards are comfortably +/// free, so without this seam the branch is unreachable in E2E and the note has +/// no live coverage. Synthesizing one near-full single-GPU reading makes that +/// path deterministic on the vLLM GPU lane (a non-APU host, so +/// [`vram_capacity_is_meaningful`] still holds) without touching the device the +/// engine actually launches on. Compiled only into the `e2e-test-hooks` binary; +/// production builds never see it. +#[cfg(feature = "e2e-test-hooks")] +fn simulated_low_vram_usage() -> Option> { + let index: u32 = std::env::var("ROCM_E2E_FORCE_LOW_VRAM") + .ok()? + .trim() + .parse() + .ok()?; + // A single 24 GiB device with ~0.5 GiB free is well below the 0.90 free + // threshold, so `gpu_low_memory_warning` fires; one GPU keeps + // `vram_capacity_is_meaningful` honest on the non-APU vLLM lane. + let total_mb = 24 * 1024; + Some(vec![GpuVramUsage { + index, + used_mb: total_mb - 512, + total_mb, + }]) +} + +/// Per-GPU VRAM occupancy via `amd-smi metric --json`. Returns `None` when +/// amd-smi is unavailable or its output cannot be parsed. +fn gpu_vram_usage_amd_smi() -> Option> { let binary = rocm_core::resolve_amd_smi_binary(); let output = ProcessCommand::new(&binary) .arg("metric") @@ -22032,6 +22186,114 @@ fn gpu_vram_usage() -> Option> { if rows.is_empty() { None } else { Some(rows) } } +/// Fallback per-GPU VRAM occupancy read straight from the amdgpu DRM sysfs +/// counters (`/sys/class/drm/card*/device/mem_info_vram_{total,used}`), used when +/// amd-smi is not installed. Linux-only; other platforms expose no such +/// interface and return `None`. +#[cfg(target_os = "linux")] +fn gpu_vram_usage_sysfs() -> Option> { + let rows = read_drm_vram_usage(Path::new("/sys/class/drm")); + if rows.is_empty() { None } else { Some(rows) } +} + +#[cfg(not(target_os = "linux"))] +fn gpu_vram_usage_sysfs() -> Option> { + None +} + +/// Read per-GPU VRAM occupancy from the amdgpu DRM sysfs tree rooted at +/// `drm_dir`. Enumerates primary `card` nodes (skipping connector sub-nodes +/// like `card0-DP-1`), keeps only AMD devices exposing readable +/// `mem_info_vram_{total,used}` counters, and assigns HIP ordinal 0 to the sole +/// AMD card to mirror HIP device ordering on a standard single-GPU host install. +/// +/// HIP ordinals come from the KFD *compute* topology, not DRM card numbers, and +/// the two enumerations are only guaranteed to agree when there is exactly one +/// AMD DRM card: an APU passes the same AMD filter as a discrete GPU, so a host +/// pairing an APU with a dGPU can order `card` differently than HIP's node +/// order, and that ordinal is fed to `HIP_VISIBLE_DEVICES` downstream. Rather +/// than guess, this returns no rows at all when more than one AMD card is +/// *found* — counted before the telemetry filters below, so a second AMD card +/// with an unreadable counter still trips the guard instead of leaving the +/// survivor mislabelled ordinal 0. Callers then fall back to service-state-only +/// auto-selection. Split from [`gpu_vram_usage_sysfs`] so it can be tested +/// against a planted sysfs layout. +#[cfg(any(target_os = "linux", test))] +fn read_drm_vram_usage(drm_dir: &Path) -> Vec { + const BYTES_PER_MIB: u64 = 1024 * 1024; + let Ok(entries) = fs::read_dir(drm_dir) else { + return Vec::new(); + }; + // Every AMD card is counted here, *before* the telemetry filters below can + // drop it, so the multi-card ordinal-ambiguity guard fires on how many AMD + // cards exist rather than on how many produced readable counters. + let mut amd_cards_found = 0usize; + let mut cards: Vec = Vec::new(); + for entry in entries.flatten() { + let card_path = entry.path(); + let Some(name) = card_path.file_name().and_then(|value| value.to_str()) else { + continue; + }; + if !name.starts_with("card") || name.contains('-') { + continue; + } + if name + .strip_prefix("card") + .and_then(|digits| digits.parse::().ok()) + .is_none() + { + continue; + } + let device_dir = card_path.join("device"); + if !drm_device_is_amd(&device_dir) { + continue; + } + amd_cards_found += 1; + let Some(total_bytes) = read_sysfs_u64(&device_dir.join("mem_info_vram_total")) else { + continue; + }; + // An unreadable `used` counter is unknown occupancy, not zero: treating + // it as `0` would make the card look 100% free, which is exactly what + // `--gpu auto` prefers first. Skip the card instead of guessing. + let Some(used_bytes) = read_sysfs_u64(&device_dir.join("mem_info_vram_used")) else { + continue; + }; + cards.push(GpuVramUsage { + // The sole surviving AMD card is HIP ordinal 0 (guaranteed by the + // multi-card guard below). + index: 0, + used_mb: used_bytes / BYTES_PER_MIB, + total_mb: total_bytes / BYTES_PER_MIB, + }); + } + // More than one AMD card found: ascending `card` order is not guaranteed + // to match HIP's ordinal order (see doc comment above), so do not hand out + // ordinals that might feed the wrong device into `HIP_VISIBLE_DEVICES`. + if amd_cards_found > 1 { + return Vec::new(); + } + cards +} + +/// Read a sysfs file whose entire contents are a single unsigned integer. +#[cfg(any(target_os = "linux", test))] +fn read_sysfs_u64(path: &Path) -> Option { + fs::read_to_string(path).ok()?.trim().parse::().ok() +} + +/// Whether a DRM `device` directory belongs to an AMD GPU. +/// +/// Delegates rather than re-implementing. This probe and `rocm-core`'s KFD/DRM +/// count authority have to agree on what counts as an AMD card — if this one +/// recognises fewer, it under-counts and the multi-card ordinal guard it feeds +/// silently narrows — and a second copy of the test with a comment claiming the +/// two match is not agreement, it is a promise nothing enforces. Calling the +/// same function is. +#[cfg(any(target_os = "linux", test))] +fn drm_device_is_amd(device_dir: &Path) -> bool { + rocm_core::is_amdgpu_device(device_dir) +} + /// Parse `amd-smi metric --json` output into per-GPU VRAM usage. Accepts both /// the `{"gpu_data": [...]}` envelope and a bare top-level array, mirroring the /// schema variance handled by the dashboard amd-smi collector. @@ -31664,13 +31926,23 @@ install therock"; None, Some(note), None, + false, ); assert!( notes.iter().any(|entry| entry == note), "the ignored-flag note must reach the summary: {notes:?}" ); - let quiet = collect_serve_notes(false, &GpuSelection::Auto, false, &[0], None, None, None); + let quiet = collect_serve_notes( + false, + &GpuSelection::Auto, + false, + &[0], + None, + None, + None, + false, + ); assert!( !quiet .iter() @@ -31846,6 +32118,75 @@ install therock"; ); } + #[test] + fn serve_notes_pair_low_vram_with_the_vllm_utilization_hint() { + // A busy discrete card that trips the low-VRAM warning: on vLLM the note + // must also carry the concrete `--gpu-memory-utilization` workaround, so + // the interactive summary tells the user how to avoid the OOM. + let busy = [vram(0, 182_000, 192_000)]; + let notes = collect_serve_notes( + false, + &GpuSelection::Auto, + false, + &[0], + Some(&busy), + None, + Some(&host_gpu("gfx1100")), + true, + ); + assert!( + notes.iter().any(|entry| entry.contains("has only")), + "the low-VRAM warning must be present: {notes:?}" + ); + assert!( + notes + .iter() + .any(|entry| entry.contains("--gpu-memory-utilization")), + "vLLM low-VRAM notes must hint the utilization workaround: {notes:?}" + ); + + // The literal fragments the GPU-lane scenario + // `@id:serve-vllm-low-vram-oom-guidance` matches on. That scenario runs + // only where a real card exists, so pin the wording here too: a rename + // then fails on every lane rather than silently on the one lane that can + // observe it. `GPU 0 has only` is what makes the warning provably about + // the *pinned* device; `vLLM reserves ~90%` is what distinguishes the + // pre-launch hint from any other line that merely names the flag. + assert!( + notes.iter().any(|entry| entry.contains("GPU 0 has only")), + "the warning must name the selected GPU: {notes:?}" + ); + assert!( + notes + .iter() + .any(|entry| entry.contains("vLLM reserves ~90%")), + "the hint must explain vLLM's total-VRAM reservation: {notes:?}" + ); + + // The same busy card on a non-vLLM engine keeps the warning but omits the + // vLLM-only knob, which that engine cannot honor. + let lemonade = collect_serve_notes( + false, + &GpuSelection::Auto, + false, + &[0], + Some(&busy), + None, + Some(&host_gpu("gfx1100")), + false, + ); + assert!( + lemonade.iter().any(|entry| entry.contains("has only")), + "the low-VRAM warning still fires for other engines: {lemonade:?}" + ); + assert!( + !lemonade + .iter() + .any(|entry| entry.contains("--gpu-memory-utilization")), + "non-vLLM engines must not be told to pass a vLLM-only flag: {lemonade:?}" + ); + } + #[test] fn engine_recipe_enables_tool_choice_reflects_flags() { assert!(!engine_recipe_enables_tool_choice(None)); @@ -31972,6 +32313,188 @@ install therock"; ); } + #[test] + fn auto_selection_uses_vram_row_count_when_amd_smi_count_is_unknown() { + // `detect_gpu_count()` is `None` while VRAM telemetry is present. With + // *several* rows that is the amd-smi shape of the state -- `metric` + // answered while `list` did not -- not the DRM sysfs fallback, which + // withholds telemetry entirely once a second AMD card is found and so + // can never produce more than one row (`read_drm_vram_usage`). Either + // way auto-selection must derive the count from the rows and skip the + // busy GPU 0 rather than returning no selection (which would land the + // engine on GPU 0). + let usage = [ + vram(0, 182_000, 192_000), + vram(1, 1_000, 192_000), + vram(2, 500, 192_000), + ]; + assert_eq!( + select_auto_gpu_index(None, None, &[], Some(&usage)), + vec![1], + "with no amd-smi count, rank the reported VRAM rows and pick the first idle GPU" + ); + // The state the DRM sysfs fallback can actually reach: exactly one row. + // It is still ranked (and still selected) rather than discarded for + // being a single device. + assert_eq!( + select_auto_gpu_index(None, None, &[], Some(&[vram(0, 1_000, 192_000)])), + vec![0], + "the single row the sysfs fallback can emit must still be selectable" + ); + // Still nothing to go on when neither the count nor telemetry is present. + assert_eq!( + select_auto_gpu_index(None, None, &[], Some(&[])), + Vec::::new() + ); + } + + #[test] + fn auto_selection_ranks_the_actual_reported_row_indices() { + // A visibility mask hides GPUs 0 and 1, so amd-smi metric reports only + // the surviving devices with their absolute, non-contiguous ordinals + // [2, 3] -- there is no row at index 0 or 1. Auto-selection must scan + // those real indices, not a synthetic `0..count` range (which would + // look up absent rows and fall through to a bogus GPU 0). + let usage = [vram(2, 182_000, 192_000), vram(3, 1_000, 192_000)]; + assert_eq!( + select_auto_gpu_index(None, None, &[], Some(&usage)), + vec![3], + "should skip the busy GPU 2 and pick the idle GPU 3 by its real ordinal" + ); + // With GPU 3 also pinned by a managed service, pass 3 still returns a + // real reported ordinal (GPU 2), never a fabricated index 0. + assert_eq!( + select_auto_gpu_index(None, None, &[3], Some(&usage)), + vec![2], + "the sole non-busy reported GPU must be selected by its real ordinal" + ); + } + + #[test] + fn auto_selection_all_reported_busy_falls_back_to_a_reported_ordinal() { + // Same sparse-ordinal host as above ([2, 3] reported, nothing at 0 or 1) + // but now BOTH reported GPUs are pinned by a running service, so every + // pass falls through to the terminal "all busy, pick one anyway" + // fallback. That fallback used to hand back a hardcoded `0` — safe only + // while candidates were a dense `0..count`, and simply wrong once they + // come from the reported rows: index 0 is a device nothing reported, yet + // it would be exported verbatim as `HIP_VISIBLE_DEVICES`. It must return + // the lowest ordinal that was actually reported instead. + let usage = [vram(2, 182_000, 192_000), vram(3, 190_000, 192_000)]; + assert_eq!( + select_auto_gpu_index(None, None, &[2, 3], Some(&usage)), + vec![2], + "the all-busy fallback must name a reported GPU, never a fabricated index 0" + ); + // "Lowest reported" has to mean lowest by ordinal, not first in the + // rows: `amd-smi` orders its output by enumeration, not by index, so + // feed the same two rows in descending order. Varying the *busy* slice's + // order instead would prove nothing — it is only ever read through + // `.contains()`, so that assertion was a duplicate of the one above. + let descending = [vram(3, 190_000, 192_000), vram(2, 182_000, 192_000)]; + assert_eq!( + select_auto_gpu_index(None, None, &[2, 3], Some(&descending)), + vec![2], + "the fallback must take the lowest reported ordinal, not the first row" + ); + } + + #[test] + fn auto_selection_considers_a_detected_gpu_that_reported_no_vram_row() { + // A short row set is not always a visibility mask. `parse_gpu_vram_usage` + // drops any device entry missing its `used_vram`/`total_vram` pointers, so + // here `list` counts two GPUs, both are visible, but only GPU 0 produced a + // row — and GPU 0 is pinned by a managed service. Driving candidates from + // the rows alone leaves `[0]`, the busy filter empties it, every pass + // iterates nothing and the terminal fallback hands back GPU 0: `serve` + // pinned to an already-occupied GPU, the exact failure this selection + // exists to prevent. The untelemetried GPU 1 is confirmed by both the + // count and the visible set, so it must be a candidate and must win. + assert_eq!( + select_auto_gpu_index( + Some(2), + Some(&[0, 1]), + &[0], + Some(&[vram(0, 182_000, 192_000)]) + ), + vec![1], + "a detected, visible GPU with no VRAM row must be preferred over a busy reported one" + ); + // The same shape without telemetry for the busy device being conclusive: + // GPU 1 is still the only non-busy ordinal either source confirms. + assert_eq!( + select_auto_gpu_index( + Some(2), + Some(&[0, 1]), + &[0], + Some(&[vram(0, 1_000, 192_000)]) + ), + vec![1], + "an idle-looking but service-pinned GPU 0 must still lose to the free GPU 1" + ); + // The masking behaviour this rows-only construction was introduced for is + // untouched: rows `[2, 3]` against `count == 2` put a row index at or above + // the count, which is the proof the ordinal space is re-indexed, so the + // union never happens and the rows stand alone. (The visible retain would + // also have dropped `0`/`1` here — this pins the row-index rule itself, so + // the masked case survives on a host where the mask is unknown too.) + let masked = [vram(2, 182_000, 192_000), vram(3, 190_000, 192_000)]; + assert_eq!( + select_auto_gpu_index(Some(2), Some(&[2, 3]), &[2, 3], Some(&masked)), + vec![2], + "a masked host must not gain candidates 0/1 from the detected count" + ); + } + + #[test] + fn auto_selection_considers_an_untelemetried_gpu_when_the_visible_set_is_unknown() { + // Same shape as the test above, but `visible` is `None`. That is not an + // exotic host: `probe_usable_amd_gpu_indices` returns `None` + // *unconditionally* off Linux, and `serve` threads that straight into + // `visible`, so every Windows run takes this path — as does any Linux host + // whose KFD topology and DRM cards are both unreadable. Gating the + // `0..count` union on `visible.is_some()` therefore left the whole bug + // standing on a supported platform. + // + // Nothing about the mask is needed to justify GPU 1 here: the amd-smi + // `list` count asserts two devices and the rows name only ordinals below + // that count, so the rows are a subset of a dense range, not a re-indexed + // one. GPU 0 is pinned by a managed service, so the untelemetried GPU 1 is + // the only free ordinal and must win over the busy reported fallback. + assert_eq!( + select_auto_gpu_index(Some(2), None, &[0], Some(&[vram(0, 182_000, 192_000)])), + vec![1], + "an unprobeable host must still consider a counted GPU that reported no VRAM row" + ); + // A row index at or above the count is the only evidence available that the + // ordinal space is *not* a dense `0..count`, so there the rows stay + // authoritative even with no mask to retain against — otherwise an + // unprobeable masked host would have `[0, 1]` invented for it. + let masked = [vram(2, 182_000, 192_000), vram(3, 190_000, 192_000)]; + assert_eq!( + select_auto_gpu_index(Some(2), None, &[2, 3], Some(&masked)), + vec![2], + "re-indexed rows must not gain unconfirmed candidates 0/1 from the detected count" + ); + // The count must come from the `list` enumeration to confirm anything. + // With `detected` absent it is `rows.len()`, i.e. the rows restating their + // own size, and `parse_gpu_vram_usage` can repeat an ordinal: it falls back + // to the array position only when an entry has no `gpu` field, so a payload + // naming `"gpu": 0` twice yields two index-0 rows. That makes `rows.len()` + // 2 with every index below it, and dropping the `detected` half of the + // condition would invent a GPU 1 nothing ever enumerated. + assert_eq!( + select_auto_gpu_index( + None, + None, + &[0], + Some(&[vram(0, 182_000, 192_000), vram(0, 182_000, 192_000)]) + ), + vec![0], + "a row-derived count must not synthesise an ordinal no source reported" + ); + } + #[test] fn validate_pinned_gpu_index_rejects_out_of_range() { // Index equal to or beyond the detected count is rejected. @@ -31999,6 +32522,27 @@ install therock"; ); } + #[test] + fn effective_gpu_count_prefers_amd_smi_then_falls_back_to_vram_rows() { + let usage = [vram(0, 1_000, 192_000), vram(1, 1_000, 192_000)]; + // amd-smi's count wins when it is known, even over a differing row count. + assert_eq!(effective_gpu_count(Some(4), Some(&usage)), Some(4)); + // amd-smi unavailable (`None`): the sysfs VRAM fallback row count stands in. + assert_eq!(effective_gpu_count(None, Some(&usage)), Some(2)); + // The second, previously untested fallback trigger: amd-smi ran and + // reported zero devices. `Some(0)` must fall through to the VRAM rows + // just as `None` does, or a host amd-smi cannot enumerate but sysfs can + // would have nothing for `--gpu auto` to rank. Dropping the + // `filter(|&count| count > 0)` on `detected` turns this line red. + assert_eq!(effective_gpu_count(Some(0), Some(&usage)), Some(2)); + // Neither source available. + assert_eq!(effective_gpu_count(None, None), None); + assert_eq!(effective_gpu_count(None, Some(&[])), None); + // Both present but both empty: still nothing to rank. + assert_eq!(effective_gpu_count(Some(0), Some(&[])), None); + assert_eq!(effective_gpu_count(Some(0), None), None); + } + #[test] fn validate_pinned_gpu_index_rejects_masked_out_device() { // The visible set is authoritative: an index outside it is rejected early @@ -32088,6 +32632,126 @@ install therock"; assert!((rows[1].free_fraction().unwrap() - (142_000.0 / 192_000.0)).abs() < 1e-9); } + #[test] + fn read_drm_vram_usage_reads_a_single_amdgpu_card_and_assigns_ordinal_zero() { + // Plant a minimal `/sys/class/drm`-shaped tree: one AMD card, a connector + // sub-node that must be skipped, and a non-AMD card that must be ignored. + let root = std::env::temp_dir().join(format!("rocm-drm-vram-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let plant_amd = |card: &str, total: u64, used: u64| { + let device = root.join(card).join("device"); + fs::create_dir_all(&device).unwrap(); + fs::write(device.join("vendor"), "0x1002\n").unwrap(); + fs::write(device.join("mem_info_vram_total"), format!("{total}\n")).unwrap(); + fs::write(device.join("mem_info_vram_used"), format!("{used}\n")).unwrap(); + }; + // 192 GiB total, mostly free (values in bytes). + plant_amd("card0", 206_158_430_208, 1_073_741_824); + // A connector sub-node under card0 — must be skipped, not parsed as a card. + fs::create_dir_all(root.join("card0-DP-1")).unwrap(); + // A non-AMD primary card — different vendor, must be ignored. + let intel = root.join("card1").join("device"); + fs::create_dir_all(&intel).unwrap(); + fs::write(intel.join("vendor"), "0x8086\n").unwrap(); + fs::write(intel.join("mem_info_vram_total"), "1000000\n").unwrap(); + + let rows = read_drm_vram_usage(&root); + let _ = fs::remove_dir_all(&root); + + assert_eq!(rows.len(), 1, "only the one AMD card counts: {rows:?}"); + assert_eq!(rows[0].index, 0); + // 206_158_430_208 bytes / 1 MiB == 196_608 MiB. + assert_eq!(rows[0].total_mb, 196_608); + assert!(rows[0].free_fraction().unwrap() > AUTO_FREE_VRAM_FRACTION); + } + + #[test] + fn read_drm_vram_usage_withholds_telemetry_when_multiple_amd_cards_are_present() { + // Ascending `card` order is not guaranteed to match HIP's compute + // ordinal once more than one AMD card exists (e.g. an APU alongside a + // dGPU), so the fallback must not hand out ordinals it cannot vouch for. + let root = std::env::temp_dir().join(format!("rocm-drm-vram-multi-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let plant_amd = |card: &str, total: u64, used: u64| { + let device = root.join(card).join("device"); + fs::create_dir_all(&device).unwrap(); + fs::write(device.join("vendor"), "0x1002\n").unwrap(); + fs::write(device.join("mem_info_vram_total"), format!("{total}\n")).unwrap(); + fs::write(device.join("mem_info_vram_used"), format!("{used}\n")).unwrap(); + }; + plant_amd("card0", 206_158_430_208, 1_073_741_824); + plant_amd("card1", 206_158_430_208, 189_284_651_008); + + let rows = read_drm_vram_usage(&root); + let _ = fs::remove_dir_all(&root); + + assert!( + rows.is_empty(), + "multi-card sysfs telemetry must be withheld: {rows:?}" + ); + } + + #[test] + fn read_drm_vram_usage_withholds_telemetry_when_a_second_amd_card_has_no_counter() { + // The guard must count AMD cards *found*, not surviving rows: a genuine + // two-AMD-card host where the second card's `used` counter is unreadable + // would otherwise slip past a `rows.len() > 1` check and mislabel the + // survivor ordinal 0 — the exact APU+dGPU misattribution the guard exists + // to prevent. + let root = + std::env::temp_dir().join(format!("rocm-drm-vram-partial-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + // card0: fully readable. + let card0 = root.join("card0").join("device"); + fs::create_dir_all(&card0).unwrap(); + fs::write(card0.join("vendor"), "0x1002\n").unwrap(); + fs::write(card0.join("mem_info_vram_total"), "206158430208\n").unwrap(); + fs::write(card0.join("mem_info_vram_used"), "1073741824\n").unwrap(); + // card1: AMD, but its `used` counter cannot be read. + let card1 = root.join("card1").join("device"); + fs::create_dir_all(&card1).unwrap(); + fs::write(card1.join("vendor"), "0x1002\n").unwrap(); + fs::write(card1.join("mem_info_vram_total"), "206158430208\n").unwrap(); + + let rows = read_drm_vram_usage(&root); + let _ = fs::remove_dir_all(&root); + + assert!( + rows.is_empty(), + "a second AMD card must trip the guard even without readable counters: {rows:?}" + ); + } + + #[test] + fn read_drm_vram_usage_skips_a_card_with_an_unreadable_used_counter() { + // A card whose `mem_info_vram_used` cannot be read must not be treated as + // 0 bytes used (100% free) — that would make a broken counter look like + // the ideal `--gpu auto` pick. It should be skipped entirely instead. + let root = + std::env::temp_dir().join(format!("rocm-drm-vram-unreadable-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let device = root.join("card0").join("device"); + fs::create_dir_all(&device).unwrap(); + fs::write(device.join("vendor"), "0x1002\n").unwrap(); + fs::write(device.join("mem_info_vram_total"), "206158430208\n").unwrap(); + // No `mem_info_vram_used` file written at all. + + let rows = read_drm_vram_usage(&root); + let _ = fs::remove_dir_all(&root); + + assert!( + rows.is_empty(), + "a card with no readable used counter must be skipped: {rows:?}" + ); + } + + #[test] + fn read_drm_vram_usage_is_empty_without_a_drm_tree() { + let missing = std::env::temp_dir().join("rocm-drm-vram-absent-98765"); + let _ = fs::remove_dir_all(&missing); + assert!(read_drm_vram_usage(&missing).is_empty()); + } + #[test] fn gpu_low_memory_warning_flags_busy_selected_gpu() { let usage = [vram(0, 182_000, 192_000), vram(1, 1_000, 192_000)]; @@ -32332,6 +32996,44 @@ install therock"; } } + /// The `ROCM_E2E_FORCE_LOW_VRAM` hook must synthesize a reading that actually + /// trips the serve-plan low-VRAM warning on a non-APU (vLLM-lane) host, and + /// stay inert when the var is unset. This is what the `@requires-gpu` + /// `serve-vllm-low-vram-oom-guidance` scenario relies on to fire the note + /// deterministically on cards that are really free. + #[cfg(feature = "e2e-test-hooks")] + #[test] + fn forced_low_vram_hook_trips_the_serve_plan_warning() { + let mut env = ScopedTestEnv::new(); + + // Unset: the hook contributes nothing and real telemetry is consulted. + env.clear("ROCM_E2E_FORCE_LOW_VRAM"); + assert!( + simulated_low_vram_usage().is_none(), + "no override without the env var" + ); + + // Set to ordinal 0: a single near-full device that the serve-plan wrapper + // reports on a discrete (non-APU) host but that stays honest there. + env.set("ROCM_E2E_FORCE_LOW_VRAM", "0"); + let forced = simulated_low_vram_usage().expect("override present when the var is set"); + assert_eq!( + forced.len(), + 1, + "single synthetic GPU keeps the APU guard honest" + ); + assert_eq!(forced[0].index, 0); + assert!( + forced[0] + .free_fraction() + .is_some_and(|f| f < AUTO_FREE_VRAM_FRACTION), + "the synthetic reading must be below the free-VRAM bar so the warning fires" + ); + let warning = serve_gpu_low_memory_warning(&[0], Some(&forced), Some(&host_gpu("gfx1100"))) + .expect("a near-full discrete card warrants the serve-plan warning"); + assert!(warning.contains("GPU 0")); + } + #[test] fn driver_plan_ubuntu_2404_uses_official_dkms_commands() { let _env = ScopedTestEnv::with_amd_overrides_cleared(); diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 70fc1a83a..81c9f7b69 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -100,9 +100,14 @@ pub struct DiagnoseReport { pub min_score_for_match: i32, pub high_confidence_threshold: i32, pub route_when_no_match: Route, - /// Set when the host is out of scope for this catalog (e.g. WSL2). When - /// present, `matched` is empty — the catalog is deliberately not run, to - /// avoid emitting bare-metal-Linux diagnoses that don't apply. + /// Set when the catalog has no entries at all for the running platform, and + /// carrying the routing note that says so. + /// + /// Decided purely by whether the catalog covers the running platform — never + /// by a score. When this is `Some`, no checker ran, so `matched` is empty and + /// `has_match` is false; a consumer must not wait for sub-threshold rows + /// alongside it. Pinned by + /// `an_out_of_scope_report_never_carries_matched_entries`. #[serde(default)] pub out_of_scope: Option, /// The verdict on a model, when `--model` named one. @@ -364,6 +369,79 @@ const KEYWORDS_PAGE_FAULT: KeywordTable = &[ ("out_of_registers", 30, "compiler OUT_OF_REGISTERS"), ]; +// vLLM's startup OOM. The distinctive signature is a torch/HIP allocation +// failure while the engine is reserving KV-cache VRAM; a bare "out of memory" +// on its own is deliberately sub-threshold so this only claims the failure mode +// when the vLLM/HIP shape of the error is present. +// +// None of these tokens are vLLM-specific on their own: `torch.OutOfMemoryError: +// CUDA out of memory` is the identical shape any ROCm PyTorch job emits (ROCm's +// PyTorch build reports the CUDA-compat name), so this table alone cannot tell +// vLLM's OOM apart from an arbitrary training script hitting the same +// allocator error. `check_16_vllm_oom` therefore scores *only* the lines +// carrying `VLLM_ANCHOR_PATTERN` (see `vllm_anchored_lines`), never the whole +// pasted log: an OOM on an unanchored line contributes nothing at all. +// +// Weighting splits the *allocator message* from the *exception class name*: +// - The HIP allocator's own messages (`HIP out of memory`, the +// `hipErrorOutOfMemory` status) and the CUDA-compat message ROCm's PyTorch +// prints clear `MIN_SCORE_FOR_MATCH` on their own — with the required +// anchor already establishing this is vLLM's line, one of those messages is +// a genuine OOM signal. +// - The bare exception *class* (`torch.OutOfMemoryError`, with or without the +// `.cuda.` module path) and a bare `out of memory` stay sub-threshold: any +// PyTorch job emits the class name, so it must corroborate rather than +// carry the verdict alone (`torch_oom_class_alone_is_not_a_vllm_match`). +// This table alone is scored through `keyword_score_collapsing_overlaps`, which +// de-duplicates overlapping spans, so `HIP out of memory` counts once (as the +// 50-point message), not also as the nested 25-point `out of memory` — one +// phrase yields one evidence bullet. Every other table keeps the catalog's +// default "each entry is an independent signal" scoring. +const KEYWORDS_VLLM_OOM: KeywordTable = &[ + ( + "hip out of memory", + 50, + "error mentions 'HIP out of memory'", + ), + ( + // ROCm's HIP runtime OOM status code, e.g. `RuntimeError: + // hipErrorOutOfMemory`. `\boutofmemory\b` does not match inside it (no + // word boundary after `hipError`), so it needs its own entry. + r"hiperroroutofmemory", + 50, + "error mentions hipErrorOutOfMemory", + ), + ( + r"cuda out of memory", + 50, + "error mentions 'CUDA out of memory' (ROCm reports the CUDA-compat name)", + ), + ( + // The exception class name, under either `torch.OutOfMemoryError` or the + // `.cuda.` module path ROCm's PyTorch reports it under. Sub-threshold + // alone: it is the class every PyTorch OOM raises, not vLLM's signature. + r"torch\.(?:cuda\.)?outofmemoryerror", + 45, + "error mentions torch.OutOfMemoryError", + ), + ( + r"tried to allocate .*(?:gib|mib)", + 30, + "error names an allocation it could not satisfy", + ), + ( + r"\boutofmemory\b", + 30, + "error mentions standalone OutOfMemory", + ), + ("out of memory", 25, "error mentions 'out of memory'"), + ( + r"gpu[-_]memory[-_]utilization", + 20, + "log mentions gpu_memory_utilization (vLLM VRAM reservation)", + ), +]; + /// The vLLM engine-startup import failure: `torch-c-dlpack-ext` picks its CUDA /// prebuilt on a ROCm build of torch, and `ctypes.CDLL` aborts the import. /// @@ -393,15 +471,49 @@ const KEYWORDS_TORCH_DLPACK_CUDA_VARIANT: KeywordTable = &[ ]; /// Score the strongest (top-2) keyword matches in `table` against `symptom`. +/// +/// Every entry that matches counts as an independent signal. This is the +/// behaviour every table in the catalog except [`KEYWORDS_VLLM_OOM`] is tuned +/// for -- see [`keyword_score_collapsing_overlaps`] for why that one differs and +/// why the difference must not be generalized. fn keyword_score(symptom: &str, table: KeywordTable) -> (i32, Vec) { + keyword_score_impl(symptom, table, false) +} + +/// Like [`keyword_score`], but collapses matches whose spans overlap to the +/// strongest one, so a single phrase cannot masquerade as two independent +/// signals (e.g. `HIP out of memory` must not also count the `out of memory` +/// nested inside it). +/// +/// This is **deliberately not** applied to the rest of the catalog. Several +/// older tables pair a greedy `.*` pattern with a second, genuinely independent +/// keyword, and the greedy span swallows it: `api-ms-win-crt-.*\.dll` runs to +/// the last `.dll` on the line and covers an independent `msvcp140.dll`, +/// `dkms .*failed` covers an independent `dpkg: error`, and so on. Since +/// collapsing can only lower a score, applying it there would push real +/// diagnoses below [`MIN_SCORE_FOR_MATCH`] and make them vanish +/// (`overlap_dedup_does_not_demote_other_catalog_keyword_tables` pins this). +/// [`KEYWORDS_VLLM_OOM`] is safe because its overlaps are true nestings of +/// literal phrases, not artifacts of a greedy wildcard. +fn keyword_score_collapsing_overlaps(symptom: &str, table: KeywordTable) -> (i32, Vec) { + keyword_score_impl(symptom, table, true) +} + +fn keyword_score_impl( + symptom: &str, + table: KeywordTable, + collapse_overlaps: bool, +) -> (i32, Vec) { if symptom.is_empty() { return (0, Vec::new()); } let sym = symptom.to_lowercase(); - let mut hits: Vec<(i32, &'static str)> = Vec::new(); + let mut hits: Vec<(i32, &'static str, usize, usize)> = Vec::new(); for (pattern, weight, label) in table { - if Regex::new(pattern).is_ok_and(|re| re.is_match(&sym)) { - hits.push((*weight, label)); + if let Ok(re) = Regex::new(pattern) + && let Some(m) = re.find(&sym) + { + hits.push((*weight, label, m.start(), m.end())); } } if hits.is_empty() { @@ -409,9 +521,18 @@ fn keyword_score(symptom: &str, table: KeywordTable) -> (i32, Vec) { } // Mirror diagnose.py's `hits.sort(reverse=True)`: weight desc, then label desc. hits.sort_by(|a, b| b.0.cmp(&a.0).then_with(|| b.1.cmp(a.1))); - hits.truncate(2); - let score = hits.iter().map(|(w, _)| *w).sum(); - let labels = hits.iter().map(|(_, l)| (*l).to_owned()).collect(); + // Keep the strongest hit for any overlapping span: the same substring must + // not contribute two evidence bullets (nor two weights toward the score). + let mut kept: Vec<(i32, &'static str, usize, usize)> = Vec::new(); + for hit in hits { + let overlaps = collapse_overlaps && kept.iter().any(|k| hit.2 < k.3 && k.2 < hit.3); + if !overlaps { + kept.push(hit); + } + } + kept.truncate(2); + let score = kept.iter().map(|(w, ..)| *w).sum(); + let labels = kept.iter().map(|(_, l, ..)| (*l).to_owned()).collect(); (score, labels) } @@ -1398,7 +1519,7 @@ fn check_15_msvc_redist(e: &Examination, symptom: &str) -> Diagnosis { ], fix_id: "fix-15-msvc-redist".to_owned(), verify: "where vcruntime140.dll && where vcruntime140_1.dll".to_owned(), - notes: vec!["If installing the redistributable still leaves a missing-DLL error, the failing DLL is probably amdhip64_X.dll itself; that points at fix-13-hip-sdk-missing (the HIP SDK install) rather than this fix.".to_owned()], + notes: vec!["If installing the redistributable still leaves a missing-DLL error, the failing DLL is probably amdhip64_X.dll itself; that points at fix-13-hip-sdk-missing rather than this fix.".to_owned()], ..Fix::default() }; finalize( @@ -1410,6 +1531,186 @@ fn check_15_msvc_redist(e: &Examination, symptom: &str) -> Diagnosis { ) } +/// A required anchor before [`KEYWORDS_VLLM_OOM`] is even scored: the word +/// "vllm" itself, or its distinctive VRAM-reservation flag. Without one of +/// these, a generic HIP/CUDA/PyTorch OOM string is not evidence of *this* +/// failure mode -- see the comment on [`KEYWORDS_VLLM_OOM`]. `tensor[-_]parallel` +/// is deliberately NOT an anchor: it is a Megatron/DeepSpeed term (rocm-cli does +/// not serve one model across GPUs), so anchoring on it would misattribute those +/// frameworks' OOMs to vLLM. +/// +/// `gpu[-_]memory[-_]utilization` is also a (20-point) entry in +/// [`KEYWORDS_VLLM_OOM`], so a line that only echoes the flag -- a config dump, +/// or a paste of rocm-cli's own low-VRAM hint, which prints +/// `--gpu-memory-utilization` -- is both anchored and self-scoring. That is +/// harmless *because* only anchored lines are scored: such a line is worth 20, +/// far below [`MIN_SCORE_FOR_MATCH`], so it can surface as a weak signal in +/// `matched` but can never carry a verdict, and it can no longer lend its anchor +/// to an OOM elsewhere in the paste. It stays an anchor because the flag is +/// vLLM-specific, and because a real vLLM OOM often names it on the failing line. +const VLLM_ANCHOR_PATTERN: &str = r"vllm|gpu[-_]memory[-_]utilization"; + +/// A canonical vLLM-OOM `--symptom` string guaranteed to clear +/// [`MIN_SCORE_FOR_MATCH`]. +/// +/// It carries two independent signals (the torch exception class *and* the HIP +/// allocator message) on one anchored line, so it scores even after overlap +/// de-duplication. The vLLM engine falls back to this when a user's actual +/// failing line would not itself be diagnosable, so the `rocm diagnose` command +/// it prints always reports a cause. +pub const VLLM_OOM_CANONICAL_SYMPTOM: &str = "vllm: torch.OutOfMemoryError: HIP out of memory"; + +/// The lines of `symptom` that carry the vLLM OOM anchor (`vllm` or +/// `gpu_memory_utilization`), joined by newlines — the only text +/// [`check_16_vllm_oom`] is allowed to score. +/// +/// Co-occurrence anywhere in a pasted log is too loose -- a stray `vllm` mention +/// re-enables the very misattribution the anchor exists to prevent (a +/// `llama.cpp` OOM in a paste that also names vLLM). Returning the anchored +/// lines rather than a yes/no gate is what makes that true: regex matching +/// ignores line boundaries, so scoring the whole symptom behind a boolean gate +/// still counted keyword hits from *unanchored* lines at full weight, and one +/// benign `gpu_memory_utilization` config echo was enough to hand another +/// framework's OOM a HIGH_CONFIDENCE vLLM verdict +/// (`only_the_anchored_lines_are_scored_not_the_whole_paste`). +/// +/// The join is by `\n` so that no pattern can straddle two lines: the regexes +/// here are literals or use `.*`, which does not match a newline. Dropping the +/// unanchored lines therefore cannot fabricate a match across the seam. +/// +/// The same-line "anchor + OOM token" requirement falls out of this: an anchored +/// line with no OOM token contributes nothing, so the score is 0 and the checker +/// returns no diagnosis. The engine's emitted `vllm: ` keeps the +/// anchor and the error on one line by construction. +/// +/// The split is [`crate::terminal::rendered_lines`], not `str::lines()` and not +/// a set of separator characters. `lines()` splits on `\n` and `\r\n` only, so a +/// lone `\r` was ordinary text to it — and a bare CR is what every progress bar +/// in this ecosystem emits to repaint its line (tqdm, pip, huggingface). A raw +/// terminal capture pasted into `rocm diagnose --symptom '...'` therefore +/// collapsed into one giant "line", the anchor matched somewhere in it, and +/// every keyword in the paste scored — exactly the whole-paste scoring this +/// function exists to prevent, reinstated by one byte +/// (`any_line_advance_is_a_boundary_not_scoreable_text`). +/// +/// Adding `\r` to the split fixed that one byte and nothing else, because a +/// two-character allowlist is not the boundary a terminal renders. Every other +/// way of starting a new line — `ESC E` (`NEL`), `ESC D` (`IND`), `CSI n B` +/// (`CUD`), which are terminfo's `nel`/`ind`/`cud1` and ordinary output from +/// curses- and `rich`-style progress UIs — collapsed the paste just as +/// completely, as did `\x0b`, `\x0c`, `\u{85}`, `U+2028`/`U+2029` and any stray +/// control byte. Each one scored a `llama.cpp` OOM at 95 against a `vllm` anchor +/// from a different rendered line, past a `HIGH_CONFIDENCE` threshold of 75, and +/// cited the other engine's tokens as its evidence. +/// +/// What this is *not* is a terminal emulator. The contract is one-sided, and +/// [`crate::terminal::rendered_lines`] is where to read it — that doc carries +/// the contract itself and points on to the module section holding the detail +/// behind it, including the single shape in which two rendered rows still come +/// back as one segment. It is deliberately not restated here: a second copy is +/// a stale copy waiting to happen, and this function is where a stale one would +/// do the damage. What matters at this call site is the direction of the error +/// that remains: a capture whose cursor addressing that walk does not model +/// loses a diagnosis rather than inventing one, and the canonical-symptom +/// fallback already covers that. The misattribution is the failure mode with a +/// user-visible cost, and this is what narrows it to the one shape that +/// contract calls out. +/// +/// Sharing that walk with the vLLM engine's sanitizer is also what keeps the +/// `SGR` exception right. A colourised logger emits `ESC [ 31 m` *inside* a +/// line, so a splitter that treats every escape as a boundary cuts a genuine +/// `vllm: ... torch.OutOfMemoryError` between its anchor and its error token and +/// silently stops diagnosing it (measured: 95 → 0). +/// +/// `\r\n` yields an empty segment, which carries no anchor and is dropped. +fn vllm_anchored_lines(symptom: &str) -> String { + let Ok(anchor) = Regex::new(VLLM_ANCHOR_PATTERN) else { + return String::new(); + }; + crate::terminal::rendered_lines(symptom) + .into_iter() + .filter(|line| anchor.is_match(&line.to_lowercase())) + .collect::>() + .join("\n") +} + +/// Whether `symptom` clears [`MIN_SCORE_FOR_MATCH`] for the vLLM-OOM failure +/// mode. +/// +/// The vLLM engine calls this to decide whether the user's actual failing line +/// is worth routing into a `--symptom` example, or whether to fall back to +/// [`VLLM_OOM_CANONICAL_SYMPTOM`] so the printed command is guaranteed to report +/// a cause rather than reading as "the tool checked and found nothing". +#[must_use] +pub fn vllm_oom_symptom_is_diagnosable(symptom: &str) -> bool { + let e = Examination { + os_family: "linux".to_owned(), + ..Examination::default() + }; + check_16_vllm_oom(&e, symptom).score >= MIN_SCORE_FOR_MATCH +} + +/// vLLM ran the GPU out of memory. Keyword-only: an [`Examination`] carries no +/// per-GPU VRAM or tenancy fields, so nothing structural can corroborate this — +/// the user must pass the error via `--symptom`, or arrive from the serve +/// failure note that points here. `e` is therefore unused. +fn check_16_vllm_oom(_e: &Examination, symptom: &str) -> Diagnosis { + // Score the anchored lines only, never the whole paste. The anchor is the + // *only* thing separating this failure mode from any other framework's + // PyTorch/HIP OOM, so a boolean gate over the whole symptom is not enough: + // keyword hits from unanchored lines would still count at full weight. Any + // future change here must keep the scored text restricted to anchored lines. + let anchored = vllm_anchored_lines(symptom); + if anchored.is_empty() { + // No vLLM anchor anywhere: don't attribute a bare framework OOM to vLLM. + return zero("fix-16-vllm-oom", "vLLM GPU out of memory"); + } + let (score, evidence) = keyword_score_collapsing_overlaps(&anchored, KEYWORDS_VLLM_OOM); + if score <= 0 { + return zero("fix-16-vllm-oom", "vLLM GPU out of memory"); + } + // Two different faults share this error, and the remediation splits on which + // one it is: on a shared/busy GPU vLLM's fixed ~90% reservation collides + // with memory already in use, so lowering the reservation is the fix; for a + // model that genuinely does not fit, lowering it only trades an earlier OOM + // for a later one, so the wording must not present the knob as the answer in + // that case. + let summary = format!( + "{} If instead the model genuinely does not fit in this GPU's VRAM, lowering the \ + reservation will not help — use a smaller or quantized model (rocm-cli serves one \ + model on a single GPU; it does not shard a model across GPUs).", + crate::VLLM_GPU_MEMORY_UTILIZATION_HINT + ); + let fix = Fix { + summary, + // Byte-identical to the `fix-16-vllm-oom` catalog recipe's block, and + // pinned there by `the_oom_plan_matches_the_catalog_copy`. The two are + // one plan in two modules and a user may meet either copy. + commands: vec![ + "# Case 1 -- shared/busy GPU (tenancy collision): lower the reservation,".to_owned(), + "# or steer vLLM onto a less-busy device:".to_owned(), + "rocm serve --gpu-memory-utilization 0.5".to_owned(), + "rocm serve --gpu ".to_owned(), + "# Case 2 -- the model genuinely does not fit: the reservation is not the".to_owned(), + "# problem; pick a smaller or quantized model (single-GPU serving only).".to_owned(), + ], + fix_id: "fix-16-vllm-oom".to_owned(), + verify: "rocm serve # re-run and watch for a clean startup".to_owned(), + notes: vec![ + "Only lower --gpu-memory-utilization when the GPU is shared or already busy; on a GPU dedicated to this server it cannot create the room a too-large model needs.".to_owned(), + "This entry is keyword-matched from the error text: `rocm diagnose` cannot see per-GPU VRAM or tenancy, so pass the failure with --symptom (or arrive from the `rocm serve` failure note).".to_owned(), + ], + ..Fix::default() + }; + finalize( + "fix-16-vllm-oom", + "vLLM ran the GPU out of memory at startup", + score, + evidence, + fix, + ) +} + /// A code object manager library that does not belong to the active runtime. /// /// The rule is symmetric, and that is what makes it safe. Rather than asking @@ -2178,9 +2479,10 @@ const CHECKERS: &[Checker] = &[ (check_13_hip_sdk_missing, &["windows"]), (check_14_adrenalin_too_old, &["windows"]), (check_15_msvc_redist, &["windows"]), + (check_16_vllm_oom, &["linux", "wsl"]), // Linux-only: `libtorch_cuda.so` is an ELF name, and the vLLM engine is // gated off native Windows. 17 rather than 16 because the vLLM - // out-of-memory entry reserves 16 on its own branch; the number is a stable + // out-of-memory entry above already holds 16; the number is a stable // handle, so the two do not get to share one. (check_17_torch_dlpack_cuda_variant, &["linux"]), (check_wsl_1_gpu_not_exposed, WSL_ONLY), @@ -2293,6 +2595,10 @@ pub fn diagnose(e: &Examination, symptom: &str) -> DiagnoseReport { // in the catalog now, with its own entries; what keeps the bare-metal checks // off it is `platform_family`, not a special case at this level. // + // `fix-16-vllm-oom` opts into `wsl` in CHECKERS for that reason: it is a + // keyword-only check whose failure mode is just as real under WSL2, so it + // still answers there alongside the WSL entries. + // // `out_of_scope` still exists, for the platforms that genuinely have no // entries. That case used to fall through to an empty catalog and report "no // known misconfiguration", which reads as "your machine looks fine" when the @@ -3384,6 +3690,28 @@ mod tests { crate::fix::assert_plan_matches_the_catalog_copy(&fix.fix_id, &commands); } + #[test] + fn the_oom_plan_matches_the_catalog_copy() { + // Same two-copies-of-one-plan risk as `fix-17-torch-dlpack`, and it had + // already opened: the executable lines agreed, so the cross-check over + // the fixes shared with the catalog + // (`diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids`) + // saw nothing -- it filters `#` lines by construction -- while the comments explaining + // *which of the two faults each step addresses* had drifted apart. That + // prose is the whole point of this entry: the two faults need opposite + // responses, so a user meeting the `diagnose` copy and a user meeting + // the `rocm fix` copy must be told the same thing. + let report = diagnose(&linux_base(), VLLM_OOM_CANONICAL_SYMPTOM); + let fix = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .and_then(|d| d.fix.as_ref()) + .expect("the finding must carry a plan"); + let commands: Vec<&str> = fix.commands.iter().map(String::as_str).collect(); + crate::fix::assert_plan_matches_the_catalog_copy(&fix.fix_id, &commands); + } + #[test] fn the_extension_name_alone_does_not_establish_the_variant_failure() { // The extension appearing in a traceback says it is involved, not that @@ -4464,6 +4792,42 @@ mod tests { assert!(!report.has_match()); } + #[test] + fn an_out_of_scope_report_never_carries_matched_entries() { + // The serialized JSON contract on `out_of_scope`: when it is set the + // catalog did not run, so `matched` cannot hold anything -- not even a + // weak sub-threshold signal. One direction only: a covered platform with + // nothing wrong also has an empty `matched`, with `out_of_scope` unset. + // Nothing else in the tree pins the pair, which is how the field's own + // doc comment came to claim the opposite. Two independent mechanisms + // uphold it today -- `diagnose` clears `matched` when `out_of_scope` is + // set, and `run_all_checks` filters on the same platform family + // `catalog_covers` consults -- so this fails once both are gone, which + // is exactly the "a checker somehow ran on an uncovered host" shape the + // JSON contract must not develop. + let mut uncovered = linux_base(); + uncovered.os_family = "other".to_owned(); + let mut windows = linux_base(); + windows.os_family = "windows".to_owned(); + for e in [linux_base(), wsl_base(), windows, uncovered] { + // A symptom that scores for at least one entry where the catalog + // applies, so "matched is empty" is a real consequence of the + // platform being uncovered rather than of a silent symptom. + let report = diagnose(&e, VLLM_OOM_CANONICAL_SYMPTOM); + assert!( + report.out_of_scope.is_none() || report.matched.is_empty(), + "out_of_scope set with matched entries on {}: {:?}", + e.os_family, + report.matched.iter().map(|d| &d.id).collect::>() + ); + assert!( + report.out_of_scope.is_none() || !report.has_match(), + "out_of_scope set alongside has_match on {}", + e.os_family + ); + } + } + #[test] fn non_wsl_still_diagnoses_normally() { let mut e = linux_base(); @@ -4630,6 +4994,603 @@ mod tests { assert!(report.matched.iter().any(|d| d.id == "fix-11-iommu")); } + #[test] + fn vllm_oom_signature_is_a_high_confidence_match() { + // The distinctive vLLM startup OOM: torch/HIP allocation failure, with + // the vLLM anchor that tells this apart from an arbitrary PyTorch OOM. + let report = diagnose( + &linux_base(), + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + ); + let top = &report.matched[0]; + assert_eq!(top.id, "fix-16-vllm-oom"); + assert!(top.score >= HIGH_CONFIDENCE, "score was {}", top.score); + assert!(report.has_match()); + // The remediation must stay conditional: the reservation knob is right + // for a tenancy collision and wrong for a model that does not fit. + let fix = top.fix.as_ref().unwrap(); + assert!(!fix.auto_applicable, "OOM workaround must be print-only"); + assert!(fix.summary.contains("--gpu-memory-utilization")); + assert!( + fix.summary.contains("does not fit"), + "the 'genuinely does not fit' caveat must survive: {}", + fix.summary + ); + } + + #[test] + fn generic_pytorch_oom_without_a_vllm_signal_does_not_match() { + // The reviewed false positive: `torch.OutOfMemoryError: CUDA out of + // memory` scores 45+45=90 under the keyword table alone, but nothing + // about that string is vLLM-specific -- any ROCm PyTorch job emits the + // identical shape (ROCm's PyTorch build reports the CUDA-compat name). + // Without an explicit vLLM anchor, this failure mode must not fire. + let report = diagnose(&linux_base(), "torch.OutOfMemoryError: CUDA out of memory"); + assert!( + report.matched.iter().all(|d| d.id != "fix-16-vllm-oom"), + "a bare framework OOM with no vLLM signal must not be attributed to vLLM: {:?}", + report.matched + ); + assert!(!report.has_match()); + } + + #[test] + fn torch_oom_class_alone_is_not_a_vllm_match() { + // `outofmemory` is nested inside the exception class name. It must not + // count as a second, independent signal and turn one token into a + // high-confidence vLLM diagnosis for an arbitrary PyTorch workload. + let report = diagnose(&linux_base(), "vllm: torch.OutOfMemoryError"); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("the weak keyword signal remains visible"); + assert!(oom.score < MIN_SCORE_FOR_MATCH, "score was {}", oom.score); + assert!(!report.has_match()); + } + + #[test] + fn a_bare_out_of_memory_stays_below_the_match_threshold() { + // Without the vLLM/HIP shape, "out of memory" alone must not claim this + // failure mode -- it scores (the `vllm` anchor is present and "out of + // memory" is a keyword), but stays below MIN_SCORE_FOR_MATCH. + let report = diagnose(&linux_base(), "vllm: the process was killed: out of memory"); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("the weak `out of memory` keyword signal must remain visible in `matched`"); + assert!( + oom.score < MIN_SCORE_FOR_MATCH, + "a bare OOM must stay sub-threshold, got {}", + oom.score + ); + } + + #[test] + fn vllm_oom_is_not_available_on_native_windows() { + // vLLM is Linux/WSL-only (native Windows is unsupported), so the checker + // must not fire on a Windows examination even with the error text. + let e = Examination { + os_family: "windows".to_owned(), + ..Examination::default() + }; + let report = diagnose(&e, "vllm: torch.OutOfMemoryError: HIP out of memory."); + assert!(report.matched.iter().all(|d| d.id != "fix-16-vllm-oom")); + } + + #[test] + fn vllm_oom_keyword_diagnosis_is_available_on_wsl() { + let mut e = linux_base(); + e.is_wsl = true; + let report = diagnose( + &e, + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + ); + assert!(report.out_of_scope.is_none()); + assert!(report.has_match()); + assert_eq!(report.matched[0].id, "fix-16-vllm-oom"); + } + + #[test] + fn wsl_sub_threshold_vllm_signal_is_preserved_in_matched() { + // A weak (sub-threshold) vLLM OOM signal on WSL must still surface in + // `matched` per the `DiagnoseReport::matched` contract: `run_all_checks` + // drops only zero-score results, so a score-25 mention is reported as a + // WEAK row rather than silently discarded. + // + // This used to also assert that the weak hit did not bury a WSL + // out-of-scope routing note. There is no such note any more -- WSL2 is a + // covered platform with its own catalog entries -- so what is left to pin + // is the half that still has teeth: the entry survives in `matched`, and + // it does not promote itself into a match. + // + // `wsl_base()` (a healthy WSL GPU stack) rather than a default + // examination: a default one has no /dev/dxg, so fix-wsl-1 would fire at + // 55 and the report would match for reasons that have nothing to do with + // the signal under test. + let report = diagnose(&wsl_base(), "vllm: out of memory"); + assert!(!report.has_match()); + assert!( + report.out_of_scope.is_none(), + "WSL2 is a covered platform, not an out-of-scope one" + ); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("a nonzero sub-threshold hit must stay in `matched`, not be dropped"); + assert!(oom.score < MIN_SCORE_FOR_MATCH, "score was {}", oom.score); + } + + #[test] + fn a_genuine_vllm_oom_match_on_wsl_is_surfaced_not_routed_out_of_scope() { + // The other side of the has_match gate: a real, at-or-above-threshold + // vLLM OOM on WSL is surfaced, and ranks top on a host whose own WSL GPU + // stack is healthy -- so the verdict is the OOM, not an incidental WSL + // finding. + let report = diagnose(&wsl_base(), VLLM_OOM_CANONICAL_SYMPTOM); + assert!(report.has_match()); + assert!(report.out_of_scope.is_none()); + assert_eq!(report.matched[0].id, "fix-16-vllm-oom"); + } + + #[test] + fn hip_error_out_of_memory_status_is_a_match() { + // Regression: `RuntimeError: hipErrorOutOfMemory` previously scored 0 + // (`\boutofmemory\b` has no word boundary after `hipError`), so the + // engine printed a diagnose command that reported nothing. + let report = diagnose(&linux_base(), "vllm: RuntimeError: hipErrorOutOfMemory"); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("hipErrorOutOfMemory must now be recognized"); + assert!( + oom.score >= MIN_SCORE_FOR_MATCH, + "hipErrorOutOfMemory should clear the threshold, got {}", + oom.score + ); + } + + #[test] + fn torch_cuda_module_path_oom_class_is_recognized() { + // Regression: ROCm's PyTorch reports the class under the `.cuda.` module + // path (`torch.cuda.OutOfMemoryError`); the class pattern must match it. + // It is still the (sub-threshold) class name, not a stronger message. + let report = diagnose(&linux_base(), "vllm: torch.cuda.OutOfMemoryError"); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("the torch.cuda.* class name must be recognized as evidence"); + assert!(oom.score > 0, "score was {}", oom.score); + assert!( + oom.score < MIN_SCORE_FOR_MATCH, + "the bare class name must stay sub-threshold, got {}", + oom.score + ); + } + + #[test] + fn hip_out_of_memory_yields_a_single_evidence_line() { + // Overlap guard: "HIP out of memory" must not be counted both as the + // 50-point HIP message and the 25-point `out of memory` nested inside + // it. One phrase, one evidence bullet. + let report = diagnose(&linux_base(), "vllm: HIP out of memory"); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("HIP out of memory must match"); + let fix = oom.fix.as_ref().unwrap(); + let evidence_lines = oom + .evidence + .iter() + .filter(|e| e.to_lowercase().contains("out of memory")) + .count(); + assert_eq!( + evidence_lines, 1, + "one phrase must yield one evidence line, got {:?}", + oom.evidence + ); + assert_eq!(oom.score, 50, "the HIP message carries the match alone"); + assert!(fix.summary.contains("--gpu-memory-utilization")); + } + + #[test] + fn overlap_dedup_does_not_demote_other_catalog_keyword_tables() { + // The vLLM-OOM table needs overlapping spans collapsed ("out of memory" + // nested inside "HIP out of memory"). Other tables must NOT get that + // treatment: several of them pair a greedy `.*` pattern with a second, + // genuinely independent keyword, and the greedy span swallows the + // independent one. Collapsing there silently loses real diagnoses, + // because a keyword score can only fall. + // + // Each case below pins the score these tables produced before the + // de-duplication was introduced. + + // `api-ms-win-crt-.*\.dll` (35) runs to the *last* `.dll` on the line, + // covering the independent `msvcp140.dll` (30). Two distinct missing + // DLLs, two signals -- 65 clears MIN_SCORE_FOR_MATCH, 35 does not. + let (score, ev) = keyword_score( + "api-ms-win-crt-runtime-l1-1-0.dll missing and msvcp140.dll not found", + KEYWORDS_MSVC_REDIST, + ); + assert_eq!( + score, 65, + "two distinct missing DLLs are two signals: {ev:?}" + ); + assert!(score >= MIN_SCORE_FOR_MATCH); + + // `dkms .*failed` (45) spans the whole line, covering `dpkg: error` (25). + let (score, ev) = keyword_score( + "dkms status: dpkg: error processing amdgpu-dkms, build failed", + KEYWORDS_DPKG_BROKEN, + ); + assert_eq!( + score, 70, + "a DKMS failure and a dpkg error are two signals: {ev:?}" + ); + assert!(score >= MIN_SCORE_FOR_MATCH); + + // `amdhip64.*not found` (50) spans past `could not find hip` (40): + // 90 is HIGH confidence, 50 is only a bare match. + let (score, ev) = keyword_score( + "amdhip64.dll: could not find hip runtime, not found", + KEYWORDS_HIP_SDK_MISSING, + ); + assert_eq!(score, 90, "both HIP-SDK signals must count: {ev:?}"); + assert!(score >= HIGH_CONFIDENCE); + + // `404.*repo\.radeon\.com` (50) runs to the *last* repo.radeon.com on + // the line, covering `unable to locate package rocm` (35). + let (score, ev) = keyword_score( + "e: failed to fetch https://repo.radeon.com/rocm/apt/jammy/release 404 not found, \ + unable to locate package rocm, retrying https://repo.radeon.com/rocm/apt", + KEYWORDS_REPO_BROKEN, + ); + assert_eq!(score, 85, "both apt-repo signals must count: {ev:?}"); + assert!(score >= HIGH_CONFIDENCE); + } + + #[test] + fn windows_missing_dll_pair_still_reaches_the_msvc_redist_fix() { + // The user-visible half of the guard above: with no PATH probe result + // (`msvc_redist_present == None`) the keyword score alone decides, so a + // demoted table makes fix-15 vanish from the report entirely. + let e = Examination { + os_family: "windows".to_owned(), + ..Examination::default() + }; + let report = diagnose( + &e, + "api-ms-win-crt-runtime-l1-1-0.dll missing and msvcp140.dll not found", + ); + let msvc = report + .matched + .iter() + .find(|d| d.id == "fix-15-msvc-redist") + .expect("the MSVC redistributable fix must still be reported"); + assert!( + msvc.score >= MIN_SCORE_FOR_MATCH, + "score was {}", + msvc.score + ); + assert!(report.has_match()); + } + + #[test] + fn vllm_anchor_must_be_on_the_same_line_as_the_oom_token() { + // Co-occurrence anywhere is too loose: a paste that mentions vLLM on one + // line and an unrelated framework's OOM on another must not attribute + // the OOM to vLLM. + let report = diagnose( + &linux_base(), + "I installed vllm last week.\nToday my llama.cpp run died: CUDA out of memory", + ); + assert!( + report.matched.iter().all(|d| d.id != "fix-16-vllm-oom"), + "an OOM on a different line from the vLLM mention must not match: {:?}", + report.matched + ); + // The engine's emitted `vllm: ` keeps them on one line and + // still matches. + let anchored = diagnose(&linux_base(), "vllm: CUDA out of memory"); + assert!(anchored.matched.iter().any(|d| d.id == "fix-16-vllm-oom")); + } + + #[test] + fn only_the_anchored_lines_are_scored_not_the_whole_paste() { + // Regression: the anchor was a boolean *gate*, after which the entire + // symptom was scored. Regex matching ignores line boundaries, so a + // benign vLLM config echo on one line unlocked full-weight scoring of + // another framework's OOM on a different line: `cuda out of memory` + // (50) + `torch.outofmemoryerror` (45) do not overlap, so both survived + // collapsing for 95 -- above HIGH_CONFIDENCE -- and the user got a + // confident "vLLM ran the GPU out of memory" verdict for a llama.cpp + // failure. Only the anchored lines may be scored. + let report = diagnose( + &linux_base(), + "vllm config: gpu_memory_utilization=0.9\n\ + llama.cpp: torch.OutOfMemoryError: CUDA out of memory", + ); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("the anchored config line is still a (weak) vLLM signal"); + assert!( + oom.score < MIN_SCORE_FOR_MATCH, + "an unanchored framework's OOM must not score for vLLM, got {} from {:?}", + oom.score, + oom.evidence + ); + assert!( + oom.evidence + .iter() + .all(|e| !e.to_lowercase().contains("out of memory")), + "evidence must come from the anchored line only: {:?}", + oom.evidence + ); + + // Same shape with the anchored line carrying no OOM token at all: the + // unanchored OOM must not be borrowed to reach a verdict either. + let report = diagnose( + &linux_base(), + "starting vllm serve on gpu 0\n\ + llama.cpp: torch.OutOfMemoryError: CUDA out of memory. Tried to allocate 7.21 GiB.", + ); + assert!( + report.matched.iter().all(|d| d.id != "fix-16-vllm-oom"), + "an OOM on an unanchored line must not be attributed to vLLM: {:?}", + report.matched + ); + + // The control: once the OOM is on the anchored line, it still matches at + // full strength, so the scoping did not simply disable the checker. + let report = diagnose( + &linux_base(), + "vllm config: gpu_memory_utilization=0.9\n\ + vllm: torch.OutOfMemoryError: CUDA out of memory", + ); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("an anchored OOM line must still match"); + assert!(oom.score >= HIGH_CONFIDENCE, "score was {}", oom.score); + } + + /// The `fix-16-vllm-oom` score for `symptom`, or 0 when it did not match. + fn vllm_oom_score(symptom: &str) -> i32 { + diagnose(&linux_base(), symptom) + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .map_or(0, |d| d.score) + } + + #[test] + fn any_line_advance_is_a_boundary_not_scoreable_text() { + // Regression, in two rounds. + // + // The filter first used `str::lines()`, which splits on `\n` and `\r\n` + // only -- a lone `\r` is ordinary text to it. Every progress bar in this + // ecosystem (tqdm, pip, huggingface) repaints with a bare CR, and a + // pasted terminal capture is the expected way to use `--symptom`, so the + // whole capture collapsed into one "line": the vLLM anchor matched + // somewhere in it and another framework's OOM elsewhere in it scored at + // full weight. That is + // `only_the_anchored_lines_are_scored_not_the_whole_paste` defeated by + // one byte, and neither existing guard saw it because both use `\n`-only + // fixtures. + // + // Adding `\r` to the split fixed that byte and left the hole open for + // every other one. Measured on the round that did so: each separator + // below put the anchor and a llama.cpp OOM on genuinely separate + // rendered lines, and each scored 95 -- past a HIGH_CONFIDENCE of 75, + // citing the llama.cpp line's tokens as the evidence for a vLLM verdict. + // Hence a table rather than one test per byte: the property is "a line + // advance is a line advance", and enumerating it is what stops the next + // separator from reopening this. + for (label, sep) in [ + ("LF", "\n"), + ("CR", "\r"), + ("CRLF", "\r\n"), + ("ESC E (NEL)", "\u{1b}E"), + ("ESC D (IND)", "\u{1b}D"), + ("CSI 1 B (CUD)", "\u{1b}[1B"), + ("CSI H (CUP)", "\u{1b}[2;1H"), + ("VT", "\u{b}"), + ("FF", "\u{c}"), + ("C1 NEL", "\u{85}"), + ("U+2028 LINE SEPARATOR", "\u{2028}"), + ("U+2029 PARAGRAPH SEPARATOR", "\u{2029}"), + ("a stray control byte", "\u{1}"), + ] { + let capture = format!( + "Downloading shards: 10%{sep}vllm serve starting up{sep}\ + Downloading shards: 90%{sep}\ + llama.cpp: torch.OutOfMemoryError: CUDA out of memory. \ + Tried to allocate 7.21 GiB.{sep}" + ); + assert_eq!( + vllm_oom_score(&capture), + 0, + "a {label}-separated llama.cpp OOM must not be attributed to vLLM" + ); + } + + // The control, per separator: a capture whose *anchored* segment carries + // the OOM must still match, so the boundary set did not simply blind the + // checker to every capture that contains one. + for (label, sep) in [ + ("CR", "\r"), + ("ESC E (NEL)", "\u{1b}E"), + ("VT", "\u{b}"), + ("a stray control byte", "\u{1}"), + ] { + let capture = format!( + "Downloading shards: 90%{sep}\ + vllm: torch.OutOfMemoryError: HIP out of memory{sep}" + ); + let score = vllm_oom_score(&capture); + assert!( + score >= MIN_SCORE_FOR_MATCH, + "an anchored OOM segment must still match across {label}: score was {score}" + ); + } + } + + #[test] + fn colour_codes_inside_a_line_do_not_split_a_genuine_vllm_oom() { + // The other half of the boundary question, and the reason this is a + // grammar walk and not `char::is_control()`. A colourised logger emits + // SGR *inside* a line -- level name in colour, message plain -- so a + // splitter that treats every escape, or every control character, as a + // boundary cuts a real `vllm: ... torch.OutOfMemoryError` between its + // anchor and its error token. Measured on the one-line + // `is_control()`-split candidate: this case went from 95 to 0, trading + // the false positive above for a silent miss on the most ordinary real + // input there is. + let plain = "vllm: torch.OutOfMemoryError: HIP out of memory"; + let coloured = + "\u{1b}[31mvllm: \u{1b}[1mtorch.OutOfMemoryError: HIP out of memory\u{1b}[0m"; + assert_eq!( + vllm_oom_score(coloured), + vllm_oom_score(plain), + "SGR inside a line must not change the verdict" + ); + let score = vllm_oom_score(coloured); + assert!(score >= HIGH_CONFIDENCE, "score was {score}"); + + // And the same line after a progress-bar repaint, which is how it + // actually arrives: the CR is a boundary, the SGR around it is not. + assert!( + vllm_oom_score(&format!("Loading: 90%\r{coloured}\n")) >= HIGH_CONFIDENCE, + "a colourised OOM after a CR repaint must still match" + ); + } + + #[test] + fn rocm_cli_s_own_low_vram_hint_is_never_a_verdict_on_its_own() { + // `gpu[-_]memory[-_]utilization` is both an anchor and a scoring entry, + // so a line that merely echoes the flag anchors itself. rocm-cli's own + // low-VRAM hint prints `--gpu-memory-utilization`, and users paste it + // back in. That must stay a weak signal, never a diagnosis: worth 20, + // far below MIN_SCORE_FOR_MATCH, with nothing else on the line to + // corroborate it. + let report = diagnose(&linux_base(), crate::VLLM_GPU_MEMORY_UTILIZATION_HINT); + let oom = report + .matched + .iter() + .find(|d| d.id == "fix-16-vllm-oom") + .expect("the flag mention stays visible as a weak signal"); + assert!( + oom.score < MIN_SCORE_FOR_MATCH, + "a bare flag echo must not diagnose an OOM, got {} from {:?}", + oom.score, + oom.evidence + ); + } + + #[test] + fn canonical_vllm_oom_symptom_is_diagnosable() { + // The engine's fallback symptom must always clear the threshold, so the + // `rocm diagnose` command it prints never reports nothing. + assert!(vllm_oom_symptom_is_diagnosable(VLLM_OOM_CANONICAL_SYMPTOM)); + // And a bare framework mention the engine would reject is not diagnosable. + assert!(!vllm_oom_symptom_is_diagnosable("vllm: out of memory")); + } + + #[test] + fn every_checker_platform_is_covered_by_its_recipe() { + // `render_report_text` ends a matched diagnosis with `apply with: rocm + // fix {id}`, and `fix::apply` then gates that id on the recipe's + // `applies_on` against the *running* OS. So any platform family a + // checker is registered for but its recipe omits is a platform where the + // tool names a command and then refuses to run it -- which is exactly + // what `fix-16-vllm-oom` did on WSL2, where the checker opts into `wsl` + // and the recipe was LINUX_ONLY. + // + // Only one direction is an error. A recipe may legitimately apply more + // widely than its checker answers (a user can reach `rocm fix ` + // directly, without a diagnosis), so this asserts containment rather + // than equality. + for (check, families) in CHECKERS { + let id = check(&Examination::default(), "").id; + let applies_on = crate::fix::recipe_applies_on(&id).unwrap_or_else(|| { + panic!( + "checker id `{id}` has no recipe in the fix catalog, so the \ + `apply with: rocm fix {id}` line diagnose prints is dead" + ) + }); + for family in *families { + assert!( + applies_on.contains(family), + "`{id}` is diagnosed on `{family}` but its recipe only applies on \ + {applies_on:?}; `rocm fix {id}` would print the plan and then exit 3 there" + ); + } + } + } + + #[test] + fn diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids() { + // fix-15 and fix-16 live in two places: the `rocm fix` catalog + // (crate::fix::RECIPES) and the Fix these checks embed in a Diagnosis. + // The prose *framing* legitimately differs by surface (a catalog + // rationale vs a diagnosis summary), but the actionable remediation must + // not silently diverge -- the verify command, the notes, and the + // executable command lines have to agree. This is the guard that they + // do; it caught a two-vs-three-space `verify` and a reworded note. + let vllm_oom = check_16_vllm_oom( + &Examination { + os_family: "linux".to_owned(), + ..Examination::default() + }, + VLLM_OOM_CANONICAL_SYMPTOM, + ); + let msvc = check_15_msvc_redist( + &Examination { + os_family: "windows".to_owned(), + msvc_redist_present: Some(false), + ..Examination::default() + }, + "ImportError: DLL load failed: vcruntime140_1.dll not found", + ); + for diag in [vllm_oom, msvc] { + assert!( + diag.score >= MIN_SCORE_FOR_MATCH, + "test setup must yield a real match for {}", + diag.id + ); + let fix = diag.fix.expect("a matched diagnosis must carry a Fix"); + let (verify, notes, run_commands) = + crate::fix::recipe_verify_notes_and_run_commands(&fix.fix_id) + .expect("a shared fix_id must exist in the recipe catalog"); + assert_eq!(fix.verify, verify, "verify diverged for {}", fix.fix_id); + assert_eq!( + fix.notes, + notes.iter().map(|s| (*s).to_owned()).collect::>(), + "notes diverged for {}", + fix.fix_id + ); + let emitted_run = fix + .commands + .iter() + .map(String::as_str) + .filter(|line| !line.trim_start().starts_with('#')) + .collect::>(); + assert_eq!( + emitted_run, run_commands, + "executable command lines diverged for {}", + fix.fix_id + ); + } + } + #[test] fn keyword_score_takes_top_two() { // INVALID_ISA: two hits (50 + 40) -> 90, not the sum of all. diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 18de40b5c..bd55060e3 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -574,9 +574,38 @@ const RECIPES: &[FixRecipe] = &[ applies_on: PRINT_ON_WINDOWS, runner: None, }, - // The number is a stable handle, not a position: `fix-16` is reserved by the - // vLLM out-of-memory entry on its own branch, so this one takes 17 rather - // than colliding and forcing whichever lands second to rename a published id. + FixRecipe { + fix_id: "fix-16-vllm-oom", + title: "vLLM ran the GPU out of memory at startup", + rationale: "vLLM reserves a fixed fraction (~90%) of each GPU's TOTAL VRAM for its KV cache by default, regardless of the model size or how much is currently free. Two different faults surface the same OOM, and they need opposite responses. If the GPU is shared or already busy, that reservation collides with memory in use and lowering it (or moving to a less-busy GPU) is the fix. If the model genuinely does not fit in this GPU's VRAM, lowering the reservation only trades an earlier OOM for a later one -- use a smaller or quantized model instead (rocm-cli serves one model on a single GPU; it does not shard a model across GPUs).", + commands: &[ + "# Case 1 -- shared/busy GPU (tenancy collision): lower the reservation,", + "# or steer vLLM onto a less-busy device:", + "rocm serve --gpu-memory-utilization 0.5", + "rocm serve --gpu ", + "# Case 2 -- the model genuinely does not fit: the reservation is not the", + "# problem; pick a smaller or quantized model (single-GPU serving only).", + ], + needs_sudo: false, + needs_reboot: false, + needs_relogin: false, + verify: "rocm serve # re-run and watch for a clean startup", + notes: &[ + "Only lower --gpu-memory-utilization when the GPU is shared or already busy; on a GPU dedicated to this server it cannot create the room a too-large model needs.", + "This entry is keyword-matched from the error text: `rocm diagnose` cannot see per-GPU VRAM or tenancy, so pass the failure with --symptom (or arrive from the `rocm serve` failure note).", + ], + // Matches `check_16_vllm_oom`'s `["linux", "wsl"]` registration: WSL2 is + // the supported way to serve from a Windows host and the failure mode is + // just as real there, so the fix has to be runnable where the diagnosis + // is reachable. Pinned by + // `every_checker_platform_is_covered_by_its_recipe`. + doc_url: None, + applies_on: PRINT_ON_LINUX_AND_WSL, + runner: None, + }, + // The number is a stable handle, not a position: `fix-16` is held by the + // vLLM out-of-memory entry above, so this one takes 17 rather than + // colliding and forcing whichever lands second to rename a published id. FixRecipe { fix_id: "fix-17-torch-dlpack", title: "Restore the engine's pinned torch (torch-c-dlpack-ext loads the CUDA variant)", @@ -976,6 +1005,35 @@ pub(crate) fn torch_rocm_indexes_named_in<'a>( .collect() } +/// Test-only view of a recipe's actionable remediation: its `verify` command, +/// its `notes`, and its *executable* command lines (comment/prose lines +/// stripped). Lets the diagnosis catalog cross-check that the Fix it emits for a +/// shared `fix_id` has not silently diverged from this recipe. +#[cfg(test)] +pub(crate) fn recipe_verify_notes_and_run_commands( + fix_id: &str, +) -> Option<(&'static str, &'static [&'static str], Vec<&'static str>)> { + find_recipe(fix_id).map(|recipe| { + let run_commands = recipe + .commands + .iter() + .copied() + .filter(|line| !line.trim_start().starts_with('#')) + .collect::>(); + (recipe.verify, recipe.notes, run_commands) + }) +} + +/// Test-only view of the platform families a recipe is runnable on. +/// +/// `None` when no recipe carries `fix_id`. Lets the diagnosis catalog assert +/// that every platform a checker answers on is a platform `rocm fix` will +/// actually act on, which `recipe_verify_notes_and_run_commands` cannot see. +#[cfg(test)] +pub(crate) fn recipe_applies_on(fix_id: &str) -> Option> { + find_recipe(fix_id).map(|recipe| recipe.applies_on.iter().map(|p| p.os).collect()) +} + /// Whether `fix_id` names an entry in the catalog. /// /// A predicate rather than the lookup itself, because the caller that needs it @@ -2315,9 +2373,10 @@ mod tests { let count = ids.len(); ids.dedup(); assert_eq!(ids.len(), count, "duplicate fix-id in RECIPES"); - // 18 bare-metal/Windows entries (fix-17, fix-18 and fix-19 among them) - // plus the 7 WSL ones. - assert_eq!(count, 25, "expected 25 catalog entries"); + // 19 bare-metal/Windows entries (fix-16-vllm-oom, fix-17-torch-dlpack, + // fix-18-comgr-conflict and fix-19-shm-too-small among them) plus the + // 7 WSL ones. + assert_eq!(count, 26, "expected 26 catalog entries"); } /// Restored, not new. This PR made the `wsl` arm of the platform lookup @@ -2362,6 +2421,43 @@ mod tests { /// `apply` reaches its internal-error path. The converse is deliberately /// not asserted: a print-only recipe may hold a runner that reports without /// writing, which is exactly what `fix-2-unset-override` does on Linux. + #[test] + fn the_utilization_hint_example_matches_the_recipe_command() { + // The shared hint carries a worked value, and `fix-16-vllm-oom` hands + // the user a command carrying another. They drifted once already: the + // docs and the recipe moved to 0.5 while the const kept recommending + // 0.1, so one `rocm diagnose` printed "e.g. 0.1 for a small model" + // directly above `rocm serve --gpu-memory-utilization 0.5`. + // Nothing caught it -- the cross-check in `diagnose` exempts the summary + // as prose framing, and a line-based grep misses the const because the + // flag and the value sit on different continuation lines. + const FLAG: &str = "--gpu-memory-utilization"; + let hint = crate::VLLM_GPU_MEMORY_UTILIZATION_HINT; + let example = hint + .split_once("e.g. ") + .and_then(|(_, rest)| rest.split_whitespace().next()) + .unwrap_or_else(|| { + panic!("the shared hint no longer carries a worked `e.g. `:\n{hint}") + }); + let recipe = find_recipe("fix-16-vllm-oom").expect("fix-16-vllm-oom is in the catalog"); + let commanded = recipe + .commands + .iter() + .find_map(|line| line.split_once(FLAG)) + .and_then(|(_, rest)| rest.split_whitespace().next()) + .unwrap_or_else(|| { + panic!( + "fix-16-vllm-oom no longer commands `{FLAG}`:\n{:#?}", + recipe.commands + ) + }); + assert_eq!( + example, commanded, + "the shared hint recommends `{FLAG} {example}` while `rocm fix fix-16-vllm-oom` \ + commands `{FLAG} {commanded}`; both can print in a single `rocm diagnose` report" + ); + } + #[test] fn a_recipe_that_acts_has_a_runner() { for r in RECIPES { @@ -2510,6 +2606,17 @@ mod tests { ("fix-9-igpu-dgpu", "windows", FixClass::NeedsArgument), ] ); + // fix-16-vllm-oom carries a workaround the user must weigh (lowering the + // VRAM reservation is wrong for a model that genuinely does not fit), so + // it is deliberately PRINT-ONLY and must never join the AUTO set. + let oom = find_recipe("fix-16-vllm-oom").expect("OOM recipe is in the catalog"); + assert!( + oom.applies_on + .iter() + .all(|p| p.class == FixClass::PrintOnly), + "fix-16-vllm-oom must stay PRINT-ONLY on every platform it applies to" + ); + assert!(oom.runner.is_none(), "a PRINT-ONLY recipe has no runner"); } #[test] diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index dddd0a842..708eea9b7 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -38,12 +38,14 @@ pub mod proc_lifecycle; pub mod report; pub mod report_delivery; pub mod runtime; +pub mod terminal; + #[cfg(test)] mod test_env; pub mod uv; pub use diagnose::{ - DiagnoseReport, Diagnosis, Fix, Route, diagnose as run_diagnose, - render_report_text as render_diagnose_text, + DiagnoseReport, Diagnosis, Fix, Route, VLLM_OOM_CANONICAL_SYMPTOM, diagnose as run_diagnose, + render_report_text as render_diagnose_text, vllm_oom_symptom_is_diagnosable, }; pub use disk_space::{ SpaceCheck, available_space_for_path, check_space_for_path, ensure_space_for, @@ -5718,8 +5720,21 @@ fn detect_linux_drm_ip_discovery_gfx_target() -> Option { None } -#[cfg(any(target_os = "linux", test))] -fn is_amdgpu_device(device_dir: &Path) -> bool { +/// Whether a DRM `device` directory belongs to an AMD GPU: the PCI `vendor` id +/// (`0x1002`), or, when that is unreadable, an `amdgpu` `uevent` `DRIVER=` line. +/// +/// Both signals are needed. A vendor-only test under-counts on hosts where +/// `vendor` is absent or unreadable, and the callers are counting *cards* — the +/// KFD/DRM count authority here and the `rocm` CLI's sysfs fallback probe — so +/// an under-count silently narrows the multi-card ordinal guard. +/// +/// `pub` and not `#[cfg(target_os = "linux")]` precisely so there is one copy: +/// the CLI's fallback probe used to carry its own, with a doc comment asserting +/// the two were "the same two-signal test" and nothing holding them to it. It +/// reads files, so there is nothing platform-specific to gate, and gating it +/// would put it out of reach of a dependent crate's `cfg(test)` build. +#[must_use] +pub fn is_amdgpu_device(device_dir: &Path) -> bool { if let Ok(vendor) = fs::read_to_string(device_dir.join("vendor")) && vendor.trim().eq_ignore_ascii_case("0x1002") { @@ -8136,6 +8151,27 @@ pub fn resolve_amd_smi_binary() -> OsString { resolve_amd_smi_binary_in_home(runtime_home_dir().as_deref()) } +/// The `--gpu-memory-utilization` workaround for a shared/busy GPU. +/// +/// Shared by the `rocm` CLI (pre-launch low-VRAM note), the vLLM engine adapter +/// (post-failure OOM hint) and the `fix-16-vllm-oom` diagnosis summary, so those +/// surfaces never drift into different wording for the same fix. The +/// `rocm fix fix-16-vllm-oom` catalog rationale is deliberately NOT this text — +/// it frames the same fault for a different reader — but its worked value is +/// pinned to this one (see below). vLLM reserves a fixed fraction of each +/// GPU's *total* VRAM by default (~0.9), independent of the model size or how +/// much is currently free, so on a shared or busy card that reservation +/// collides with memory already in use and the engine OOMs even a tiny model. +/// +/// The worked example must stay the value the `fix-16-vllm-oom` recipe and the +/// docs hand the user (`0.5`). A smaller budget such as `0.1` sits below the +/// weights of most models people actually serve, so it trades one startup +/// failure for another. Pinned by +/// `the_utilization_hint_example_matches_the_recipe_command`. +pub const VLLM_GPU_MEMORY_UTILIZATION_HINT: &str = "vLLM reserves ~90% of the GPU's total VRAM by default; on a shared or busy GPU this can \ + collide with memory already in use. Lower the reservation with `--gpu-memory-utilization \ + <0-1>` (e.g. 0.5 for a small model), or target a less-busy GPU with `--gpu `."; + /// Locate `amd-smi` inside the bin directories of the newest managed ROCm SDK /// runtime recorded in the registry. The binary ships with the TheRock wheel /// (under the SDK `bin_path` and/or the venv `install_root/bin`) and is not on diff --git a/crates/rocm-core/src/terminal.rs b/crates/rocm-core/src/terminal.rs new file mode 100644 index 000000000..20eead507 --- /dev/null +++ b/crates/rocm-core/src/terminal.rs @@ -0,0 +1,552 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! One ECMA-48 walk over untrusted terminal output, shared by the two callers +//! that need it. +//! +//! Both callers are handling the same input — a raw terminal capture from a +//! subprocess, pasted or quoted back at the user — and both need the same +//! question answered: *what would a terminal have rendered here?* They use the +//! answer differently. +//! +//! * The vLLM engine strips the sequences out, so a colourised or bell-bearing +//! log line cannot repaint the user's terminal from inside rocm-cli's own +//! error message ([`strip_terminal_control_sequences`]). +//! * The vLLM-OOM diagnostic needs the *line boundaries*, so that two things the +//! terminal drew on separate rows are not scored as one line +//! ([`rendered_lines`]) — with the single exception documented below. +//! +//! A second, independent scan for the second caller would have been a second +//! grammar to get wrong, and the first one took three rounds to get right. So +//! the grammar lives here once, in one private stepping function, and each +//! caller interprets the classified tokens it yields. +//! +//! # Line breaks inside a string body: merging and losing rows +//! +//! [`rendered_lines`] is an over-approximation of what a terminal would have +//! drawn: it may split one rendered line in two, or lose one or more +//! altogether, and in the other direction it merges two rendered rows in +//! exactly one shape. A line break *inside* the body of a string-argument +//! sequence (`OSC`, `DCS`, `SOS`, `PM`, `APC`) is consumed with that body, so +//! the text either side of the sequence joins up: +//! +//! ```text +//! rendered_lines("vllm\u{1b}]0;ti\ntle\u{7}llama.cpp OOM") +//! == ["vllmllama.cpp OOM"] +//! rendered_lines("vllm\u{1b}]0;ti\ntle\u{1b}[0mllama.cpp OOM") +//! == ["vllmllama.cpp OOM"] +//! ``` +//! +//! A terminator is not what creates the shape, as the second example shows: it +//! contains no `BEL` and no `ST` anywhere, and the two rows still merge. The +//! body scan runs to the next `BEL`, to `ST` (`ESC \`), to an unrelated `ESC` +//! — which it leaves in place for the next step to re-read, so the hole stays +//! one sequence wide — or to the end of the input, and the break is swallowed +//! in every one of those cases, terminated or not. +//! +//! Swallowing the break is not always a *merge*, though. A merge needs drawable +//! text on both sides of the break landing in one segment — however the body +//! ended. Where either side is missing, what the terminal drew past the break +//! comes back in no segment at all: +//! +//! ```text +//! rendered_lines("vllm\u{1b}]0;ti\ntle") == ["vllm"] +//! rendered_lines("vllm\u{1b}]0;ti\ntle\u{7}") == ["vllm"] +//! rendered_lines("vllm\u{1b}]0;ti\ntle\u{7}\nx") == ["vllm", "x"] +//! ``` +//! +//! The last two bodies are properly `BEL`-terminated, and the last is not near +//! the end of the input either; the `tle` is lost all the same. +//! +//! That is a loss, not a misattribution, so it belongs with the splitting +//! direction where nothing is promised: it costs a diagnosis the tool would +//! otherwise have made, and the canonical-symptom fallback covers it. +//! +//! Common terminals abort a control string on an embedded C0 byte and would +//! draw both sides of the break, so this is a real divergence rather than a +//! technicality. Nor does it take crafted input to reach, which an earlier +//! wording of this section claimed on the strength of a terminator being +//! required. What it takes is an *interleaved* capture: `rocm serve` hands the +//! subprocess's stdout and stderr the *same* file handle, so a title written on +//! one stream can be cut by the other stream's next line landing between the +//! introducer and the terminator, with neither writer doing anything unusual. +//! A process killed part-way through the same title is the other ordinary half +//! of it, leaving a body with no terminator at all — and killed output is the +//! case this walk is built for. +//! +//! Such a capture reaches [`rendered_lines`] whole when a user pastes it into +//! `rocm diagnose --symptom`. The vLLM engine's own path cannot carry an +//! embedded `\n` there: it pre-splits its log tail with `str::lines` and builds +//! its candidate symptom from a single line of it. +//! +//! Following the grammar is still the right call. The alternative is guessing +//! where an unterminated body ends, and the guess would have to be made on +//! exactly the truncated, interleaved output that is the normal case here. Guess +//! short and an `OSC` body is emitted as text — the leak this walk exists to +//! stop, and a second, quieter source of the very misattribution the line +//! boundaries are for, since a window title would then be scored as if the +//! process had printed it. The residual is therefore disclosed here rather than +//! papered over: a break inside a string body is the one way two rendered rows +//! come back as one segment, and the one way a rendered row comes back in none. +//! +//! This section states the rule normatively; the comments on the code that +//! creates it and on the test that pins it are local commentary, not the +//! statement of record. A change to the behaviour has to land here, and any +//! comment that has drifted from this section has to be brought back into line. + +/// What one step of the ECMA-48 walk found. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Token { + /// A character the terminal draws at the cursor. + Text(char), + /// Something that can move the cursor off the current row, so text either + /// side of it was rendered on different lines. + LineBreak, + /// Something that draws nothing and cannot move the cursor, so it neither + /// contributes text nor breaks a line. + Ignorable, +} + +/// Whether `c` is a character that carries no glyph of its own but changes how +/// the text around it renders. +/// +/// `char::is_control` is Unicode category `Cc` only, so it misses the `Cf` +/// format characters — and `U+202E RIGHT-TO-LEFT OVERRIDE` in a log line +/// reverses how the rest of the printed `rocm diagnose --symptom '...'` command +/// renders in the user's terminal. That cannot escape the single quotes (no +/// ASCII `'` is involved), so it is display spoofing rather than shell +/// injection, but it is the same "untrusted subprocess output must not control +/// what the terminal shows" concern the escape stripping exists for, and the +/// answer has to be the same. +/// +/// The ranges are the `Cf` category, enumerated rather than pulled from a +/// Unicode-tables dependency: the set is small, stable, and a new dependency for +/// one predicate is a worse trade. Over-inclusion is safe here — the only cost +/// of rejecting a character is falling back to the canonical symptom, which is +/// guaranteed to report a cause. +#[must_use] +pub fn is_control_or_format(c: char) -> bool { + c.is_control() + || matches!(c, + '\u{00ad}' + | '\u{0600}'..='\u{0605}' + | '\u{061c}' + | '\u{06dd}' + | '\u{070f}' + | '\u{0890}'..='\u{0891}' + | '\u{08e2}' + | '\u{180e}' + | '\u{200b}'..='\u{200f}' + | '\u{202a}'..='\u{202e}' + | '\u{2060}'..='\u{2064}' + | '\u{2066}'..='\u{206f}' + | '\u{feff}' + | '\u{fff9}'..='\u{fffb}' + | '\u{110bd}' + | '\u{110cd}' + | '\u{13430}'..='\u{1343f}' + | '\u{1bca0}'..='\u{1bca3}' + | '\u{1d173}'..='\u{1d17a}' + | '\u{e0001}' + | '\u{e0020}'..='\u{e007f}') +} + +/// Whether `c` is a `Cc` control or one of the two Unicode separators that +/// mandate a line break. +/// +/// `U+2028 LINE SEPARATOR` and `U+2029 PARAGRAPH SEPARATOR` are `Zl`/`Zp`, so +/// neither `char::is_control` nor [`is_control_or_format`]'s enumerated `Cf` +/// set covers them, yet a terminal draws the text after either on the next row. +/// The two places that must not let that happen — this module's classifier and +/// the vLLM engine's guard on what may be quoted into the `rocm diagnose +/// --symptom '...'` command it prints — therefore share this predicate instead +/// of each deciding for itself what breaks a line. The classifier was widened +/// to the pair when this walk moved out of the vLLM engine (the commit that +/// moved it says "behaviour is unchanged", which is true of everything except +/// this); the guard was not, and a line separator went on riding into the +/// printed command. +#[must_use] +pub fn is_control_or_line_separator(c: char) -> bool { + c.is_control() || matches!(c, '\u{2028}' | '\u{2029}') +} + +/// Consumes one glyph, control character or escape sequence and says which of +/// the three it was. +/// +/// The sequence grammar is ECMA-48's, followed exactly, and the reason is that +/// the input is a *killed* process's output: truncated and interleaved escapes +/// are the normal case here, not an exotic one. A scan that just ran to the +/// next byte in `0x40..=0x7E` mis-handled every one of them — it swallowed the +/// following sequence's introducer (`ESC [ 1 ; 2 ESC [ 0 m Killed` lost +/// `Killed`'s first six characters), ran straight past a multi-byte scalar +/// (which can never be in that ASCII range) and ate everything up to the next +/// byte that happened to land in it, emitted the body of an OSC title +/// sequence as text, and left a stray `ESC ESC` unrecoverable. +/// +/// Consuming only what the grammar allows and then *stopping without consuming* +/// the offending byte bounds the damage to the malformed sequence itself: the +/// text after it survives, and a following well-formed sequence is still +/// recognised because its `ESC` is left for the main loop to re-read. +/// +/// What this deliberately does not do is second-guess a well-formed sequence. +/// `ESC [ SP K` is a valid CSI (`SP` is an intermediate, `K` the final byte), so +/// it is consumed whole even though the `K` may have been the first letter of a +/// truncated process's "Killed": a well-formed sequence is consumed whole +/// because a real terminal consumes it too. That rule settles *how much* of the +/// byte stream one step eats, and only that — which [`Token`] the consumed +/// sequence is then labelled with is a separate question, answered below, and +/// answered deliberately unlike a terminal. +/// +/// # Why [`Token::LineBreak`] is the default for anything unrecognised +/// +/// The classification is deliberately lopsided, because the two ways of being +/// wrong do not cost the same. Calling a real boundary [`Token::Ignorable`] +/// merges two rendered lines into one, which is what lets a `llama.cpp` OOM be +/// scored against a `vllm` anchor from a different line — a confident, wrong +/// answer. Calling a non-boundary a [`Token::LineBreak`] splits one rendered +/// line in two, which at worst loses a diagnosis the tool would otherwise have +/// made — a silent miss, and one the canonical-symptom fallback already covers. +/// +/// So only two classes are [`Token::Ignorable`], and only because neither can +/// move the cursor at all: +/// +/// * `SGR` (`CSI ... m`) — colour and attributes, and the overwhelmingly common +/// thing to find *inside* a real log line. This is the one exception that has +/// to be right: treating it as a boundary would split +/// `ESC[31m vllm: ESC[1m torch.OutOfMemoryError` between its anchor and its +/// error token and lose a genuine OOM. +/// * The string-argument sequences (`OSC`, `DCS`, `SOS`, `PM`, `APC`) — their +/// bodies are arbitrary text, like a window title, that is never drawn at the +/// cursor. Consuming such a body whole is the one exception to the never-merge +/// property, stated in full in the module documentation because +/// [`rendered_lines`]'s public callers are the ones it binds. +/// +/// Everything else with an `ESC` in it — `ESC E` (`NEL`), `ESC D` (`IND`), +/// `CSI n B` (`CUD`), and equally the sequences that do *not* move the cursor, +/// such as a charset designator — is a boundary. Enumerating the cursor-moving +/// finals exactly would be a terminal emulator, and a terminal emulator is not +/// decidable on a byte stream with no screen model: `CSI H` addresses a row that +/// depends on what came before, and overwriting text with `\r` makes "which line +/// was this on" a question about the whole capture. Over-splitting needs none of +/// that and cannot produce the misattribution. +fn next_token(chars: &mut std::iter::Peekable>) -> Option { + let c = chars.next()?; + if c != '\u{1b}' { + return Some(classify_char(c)); + } + match chars.peek() { + // CSI — what a colourised logger emits. + Some('[') => { + chars.next(); + Some(if skip_csi_body(chars) { + Token::Ignorable + } else { + Token::LineBreak + }) + } + // The string-argument sequences: OSC, DCS, SOS, PM, APC. Their bodies + // are arbitrary text (a window title, say) and must not be emitted as if + // the process had printed it, but they draw nothing at the cursor. + // + // Consuming the body whole is also the one hole in the never-merge + // property, since a line break inside the body goes with it — and the + // scan below ends on a bare `ESC` or on end of input as readily as on a + // terminator, so a terminator is not what creates the hole. + // The module documentation is where that exception is stated, and it is + // stated there rather than here because a body comment in a private + // function reaches no reader of the public docs. Any change to what this + // arm consumes has to change that section, `rendered_lines`'s own + // qualifier, and the test that pins the worked examples, in one commit. + Some(']' | 'P' | 'X' | '^' | '_') => { + chars.next(); + skip_string_sequence_body(chars); + Some(Token::Ignorable) + } + // `ESC` with nothing after it, or `ESC ESC`: drop just this one and let + // the loop re-read the next as a fresh introducer. A bare `ESC` draws + // nothing and moves nothing on its own. + None | Some('\u{1b}') => Some(Token::Ignorable), + // Any other escape: optional intermediates, then one final byte. This is + // where `ESC E` (NEL) and `ESC D` (IND) live. + Some(_) => { + skip_simple_escape_body(chars); + Some(Token::LineBreak) + } + } +} + +/// Classifies a character that is not an escape introducer. +/// +/// `\t` is [`Token::Text`] rather than a boundary: it moves the cursor along the +/// row it is already on, so it is legitimate intra-line whitespace. Everything +/// else [`is_control_or_line_separator`] accepts is a boundary — `\n` and `\r` +/// obviously, but equally `\x0b` (`VT`) and `\x0c` (`FF`), which advance a line, +/// `\u{85}` (`NEL`), and the bytes that do nothing at all. The last group is the +/// lopsidedness above: a stray `\x01` between two rendered lines is far likelier +/// to be a mangled line advance than intra-line text, and guessing "boundary" +/// costs a missed diagnosis where guessing "text" costs a wrong one. +fn classify_char(c: char) -> Token { + if c == '\t' { + return Token::Text(c); + } + if is_control_or_line_separator(c) { + return Token::LineBreak; + } + if is_control_or_format(c) { + return Token::Ignorable; + } + Token::Text(c) +} + +/// Consumes a CSI body — parameter bytes, then intermediate bytes, then one +/// final byte — from just after the `ESC [`. Stops without consuming anything +/// that does not belong, leaving it to be treated as text. +/// +/// Returns whether the sequence was a well-formed `SGR` (final byte `m`), the +/// only CSI that cannot move the cursor. A CSI truncated before its final byte +/// returns `false`: an unterminated sequence is exactly the mangled output the +/// boundary question matters for, so it takes the conservative branch. +fn skip_csi_body(chars: &mut std::iter::Peekable>) -> bool { + while chars + .next_if(|c| ('\u{30}'..='\u{3f}').contains(c)) + .is_some() + {} + while chars + .next_if(|c| ('\u{20}'..='\u{2f}').contains(c)) + .is_some() + {} + chars.next_if(|c| ('\u{40}'..='\u{7e}').contains(c)) == Some('m') +} + +/// Consumes a string-argument sequence's body and terminator (`BEL`, or `ST` = +/// `ESC \`) from just after the introducer. A body truncated by anything else — +/// including a bare `ESC` starting the next sequence — ends the scan with that +/// byte left in place. +fn skip_string_sequence_body(chars: &mut std::iter::Peekable>) { + while let Some(&c) = chars.peek() { + if c == '\u{7}' { + chars.next(); + return; + } + if c == '\u{1b}' { + let mut lookahead = chars.clone(); + lookahead.next(); + if lookahead.peek() == Some(&'\\') { + chars.next(); + chars.next(); + } + return; + } + chars.next(); + } +} + +/// Consumes a non-CSI escape's body — optional intermediates, then one final +/// byte — from just after the `ESC`. +fn skip_simple_escape_body(chars: &mut std::iter::Peekable>) { + while chars + .next_if(|c| ('\u{20}'..='\u{2f}').contains(c)) + .is_some() + {} + chars.next_if(|c| ('\u{30}'..='\u{7e}').contains(c)); +} + +/// Removes ANSI escape sequences and any remaining control or format character. +/// +/// A colourised or bell-bearing log line therefore cannot repaint the user's +/// terminal, or reorder how the message renders, from inside rocm-cli's own +/// error message. +/// +/// Line boundaries are dropped rather than preserved: every caller here has +/// already split its input into lines and wants a single-line result. +#[must_use] +pub fn strip_terminal_control_sequences(line: &str) -> String { + let mut out = String::with_capacity(line.len()); + let mut chars = line.chars().peekable(); + while let Some(token) = next_token(&mut chars) { + if let Token::Text(c) = token { + // `\t` is intra-line text to the boundary question but still a + // control byte in a message this crate is about to print, and the + // callers' contract is that nothing control-ish survives. + if !is_control_or_format(c) { + out.push(c); + } + } + } + out +} + +/// Approximates the lines a terminal would have rendered `text` as, with the +/// escape sequences and non-drawing characters removed from each segment. +/// +/// It is deliberately an *over*-approximation rather than a terminal emulator, +/// and the approximation is one-sided. Nothing at all is promised in the losing +/// direction: one rendered line may come back split in two, and text the +/// terminal drew may come back in no segment at all. +/// +/// In the merging direction the promise holds with exactly one exception. Two +/// pieces of text the terminal drew on different rows share a returned segment +/// only when the break between them sits inside the body of a string-argument +/// sequence (`OSC`, `DCS`, `SOS`, `PM`, `APC`), which is consumed whole. See +/// [the module documentation](self) for the worked examples, for which ordinary +/// captures reach that shape, for why the exception is not worth closing +/// anyway, and for exactly which sequences are treated as boundaries. +/// +/// Empty segments are returned as-is; callers that filter their lines drop them +/// for free. +#[must_use] +pub fn rendered_lines(text: &str) -> Vec { + let mut lines = vec![String::new()]; + let mut chars = text.chars().peekable(); + while let Some(token) = next_token(&mut chars) { + match token { + Token::Text(c) => { + if let Some(last) = lines.last_mut() { + last.push(c); + } + } + Token::LineBreak => lines.push(String::new()), + Token::Ignorable => {} + } + } + lines +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_line_advance_of_any_form_is_a_boundary_and_sgr_is_not() { + // The table is the reviewer's reproduction on PR #251, generalised: the + // fix that landed before it split on `['\n', '\r']`, an allowlist of two + // characters behind a doc comment claiming the boundary was "the one the + // terminal actually renders". Every other way a terminal is told to + // start a new line collapsed the paste back into one line. + // + // `sgr` is the control in the other direction, and the reason this is a + // grammar walk rather than the one-line `char::is_control` split that + // was suggested: splitting at `ESC` fixes the rows below but cuts a + // genuine colourised log line between its anchor and its error token. + let cases: &[(&str, &str, &[&str])] = &[ + ("lf", "a\nb", &["a", "b"]), + ("cr", "a\rb", &["a", "b"]), + ("crlf", "a\r\nb", &["a", "", "b"]), + ("nel esc-e", "a\u{1b}Eb", &["a", "b"]), + ("ind esc-d", "a\u{1b}Db", &["a", "b"]), + ("ri esc-m", "a\u{1b}Mb", &["a", "b"]), + ("cud csi-1-b", "a\u{1b}[1Bb", &["a", "b"]), + ("cuu csi-a", "a\u{1b}[Ab", &["a", "b"]), + ("cup csi-h", "a\u{1b}[2;3Hb", &["a", "b"]), + ("el csi-k", "a\u{1b}[Kb", &["a", "b"]), + ("vt", "a\u{b}b", &["a", "b"]), + ("ff", "a\u{c}b", &["a", "b"]), + ("c1 nel", "a\u{85}b", &["a", "b"]), + ("u2028 line sep", "a\u{2028}b", &["a", "b"]), + ("u2029 para sep", "a\u{2029}b", &["a", "b"]), + ("stray c0", "a\u{1}b", &["a", "b"]), + ("bel", "a\u{7}b", &["a", "b"]), + // `ESC [ 1 ; 2 b` is *complete*, not truncated -- `b` is a valid CSI + // final byte (`REP`), so the grammar consumes it and it is not text. + // A genuinely truncated CSI is one the next `ESC` interrupts; it + // must not swallow that introducer, so the SGR after it still reads + // as an SGR and only the truncated sequence is a boundary. + ("csi with a letter final", "a\u{1b}[1;2b", &["a", ""]), + ("truncated csi", "a\u{1b}[1;2\u{1b}[0mb", &["a", "b"]), + // Not boundaries. + ("sgr", "a\u{1b}[31mb", &["ab"]), + ("sgr reset", "a\u{1b}[0mb", &["ab"]), + ("sgr multi param", "a\u{1b}[1;38;5;9mb", &["ab"]), + ("osc title", "a\u{1b}]0;title\u{7}b", &["ab"]), + ("dcs", "a\u{1b}Pq\u{1b}\\b", &["ab"]), + ("tab is intra-line", "a\tb", &["a\tb"]), + ("zero-width format char", "a\u{200b}b", &["ab"]), + ("bidi override", "a\u{202e}b", &["ab"]), + ("bare esc", "a\u{1b}", &["a"]), + ]; + for (label, input, want) in cases { + let got = rendered_lines(input); + assert_eq!( + &got.iter().map(String::as_str).collect::>(), + want, + "{label}" + ); + } + } + + #[test] + fn a_lone_non_drawing_scalar_never_merges_two_rendered_lines() { + // The property the over-approximation actually promises, over every + // non-drawing scalar that could sit between two lines of a capture: the + // whole C0 range, DEL, the whole C1 range, and the Unicode separators + // and format characters. The printable range is excluded because those + // are text by definition and merging is what they are for. + // + // The expectation is derived from `char::is_control` (std) and the + // enumerated `Cf` set rather than from this module's own classifier, so + // a change to `classify_char` cannot move the goalposts with it. + for c in (0u32..=0x1f) + .chain(0x7f..=0x9f) + .filter_map(char::from_u32) + .chain(['\u{2028}', '\u{2029}', '\u{200b}', '\u{feff}', '\u{202e}']) + { + let joined = format!("left{c}right"); + let lines = rendered_lines(&joined); + let merged = lines + .iter() + .any(|l| l.contains("left") && l.contains("right")); + // Exactly the characters a terminal draws on the same row may merge: + // tab, and the non-drawing format characters that move no cursor. + let may_merge = c == '\t' || (!c.is_control() && is_control_or_format(c)); + assert_eq!(merged, may_merge, "U+{:04X}", c as u32); + } + } + + #[test] + fn a_line_break_inside_a_string_sequence_body_is_consumed_with_it() { + // The one exception to the never-merge property, and the worked examples + // the module documentation prints. They are pinned here so the prose and + // the behaviour move together: if either expectation ever changes, the + // module section and `rendered_lines`'s own qualifier are wrong and + // have to change in the same commit. The sweep above cannot catch this + // -- it feeds one scalar at a time, and this shape needs a + // string-sequence introducer before the break. + assert_eq!( + rendered_lines("vllm\u{1b}]0;ti\ntle\u{7}llama.cpp OOM"), + ["vllmllama.cpp OOM"], + ); + // The same break outside a string body still splits, so the exception is + // the sequence's framing and not the newline losing its meaning. + assert_eq!( + rendered_lines("vllm\u{1b}]0;title\u{7}\nllama.cpp OOM"), + ["vllm", "llama.cpp OOM"], + ); + // And an *unterminated* body is the same merge over again, not a + // separate containment case: there is no `BEL` and no `ST` anywhere + // below, and the break is still swallowed, so a terminator is not what + // creates the exception. What the following `ESC` bounds is only how far + // the body runs -- it is left in place -- so the hole does not widen to + // swallow the rest of a capture. + assert_eq!( + rendered_lines("vllm\u{1b}]0;ti\ntle\u{1b}[0mllama.cpp OOM"), + ["vllmllama.cpp OOM"], + ); + // Swallowing the break merges only where drawable text follows the + // sequence in the same segment. Where none does, the `tle` a terminal + // would have drawn on its second row comes back in no segment at all -- + // and how the body ended is not what decides that. The second case below + // is properly `BEL`-terminated and the third is terminated with more of + // the capture still to come, only behind a break; both lose the row just + // as the unterminated first one does. That is the losing direction, not + // the misattributing one, and it is why the module section documents a + // third behaviour rather than two. Pinned because prose has three times + // now named a narrower trigger than the code has. + assert_eq!(rendered_lines("vllm\u{1b}]0;ti\ntle"), ["vllm"]); + assert_eq!(rendered_lines("vllm\u{1b}]0;ti\ntle\u{7}"), ["vllm"]); + assert_eq!( + rendered_lines("vllm\u{1b}]0;ti\ntle\u{7}\nx"), + ["vllm", "x"] + ); + } +} diff --git a/docs/vllm.md b/docs/vllm.md index 63d892703..d3073a432 100644 --- a/docs/vllm.md +++ b/docs/vllm.md @@ -216,6 +216,44 @@ Earlier releases pinned this to `0.80` to leave display/WSL headroom. That pin i gone, so an unchanged command now reserves vLLM's own (higher) default. Pass `--gpu-memory-utilization 0.8` to restore the previous reservation. +### Shared or busy GPUs + +Because the reservation is a fraction of **total** VRAM, it ignores memory +already held by other workloads. On a shared multi-GPU node the default can +collide with in-use memory and the engine fails with `HIP out of memory` even for +a tiny model. rocm-cli helps in three ways: + +- **Auto-selection avoids busy cards.** `--gpu auto` ranks GPUs by free VRAM and + skips heavily-used ones. When `amd-smi` is not installed it falls back to the + amdgpu DRM sysfs counters + (`/sys/class/drm/card*/device/mem_info_vram_{total,used}`), so selection still + works on stripped-down container images with a single GPU. That fallback + withholds telemetry on a multi-GPU host, since its `card` numbering is not + guaranteed to match HIP's device ordinal there. +- **The serve summary warns on low free VRAM.** When the selected GPU is already + heavily used, `rocm serve` prints a note — before launch on the plain path, or + in the post-readiness summary in the default interactive mode — and, for vLLM, + points at `--gpu-memory-utilization` as the fix. +- **OOM failures hint the workaround.** When a startup failure log shows an + out-of-memory error, the failure message suggests retrying with a smaller + reservation, e.g. `--gpu-memory-utilization 0.5`, or targeting a less-busy GPU + with `--gpu `, and points at `rocm diagnose --symptom ''` + for the full conditional remediation (busy GPU vs. a model that does not fit). + The printed command quotes your actual failing line when it can be rendered as + one intact single-quoted argument; a line carrying an apostrophe or terminal + control bytes falls back to the canonical symptom below rather than handing you + a command whose quoting the log text broke. + +Explicitly, the workaround for an OOM on a shared card is: + +```bash +rocm serve --engine vllm --gpu-memory-utilization 0.5 +# optionally target a specific, less-busy GPU by index +rocm serve --engine vllm --gpu 1 --gpu-memory-utilization 0.5 +# for the full busy-GPU-vs-model-too-large breakdown, pass the error to diagnose +rocm diagnose --symptom 'vllm: torch.OutOfMemoryError: HIP out of memory' +``` + ## Tool calling The TUI chat tab attaches tool definitions to every chat request. vLLM rejects diff --git a/doctor-catalog.json b/doctor-catalog.json index d37960451..2640693c7 100644 --- a/doctor-catalog.json +++ b/doctor-catalog.json @@ -240,6 +240,23 @@ "needs_relogin": false, "doc_url": "https://aka.ms/vs/17/release/vc_redist.x64.exe" }, + { + "id": "fix-16-vllm-oom", + "title": "vLLM ran the GPU out of memory at startup", + "platforms": [ + { + "os": "linux", + "class": "print-only" + }, + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, { "id": "fix-17-torch-dlpack", "title": "Restore the engine's pinned torch (torch-c-dlpack-ext loads the CUDA variant)", diff --git a/engines/vllm/src/process.rs b/engines/vllm/src/process.rs index fef713a6c..ba1938a05 100644 --- a/engines/vllm/src/process.rs +++ b/engines/vllm/src/process.rs @@ -3,6 +3,9 @@ // SPDX-License-Identifier: MIT use anyhow::{Context, Result, anyhow, bail}; +use rocm_core::terminal::{ + is_control_or_format, is_control_or_line_separator, strip_terminal_control_sequences, +}; use rocm_core::{AppPaths, DEFAULT_LOCAL_PORT, require_nonempty}; use rocm_engine_protocol::{ DevicePolicy, ENGINE_RECIPE_CONTRACT_VERSION, EngineRecipeHint, GpuSelection, LaunchRequest, @@ -713,16 +716,119 @@ fn prepend_path_entries(entries: &[PathBuf], current: Option) -> Resul } /// Builds a human-readable summary of the tail of the startup log, if available. /// Returns an empty string when no log is present or it cannot be read. +/// +/// The tail is sanitized before it is embedded. It is untrusted subprocess +/// output and this whole string is `bail!`-ed to the user's terminal, so +/// stripping control bytes out of the one echoed line further down while +/// printing the raw tail a few lines above would have left the escapes a clear +/// path to the terminal inside the same message. Stripping is per line so the +/// tail keeps its shape; `oom_utilization_hint` still reads the *raw* tail, +/// because what it selects from it is quoted into a command and has its own, +/// stricter admissibility test. fn startup_log_context(log_path: Option<&Path>) -> String { let summary = log_path .and_then(|p| summarize_startup_log_tail(p, STARTUP_FAILURE_LOG_TAIL_LINES).ok()) .unwrap_or_default(); if summary.is_empty() { - String::new() - } else { - format!("\n\nLast {STARTUP_FAILURE_LOG_TAIL_LINES} lines of startup log:\n{summary}") + return String::new(); } + let hint = oom_utilization_hint(&summary); + let summary = summary + .lines() + .map(strip_terminal_control_sequences) + .collect::>() + .join("\n"); + format!("\n\nLast {STARTUP_FAILURE_LOG_TAIL_LINES} lines of startup log:\n{summary}{hint}") +} + +/// Appends [`rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT`] when the startup-log +/// tail shows an out-of-memory failure, and nothing otherwise so healthy +/// failures are not cluttered with an irrelevant memory hint. Shares its wording +/// with the `rocm` CLI's pre-launch low-VRAM note so both surfaces point the +/// user at the same fix rather than drifting into different phrasing. +fn oom_utilization_hint(log_tail: &str) -> String { + if !log_tail_shows_oom(log_tail) { + return String::new(); + } + // Route the user's *actual* failing line into the `--symptom` example when + // the diagnose checker would actually score it *and* it can be rendered as + // one intact single-quoted argument; otherwise fall back to the canonical + // symptom so the printed command always reports a cause. The detector here + // is a coarse substring scan that accepts lines the scorer rates + // sub-threshold (e.g. a bare "... out of memory"), so without this fallback + // the diagnose command could report nothing -- which reads as "the tool + // checked and there's no known cause", worse than not printing it. + // `.rev()` is deliberate, not incidental: when several lines of the tail + // mention running out of memory, the *last* one is the proximate failure — + // the point where the process actually gave up — and the earlier ones are + // usually the allocator's own retry chatter leading up to it. The cost is + // real and worth stating: on a tail like "torch.OutOfMemoryError: ... Tried + // to allocate 7.21 GiB. GPU 0 has a total capacity of 24.00 GiB." followed + // by a bare "RuntimeError: ... killed: out of memory", the vaguer line wins + // even though the first carries the allocation size and the capacity. Both + // are diagnosable, so this picks which line is quoted, never whether one is. + // The user still sees every line: the log tail is printed above this hint. + let raw_line = log_tail + .lines() + .rev() + .map(str::trim) + .find(|line| !line.is_empty() && log_tail_shows_oom(line)) + .unwrap_or("out of memory"); + // The line is subprocess output, so it is echoed only after the terminal + // control bytes vLLM's colourised logger emits are removed. + let symptom_line = strip_terminal_control_sequences(raw_line); + let candidate = format!("vllm: {raw_line}"); + let symptom = if quotable_in_single_quotes(&candidate) + && rocm_core::vllm_oom_symptom_is_diagnosable(&candidate) + { + candidate + } else { + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM.to_owned() + }; + format!( + "\n\nDetected an out-of-memory failure ({symptom_line}). {}\n\ + For conditional remediation, run `rocm diagnose --symptom '{symptom}'`.", + rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT + ) +} + +/// Whether `symptom` can be placed inside a `'...'` shell word verbatim. +/// +/// The value is untrusted subprocess output (the vLLM startup log tail) and the +/// message it lands in invites the user to paste the command into a shell, so a +/// bare `'` would close the quote and let text nobody vetted become shell +/// syntax. An apostrophe is routine in Python error text (`can't allocate`, +/// `model 'foo'`), and a line only has to mention running out of memory to be +/// selected, so this is an ordinary case rather than an exotic one. +/// +/// Rejecting instead of escaping (`'` -> `'\''`) is deliberate. Escaping keeps +/// the exact bytes but yields a command a reader cannot check by eye, and a +/// wrong escape is *runnable* and misleading rather than obviously broken; +/// control bytes would still reach the terminal. The canonical fallback is the +/// branch that already exists for "this line cannot be used", and it is +/// guaranteed to report a cause. The user's own line stays visible in the +/// human-readable sentence above the command (and in the log tail printed with +/// it), so nothing is lost but the copy-paste convenience. +/// +/// The character test is [`is_control_or_format`] together with +/// [`is_control_or_line_separator`], because each covers scalars the other does +/// not and `symptom` is the *raw* log line, not the stripped one: a `Cf` bidi +/// override reordered how the printed command renders, and a `Zl` line +/// separator broke it across two rows. +fn quotable_in_single_quotes(symptom: &str) -> bool { + !symptom.contains('\'') + && !symptom + .chars() + .any(|c| is_control_or_format(c) || is_control_or_line_separator(c)) } + +/// Case-insensitive scan for the out-of-memory signatures vLLM/PyTorch emit on a +/// HIP allocation failure (e.g. `torch.OutOfMemoryError: HIP out of memory`). +fn log_tail_shows_oom(log_tail: &str) -> bool { + let lower = log_tail.to_ascii_lowercase(); + lower.contains("out of memory") || lower.contains("outofmemory") +} + /// Polls the vLLM endpoint until it reports the model is loaded, or times out. /// Uses a monotonic clock (`Instant`) so wall-clock adjustments cannot corrupt the /// timeout, and surfaces an early process exit immediately instead of waiting out @@ -1001,6 +1107,363 @@ mod tests { ); Ok(()) } + #[test] + fn oom_utilization_hint_fires_on_out_of_memory_log_tails() { + // The exact PyTorch/HIP signature from the field report. + let torch = "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB."; + assert!(log_tail_shows_oom(torch)); + let hint = oom_utilization_hint(torch); + assert!( + hint.contains("--gpu-memory-utilization"), + "an OOM tail must surface the utilization workaround: {hint}" + ); + assert!(hint.contains("--gpu ")); + assert!( + hint.contains("rocm diagnose --symptom"), + "an OOM tail must route the user to the conditional catalog entry: {hint}" + ); + assert!( + hint.contains( + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB." + ), + "the user's real failing line must be routed into --symptom, not a canned string: {hint}" + ); + + // Detection is case-insensitive and also matches the spaced phrasing. + assert!(log_tail_shows_oom("HIP OUT OF MEMORY")); + assert!(log_tail_shows_oom("RuntimeError: CUDA out of memory")); + } + + #[test] + fn every_emitted_oom_symptom_is_diagnosable_and_names_the_branch_that_produced_it() { + // Closes the loop between the two layers: the engine prints + // `rocm diagnose --symptom ''`, so whatever it emits must + // actually score for the diagnose checker -- including for lines the + // coarse substring detector accepts but the scorer rates sub-threshold + // on their own (those fall back to the canonical symptom). + // + // Each case pins the *exact* symptom, not just that one is diagnosable. + // Diagnosability alone cannot fail for the defect this test is named + // for: the canonical symptom is diagnosable by construction, so a + // mutant that deletes the "route the user's real failing line" feature + // entirely and always returns VLLM_OOM_CANONICAL_SYMPTOM satisfies + // every such assertion. Naming the expected symptom per line is what + // makes the routing branch and the fallback branch separately + // falsifiable -- and it is why the table below deliberately mixes the + // two: three lines score on their own and must be quoted verbatim, + // three do not and must fall back. + let accepted_lines = [ + ( + "torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.", + ), + ( + "RuntimeError: hipErrorOutOfMemory", + "vllm: RuntimeError: hipErrorOutOfMemory", + ), + ("CUDA out of memory", "vllm: CUDA out of memory"), + // Sub-threshold on their own: the class name with no allocator + // message, the allocator message with no class name, and a bare + // kill notice. These must reach the canonical fallback. + ( + "torch.cuda.OutOfMemoryError", + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, + ), + ( + "HIP error: out of memory", + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, + ), + ( + "the process was killed: out of memory", + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, + ), + ]; + let mut routed_verbatim = 0; + for (line, expected) in accepted_lines { + assert!(log_tail_shows_oom(line), "detector must accept: {line}"); + let hint = oom_utilization_hint(line); + let symptom = quoted_symptom_argument(&hint); + assert_eq!( + symptom, expected, + "wrong branch for {line:?}: the engine must quote the user's own line when \ + the checker scores it and fall back to the canonical symptom only when it \ + does not" + ); + assert!( + rocm_core::vllm_oom_symptom_is_diagnosable(symptom), + "emitted symptom must be diagnosable, got {symptom:?} for line {line:?}" + ); + if symptom != rocm_core::VLLM_OOM_CANONICAL_SYMPTOM { + routed_verbatim += 1; + } + } + assert_eq!( + routed_verbatim, 3, + "the table must keep exercising both branches; if a scoring change moved a line \ + across the threshold, re-pick the fixture rather than relaxing the expectation" + ); + } + + /// The `--symptom` value the hint actually hands the user, read back out of + /// the rendered text exactly the way a shell would: everything between the + /// opening quote that follows `--symptom ` and the next `'`. + fn quoted_symptom_argument(hint: &str) -> &str { + hint.split("--symptom '") + .nth(1) + .and_then(|rest| rest.split('\'').next()) + .expect("hint must carry a --symptom value") + } + + #[test] + fn an_apostrophe_in_the_failing_line_cannot_break_out_of_the_printed_command() { + // A realistic vLLM/PyTorch failing line: apostrophes are routine in + // Python error text, and this one scores well above MIN_SCORE_FOR_MATCH + // (torch.OutOfMemoryError + HIP out of memory), so the "route the user's + // real line" branch selects it. + let log_tail = concat!( + "ERROR 09-14 12:00:01 engine.py:389] torch.OutOfMemoryError: HIP out of memory. ", + "Tried to allocate 7.21 GiB. GPU 0 can't allocate the model's weights." + ); + assert!(log_tail_shows_oom(log_tail)); + let hint = oom_utilization_hint(log_tail); + + // The rendered command must be one intact single-quoted argument: no + // user-controlled byte may close the quote, so nothing the subprocess + // printed can land outside it in a command the user is told to paste. + let command = hint + .split("run `") + .nth(1) + .and_then(|rest| rest.split('`').next()) + .expect("hint must print a runnable command"); + assert_eq!( + command.matches('\'').count(), + 2, + "the --symptom argument must stay a single balanced quoted word: {command}" + ); + let symptom = quoted_symptom_argument(&hint); + // Pin which branch ran, not just that no apostrophe survived: "contains + // no `'`" also holds if the apostrophes were silently deleted from the + // user's line, which is the escaping-style behaviour the fallback exists + // to avoid. Rejection means the canonical symptom, exactly. + assert_eq!( + symptom, + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM, + "a quote-bearing line must be rejected in favour of the canonical symptom, \ + not silently rewritten: {symptom:?}" + ); + // ...and the command it does print must still report a cause. + assert!( + rocm_core::vllm_oom_symptom_is_diagnosable(symptom), + "the fallback symptom must still be diagnosable: {symptom:?}" + ); + } + + #[test] + fn control_bytes_from_the_log_are_stripped_from_the_echoed_line() { + // vLLM's logger colourises; an ANSI-coloured OOM line must not repaint + // the user's terminal from inside rocm-cli's own error message. + let log_tail = "\u{1b}[31mRuntimeError: HIP out of memory\u{1b}[0m\u{7}"; + let hint = oom_utilization_hint(log_tail); + assert!( + !hint.chars().any(|c| c.is_control() && c != '\n'), + "no control byte may survive into the printed hint: {hint:?}" + ); + // Pin the exact rendering rather than the absence of a few fragments. + // Absence checks cannot fail for the defect they name: a stripper that + // drops the escape byte and the `[` it introduces and nothing further + // leaves `31m`/`0m` behind, which contains neither `[31m` nor `[0m` and + // carries no control byte, so every absence check still passes. + assert_eq!( + strip_terminal_control_sequences(log_tail), + "RuntimeError: HIP out of memory", + "the ANSI sequence must be removed whole, not just its escape byte" + ); + // The colourised line is not quotable (control bytes), so the command + // falls back to the canonical symptom while the echoed sentence keeps + // the user's own line, stripped. + assert_eq!( + hint, + format!( + "\n\nDetected an out-of-memory failure (RuntimeError: HIP out of memory). {}\n\ + For conditional remediation, run `rocm diagnose --symptom '{}'`.", + rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT, + rocm_core::VLLM_OOM_CANONICAL_SYMPTOM + ), + "the echoed line must render exactly, with the whole escape sequence gone" + ); + } + + #[test] + fn the_stripper_follows_the_escape_grammar_not_just_the_colour_case() { + // The `'m'`-terminated SGR case above is the *easy* one, and on its own + // it pins almost nothing: narrowing the CSI final-byte range from + // `0x40..=0x7E` to just `'m'` leaves it green. These cases pin the + // range, the parameter/intermediate classes, and the introducers. + // + // They are not academic. This input is a *killed* process's output, so + // truncated and interleaved sequences are the normal case on this code + // path, and every one of them used to corrupt the message the user + // reads -- the failures are quoted per case below. + for (raw, expected, defect) in [ + // Non-`m` CSI finals: `K` (erase-in-line) and `A` (cursor-up) are + // ordinary logger output, and narrowing the final-byte range to + // `'m'` leaves them in the message verbatim. + ( + "\u{1b}[2KRuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a non-`m` CSI final must terminate the sequence", + ), + ( + "\u{1b}[1ARuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a non-`m` CSI final must terminate the sequence", + ), + // Truncated CSI immediately followed by a well-formed one: the old + // scan consumed the second sequence's `ESC [` as the first one's + // parameters and stopped at `0`, yielding "0mKilled: out of memory". + ( + "\u{1b}[1;2\u{1b}[0mKilled: out of memory", + "Killed: out of memory", + "a truncated CSI must not swallow the next sequence's introducer", + ), + // A multi-byte scalar can never be in `0x40..=0x7E`, so the old scan + // ran past it and ate to the next byte that happened to land in + // range -- this yielded "ut of memory", losing the `o`. + ( + "\u{1b}[12\u{e9} out of memory", + "\u{e9} out of memory", + "an invalid CSI byte must end the sequence, not be scanned past", + ), + // `ESC ESC`: the old code dropped the second `ESC` as the first + // one's argument, then emitted `[0m` as literal text. + ( + "\u{1b}\u{1b}[0mRuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a stray ESC must not consume the next sequence's introducer", + ), + // OSC: the old code took the `else` branch on `]`, so the window + // title leaked into the message as "0;titleRuntimeError: ...". + ( + "\u{1b}]0;title\u{7}RuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "an OSC body must not be emitted as text", + ), + // ...and with the ST terminator (`ESC \`) rather than BEL. + ( + "\u{1b}]0;title\u{1b}\\RuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "an OSC terminated by ST must be consumed whole", + ), + // A two-character escape with no CSI at all. + ( + "\u{1b}7RuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a simple escape must consume exactly its final byte", + ), + // Cf format characters: `char::is_control` is category Cc only, so + // U+202E survived and reversed how the rest of the line renders. + ( + "RuntimeError: \u{202e}HIP out of memory", + "RuntimeError: HIP out of memory", + "a bidi override must not survive into the message", + ), + // The `Zl`/`Zp` pair that `is_control_or_line_separator` adds to + // `char::is_control`: drop it from that predicate and both of these + // go red, as do the quoting assertions below. + ( + "RuntimeError: HIP\u{2028}out of memory", + "RuntimeError: HIPout of memory", + "a line separator must not survive into the message", + ), + ( + "RuntimeError: HIP\u{2029}out of memory", + "RuntimeError: HIPout of memory", + "a paragraph separator must not survive into the message", + ), + // The text-only control: nothing is removed from a clean line. + ( + "RuntimeError: HIP out of memory", + "RuntimeError: HIP out of memory", + "a clean line must pass through untouched", + ), + ] { + assert_eq!( + strip_terminal_control_sequences(raw), + expected, + "{defect}: {raw:?}" + ); + } + + // Every one of those scalars must also be inadmissible in the quoted + // command, not merely stripped from the echoed sentence: the candidate + // the guard sees is built from the raw line, not the stripped one, so + // the second call site needs its own pins. + assert!( + !quotable_in_single_quotes("vllm: \u{202e}HIP out of memory"), + "a bidi override must make a line unquotable, not ride into the command" + ); + assert!( + !quotable_in_single_quotes("vllm: HIP\u{2028}out of memory"), + "a line separator must make a line unquotable, not ride into the command" + ); + assert!( + !quotable_in_single_quotes("vllm: HIP\u{2029}out of memory"), + "a paragraph separator must make a line unquotable, not ride into the command" + ); + assert!( + quotable_in_single_quotes("vllm: HIP out of memory"), + "the rejection must not be so broad that ordinary lines stop qualifying" + ); + } + + #[test] + fn the_printed_log_tail_is_sanitized_not_just_the_echoed_line() -> Result<()> { + // The stripper's contract is that nothing in this message can repaint + // the user's terminal. The log tail is printed a few lines above the + // echoed line in the same `bail!`-ed string, and it used to go out raw, + // so the escapes removed from one had a clear path to the terminal via + // the other. + let path = std::env::temp_dir().join(format!( + "rocm-vllm-sanitize-{}-{}.log", + std::process::id(), + current_unix_millis() + )); + fs::write( + &path, + "\u{1b}[31mINFO starting\u{1b}[0m\n\u{1b}]0;pwned\u{7}RuntimeError: HIP out of memory\n", + )?; + let context = startup_log_context(Some(path.as_path())); + fs::remove_file(&path).ok(); + + assert!( + !context.chars().any(|c| c.is_control() && c != '\n'), + "no control byte may survive into the printed failure message: {context:?}" + ); + assert!( + !context.contains("pwned"), + "an OSC body in the tail must not be emitted as text: {context:?}" + ); + assert_eq!( + context + .split("startup log:\n") + .nth(1) + .and_then(|rest| rest.split("\n\nDetected").next()), + Some("INFO starting\nRuntimeError: HIP out of memory"), + "the tail must keep its text and its line structure, minus the escapes: {context:?}" + ); + Ok(()) + } + + #[test] + fn oom_utilization_hint_stays_quiet_for_unrelated_failures() { + let unrelated = "ValueError: model architecture 'FooForCausalLM' is not supported"; + assert!(!log_tail_shows_oom(unrelated)); + assert!( + oom_utilization_hint(unrelated).is_empty(), + "non-OOM failures must not carry a memory hint" + ); + } + #[test] fn vllm_serve_args_omit_gpu_memory_utilization_without_override() { let args = vllm_serve_args("facebook/opt-125m", "127.0.0.1", 8000, false, None); diff --git a/skills/rocm-cli-assistant/SKILL.md b/skills/rocm-cli-assistant/SKILL.md index 2396b35b8..0b54a2208 100644 --- a/skills/rocm-cli-assistant/SKILL.md +++ b/skills/rocm-cli-assistant/SKILL.md @@ -27,8 +27,9 @@ Use this skill when answering ROCm CLI local assistant questions. - vLLM and Lemonade are serving engines. - The built-in assistant is fixed to qwen served by Lemonade with GPU required. Do not switch the built-in assistant to vLLM. - Installing an engine and running a model server are different states. Answer each separately when the user asks. -- `rocm serve` accepts `--gpu auto|` to pick the AMD GPU. `auto` (default) prefers a GPU that looks idle from `amd-smi` VRAM telemetry and is not already used by another rocm-cli server (managed or foreground), falling back to the GPU with the most free memory; a single index pins one GPU. Serving one model across multiple GPUs is not supported. Do not suggest CPU fallback when a GPU is busy or out of range. +- `rocm serve` accepts `--gpu auto|` to pick the AMD GPU. `auto` (default) prefers a GPU that looks idle from VRAM telemetry (`amd-smi`, or the amdgpu DRM sysfs counters when `amd-smi` is absent) and is not already used by another rocm-cli server (managed or foreground), falling back to the GPU with the most free memory; a single index pins one GPU. Serving one model across multiple GPUs is not supported. Do not suggest CPU fallback when a GPU is busy or out of range. - Which engines actually run on the user's machine is stated in the host facts the CLI appends to this prompt. Answer from those facts; do not assume a platform. Never suggest CPU fallback. +- On native Windows, vLLM serving/install live checks are skipped; tell the user to use WSL/Linux for that ROCm GPU engine and do not suggest CPU fallback. ## ComfyUI diff --git a/skills/rocm-doctor/SKILL.md b/skills/rocm-doctor/SKILL.md index 4b9ff4225..99c07e1e2 100644 --- a/skills/rocm-doctor/SKILL.md +++ b/skills/rocm-doctor/SKILL.md @@ -145,7 +145,7 @@ passes — the GPU is AMD. Linux, Windows and WSL2 all run the same workflow. rocm fix --yes # required to apply in a non-interactive shell ``` - Only the four auto-applicable fixes are ones the CLI runs itself. The other 21 + Only the four auto-applicable fixes are ones the CLI runs itself. The other 22 are **print-only** (bootloader, kernel, reinstall, Windows driver, …): `rocm fix ` just prints the plan for the user to run themselves — no prompt, and the CLI never performs those. diff --git a/skills/rocm-doctor/reference.md b/skills/rocm-doctor/reference.md index 2ad54f6a2..cd2d59995 100644 --- a/skills/rocm-doctor/reference.md +++ b/skills/rocm-doctor/reference.md @@ -93,7 +93,7 @@ non-interactive shell without `--yes`, and confirm first. **Two exceptions:** pinned. Run `rocm fix fix-9-igpu-dgpu --device-index N` (not the bare id) once you know N. -## Closed catalog (25 failure modes) +## Closed catalog (26 failure modes) The OS column is the platform family the CLI scopes an entry to, and WSL2 is a family of its own — not a flavour of `linux`. An entry reaches a WSL host only @@ -118,6 +118,7 @@ reporting confident nonsense. | `fix-13-hip-sdk-missing` | windows | HIP SDK not installed | no HIP SDK under Program Files, `hipInfo` not recognized | print-only | | `fix-14-adrenalin-too-old` | windows | Adrenalin / kernel-mode driver too old for the HIP SDK | `hipInfo` can't enumerate, "driver too old", HSA "no agents found" | print-only | | `fix-15-msvc-redist` | windows | MSVC runtime missing (HIP DLLs can't load) | `vcruntime140.dll` / `vcruntime140_1.dll` missing | print-only | +| `fix-16-vllm-oom` | linux/wsl | vLLM reserved ~90% of the GPU's TOTAL VRAM for its KV cache and ran out at startup | vLLM engine start fails with an out-of-memory error; the GPU is shared/busy, or the model does not fit | print-only | | `fix-17-torch-dlpack` | linux | `torch-c-dlpack-ext` loads its CUDA prebuilt on a ROCm torch, aborting vLLM's engine start at import time | vLLM engine start fails on import; error names `torch_c_dlpack_ext` or tvm_ffi's `_optional_torch_c_dlpack` | print-only | | `fix-18-comgr-conflict` | linux/wsl | The code object manager library (`libamd_comgr`) that would load belongs to a different installation than the HIP runtime that would load, so device code compilation fails with an error naming neither | compilation error naming neither library; `rocm examine --json`'s `comgr_selected`/`hip_selected` resolve to two different `install_root`s | print-only | | `fix-19-shm-too-small` | linux/wsl | `/dev/shm` too small for a serving workload, which needs gigabytes where a container and WSL2 both default to 64 MB | reported under 1 GiB; a data-loader worker killed by a bus error, or a failed write to a temporary file, with nothing naming shared memory | print-only | @@ -129,14 +130,15 @@ reporting confident nonsense. | `fix-wsl-6-host-driver-too-old` | wsl | Windows host driver too old or absent, with the distro side already complete | the Windows host reports no AMD display adapter | print-only | | `fix-wsl-7-wsl1` | wsl | Distro running under WSL 1, which exposes no GPU device at all | the running kernel is a WSL 1 kernel | print-only | -Two things the numbering does not tell you. `fix-16` is a reserved handle, not a -missing row — ids are stable handles rather than positions. And the `fix-wsl-N` -entries are a parallel series, not a continuation of the numeric one, because -they answer for a different platform family. +One thing the numbering does not tell you: the `fix-wsl-N` entries are a parallel +series, not a continuation of the numeric one, because they answer for a +different platform family. The numeric series itself now runs `fix-1` through +`fix-19` with no gaps, but the ids remain stable handles rather than positions — +a retired entry would leave a hole rather than renumber the ones after it. Linux-only: fix-3, -4, -5, -7, -10, -11, -12, -17. Windows-only: fix-13, -14, -15. WSL-only: fix-wsl-1 through fix-wsl-7. Linux + Windows: fix-9. -Linux + WSL: fix-18, -19. Linux + Windows + WSL: fix-1, -2, -6, -8. +Linux + WSL: fix-16, -18, -19. Linux + Windows + WSL: fix-1, -2, -6, -8. ## Framework routing diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index a09b0d614..86fafc5d0 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -524,3 +524,35 @@ Feature: Diagnosing failures and listing fixes Given a user who has chosen a known fix When the user asks the CLI to apply that fix in machine-readable form Then the CLI refuses and explains that the two cannot be combined + + # vLLM runs on Linux and WSL, but not native Windows. This scenario is + # GPU-independent: it supplies the captured startup error as symptom text and + # proves the public diagnosis output preserves both branches of the remedy. + @id:diagnose-vllm-oom-is-conditional @requires-os:linux + Scenario: diagnose-36 - A vLLM startup OOM receives conditional remediation + Given a user whose vLLM server ran out of GPU memory + When the user asks the CLI to diagnose that symptom in machine-readable form + Then the diagnosis identifies the vLLM startup OOM + And the OOM remedy distinguishes a busy GPU from a model that does not fit + # A report that names `rocm fix ` and a `rocm fix` that then refuses that + # id leaves the user worse off than no fix path at all. The two halves are + # selected by separate platform lists -- the checker's, and the recipe's + # against the RUNNING os, where WSL2 is its own family -- so they can + # disagree while each looks right alone. Every Linux lane runs this step; the + # WSL one is where the two lists can differ. + And the CLI can act on the fix the diagnosis named + + # `--symptom` is where a user pastes a raw terminal capture, and a terminal + # capture is full of line advances that are not `\n`: a progress bar repaints + # with `\r`, and curses- and `rich`-style progress UIs move down with `ESC E`, + # `ESC D` or `CSI n B` (terminfo's `nel`, `ind` and `cud1`). If those are not + # line boundaries the whole paste collapses into a single line, a `vllm` + # mention anywhere in it anchors another engine's OOM, and the user is told + # with high confidence that vLLM ran out of memory -- quoting the other + # engine's error text back as the evidence for it. A confidently wrong cause is + # worse than no cause, so this is pinned at the level the user sees it. + @id:diagnose-vllm-oom-not-attributed-across-rendered-lines @requires-os:linux + Scenario: diagnose-37 - Another engine's OOM is not blamed on a vLLM mention elsewhere in the paste + Given a user who pasted a capture naming vLLM and another engine's OOM on separate rendered lines + When the user asks the CLI to diagnose that symptom in machine-readable form + Then no vLLM startup OOM is reported diff --git a/tests/e2e-cucumber/features/model_serving.feature b/tests/e2e-cucumber/features/model_serving.feature index 0cecc8c25..12bac602f 100644 --- a/tests/e2e-cucumber/features/model_serving.feature +++ b/tests/e2e-cucumber/features/model_serving.feature @@ -160,6 +160,13 @@ Feature: Model serving # fallback). Runs on GPU hardware: on a no-GPU host the GPU-required pre-flight # refuses ("no usable AMD GPU") before the index is ever validated, so the # index-specific rejection can only be observed where a real device is present. + # + # This is the live coverage for an index the host does not have. The *masked* + # sub-case — an index that exists but is hidden by an active visibility mask — + # is a different refusal and owns its own scenarios, serve-19 and serve-20. + # The step here accepts several refusal wordings only because which authority + # answers depends on the lane: the visible set when it could be enumerated, + # the raw amd-smi count when it could not. @id:serve-absent-gpu-index-rejected @requires-gpu @requires-os:linux Scenario: serve-16 - Serving pinned to a GPU that does not exist is refused When the user serves a model pinned to a GPU index that does not exist @@ -268,3 +275,23 @@ Feature: Model serving Given a local server attempt has failed When the user lists running services Then the list reports the attempt and how to look at it + + # Pre-launch OOM guidance (EAI-8058). When the GPU a serve is pinned to is + # nearly out of VRAM, the plan must warn before launch and — for vLLM — name the + # `--gpu-memory-utilization` knob that avoids the OOM, closing the loop with the + # `diagnose`/`fix` catalog. The GPU lane's real cards are comfortably free, so the + # near-full reading is injected via `ROCM_E2E_FORCE_LOW_VRAM` in debug/test builds: + # a single synthetic device on this non-APU host keeps the warning honest, while + # the engine still launches against the real, free device. Runs on the vLLM GPU + # lane; the mock lane has no GPU to pin, and an APU's shared-memory carveout would + # legitimately withhold the warning. `--managed` serves under the GPU-required + # device policy, which refuses before it ever builds a plan unless a ROCm + # runtime is active — hence the same `a managed runtime is active` precondition + # every other real-serve scenario on this lane opens with. + @id:serve-vllm-low-vram-oom-guidance @requires-gpu @requires-engine:vllm @requires-os:linux + Scenario: serve-23 - A vLLM serve plan on a nearly-full GPU points at the memory knob + Given a managed runtime is active + And the selected GPU is reported nearly out of VRAM + When the user previews a vLLM serve plan pinned to that GPU + Then the serve plan warns the GPU is low on VRAM + And the serve plan explains how to lower vLLM's memory reservation diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index 145d1e7be..02f21c899 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -108,11 +108,14 @@ pub struct ScenarioDecl { /// in-tree amdgpu driver, so it does not hold under WSL2 — which uses /// `/dev/dxg` and the Windows host driver instead. /// - /// Two things stop short there. `rocm diagnose` skips its bare-metal catalog - /// *by design* rather than emitting Linux diagnoses that cannot apply, so a - /// scenario needing a catalog match has no premise. And `examine`'s probe - /// returns as soon as it recognises WSL2, before most of its steps run, so a - /// scenario asserting anything those steps populate has none either. + /// Two things stop short there. `rocm diagnose` skips its *bare-metal* + /// catalog *by design* rather than emitting Linux diagnoses that cannot + /// apply, so a scenario needing one of those bare-metal matches has no + /// premise. (Keyword-only checks explicitly registered for WSL — e.g. the + /// vLLM startup-OOM entry — do run there; those belong under + /// `@requires-os:linux`, not this tag.) And `examine`'s probe returns as + /// soon as it recognises WSL2, before most of its steps run, so a scenario + /// asserting anything those steps populate has none either. /// /// This is a SKIP and not an xfail row: the diagnose half is deliberate and /// separately unit-tested, and recording it as a known bug would tell every diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 488d0545b..ab993e467 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -78,9 +78,7 @@ const CATALOG_FIX_IDS: &[&str] = &[ "fix-13-hip-sdk-missing", "fix-14-adrenalin-too-old", "fix-15-msvc-redist", - // Not a typo, and not a hole to fill: `fix-16` is reserved by the vLLM - // out-of-memory entry on its own branch. The number is a stable handle, so - // the two are kept distinct rather than renamed after the fact. + "fix-16-vllm-oom", "fix-17-torch-dlpack", "fix-wsl-1-gpu-not-exposed", "fix-wsl-2-dxcore-missing", @@ -237,6 +235,31 @@ async fn user_hit_unknown_failure(world: &mut E2eWorld) { world.model_name = Some("xyzzy totally unrelated gibberish".to_string()); } +#[given("a user whose vLLM server ran out of GPU memory")] +async fn user_hit_vllm_oom(world: &mut E2eWorld) { + world.model_name = Some( + "vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.".to_string(), + ); +} + +/// The separator is `ESC E` (`NEL`), deliberately, not `\n` or `\r`. Both of +/// those were already split on by the time this scenario was written; `NEL` is +/// what a `rich`/curses progress UI emits to advance a line, and it is what the +/// two-character allowlist did not cover. The anchor (`vllm`) and the OOM are on +/// different rendered lines, so no part of this paste is a vLLM fault. +#[given( + "a user who pasted a capture naming vLLM and another engine's OOM on separate rendered lines" +)] +async fn user_pasted_a_capture_with_a_foreign_oom(world: &mut E2eWorld) { + world.model_name = Some( + "Downloading shards: 10%\u{1b}Evllm serve starting up\u{1b}E\ + Downloading shards: 90%\u{1b}E\ + llama.cpp: torch.OutOfMemoryError: CUDA out of memory. \ + Tried to allocate 7.21 GiB.\u{1b}E" + .to_string(), + ); +} + #[given("a user who hit the vLLM engine-startup import failure")] async fn user_hit_engine_import_failure(world: &mut E2eWorld) { world.model_name = Some(ENGINE_IMPORT_SYMPTOM.to_string()); @@ -618,6 +641,121 @@ async fn assert_json_identifies_match(world: &mut E2eWorld) { ); } +/// Locate the `fix-16-vllm-oom` diagnosis in a parsed report's `matched` array, +/// panicking with the raw output if it is absent. Shared by the two `Then` +/// steps that assert on the vLLM OOM entry. +fn find_vllm_oom<'a>(report: &'a serde_json::Value, output: &str) -> &'a serde_json::Value { + report + .get("matched") + .and_then(serde_json::Value::as_array) + .and_then(|matches| { + matches.iter().find(|diagnosis| { + diagnosis.get("id").and_then(serde_json::Value::as_str) == Some("fix-16-vllm-oom") + }) + }) + .unwrap_or_else(|| panic!("expected fix-16-vllm-oom in diagnosis:\n{output}")) +} + +#[then("no vLLM startup OOM is reported")] +async fn assert_no_vllm_oom_reported(world: &mut E2eWorld) { + assert_eq!( + world.cli_rc, + Some(0), + "diagnose should exit 0 (it is a query)" + ); + let (report, output) = parsed_diagnosis(world); + let matched = report + .get("matched") + .and_then(serde_json::Value::as_array) + .unwrap_or_else(|| panic!("report must carry a `matched` array:\n{output}")); + assert!( + !matched.iter().any(|diagnosis| { + diagnosis.get("id").and_then(serde_json::Value::as_str) == Some("fix-16-vllm-oom") + }), + "another engine's OOM must not be attributed to vLLM:\n{output}" + ); +} + +#[then("the diagnosis identifies the vLLM startup OOM")] +async fn assert_diagnosis_identifies_vllm_oom(world: &mut E2eWorld) { + assert_eq!(world.cli_rc, Some(0), "diagnose should exit 0"); + let (report, output) = parsed_diagnosis(world); + let oom = find_vllm_oom(&report, &output); + let high_confidence = report + .get("high_confidence_threshold") + .and_then(serde_json::Value::as_i64) + .unwrap_or_else(|| panic!("report must publish its high_confidence_threshold:\n{output}")); + assert!( + oom.get("score") + .and_then(serde_json::Value::as_i64) + .is_some_and(|score| score >= high_confidence), + "the distinctive OOM signature should be high-confidence:\n{output}" + ); + assert_eq!( + oom.pointer("/fix/auto_applicable") + .and_then(serde_json::Value::as_bool), + Some(false), + "the OOM remedy must remain print-only:\n{output}" + ); +} + +/// The other half of the loop: the `apply with: rocm fix ` the report prints +/// has to name a command this host will actually act on. +/// +/// `rocm diagnose` selects a checker by the platform families the checker +/// registers; `rocm fix` then re-gates the same id on the *recipe's* list +/// against the running OS. WSL2 is its own family on both sides, so a recipe +/// that omits it printed the plan and then exited 3 on a platform its checker +/// deliberately answers for. This asserts the rc, not merely the output: the +/// wrong-OS refusal prints the recipe first, so the text alone cannot tell the +/// two outcomes apart. +#[then("the CLI can act on the fix the diagnosis named")] +async fn assert_named_fix_is_runnable_here(world: &mut E2eWorld) { + let (report, output) = parsed_diagnosis(world); + let fix_id = find_vllm_oom(&report, &output) + .pointer("/fix/fix_id") + .and_then(serde_json::Value::as_str) + .unwrap_or_else(|| panic!("the matched cause must name a fix id:\n{output}")) + .to_owned(); + let (fix_output, fix_stderr, rc) = crate::run_rocm(world, &["fix", &fix_id]); + assert_eq!( + rc, + 0, + "{}", + e2e_cucumber::cli_failure_report(&["fix", &fix_id], rc, &fix_output, &fix_stderr) + ); + assert!( + fix_output.to_lowercase().contains("print-only"), + "{fix_id} is advisory, so it must say it only printed a plan:\n{fix_output}" + ); +} + +#[then("the OOM remedy distinguishes a busy GPU from a model that does not fit")] +async fn assert_oom_remedy_is_conditional(world: &mut E2eWorld) { + let (report, output) = parsed_diagnosis(world); + let fix = find_vllm_oom(&report, &output) + .get("fix") + .unwrap_or_else(|| panic!("expected fix-16-vllm-oom remediation:\n{output}")); + let rendered = fix.to_string(); + assert!( + rendered.contains("shared/busy") + && rendered.contains("does not fit") + && rendered.contains("--gpu-memory-utilization") + && rendered.contains("smaller or quantized"), + "the remedy must retain both conditional branches:\n{output}" + ); + assert!( + !rendered.contains("--tensor-parallel-size"), + "the remedy must not prescribe the non-existent multi-GPU sharding flag:\n{output}" + ); + assert!( + !fix.get("verify") + .and_then(serde_json::Value::as_str) + .is_some_and(|verify| verify.contains("--gpu-memory-utilization")), + "verification must not unconditionally prescribe the tenancy workaround:\n{output}" + ); +} + #[then("the CLI reports the engine-startup import failure as an established cause")] async fn assert_engine_import_failure_established(world: &mut E2eWorld) { assert_eq!( diff --git a/tests/e2e-cucumber/tests/e2e/serving_steps.rs b/tests/e2e-cucumber/tests/e2e/serving_steps.rs index a36489808..391d03c13 100644 --- a/tests/e2e-cucumber/tests/e2e/serving_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/serving_steps.rs @@ -853,6 +853,104 @@ async fn user_sends_completion(world: &mut E2eWorld) { crate::send_chat(world).await; } +/// Arm the debug/test VRAM hook so the *selected* GPU is reported nearly full. +/// The GPU lane's real cards are comfortably free, so the serve-plan low-VRAM +/// OOM warning is otherwise unreachable in E2E; this makes it deterministic +/// without touching the device the engine actually launches on. Consumed by the +/// following serve via `run_rocm_with_scenario_env`. +#[given("the selected GPU is reported nearly out of VRAM")] +async fn selected_gpu_nearly_out_of_vram(world: &mut E2eWorld) { + world + .command_env + .push(("ROCM_E2E_FORCE_LOW_VRAM", "0".into())); +} + +/// Preview a vLLM serve plan pinned to the nearly-full GPU. `--managed` +/// backgrounds the supervisor (teardown stops it), so this returns once the plan +/// has printed and the service has launched — the plan text is what the scenario +/// inspects. The engine still launches against the real, free device. +#[when("the user previews a vLLM serve plan pinned to that GPU")] +async fn user_previews_vllm_low_vram_serve_plan(world: &mut E2eWorld) { + ensure_serve_port_free().await; + let (stdout, stderr, rc) = crate::run_rocm_with_scenario_env( + world, + &[ + "serve", + "Qwen/Qwen2.5-0.5B-Instruct", + "--engine", + "vllm", + "--gpu", + "0", + "--managed", + ], + ); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + +/// The serve must have actually launched, not just printed a plausible plan: a +/// non-zero rc after a good plan-print would otherwise go undetected since this +/// scenario only inspects the plan lines (mirrors `assert_vllm_default`). +/// +/// This step states the scenario's *premise*, not its verdict: the low-VRAM +/// warning itself is long-standing behaviour, so the warning text alone proves +/// nothing about the vLLM guidance this scenario exists for — that is +/// `assert_serve_plan_names_memory_knob`'s job. What it does pin is that the +/// warning names the GPU the serve was *pinned to* (`--gpu 0`), so a warning +/// raised for some other device cannot stand in for the premise. +#[then("the serve plan warns the GPU is low on VRAM")] +async fn assert_serve_plan_low_vram_warning(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no serve output"); + let rc = world.cli_rc.expect("no serve rc recorded"); + assert!( + rc == 0, + "{}", + e2e_cucumber::cli_failure_report( + &[ + "serve", + "", + "--engine", + "vllm", + "--gpu", + "0", + "--managed", + ], + rc, + output, + world.cli_stderr.as_deref().unwrap_or(""), + ) + ); + assert!( + output.contains("serving may fail on VRAM"), + "expected a low-VRAM serve-plan warning, got:\n{output}" + ); + assert!( + output.contains("GPU 0 has only"), + "the low-VRAM warning must be about the pinned GPU 0, got:\n{output}" + ); +} + +/// The scenario's discriminating assertion — the behaviour added for EAI-8058. +/// +/// Matching the flag name alone would be too weak to be that: `rocm serve` echoes +/// engine recipe flags in the same plan, so a `--gpu-memory-utilization` the user +/// passed themselves would satisfy it. Require the note's own explanation of *why* +/// a busy GPU OOMs (vLLM's fixed total-VRAM reservation) alongside the flag, so +/// only the pre-launch hint can satisfy this step. +#[then("the serve plan explains how to lower vLLM's memory reservation")] +async fn assert_serve_plan_names_memory_knob(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no serve output"); + assert!( + output.contains("--gpu-memory-utilization"), + "expected the vLLM memory-utilization note in the serve plan, got:\n{output}" + ); + assert!( + output.contains("vLLM reserves ~90%"), + "the note must explain vLLM's total-VRAM reservation, not merely name the flag, got:\n{output}" + ); +} + /// Serve under the GPU-required default (no `--device`), pinning the engine to the /// host's effective serve engine so the serve reaches GPU enforcement rather than /// tripping on engine selection.