diff --git a/README.md b/README.md index 499a5b8be..abd6e803d 100644 --- a/README.md +++ b/README.md @@ -284,17 +284,20 @@ fix` takes the id, not the position. `fix` applies a known fix by the `id:` that `diagnose` reported — not the ranking position noted above, which isn't a stable name. Run it with no id -to list the whole catalog. Each fix is marked AUTO (this command carries out -the change) or PRINT-ONLY (it prints the steps for you to run yourself — -usually because the right command depends on a choice only you can make, -sometimes because it also needs sudo or a reboot). +to list the whole catalog. Each fix carries a marker saying what happens on +this machine: AUTO (this command carries out the change), NEEDS-ARG (it will, +once given the argument it names), PRINT-ONLY (it prints the steps for you to +run yourself — usually because the right command depends on a choice only you +can make, sometimes because it also needs sudo or a reboot), or DIAGNOSE-ONLY +(no reliable fix exists, so nothing will be changed -- no catalog entry +carries this marker today; it is reserved for a future detect-but-cannot-repair +failure). - `--dry-run` shows any fix's plan without changing anything. - `--yes` skips the interactive confirmation once you've reviewed it. -- `--device-index` pins the discrete GPU index for `fix-9-igpu-dgpu`; - without it, that fix only prints the `rocminfo` (Linux) or `hipInfo.exe` - (Windows) query needed to find the index and makes no change, despite - being marked AUTO. +- `--device-index` pins the discrete GPU index for `fix-9-igpu-dgpu`, marked + NEEDS-ARG; without it, that fix only prints the `rocminfo` (Linux) or + `hipInfo.exe` (Windows) query needed to find the index and makes no change. ### ROCm installation diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index e600faf20..a2e94bb53 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -160,10 +160,14 @@ enum Command { /// `#1`/`#2` ranking position, which belongs to one report and is not a name. /// With no id it lists the whole catalog. /// - /// Fixes are marked AUTO or PRINT-ONLY: AUTO means this command carries the - /// change out, PRINT-ONLY means it prints the steps for you to run yourself - /// (typically because they need sudo or a reboot). Use `--dry-run` to see any - /// fix's plan without changing anything. + /// Every fix carries a marker saying what happens on the machine in front of + /// you: AUTO means this command carries the change out; NEEDS-ARG means it + /// will, once told what to act on; PRINT-ONLY means it prints the steps for + /// you to run yourself (typically because they need sudo or a reboot); and + /// DIAGNOSE-ONLY means no reliable fix exists and nothing will be changed + /// (no catalog entry carries this marker today; it is reserved for a + /// future detect-but-cannot-repair failure). + /// Use `--dry-run` to see any fix's plan without changing anything. Fix { /// Fix id, e.g. fix-4-render-group. Omit to list available fixes. fix_id: Option, diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index e4652479c..05e67e653 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -33,7 +33,25 @@ pub struct Fix { pub needs_reboot: bool, pub needs_relogin: bool, pub fix_id: String, + /// Whether `rocm fix `, run with no extra arguments, will change + /// the machine. + /// + /// Kept alongside [`Fix::class`], which it is derived from, because it is + /// the field published consumers already read. It is not redundant detail + /// so much as a narrower question: `class` says *what* the CLI will do, + /// this says only whether the machine is about to change. pub auto_applicable: bool, + /// What the CLI will do with this entry on the examined machine. + /// + /// `#[serde(default)]` so a payload written before this field existed still + /// deserializes; the default understates rather than overstates. + #[serde(default)] + pub class: crate::fix::FixClass, + /// The argument this entry is waiting for, when `class` is + /// [`crate::fix::FixClass::NeedsArgument`]. Read from the catalog beside + /// `class` so the flags line renders identically from `rocm fix` and here. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub needs: Option, pub notes: Vec, pub verify: String, } @@ -468,7 +486,6 @@ fn check_1_arch_not_in_wheel(e: &Examination, symptom: &str) -> Diagnosis { "# cmake -B build -DGGML_HIP=ON -DAMDGPU_TARGETS=".to_owned(), ], fix_id: "fix-1-arch".to_owned(), - auto_applicable: false, verify: "python -c \"import torch; print(torch.cuda.is_available(), torch.cuda.get_arch_list())\"".to_owned(), notes: notes_1_arch(e), ..Fix::default() @@ -552,7 +569,6 @@ fn check_2_hsa_override_unneeded(e: &Examination, symptom: &str) -> Diagnosis { "# Or remove via System Properties -> Environment Variables.".to_owned(), ], fix_id: "fix-2-unset-override".to_owned(), - auto_applicable: true, verify: "powershell -NoProfile -Command \"[Environment]::GetEnvironmentVariable('HSA_OVERRIDE_GFX_VERSION','User')\"".to_owned(), ..Fix::default() } @@ -564,7 +580,6 @@ fn check_2_hsa_override_unneeded(e: &Examination, symptom: &str) -> Diagnosis { "# Also remove it from ~/.bashrc / ~/.zshrc / ~/.profile if persisted.".to_owned(), ], fix_id: "fix-2-unset-override".to_owned(), - auto_applicable: true, verify: "env | grep HSA_OVERRIDE_GFX_VERSION || echo OK_UNSET; python -c \"import torch; print(torch.cuda.is_available())\"".to_owned(), ..Fix::default() } @@ -612,7 +627,6 @@ fn check_3_rocm_kernel_unsupported(e: &Examination, symptom: &str) -> Diagnosis "# kernel that matches ROCm, or rerun amdgpu-install with --no-dkms.".to_owned(), ], fix_id: "fix-3-rocm-kernel".to_owned(), - auto_applicable: false, needs_reboot: true, verify: "lsmod | grep amdgpu && rocminfo | head -n 20".to_owned(), ..Fix::default() @@ -670,7 +684,6 @@ fn check_4_render_group(e: &Examination, symptom: &str) -> Diagnosis { needs_sudo: true, needs_relogin: true, fix_id: "fix-4-render-group".to_owned(), - auto_applicable: true, verify: "groups | tr ' ' '\\n' | grep -E '^(render|video)$' && ls -l /dev/kfd && rocminfo | head -n 5".to_owned(), notes: vec![ "Group membership only takes effect after a full re-login (or reboot). `newgrp render` will give the current shell access but not other terminals or services.".to_owned(), @@ -740,7 +753,6 @@ fn check_5_amdgpu_blacklisted(e: &Examination, symptom: &str) -> Diagnosis { // the plain no-Secure-Boot sub-case, judged cheaper than the drift it replaces. needs_reboot: true, fix_id: "fix-5-amdgpu-load".to_owned(), - auto_applicable: false, verify: "lsmod | grep amdgpu && rocminfo | head -n 5".to_owned(), ..Fix::default() }; @@ -815,7 +827,6 @@ fn check_6_path_missing(e: &Examination, symptom: &str) -> Diagnosis { .to_owned(), ], fix_id: "fix-6-path".to_owned(), - auto_applicable: true, verify: format!( "powershell -NoProfile -Command \"& \\\"{bin_dir}\\hipInfo.exe\\\" | Select-Object -First 5\"" ), @@ -829,7 +840,6 @@ fn check_6_path_missing(e: &Examination, symptom: &str) -> Diagnosis { format!("echo 'export PATH={bin_dir}:$PATH' >> ~/.bashrc # or ~/.zshrc"), ], fix_id: "fix-6-path".to_owned(), - auto_applicable: true, verify: "rocminfo | head -n 5 && hipcc --version".to_owned(), ..Fix::default() } @@ -879,7 +889,6 @@ fn check_7_stale_repos(e: &Examination, symptom: &str) -> Diagnosis { commands, needs_sudo: true, fix_id: "fix-7-stale-repos".to_owned(), - auto_applicable: false, verify: "sudo apt update 2>&1 | tail -n 20".to_owned(), ..Fix::default() }; @@ -939,7 +948,6 @@ fn check_8_wheel_rocm_mismatch(e: &Examination, symptom: &str) -> Diagnosis { "python -c \"import torch; print(torch.__version__, torch.version.hip)\"".to_owned(), ], fix_id: "fix-8-wheel-rocm".to_owned(), - auto_applicable: false, verify: "python -c \"import torch; print(torch.cuda.is_available(), torch.version.hip)\"".to_owned(), ..Fix::default() } @@ -956,7 +964,6 @@ fn check_8_wheel_rocm_mismatch(e: &Examination, symptom: &str) -> Diagnosis { "python -c \"import torch; print(torch.__version__, torch.version.hip)\"".to_owned(), ], fix_id: "fix-8-wheel-rocm".to_owned(), - auto_applicable: false, verify: "python -c \"import torch; print(torch.cuda.is_available(), torch.version.hip)\"".to_owned(), ..Fix::default() } @@ -1023,14 +1030,14 @@ fn check_9_igpu_dgpu_collision(e: &Examination, symptom: &str) -> Diagnosis { "Detected gfx targets: {gfx_targets:?}. Discrete GPU(s): {discrete_targets:?}; integrated APU(s): {apu_targets:?}. Pin HIP_VISIBLE_DEVICES to the discrete GPU — do not assume the higher-numbered gfx target is the dGPU (on RDNA3 the APU can be higher)." ) }; - // Both branches below are marked auto_applicable, but `rocm fix - // fix-9-igpu-dgpu` still needs --device-index to actually make the change: - // without it, the Linux and the Windows runner alike only print the query - // that finds the index and change nothing (see README's --device-index - // caveat). Each branch states that once, in its second note. - // `render_report_text` prints every note on its own line, so saying it - // again here -- appended to the detected-targets note -- would show up as a - // second bullet repeating the first. + // The catalog classes this NEEDS-ARG, which is what the marker now says: + // without --device-index both the Linux and Windows runners only print the + // query that finds the index and change nothing. The note used to end + // "despite being marked AUTO", which was true of the old flat flag and is + // exactly the overstatement this change removes. + let note = format!( + "{note} Without --device-index, `rocm fix` only prints this query and makes no change." + ); let fix = if e.os_family == "windows" { Fix { summary: "Pin the HIP runtime to the discrete GPU with HIP_VISIBLE_DEVICES so the iGPU is hidden.".to_owned(), @@ -1042,16 +1049,8 @@ fn check_9_igpu_dgpu_collision(e: &Examination, symptom: &str) -> Diagnosis { "# `setx` only takes effect in NEW shells; reopen the terminal.".to_owned(), ], fix_id: "fix-9-igpu-dgpu".to_owned(), - auto_applicable: true, verify: "powershell -NoProfile -Command \"$env:HIP_VISIBLE_DEVICES=1; python -c \\\"import torch; print(torch.cuda.device_count())\\\"\"".to_owned(), - notes: vec![ - note, - "auto-applicable here means `rocm fix fix-9-igpu-dgpu` has a runner \ - for it — but that runner only pins HIP_VISIBLE_DEVICES when you pass \ - --device-index N. Without it, `rocm fix` just prints the query that \ - identifies which index is the discrete GPU." - .to_owned(), - ], + notes: vec![note], ..Fix::default() } } else { @@ -1065,22 +1064,8 @@ fn check_9_igpu_dgpu_collision(e: &Examination, symptom: &str) -> Diagnosis { "# Persist in your shell rc or your launch script.".to_owned(), ], fix_id: "fix-9-igpu-dgpu".to_owned(), - // Matches the `fix-9-igpu-dgpu` FixRecipe in fix.rs (auto_applicable: - // true, runner: run_hip_visible_devices) -- `rocm fix - // fix-9-igpu-dgpu --device-index N` really does carry this out on - // Linux, so the report must not claim otherwise. An agent branches - // on this flag to decide whether to offer to run the fix or only - // print it, so a wrong value here costs more than a stale sentence. - auto_applicable: true, verify: "HIP_VISIBLE_DEVICES=1 python -c \"import torch; print(torch.cuda.device_count())\"".to_owned(), - notes: vec![ - note, - "auto-applicable here means `rocm fix fix-9-igpu-dgpu` has a runner \ - for it — but that runner only pins HIP_VISIBLE_DEVICES when you pass \ - --device-index N. Without it, `rocm fix` just prints the query that \ - identifies which index is the discrete GPU." - .to_owned(), - ], + notes: vec![note], ..Fix::default() } }; @@ -1139,7 +1124,6 @@ fn check_10_container_devices(e: &Examination, symptom: &str) -> Diagnosis { "# host user is in the render group; podman maps it through.".to_owned(), ], fix_id: "fix-10-container".to_owned(), - auto_applicable: false, verify: "rocminfo | head -n 5".to_owned(), notes: vec!["Use rocm/pytorch or rocm/dev-ubuntu-22.04 as a known-good image. Mixing host ROCm + container ROCm versions is a separate footgun.".to_owned()], ..Fix::default() @@ -1189,7 +1173,6 @@ fn check_11_iommu_hang(e: &Examination, symptom: &str) -> Diagnosis { needs_sudo: true, needs_reboot: true, fix_id: "fix-11-iommu".to_owned(), - auto_applicable: false, verify: "cat /proc/cmdline | grep -o 'iommu=\\w*'".to_owned(), ..Fix::default() }; @@ -1232,7 +1215,6 @@ fn check_12_amdgpu_install_broken(e: &Examination, symptom: &str) -> Diagnosis { needs_sudo: true, needs_reboot: true, fix_id: "fix-12-installer".to_owned(), - auto_applicable: false, verify: "dpkg -l | grep -E 'rocm|amdgpu' | head -n 20 && rocminfo | head -n 5".to_owned(), notes: vec!["If `apt autoremove` warns it will remove unrelated packages, stop and resolve those by hand before continuing.".to_owned()], ..Fix::default() @@ -1289,7 +1271,6 @@ fn check_13_hip_sdk_missing(e: &Examination, symptom: &str) -> Diagnosis { "# After install, reopen the shell so HIP_PATH and PATH pick up the new install.".to_owned(), ], fix_id: "fix-13-hip-sdk-missing".to_owned(), - auto_applicable: false, verify: "powershell -NoProfile -Command \"& \\\"$env:HIP_PATH\\bin\\hipInfo.exe\\\" | Select-Object -First 5\"".to_owned(), notes: vec!["If you only need PyTorch on Windows AMD and don't need the C/C++ HIP toolchain, the TheRock wheels bundle their own HIP runtime and may not require a system HIP SDK install.".to_owned()], ..Fix::default() @@ -1347,7 +1328,6 @@ fn check_14_adrenalin_too_old(e: &Examination, symptom: &str) -> Diagnosis { ], needs_reboot: true, fix_id: "fix-14-adrenalin-too-old".to_owned(), - auto_applicable: false, verify: "powershell -NoProfile -Command \"(Get-CimInstance Win32_VideoController | Where-Object { $_.Name -like '*AMD*' -or $_.Name -like '*Radeon*' } | Select-Object -First 1).DriverVersion\"".to_owned(), ..Fix::default() }; @@ -1385,7 +1365,6 @@ fn check_15_msvc_redist(e: &Examination, symptom: &str) -> Diagnosis { "# After the install, reopen the shell and re-run your import / hipInfo check.".to_owned(), ], fix_id: "fix-15-msvc-redist".to_owned(), - auto_applicable: false, 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()], ..Fix::default() @@ -1459,7 +1438,6 @@ fn check_17_torch_dlpack_cuda_variant(_e: &Examination, symptom: &str) -> Diagno "rocm engines install vllm --reinstall".to_owned(), ], fix_id: "fix-17-torch-dlpack".to_owned(), - auto_applicable: false, verify: "rocm serve --engine vllm # then `rocm services list --all` and `rocm services logs ` to confirm the import no longer aborts".to_owned(), notes: vec![ "Running vLLM on ROCm is not by itself a reason to apply this. The trigger is narrow: a ROCm build of torch in the 2.4-2.9 range (the versions torch-c-dlpack-ext ships prebuilts for), torch without a native __dlpack_c_exchange_api__, and torch-c-dlpack-ext present -- it arrives as a transitive dependency of tilelang, which vLLM pins.".to_owned(), @@ -1561,7 +1539,6 @@ fn check_19_shm_too_small(e: &Examination, symptom: &str) -> Diagnosis { ], needs_sudo: true, fix_id: ID.to_owned(), - auto_applicable: false, verify: "df -h /dev/shm".to_owned(), notes: vec![ "A running container cannot have its allowance changed; it has to be started again." @@ -1696,7 +1673,6 @@ fn check_wsl_1_gpu_not_exposed(e: &Examination, symptom: &str) -> Diagnosis { summary: "Expose the GPU to the distro: /dev/dxg is how WSL reaches it, and nothing works until it is there.".to_owned(), commands, fix_id: "fix-wsl-1-gpu-not-exposed".to_owned(), - auto_applicable: false, verify: "ls -l /dev/dxg".to_owned(), notes, ..Fix::default() @@ -1741,7 +1717,6 @@ fn check_wsl_2_dxcore_missing(e: &Examination, symptom: &str) -> Diagnosis { ], needs_sudo: true, fix_id: "fix-wsl-2-dxcore-missing".to_owned(), - auto_applicable: false, verify: "ls -l /usr/lib/wsl/lib/libdxcore.so && ldconfig -p | grep libdxcore".to_owned(), notes: vec![ "/usr/lib/wsl is mounted by WSL itself, not installed by the distro's package manager, so apt cannot repair it -- the fix is on the Windows side.".to_owned(), @@ -1786,7 +1761,6 @@ fn check_wsl_3_rocdxg_missing(e: &Examination, symptom: &str) -> Diagnosis { ], needs_sudo: true, fix_id: "fix-wsl-3-rocdxg-missing".to_owned(), - auto_applicable: false, verify: "ldconfig -p | grep librocdxg".to_owned(), notes: vec![ "This downloads and installs a .deb with sudo, so `rocm fix` prints it rather than running it. `rocm install driver` shows the full plan, and checks the download against a digest pinned for that ROCDXG release.".to_owned(), @@ -1831,7 +1805,6 @@ fn check_wsl_4_rocdxg_not_linked(e: &Examination, symptom: &str) -> Diagnosis { commands: vec!["sudo ldconfig".to_owned()], needs_sudo: true, fix_id: "fix-wsl-4-rocdxg-not-linked".to_owned(), - auto_applicable: false, verify: "ldconfig -p | grep librocdxg".to_owned(), notes: vec![ "If `ldconfig` alone does not fix it, the install went somewhere outside the linker's search path: add that directory under /etc/ld.so.conf.d/ and re-run.".to_owned(), @@ -1868,7 +1841,6 @@ fn check_wsl_5_distro_too_old(e: &Examination, _symptom: &str) -> Diagnosis { "# wsl --install -d Ubuntu-24.04".to_owned(), ], fix_id: "fix-wsl-5-distro-too-old".to_owned(), - auto_applicable: false, verify: "grep VERSION_ID /etc/os-release".to_owned(), notes: vec![ "This is a hard floor, not a recommendation: Ubuntu 22.04 ships glibc 2.35, below the glibc 2.38 / GLIBCXX_3.4.32 that every published Lemonade embeddable is linked against, so the engine cannot start there at all.".to_owned(), @@ -1967,7 +1939,6 @@ fn check_wsl_6_host_driver_too_old(e: &Examination, symptom: &str) -> Diagnosis "# install a WSL-capable AMD Adrenalin driver, then `wsl --shutdown`.".to_owned(), ], fix_id: "fix-wsl-6-host-driver-too-old".to_owned(), - auto_applicable: false, verify: "rocminfo | head -n 20".to_owned(), notes: vec![ format!("Driver and ROCm version pairing: {WSL_DOCS_URL}"), @@ -2000,7 +1971,6 @@ fn check_wsl_7_wsl1(e: &Examination, _symptom: &str) -> Diagnosis { "# wsl --set-default-version 2".to_owned(), ], fix_id: "fix-wsl-7-wsl1".to_owned(), - auto_applicable: false, verify: "uname -r".to_owned(), notes: vec![ "Converting rewrites the distro's filesystem and can take a while on a large install; back up anything you cannot lose first.".to_owned(), @@ -2150,11 +2120,12 @@ pub fn diagnose(e: &Examination, symptom: &str) -> DiagnoseReport { // known misconfiguration", which reads as "your machine looks fine" when the // truth is that nothing was ever checked. let out_of_scope = uncovered_platform_message(e); - let matched = if out_of_scope.is_some() { + let mut matched = if out_of_scope.is_some() { Vec::new() } else { run_all_checks(e, symptom) }; + take_applicability_from_the_catalog(&mut matched, platform_family(e)); DiagnoseReport { has_match: any_cleared_threshold(&matched), matched, @@ -2165,6 +2136,35 @@ pub fn diagnose(e: &Examination, symptom: &str) -> DiagnoseReport { } } +/// Fill each finding's applicability from the catalog, for the operating system +/// of the machine that was examined. +/// +/// One place, deliberately. Every checker used to state `auto_applicable` by +/// hand, which meant the catalog's answer existed twice in two modules with +/// nothing comparing them — and they had already drifted: `fix-9-igpu-dgpu` was +/// auto-applicable in the catalog and, in the Linux arm of its own checker, +/// not. Deriving it here makes that disagreement unrepresentable rather than +/// something a test has to go looking for. +/// +/// Keyed on the examined machine's platform *family*, not the running host and +/// not `os_family`. WSL reports an `os_family` of `linux` but is its own family +/// for catalog purposes, so reading `os_family` here would look up every WSL +/// recipe under the wrong platform and silently report them all as print-only. +fn take_applicability_from_the_catalog(matched: &mut [Diagnosis], platform_family: &str) { + for diagnosis in matched.iter_mut() { + let Some(fix) = diagnosis.fix.as_mut() else { + continue; + }; + // An entry the catalog does not list for this OS keeps the default: + // the CLI will not act on it here, which is exactly what the OS gate in + // `fix::apply` would tell the user. + let class = crate::fix::class_on(&fix.fix_id, platform_family).unwrap_or_default(); + fix.class = class; + fix.auto_applicable = class.applies_itself(); + fix.needs = crate::fix::needs_on(&fix.fix_id, platform_family).map(ToOwned::to_owned); + } +} + /// ROCm-on-WSL2 setup guidance (distinct from the bare-metal catalog). const WSL_DOCS_URL: &str = "https://rocm.docs.amd.com/projects/radeon-ryzen/en/latest/docs/install/installryz/wsl/howto_wsl.html"; @@ -2236,7 +2236,8 @@ pub fn render_report_text(report: &DiagnoseReport, top: usize) -> String { fix.needs_sudo, fix.needs_reboot, fix.needs_relogin, - fix.auto_applicable, + fix.class, + fix.needs.as_deref(), ); let _ = writeln!(out, " flags: {}", flags.join(", ")); for n in &fix.notes { @@ -2362,6 +2363,37 @@ mod tests { ); } + /// `take_applicability_from_the_catalog` is the only place that may fill in + /// `Fix::auto_applicable`. Every checker used to set it by hand instead -- + /// twenty of them were converted to leave it at `Fix::default()` and let the + /// catalog fill it in, but the shared-memory and WSL checkers kept the + /// inline `auto_applicable: false,` a while longer. It never produced wrong + /// output, because the catalog pass overwrites whatever a checker sets -- + /// which is exactly the problem: a checker disagreeing with the catalog + /// again (the way `fix-9-igpu-dgpu` once did) would be silently masked + /// rather than caught. A behavioral test can't see this, since the output + /// is correct either way; this reads the checkers' own source instead, so a + /// hand-set boolean is caught before it can start drifting unnoticed. + #[test] + fn no_checker_hand_sets_auto_applicable() { + let source = include_str!("diagnose.rs"); + let offenders: Vec<&str> = source + .lines() + .filter(|line| { + let trimmed = line.trim_start(); + trimmed.starts_with("auto_applicable: true") + || trimmed.starts_with("auto_applicable: false") + }) + .collect(); + assert!( + offenders.is_empty(), + "a Fix literal is hand-setting `auto_applicable` again -- only \ + `take_applicability_from_the_catalog` may set this field; every checker must \ + leave it at `Fix::default()` and let the catalog fill it in, or the catalog and \ + the checker can silently drift the way fix-9-igpu-dgpu once did:\n{offenders:#?}" + ); + } + #[test] fn a_managed_runtimes_hip_is_not_measured_against_the_system_rocm() { // A managed runtime's torch loads HIP from a sibling `_rocm_sdk_core` @@ -3048,9 +3080,11 @@ mod tests { .fix .as_ref() .unwrap_or_else(|| panic!("{os}/{}: matched with no fix", d.id)); - let catalog = crate::fix::auto_applicable_for(&fix.fix_id).unwrap_or_else(|| { - panic!("{os}/{}: emitted a fix-id not in the catalog", fix.fix_id) - }); + let catalog = crate::fix::class_on(&fix.fix_id, os) + .unwrap_or_else(|| { + panic!("{os}/{}: emitted a fix-id not in the catalog", fix.fix_id) + }) + .applies_itself(); assert_eq!( fix.auto_applicable, catalog, "{os}/{}: diagnose reports auto_applicable={}, but the fix catalog \ @@ -3147,7 +3181,7 @@ mod tests { "note must not repeat the old wrong gfx-number heuristic: {note}" ); assert!( - note.contains("--device-index"), + note.contains("Without --device-index") && !note.contains("marked AUTO"), "note must warn that fix-9 is a no-op without --device-index: {note}" ); } @@ -3226,15 +3260,20 @@ mod tests { } #[test] - fn fix_9_igpu_dgpu_is_auto_applicable_on_linux() { - // `check_9_igpu_dgpu_collision`'s Linux/else branch sets - // `auto_applicable: true` to match the `fix-9-igpu-dgpu` FixRecipe in - // fix.rs (`run_hip_visible_devices` already handles it on Linux). This - // is a behavioural change, not text-only: it flips both the `Fix` - // struct field that `rocm diagnose --json` serialises and the - // `flags:` line `render_report_text` prints. Pin it directly so a - // regression back to `false` (the pre-fix value) fails here instead of - // only being visible by eyeballing output. + fn fix_9_igpu_dgpu_waits_for_an_argument_rather_than_claiming_it_will_act() { + // This pinned `auto_applicable: true` until the catalog gained a class. + // The claim was wrong: asked plainly, both arms of + // `run_hip_visible_devices` print the query that identifies the discrete + // GPU and return without touching the machine, so a user -- and an agent + // reading the same output -- was told a change was coming that never + // came, and given exit 0 to confirm it. The entry is NEEDS-ARG: it acts + // only once `--device-index` names a target. + // + // Kept pinned for the original reason, with the expectation corrected. + // It still flips the `Fix` field `rocm diagnose --json` serialises and + // the `flags:` line `render_report_text` prints, so a regression back to + // an unconditional auto claim fails here rather than needing someone to + // eyeball the output. let mut e = linux_base(); e.has_apu = true; e.has_discrete_amd = true; @@ -3260,8 +3299,14 @@ mod tests { .expect("iGPU+dGPU collision should be diagnosed"); let fix = hit.fix.as_ref().unwrap(); assert!( - fix.auto_applicable, - "fix-9-igpu-dgpu must be auto_applicable on Linux, matching the fix.rs catalog" + !fix.auto_applicable, + "fix-9-igpu-dgpu does nothing until it is told which device to pin, so the report \ + must not say the CLI will carry it out" + ); + assert_eq!( + fix.class, + crate::fix::FixClass::NeedsArgument, + "the report's class has to be the catalog's class for this entry on Linux" ); let text = render_report_text(&report, report.matched.len()); @@ -3274,27 +3319,30 @@ mod tests { .iter() .find(|l| l.trim_start().starts_with("flags:")) .expect("fix-9-igpu-dgpu should have a flags: line"); - // Exact match, not `contains`: fix-9 carries only the auto flag, so a - // revert of the `render_report_text` call site to its pre-PR inline - // logic would still print a line containing "rocm fix can run it" and - // not "manual only" -- `contains` can't tell the two implementations - // apart. See `fix_11_iommu_rendered_flags_line_is_exact` for a fix-id - // whose optional flags actually differ between old and new wording. + // Exact match, not `contains`: fix-9 carries only the class flag, and + // the whole point of the class is that "will not run it" has more than + // one reason. A `contains` check would pass against the print-only + // wording, which would send the user off to run the steps by hand when + // the CLI will in fact do it for them once given the argument. The + // argument is named rather than described, which is why this text comes + // from the catalog and not from a literal here. assert_eq!( flags_line.trim_start(), - "flags: rocm fix can run it", + "flags: needs --device-index before `rocm fix` will run it", "rendered flags: line for fix-9-igpu-dgpu: {flags_line}" ); } #[test] fn fix_11_iommu_rendered_flags_line_is_exact() { - // Companion to `fix_9_igpu_dgpu_is_auto_applicable_on_linux`: that test - // only pins a fix-id with just the auto flag set, which an exact-match - // assertion can't distinguish from the pre-PR `render_report_text` - // inline logic (both print "rocm fix can run it" for it). fix-11-iommu - // carries sudo+reboot+manual, so this pins the full comma-joined, - // reworded `flags:` line through the real render call site. + // Companion to + // `fix_9_igpu_dgpu_waits_for_an_argument_rather_than_claiming_it_will_act`: + // that test only pins a fix-id with a single class flag and no + // sudo/reboot/relogin flags set, which an exact-match assertion can't + // distinguish from a render path that drops the comma-joining or + // misorders multiple flags. fix-11-iommu carries sudo+reboot+manual, so + // this pins the full comma-joined, reworded `flags:` line through the + // real render call site. let mut e = linux_base(); e.iommu_kernel_param = "on".to_owned(); e.gpus = vec![ diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index dc056566f..7577ababc 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -15,6 +15,7 @@ use crate::examine::{run, which}; use crate::{runtime_is_linux, runtime_is_windows}; +use serde::{Deserialize, Serialize}; use std::io::{IsTerminal, Write}; use std::path::{Path, PathBuf}; use std::time::Duration; @@ -50,45 +51,179 @@ pub struct FixOptions { pub device_index: Option, } +/// What `rocm fix ` will do with an entry — the answer to "will running +/// this plainly change my machine?". +/// +/// This was a `bool` (`auto_applicable`), and a bool could not say two things +/// the catalog needed to say. `fix-2-unset-override` applies itself on Windows +/// but only reports on Linux, where its runner takes no [`FixOptions`] and +/// never mutates. `fix-9-igpu-dgpu` applies itself only once it is told which +/// device to pin, and merely prints the identifying query otherwise. Both were +/// marked auto-applicable, so both told a user — and an agent — that the CLI +/// was about to act when it was not. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum FixClass { + /// `rocm fix ` carries it out (asking first, unless `--yes`). + Auto, + /// `rocm fix ` carries it out only once the argument named in + /// [`Platform::needs`] is supplied; without it, it reports what to pass and + /// changes nothing. + NeedsArgument, + /// `rocm fix ` leaves the machine alone. The fix is real; carrying it + /// out needs sudo, a reboot, a reinstall or a download. + /// + /// This says nothing about whether a runner runs. `fix-2-unset-override` + /// is print-only on Linux and still reads the live override and scans the + /// shell rc files to report where it is set — it just never writes. + #[default] + PrintOnly, + /// The problem can be detected and explained, but no reliable fix exists. + /// `rocm fix ` names it and applies nothing. + /// + /// Defaulting to [`FixClass::PrintOnly`] rather than this one is + /// deliberate: a missing class should understate what the CLI will do, and + /// "there is no fix" is a stronger claim than "here are the steps". + DiagnoseOnly, +} + +impl FixClass { + /// Whether `rocm fix `, invoked with no extra arguments, will change + /// the machine. This is the claim the old `auto_applicable` bool was making. + #[must_use] + pub const fn applies_itself(self) -> bool { + matches!(self, Self::Auto) + } + + /// The marker shown in the catalog listing and in `diagnose` output. + #[must_use] + pub const fn marker(self) -> &'static str { + match self { + Self::Auto => "AUTO", + Self::NeedsArgument => "NEEDS-ARG", + Self::PrintOnly => "PRINT-ONLY", + Self::DiagnoseOnly => "DIAGNOSE-ONLY", + } + } +} + +/// One operating system a recipe applies on, and what it does there. +/// +/// Scope and class are held together rather than in two parallel lists because +/// they are one fact: a fix applies *on Linux as print-only* and *on Windows as +/// auto*. Splitting them is what let `fix-2`'s flat flag contradict its own +/// platform-split runner. +struct Platform { + os: &'static str, + class: FixClass, + /// The argument that moves [`FixClass::NeedsArgument`] to acting. `None` + /// for every other class; the pairing is held by + /// `a_needs_argument_platform_names_the_argument`. + needs: Option<&'static str>, +} + +/// Shorthand for the common case: the same class on a platform, needing nothing. +const fn on(os: &'static str, class: FixClass) -> Platform { + Platform { + os, + class, + needs: None, + } +} + +const PRINT_ON_LINUX_WINDOWS_AND_WSL: &[Platform] = &[ + on("linux", FixClass::PrintOnly), + on("windows", FixClass::PrintOnly), + on("wsl", FixClass::PrintOnly), +]; +const PRINT_ON_LINUX: &[Platform] = &[on("linux", FixClass::PrintOnly)]; +const PRINT_ON_WINDOWS: &[Platform] = &[on("windows", FixClass::PrintOnly)]; +/// Every WSL recipe. All print-only: each one installs a package with sudo, +/// edits loader configuration, or belongs to the Windows host, and none of that +/// meets the bar an auto-applied fix has to clear. +const PRINT_ON_WSL: &[Platform] = &[on("wsl", FixClass::PrintOnly)]; +/// Both Linux families, for a problem that is about neither the `amdgpu` module +/// nor the Windows host driver and so is real on either. +const PRINT_ON_LINUX_AND_WSL: &[Platform] = &[ + on("linux", FixClass::PrintOnly), + on("wsl", FixClass::PrintOnly), +]; +const AUTO_ON_LINUX: &[Platform] = &[on("linux", FixClass::Auto)]; +const AUTO_ON_LINUX_WINDOWS_AND_WSL: &[Platform] = &[ + on("linux", FixClass::Auto), + on("windows", FixClass::Auto), + on("wsl", FixClass::Auto), +]; + +/// `fix-2-unset-override`. `run_unset_override` splits by platform: the Windows +/// arm takes `FixOptions` and does the dry-run/confirm dance, while +/// `run_unset_override_linux` takes no options, reports where the override is +/// set, and returns without touching a dotfile. WSL runs that same Linux arm. +const PRINT_ON_LINUX_AND_WSL_AUTO_ON_WINDOWS: &[Platform] = &[ + on("linux", FixClass::PrintOnly), + on("windows", FixClass::Auto), + on("wsl", FixClass::PrintOnly), +]; + +/// `fix-9-igpu-dgpu`. Both arms short-circuit when no device index is given: +/// they print the query that identifies the discrete GPU and return, so there +/// is nothing for `--dry-run` to preview and no prompt to refuse. +const NEEDS_DEVICE_INDEX: &[Platform] = &[ + Platform { + os: "linux", + class: FixClass::NeedsArgument, + needs: Some("--device-index"), + }, + Platform { + os: "windows", + class: FixClass::NeedsArgument, + needs: Some("--device-index"), + }, +]; + /// A remediation recipe keyed by the stable `fix-id`. struct FixRecipe { fix_id: &'static str, title: &'static str, rationale: &'static str, - auto_applicable: bool, commands: &'static [&'static str], needs_sudo: bool, needs_reboot: bool, needs_relogin: bool, verify: &'static str, notes: &'static [&'static str], - applies_on: &'static [&'static str], + /// Every operating system this applies on, and what it does on each. + applies_on: &'static [Platform], runner: Option i32>, } -/// Valid on bare-metal Linux, Windows and WSL alike. -/// -/// WSL is named explicitly rather than folded into `linux`: the default for a -/// bare-metal recipe has to be "does not apply on WSL", because the platform has -/// no amdgpu module, no /dev/kfd and no render group. Recipes that survive the -/// move are the ones about wheels, environment variables and PATH. -const LINUX_WINDOWS_AND_WSL: &[&str] = &["linux", "windows", "wsl"]; -const LINUX_AND_WINDOWS: &[&str] = &["linux", "windows"]; -const LINUX_ONLY: &[&str] = &["linux"]; -const WINDOWS_ONLY: &[&str] = &["windows"]; -const WSL_ONLY: &[&str] = &["wsl"]; -/// Both Linux families. For a problem that is neither about the `amdgpu` module -/// nor about the Windows host driver, and so is real on either. -const LINUX_AND_WSL: &[&str] = &["linux", "wsl"]; - -/// The recipe registry. Mirrors the diagnosis catalog; only the four small, -/// safe fixes carry a `runner` and are auto-applicable. +impl FixRecipe { + /// What this recipe does on `os`, or `None` when it does not apply there. + fn platform(&self, os: &str) -> Option<&Platform> { + self.applies_on.iter().find(|p| p.os == os) + } + + /// What this recipe does on the running host. [`FixClass::PrintOnly`] when + /// it does not apply here at all — callers reach the OS gate in [`apply`] + /// before the class is ever acted on, and understating is the safe default. + fn class_here(&self) -> FixClass { + self.platform(current_os()) + .map_or(FixClass::PrintOnly, |p| p.class) + } + + /// The operating systems this applies on, in catalog order. + fn os_scope(&self) -> Vec<&'static str> { + self.applies_on.iter().map(|p| p.os).collect() + } +} + +/// The recipe registry. Mirrors the diagnosis catalog; only the small, safe +/// fixes carry a `runner`, and what each one does can differ by platform. const RECIPES: &[FixRecipe] = &[ FixRecipe { fix_id: "fix-1-arch", title: "GPU gfx target not in framework arch list", rationale: "Your GPU's gfx target is not in the framework wheel's compiled kernel list. Re-install the framework from an index that includes this gfx, OR rebuild llama.cpp with AMDGPU_TARGETS=.", - auto_applicable: false, commands: &[ "# PyTorch (Linux): a nightly often carries kernels a release has not shipped yet.", "# Pick the nightly for the ROCm major you have, not an older one.", @@ -108,14 +243,13 @@ const RECIPES: &[FixRecipe] = &[ "TheRock per-gfx wheels are the recommended fallback when the official pytorch index does not yet cover your gfx (and the only first-party option on Windows AMD).", "HSA_OVERRIDE_GFX_VERSION is NOT the right fix here -- it papers over the mismatch and risks page faults at runtime.", ], - applies_on: LINUX_WINDOWS_AND_WSL, + applies_on: PRINT_ON_LINUX_WINDOWS_AND_WSL, runner: None, }, FixRecipe { fix_id: "fix-2-unset-override", title: "Unset HSA_OVERRIDE_GFX_VERSION", rationale: "HSA_OVERRIDE_GFX_VERSION is set, but your GPU now has a native wheel. The override hides the real gfx and causes page faults / OUT_OF_REGISTERS at runtime.", - auto_applicable: true, commands: &[ "# Linux:", "unset HSA_OVERRIDE_GFX_VERSION", @@ -129,14 +263,13 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "env | grep HSA_OVERRIDE_GFX_VERSION || echo OK_UNSET", notes: &[], - applies_on: LINUX_WINDOWS_AND_WSL, + applies_on: PRINT_ON_LINUX_AND_WSL_AUTO_ON_WINDOWS, runner: Some(run_unset_override), }, FixRecipe { fix_id: "fix-3-rocm-kernel", title: "ROCm/distro/kernel triple unsupported", rationale: "ROCm is installed but your kernel/distro combination is outside the supported matrix. Match the kernel to the matrix before reinstalling, or rerun with --no-dkms and accept the risk.", - auto_applicable: false, commands: &[ "# Cross-check the live AMD matrix before changing anything:", "# https://rocm.docs.amd.com/projects/install-on-linux/en/latest/reference/system-requirements.html", @@ -147,28 +280,26 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "lsmod | grep amdgpu && rocminfo | head -n 5", notes: &[], - applies_on: LINUX_ONLY, + applies_on: PRINT_ON_LINUX, runner: None, }, FixRecipe { fix_id: "fix-4-render-group", title: "Add user to render/video groups", rationale: "The current user can't open /dev/kfd because they aren't in the render group. Adding the user is the safe, standard fix.", - auto_applicable: true, commands: &["sudo usermod -a -G render,video \"$USER\""], needs_sudo: true, needs_reboot: false, needs_relogin: true, verify: "groups | tr ' ' '\\n' | grep -E '^(render|video)$' && rocminfo | head -n 5", notes: &[], - applies_on: LINUX_ONLY, + applies_on: AUTO_ON_LINUX, runner: Some(run_render_group), }, FixRecipe { fix_id: "fix-5-amdgpu-load", title: "Load amdgpu (and clear any blacklist)", rationale: "The amdgpu kernel module is not loaded. Check /etc/modprobe.d for a blacklist entry, regenerate the initramfs, and modprobe.", - auto_applicable: false, commands: &[ "grep -RIl 'blacklist amdgpu' /etc/modprobe.d /usr/lib/modprobe.d 2>/dev/null || true", "sudo $EDITOR # remove the blacklist line", @@ -183,14 +314,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "If Secure Boot is enabled and amdgpu still won't load, the DKMS module isn't signed. Either sign it with mokutil or disable Secure Boot in firmware.", ], - applies_on: LINUX_ONLY, + applies_on: PRINT_ON_LINUX, runner: None, }, FixRecipe { fix_id: "fix-6-path", title: "Add the ROCm/HIP bin directory to PATH", rationale: "Linux: ROCm is installed but its bin directory isn't on PATH, so `rocminfo` / `hipcc` aren't visible to the shell. Windows: the HIP SDK is installed but its bin directory isn't on the User PATH, so `hipInfo.exe` and the runtime DLLs can't be found.", - auto_applicable: true, commands: &[ "# Linux:", "echo 'export PATH=\"/opt/rocm/bin:$PATH\"' >> ~/.bashrc", @@ -202,14 +332,13 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "rocminfo | head -n 5 && hipcc --version", notes: &[], - applies_on: LINUX_WINDOWS_AND_WSL, + applies_on: AUTO_ON_LINUX_WINDOWS_AND_WSL, runner: Some(run_path_export), }, FixRecipe { fix_id: "fix-7-stale-repos", title: "Quarantine duplicate AMD repos", rationale: "More than one ROCm/AMDGPU repo file exists. The package manager is mixing versions; quarantine the extras before reinstalling.", - auto_applicable: false, commands: &[ "ls /etc/apt/sources.list.d/ | grep -iE 'rocm|amdgpu|radeon'", "# For each duplicate file:", @@ -221,14 +350,13 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "sudo apt update 2>&1 | tail -n 20", notes: &[], - applies_on: LINUX_ONLY, + applies_on: PRINT_ON_LINUX, runner: None, }, FixRecipe { fix_id: "fix-8-wheel-rocm", title: "Reinstall the framework against the system ROCm/HIP major", rationale: "The framework's bundled HIP version doesn't match the system ROCm (Linux) or HIP SDK (Windows). libamdhip64.so.X / amdhip64_X.dll load failures are the usual signal.", - auto_applicable: false, commands: &[ "pip uninstall -y torch torchvision torchaudio", "# Linux: install the index for the ROCm major `rocm examine` reports:", @@ -243,14 +371,13 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "python -c \"import torch; print(torch.__version__, torch.version.hip, torch.cuda.is_available())\"", notes: &[], - applies_on: LINUX_WINDOWS_AND_WSL, + applies_on: PRINT_ON_LINUX_WINDOWS_AND_WSL, runner: None, }, FixRecipe { fix_id: "fix-9-igpu-dgpu", title: "Hide the iGPU with HIP_VISIBLE_DEVICES", rationale: "Both an APU iGPU and a discrete AMD GPU are visible. Pin the runtime to the dGPU so the iGPU doesn't destabilise it.", - auto_applicable: true, commands: &[ "# Linux:", "rocminfo | grep -E 'Agent |Marketing|gfx' # find the dGPU index", @@ -266,14 +393,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "Pass --device-index N to persist the env var; without it, this fix only prints the rocminfo / hipInfo query so you can identify N.", ], - applies_on: LINUX_AND_WINDOWS, + applies_on: NEEDS_DEVICE_INDEX, runner: Some(run_hip_visible_devices), }, FixRecipe { fix_id: "fix-10-container", title: "Re-launch the container with AMD devices passed through", rationale: "The container can't see /dev/kfd or /dev/dri/renderD*. Pass the devices and the host's render group via the runtime flags.", - auto_applicable: false, commands: &[ "docker run --rm -it \\", " --device=/dev/kfd \\", @@ -290,14 +416,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "Rootless podman additionally needs `--userns=keep-id` and a host user that is in the render group; podman maps it through.", ], - applies_on: LINUX_ONLY, + applies_on: PRINT_ON_LINUX, runner: None, }, FixRecipe { fix_id: "fix-11-iommu", title: "Add iommu=pt to the kernel command line", rationale: "Multi-GPU jobs hang when the IOMMU is in the default 'on' mode with translation; pass-through mode fixes the hang. This requires editing GRUB and rebooting; we will not do that for you.", - auto_applicable: false, commands: &[ "cat /proc/cmdline", "sudo $EDITOR /etc/default/grub # add iommu=pt to GRUB_CMDLINE_LINUX_DEFAULT", @@ -310,14 +435,13 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "cat /proc/cmdline | grep -o 'iommu=\\w*'", notes: &[], - applies_on: LINUX_ONLY, + applies_on: PRINT_ON_LINUX, runner: None, }, FixRecipe { fix_id: "fix-12-installer", title: "Reset amdgpu-install state and reinstall", rationale: "amdgpu-install left a half-configured DKMS / repo state. Run the documented uninstall, clean up, and reinstall without the flag that broke things (commonly --accept-eula on newer installers).", - auto_applicable: false, commands: &[ "sudo amdgpu-install --uninstall", "sudo apt autoremove --purge -y", @@ -331,14 +455,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "If `apt autoremove --purge` warns it will remove unrelated packages, stop and resolve those by hand before continuing.", ], - applies_on: LINUX_ONLY, + applies_on: PRINT_ON_LINUX, runner: None, }, FixRecipe { fix_id: "fix-13-hip-sdk-missing", title: "Install the AMD HIP SDK for Windows", rationale: "Your framework links against HIP but the HIP SDK isn't installed on this host. The runtime DLLs (amdhip64_X.dll, hipblas.dll, hsa-runtime64.dll) and hipInfo.exe ship inside the SDK installer.", - auto_applicable: false, commands: &[ "# Download and install the HIP SDK (matched to your framework's HIP major):", "# https://www.amd.com/en/developer/resources/rocm-hub/hip-sdk.html", @@ -351,14 +474,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "If you only need PyTorch on Windows AMD and don't need the C/C++ HIP toolchain, the TheRock wheels bundle their own HIP runtime and may not require a system HIP SDK install.", ], - applies_on: WINDOWS_ONLY, + applies_on: PRINT_ON_WINDOWS, runner: None, }, FixRecipe { fix_id: "fix-14-adrenalin-too-old", title: "Update the Adrenalin / kernel-mode driver", rationale: "The HIP SDK is installed but the AMD kernel-mode driver (Adrenalin / Adrenalin Pro) is older than the SDK release notes call out. The user-space SDK and the driver have to match.", - auto_applicable: false, commands: &[ "# Cross-check the HIP SDK release notes for the exact driver pairing:", "# https://rocm.docs.amd.com/projects/install-on-windows/en/latest/install/install.html", @@ -371,14 +493,13 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "powershell -NoProfile -Command \"(Get-CimInstance Win32_VideoController | Where-Object { $_.Name -like '*AMD*' -or $_.Name -like '*Radeon*' } | Select-Object -First 1).DriverVersion\"", notes: &[], - applies_on: WINDOWS_ONLY, + applies_on: PRINT_ON_WINDOWS, runner: None, }, FixRecipe { fix_id: "fix-15-msvc-redist", title: "Install the MSVC 2015-2022 runtime redistributable", rationale: "The HIP SDK's amdhip64_X.dll links against the MSVC 2015-2022 runtime. When vcruntime140.dll / vcruntime140_1.dll aren't on PATH, `import torch` fails with a missing-DLL error that points at vcruntime140_1.dll, not at the HIP runtime itself.", - auto_applicable: false, commands: &[ "# Download and install (x64):", "# https://aka.ms/vs/17/release/vc_redist.x64.exe", @@ -391,7 +512,7 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "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.", ], - applies_on: WINDOWS_ONLY, + applies_on: PRINT_ON_WINDOWS, runner: None, }, // The number is a stable handle, not a position: `fix-16` is reserved by the @@ -401,7 +522,6 @@ const RECIPES: &[FixRecipe] = &[ fix_id: "fix-17-torch-dlpack", title: "Restore the engine's pinned torch (torch-c-dlpack-ext loads the CUDA variant)", rationale: "vLLM's engine start aborts at import time when torch-c-dlpack-ext loads its CUDA prebuilt on a ROCm torch: it picks the variant from torch.cuda.is_available(), which is True on ROCm because PyTorch reuses the torch.cuda namespace for HIP, and it ships no ROCm variant. tvm_ffi imports it as OPTIONAL but guards only ImportError/AttributeError, while ctypes.CDLL raises OSError -- so the optional import kills the process. Both defects are upstream; nothing here is misconfigured. What you can change locally is the torch version: outside the 2.4-2.9 range there is no prebuilt to load, the extension raises the handled ImportError, and tvm_ffi falls back to its JIT path with a warning.", - auto_applicable: false, // Three labelled groups, because the steps run in three different places // and `print_recipe` renders them as one undifferentiated `$`-prefixed // list. Unlabelled, a user pasting the block wholesale is relying on @@ -439,18 +559,13 @@ const RECIPES: &[FixRecipe] = &[ "The usual way a runtime lands in the failing range is `rocm install sdk` being re-run after the engine was installed, which overwrites the engine's pinned torch. Reinstalling the engine puts the pin back.", "A service that failed at startup is hidden from a plain `rocm services list`; pass --all to recover its id.", ], - applies_on: LINUX_ONLY, + applies_on: PRINT_ON_LINUX, runner: None, }, - // WSL2 recipes. All print-only: every one of them either installs a package - // with sudo, edits loader configuration, or belongs to the Windows host, and - // none of that meets the bar the four auto-applicable fixes clear (small, - // reversible, user-scoped, verifiable in one line). FixRecipe { fix_id: "fix-wsl-1-gpu-not-exposed", title: "Expose the GPU to the WSL distro (/dev/dxg)", rationale: "WSL reaches the GPU through /dev/dxg, provided by the Windows host driver via GPU-PV. Without that device nothing else in the ROCm stack can work, so this comes before any package or loader question. In a container the device has to be passed in explicitly; on a host it means the Windows driver or the WSL kernel needs attention.", - auto_applicable: false, commands: &[ "# In a container, pass the device and the WSL libraries in:", "# --device=/dev/dxg -v /usr/lib/wsl:/usr/lib/wsl", @@ -465,14 +580,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "A container running on WSL2 reports itself as WSL but sees /dev/dxg only when it was started with the device. Check that before touching the Windows driver.", ], - applies_on: WSL_ONLY, + applies_on: PRINT_ON_WSL, runner: None, }, FixRecipe { fix_id: "fix-wsl-2-dxcore-missing", title: "Restore the WSL DXCore libraries", rationale: "/usr/lib/wsl/lib holds the DXCore shims the ROCm runtime uses to talk to the Windows host driver. WSL mounts that directory itself, so a distro package manager can neither install nor repair it -- the fix is on the Windows side, plus a loader-path entry inside the distro.", - auto_applicable: false, commands: &[ "# From Windows, refresh the WSL runtime that provides these libraries:", "# wsl --update", @@ -486,14 +600,13 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "ls -l /usr/lib/wsl/lib/libdxcore.so && ldconfig -p | grep libdxcore", notes: &["apt cannot repair /usr/lib/wsl: it is a mount supplied by WSL, not a package."], - applies_on: WSL_ONLY, + applies_on: PRINT_ON_WSL, runner: None, }, FixRecipe { fix_id: "fix-wsl-3-rocdxg-missing", title: "Install ROCDXG in the WSL distro", rationale: "ROCDXG (librocdxg) is the ROCm-to-DXCore shim the WSL path runs on. It is a distro-side package, so unlike the driver and DXCore pieces this one is entirely in the user's hands.", - auto_applicable: false, commands: &[ "rocm install driver", "# Then, once the plan looks right:", @@ -507,14 +620,13 @@ const RECIPES: &[FixRecipe] = &[ "Print-only on purpose: this downloads a .deb from a release page and installs it with sudo. `rocm install driver` prints the plan first so the URL and the package are reviewable before anything runs.", "The download is checked against a digest pinned for that ROCDXG release. To install a release rocm-cli has no digest for, set ROCM_CLI_ROCDXG_SHA256 to the one published with it.", ], - applies_on: WSL_ONLY, + applies_on: PRINT_ON_WSL, runner: None, }, FixRecipe { fix_id: "fix-wsl-4-rocdxg-not-linked", title: "Refresh the linker cache so ROCDXG is loadable", rationale: "librocdxg is installed but absent from the linker cache, so the runtime will not find it at load time. Usually a missed `ldconfig` after a manual install.", - auto_applicable: false, commands: &["sudo ldconfig"], needs_sudo: true, needs_reboot: false, @@ -523,14 +635,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "If ldconfig alone does not do it, the library landed outside the linker's search path: add that directory under /etc/ld.so.conf.d/ and re-run.", ], - applies_on: WSL_ONLY, + applies_on: PRINT_ON_WSL, runner: None, }, FixRecipe { fix_id: "fix-wsl-5-distro-too-old", title: "Move to a distro release the WSL path supports", rationale: "Ubuntu 22.04 ships glibc 2.35, below the glibc 2.38 / GLIBCXX_3.4.32 floor every published Lemonade embeddable is linked against, so the engine cannot start there at all. This is a hard floor, not a recommendation.", - auto_applicable: false, commands: &[ "# From Windows, install a supported distro alongside the current one:", "# wsl --install -d Ubuntu-24.04", @@ -542,14 +653,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "Distros install side by side, so the current one can stay until the new one is set up.", ], - applies_on: WSL_ONLY, + applies_on: PRINT_ON_WSL, runner: None, }, FixRecipe { fix_id: "fix-wsl-6-host-driver-too-old", title: "Update the AMD driver on the Windows host", rationale: "Under WSL the GPU kernel-mode driver lives on the Windows host, not in the distro. When the distro-side plumbing is complete and ROCm still sees no GPU, the host driver is the remaining variable.", - auto_applicable: false, commands: &[ "# On the Windows host, not in this distro:", "# install a WSL-capable AMD Adrenalin driver, then `wsl --shutdown`.", @@ -562,14 +672,13 @@ const RECIPES: &[FixRecipe] = &[ "Nothing inside the distro can carry this out, which is why it prints rather than runs.", "The ROCm release and the Adrenalin release are paired; check the WSL install guide for the version that matches your ROCm.", ], - applies_on: WSL_ONLY, + applies_on: PRINT_ON_WSL, runner: None, }, FixRecipe { fix_id: "fix-wsl-7-wsl1", title: "Convert the distro from WSL 1 to WSL 2", rationale: "WSL 1 translates syscalls rather than running a kernel, and exposes no GPU device at all. No driver or package work can give it ROCm support; the distro has to be converted.", - auto_applicable: false, commands: &[ "# From Windows PowerShell:", "# wsl --set-version 2", @@ -582,14 +691,13 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "Converting rewrites the distro's filesystem and can take a long time on a large install. Back up anything you cannot lose first.", ], - applies_on: WSL_ONLY, + applies_on: PRINT_ON_WSL, runner: None, }, FixRecipe { fix_id: "fix-19-shm-too-small", title: "Raise the shared memory allowance", rationale: "A serving workload needs gigabytes of /dev/shm; a container gives it 64 MB by default, and WSL2 ships the same default. When the allowance runs out the workload crashes without the message ever naming shared memory -- a data-loader worker killed by a bus error, or a failed write to a temporary file -- so there is no route from what the user sees back to the cause.", - auto_applicable: false, // Two situations, one cause. A running container cannot be resized, so // the container case is a restart rather than a command that changes // this machine; the host case is a remount plus the fstab line that @@ -612,9 +720,10 @@ const RECIPES: &[FixRecipe] = &[ "This is reported below 1 GiB. Silence is not proof of enough: a container given 2 GiB clears that bar and can still be too small for a large model.", "8g matches what fix-10-container already tells you to pass, so the two stay consistent.", ], - // Not `LINUX_ONLY`: the size of a tmpfs has nothing to do with the + // Not `PRINT_ON_LINUX`: the size of a tmpfs has nothing to do with the // amdgpu module, and WSL2 ships the same 64 MB default a container does. - applies_on: LINUX_AND_WSL, + // Print-only on both because every remedy above needs sudo. + applies_on: PRINT_ON_LINUX_AND_WSL, runner: None, }, ]; @@ -765,14 +874,40 @@ pub(crate) fn torch_rocm_indexes_named_in<'a>( .collect() } -/// The platform family a recipe's `applies_on` is matched against. +/// What the catalog says `rocm fix ` does on `os`. /// -/// WSL2 is its own family rather than `linux`, mirroring `diagnose`. That is what -/// makes `rocm fix fix-4-render-group` on a WSL host refuse with "wrong OS" -/// instead of running `usermod` for a group that governs nothing there — and it -/// is why recipes valid on both platforms have to name `wsl` explicitly. +/// `None` when the id is not in the catalog, or when it is but does not apply +/// on that operating system at all. /// -/// Not `const fn`: unlike the OS, WSL has to be probed at runtime. +/// This exists so `diagnose` can read the class off the catalog instead of +/// restating it. The two were hand-maintained copies, and they had already +/// drifted: `fix-9-igpu-dgpu` was `auto_applicable: true` here and `false` in +/// the Linux arm of its checker, with no test comparing them. +#[must_use] +pub fn class_on(fix_id: &str, os: &str) -> Option { + find_recipe(fix_id) + .and_then(|r| r.platform(os)) + .map(|p| p.class) +} + +/// The argument a [`FixClass::NeedsArgument`] entry is waiting for on `os`. +/// +/// Read alongside [`class_on`] for the same reason: `format_flags` names the +/// argument, and `diagnose` renders those flags too. Without this, the same +/// entry would read "needs --device-index" from `rocm fix` and "needs an +/// argument" from `rocm diagnose`, which is the divergence `format_flags` +/// exists to prevent. +#[must_use] +pub fn needs_on(fix_id: &str, os: &str) -> Option<&'static str> { + find_recipe(fix_id) + .and_then(|r| r.platform(os)) + .and_then(|p| p.needs) +} + +/// The platform family a recipe is selected by. +/// +/// WSL is its own family, which is why recipes valid on both have to name `wsl` +/// explicitly. Not `const fn`: unlike the OS, WSL has to be probed at runtime. fn current_os() -> &'static str { if runtime_is_windows() { "windows" @@ -795,37 +930,25 @@ fn looks_like_a_diagnosis_position(value: &str) -> bool { !digits.is_empty() && digits.chars().all(|c| c.is_ascii_digit()) } -/// Whether the CLI will apply `fix_id` itself, or `None` if it isn't a known -/// fix. `RECIPES` is the authority: `apply()` dispatches on it, so this is the -/// value any other surface describing a fix has to agree with. +/// List every fix-id (id, class on this machine, OS scope, title). /// -/// Test-only: the one production consumer is `apply()`, which reads the recipe -/// directly. This exists so `diagnose`'s tests can assert the two surfaces -/// agree without exposing `RECIPES`. -#[cfg(test)] -pub(crate) fn auto_applicable_for(fix_id: &str) -> Option { - find_recipe(fix_id).map(|r| r.auto_applicable) -} - -/// List every fix-id (id, kind, OS scope, title). +/// The marker is what the entry does **here**, not everywhere: `fix-2` reports +/// on Linux and applies itself on Windows, and a listing that averaged the two +/// would be wrong on both. The question a reader is asking is what happens if +/// they run it on the machine in front of them. #[must_use] pub fn list_recipes() -> String { use std::fmt::Write as _; let mut out = String::from("Available fix-ids (mirror the diagnosis catalog):\n"); // The markers were printed with nothing saying what they mean. - out.push_str( - " AUTO = `rocm fix ` can carry it out; PRINT-ONLY = it prints the steps for you to run.\n", - ); + out.push_str(" On this machine: AUTO = `rocm fix ` carries it out; NEEDS-ARG = it does once you supply an argument;\n"); + out.push_str(" PRINT-ONLY = it prints the steps for you to run; DIAGNOSE-ONLY = it explains the problem and applies nothing.\n"); for r in RECIPES { - let kind = if r.auto_applicable { - "AUTO" - } else { - "PRINT-ONLY" - }; - let scope = r.applies_on.join("/"); + let kind = r.class_here().marker(); + let scope = r.os_scope().join("/"); let _ = writeln!( out, - " [{kind:>10}] [{scope:>14}] {} -- {}", + " [{kind:>13}] [{scope:>14}] {} -- {}", r.fix_id, r.title ); } @@ -833,16 +956,24 @@ pub fn list_recipes() -> String { } /// Canonical wording for a fix's remediation flags, shared by `rocm fix ` -/// and `rocm diagnose` so the same `(sudo, reboot, relogin, auto_applicable)` -/// values render as the same text from either command. This only -/// standardizes wording, not the underlying values: `FixRecipe` (fix.rs) and -/// diagnose's `Fix` still supply those independently, so a fix-id's rendered -/// flags can still differ if the two disagree on a value; `assert_needs_reboot_matches_the_catalog` -/// and `assert_plan_matches_the_catalog_copy` are targeted regression tests -/// that pin specific fix-ids against that drift, not a blanket guarantee for -/// every fix-id. Also out of scope: the bare `rocm fix` catalog listing -/// (`list_recipes`) describes the same `auto_applicable` property with a -/// separate, untouched AUTO/PRINT-ONLY vocabulary. +/// and `rocm diagnose` so the same `(sudo, reboot, relogin, class)` values +/// render as the same text from either command. This only standardizes wording, +/// not the underlying values: `FixRecipe` (fix.rs) and diagnose's `Fix` still +/// supply those independently, so a fix-id's rendered flags can still differ if +/// the two disagree on a value; `assert_needs_reboot_matches_the_catalog` and +/// `assert_plan_matches_the_catalog_copy` are targeted regression tests that +/// pin specific fix-ids against that drift, not a blanket guarantee for every +/// fix-id. Also out of scope: the bare `rocm fix` catalog listing +/// (`list_recipes`), which describes the same property with the shorter +/// AUTO/NEEDS-ARG/PRINT-ONLY/DIAGNOSE-ONLY markers. +/// +/// `class` replaced a bool. The bool could say "the CLI will run this" or "it +/// will not", and three of the four answers here are "not, but for different +/// reasons" -- which is the distinction this wording exists to carry. `needs` +/// names the argument a [`FixClass::NeedsArgument`] entry is waiting for, and is +/// ignored for every other class. +/// +/// Returns owned strings because that argument name is not a literal. // These mirror the `FixRecipe`/`Fix` struct fields, where // `clippy::struct_excessive_bools` is already allowed workspace-wide; that // allow doesn't reach this free function's parameters, so @@ -852,29 +983,39 @@ pub(crate) fn format_flags( needs_sudo: bool, needs_reboot: bool, needs_relogin: bool, - auto_applicable: bool, -) -> Vec<&'static str> { + class: FixClass, + needs: Option<&str>, +) -> Vec { let mut flags = Vec::new(); if needs_sudo { - flags.push("requires sudo"); + flags.push("requires sudo".to_owned()); } if needs_reboot { - flags.push("requires reboot"); + flags.push("requires reboot".to_owned()); } if needs_relogin { - flags.push("requires re-login"); - } - flags.push(if auto_applicable { - "rocm fix can run it" - } else { - "manual only (`rocm fix` will NOT run it automatically)" + flags.push("requires re-login".to_owned()); + } + flags.push(match class { + FixClass::Auto => "rocm fix can run it".to_owned(), + // Named rather than described as "manual only": the CLI *will* act, just + // not until it is told what to act on. Calling that manual sends the + // user off to run the steps by hand for no reason. + FixClass::NeedsArgument => format!( + "needs {} before `rocm fix` will run it", + needs.unwrap_or("an argument") + ), + FixClass::PrintOnly => "manual only (`rocm fix` will NOT run it automatically)".to_owned(), + FixClass::DiagnoseOnly => { + "no reliable fix (`rocm fix` will NOT change anything)".to_owned() + } }); flags } fn print_recipe(r: &FixRecipe) { println!("Fix: {} -- {}", r.fix_id, r.title); - println!("OS scope: {}", r.applies_on.join(", ")); + println!("OS scope: {}", r.os_scope().join(", ")); println!("Rationale: {}", r.rationale); if !r.commands.is_empty() { println!("Commands:"); @@ -886,7 +1027,8 @@ fn print_recipe(r: &FixRecipe) { r.needs_sudo, r.needs_reboot, r.needs_relogin, - r.auto_applicable, + r.class_here(), + r.platform(current_os()).and_then(|p| p.needs), ); println!("Flags: {}", flags.join(", ")); for n in r.notes { @@ -916,32 +1058,103 @@ pub fn apply(fix_id: &str, opts: &FixOptions) -> i32 { } return 2; }; + act_on(recipe, opts) +} + +/// What `rocm fix ` will do with an entry on this host. +/// +/// Every decision lives in [`plan_of`] and every message lives in [`act_on`], so +/// the two can be checked apart. The split exists because several of these end +/// in exit code 0: a test that watched only the code could not tell +/// [`Plan::Unfixable`] from [`Plan::PrintSteps`], and the difference matters — +/// the second tells the user to copy commands, which an unfixable entry does not +/// have. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Plan { + /// The entry does not apply on the running OS. + WrongMachine, + /// No reliable fix exists. Name the problem and change nothing. + Unfixable, + /// Hand off to the recipe's runner. + Run, + /// The catalog says the CLI carries this out, and nothing can. + MissingRunner, + /// Print the steps for the user to carry out. + PrintSteps, +} + +/// Decide what to do with `recipe` here. No output, no side effects. +fn plan_of(recipe: &FixRecipe) -> Plan { + let Some(platform) = recipe.platform(current_os()) else { + return Plan::WrongMachine; + }; + if platform.class == FixClass::DiagnoseOnly { + return Plan::Unfixable; + } + // The class says what happens to the *machine*; it does not say whether a + // runner runs. `fix-2-unset-override` is print-only on Linux and still has + // real work to do there -- it reads the live value and scans five rc files + // to name the ones that set it. Gating the runner on the class would throw + // that away and print a generic "copy the commands above" in its place. + if recipe.runner.is_some() { + return Plan::Run; + } + if platform.class.applies_itself() { + return Plan::MissingRunner; + } + Plan::PrintSteps +} + +/// Everything `apply` does once the id has resolved to a recipe. +/// +/// Split out from [`apply`] so the class gate can be reached with a recipe the +/// catalog does not contain. [`FixClass::DiagnoseOnly`] has no member today, so +/// without this seam its branch is unreachable from any test and would sit +/// unexercised until the first unfixable problem turns up. +#[must_use] +fn act_on(recipe: &FixRecipe, opts: &FixOptions) -> i32 { print_recipe(recipe); println!(); - let os = current_os(); - if !recipe.applies_on.contains(&os) { - fail!( - "This fix only applies on: {}. Running OS is: {os}.", - recipe.applies_on.join(", ") - ); - return 3; - } - if !recipe.auto_applicable { - println!("This fix is print-only (manual change required)."); - println!("Copy the commands above, run them yourself, then verify with:"); - if !recipe.verify.is_empty() { - println!(" $ {}", recipe.verify); + // Exhaustive on purpose. An arm removed here is a compile error rather than + // a silent fall-through to the next branch, which is how a disabled guard + // would otherwise keep returning the same exit code and pass its own test. + match plan_of(recipe) { + Plan::WrongMachine => { + fail!( + "This fix only applies on: {}. Running OS is: {}.", + recipe.os_scope().join(", "), + current_os() + ); + 3 + } + Plan::Unfixable => { + // Not an error, so not a nonzero code: the command did everything it + // could, which is name the problem. Exit 3 would say "not applicable + // *here*", a different claim that would send a caller looking for + // another machine to run it on. + println!("This problem has no reliable fix, so this command changes nothing."); + println!("The explanation above is the whole of what is known about it."); + 0 + } + Plan::Run => recipe + .runner + .expect("plan_of returns Run only when a runner is present")(opts), + Plan::MissingRunner => { + // A recipe the catalog says the CLI carries out, with nothing to + // carry it out with -> 1, not 4 (4 is reserved for "attempted but + // the command failed"). Held by `a_recipe_that_acts_has_a_runner`. + fail!("Internal error: a recipe the catalog says is applied here has no runner."); + 1 + } + Plan::PrintSteps => { + println!("This fix is print-only (manual change required)."); + println!("Copy the commands above, run them yourself, then verify with:"); + if !recipe.verify.is_empty() { + println!(" $ {}", recipe.verify); + } + 0 } - return 0; - } - if let Some(runner) = recipe.runner { - runner(opts) - } else { - // Internal error (auto-applicable recipe with no runner) -> 1, not 4 - // (4 is reserved for "attempted but the command failed"). - fail!("Internal error: auto-applicable recipe has no runner."); - 1 } } @@ -1565,6 +1778,15 @@ mod tests { } } + /// Whether a recipe applies on the platform the test is running on. + /// + /// Gating on `runtime_is_linux()` stopped being the same question once WSL + /// became its own family: a WSL host is Linux, but a Linux-scoped recipe is + /// correctly refused there. + fn recipe_applies_here(fix_id: &str) -> bool { + find_recipe(fix_id).is_some_and(|r| r.platform(current_os()).is_some()) + } + /// Plant a directory the shared resolver will accept as a ROCm install. /// `bin/rocminfo` is one of the markers it gates on; a bare directory is /// deliberately not enough. @@ -1759,6 +1981,116 @@ mod tests { ); } + /// A recipe that exists only here, to reach a class the catalog has no + /// member of. + fn unfixable_recipe(class: FixClass) -> FixRecipe { + FixRecipe { + fix_id: "fix-0-not-in-the-catalog", + title: "A problem that can be named but not repaired", + rationale: "Synthetic. Exists to drive the class gate in `act_on`.", + commands: &[], + needs_sudo: false, + needs_reboot: false, + needs_relogin: false, + verify: "", + notes: &[], + applies_on: Box::leak(Box::new([on(current_os(), class)])), + runner: None, + } + } + + /// `DIAGNOSE-ONLY` reports success, because naming the problem *is* the + /// whole job. + /// + /// The distinction this pins is 0 against 3. Exit 3 is "not applicable on + /// this machine", which would send a caller looking for a different machine + /// to run the fix on; there is no such machine, because there is no fix. The + /// catalog has no `DIAGNOSE-ONLY` entry yet, so nothing else in the suite + /// reaches this branch. + #[test] + fn a_problem_with_no_reliable_fix_reports_success_and_does_not_look_portable() { + let opts = FixOptions::default(); + assert_eq!( + act_on(&unfixable_recipe(FixClass::DiagnoseOnly), &opts), + 0, + "an unfixable problem is not an error and is not a wrong-machine result" + ); + // The code alone does not pin the branch: print-only exits 0 as well, so + // an assertion on the code would survive the unfixable arm being + // disabled, and the user would be told to copy commands that do not + // exist. This is the part that only holds while the arm is reached. + assert_eq!( + plan_of(&unfixable_recipe(FixClass::DiagnoseOnly)), + Plan::Unfixable, + "DIAGNOSE-ONLY must not fall through to the print-the-steps path" + ); + for actionable in [FixClass::Auto, FixClass::NeedsArgument, FixClass::PrintOnly] { + assert_ne!( + plan_of(&unfixable_recipe(actionable)), + Plan::Unfixable, + "{actionable:?} has something to offer and must not be treated as unfixable" + ); + } + } + + /// The same recipe, reclassified, takes a different path — so the assertion + /// above is about the class and not about the synthetic recipe's emptiness. + /// + /// Without this pairing, `act_on` could ignore the class entirely and both + /// this test and the one above would still pass: a runner-less print-only + /// recipe also returns 0. What separates them is the OS gate, which a + /// recipe scoped to another platform fails. + #[test] + fn the_class_gate_is_reached_only_after_the_platform_gate() { + let opts = FixOptions::default(); + let elsewhere = FixRecipe { + applies_on: Box::leak(Box::new([on( + if current_os() == "windows" { + "linux" + } else { + "windows" + }, + FixClass::DiagnoseOnly, + )])), + ..unfixable_recipe(FixClass::DiagnoseOnly) + }; + assert_eq!( + act_on(&elsewhere, &opts), + 3, + "a recipe that does not apply here is a wrong-machine result, whatever its class" + ); + } + + /// No entry is `DIAGNOSE-ONLY` on any platform. + /// + /// The class ships with no member on purpose: it is what lets a + /// detect-but-cannot-repair failure be accepted as an entry at all. It ships + /// now rather than when the first member arrives because `rocm diagnose + /// --json` already serialises `class`, and a consumer that deserialises + /// that output into a fixed enum today would reject a payload carrying a + /// variant added later -- shipping the full set from the start means that + /// variant is already something callers have to tolerate. This test is the + /// tripwire — when it fails, the first real member has arrived, and the + /// scenario that was waiting for one becomes demonstrable against it rather + /// than staying provisional. + #[test] + fn the_catalog_has_no_diagnose_only_entry_yet() { + let members: Vec<&str> = RECIPES + .iter() + .filter(|r| { + r.applies_on + .iter() + .any(|p| p.class == FixClass::DiagnoseOnly) + }) + .map(|r| r.fix_id) + .collect(); + assert!( + members.is_empty(), + "{members:?} now use DIAGNOSE-ONLY. That is the class working as intended -- update \ + this test, and demonstrate the scenario that was waiting for a member." + ); + } + #[test] fn every_recipe_id_is_unique_and_covers_the_catalog() { let mut ids: Vec<&str> = RECIPES.iter().map(|r| r.fix_id).collect(); @@ -1771,13 +2103,10 @@ mod tests { assert_eq!(count, 24, "expected 24 catalog entries"); } - #[test] - fn the_dlpack_recipe_says_which_shell_each_step_runs_in() { - let recipe = - find_recipe("fix-17-torch-dlpack").expect("fix-17-torch-dlpack must be in the catalog"); - assert_engine_shell_boundary_is_labelled(recipe.fix_id, recipe.commands); - } - + /// Restored, not new. This PR made the `wsl` arm of the platform lookup + /// carry more weight, not less -- it now drives the per-platform class, the + /// plan, the recipe printer and the listing -- so the guard tying that arm + /// to the real host detector matters more here than it did before. #[test] fn current_os_reports_wsl_exactly_when_is_wsl_host_does() { // `current_os()`'s wsl branch is `crate::is_wsl_host()`, which is @@ -1796,45 +2125,101 @@ mod tests { ); } - /// Whether a recipe applies on the platform the test is running on. - /// - /// Tests used to gate on `runtime_is_linux()`, which stopped being the same - /// question once WSL became its own family: a WSL host is Linux, but a - /// `LINUX_ONLY` recipe is correctly refused there. - fn recipe_applies_here(fix_id: &str) -> bool { - find_recipe(fix_id).is_some_and(|r| r.applies_on.contains(¤t_os())) + #[test] + fn the_dlpack_recipe_says_which_shell_each_step_runs_in() { + let recipe = + find_recipe("fix-17-torch-dlpack").expect("fix-17-torch-dlpack must be in the catalog"); + assert_engine_shell_boundary_is_labelled(recipe.fix_id, recipe.commands); } + /// Every platform class a recipe carries, as `(fix-id, os, class)`. + fn classes() -> Vec<(&'static str, &'static str, FixClass)> { + RECIPES + .iter() + .flat_map(|r| r.applies_on.iter().map(|p| (r.fix_id, p.os, p.class))) + .collect() + } + + /// One direction only, on purpose. A recipe the catalog says the CLI + /// carries out somewhere must have something to carry it out with, or + /// `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 auto_applicable_recipes_have_a_runner() { + fn a_recipe_that_acts_has_a_runner() { for r in RECIPES { - assert_eq!( - r.auto_applicable, - r.runner.is_some(), - "{}: auto_applicable must match presence of a runner", + let acts_somewhere = r + .applies_on + .iter() + .any(|p| matches!(p.class, FixClass::Auto | FixClass::NeedsArgument)); + assert!( + !acts_somewhere || r.runner.is_some(), + "{}: the catalog says the CLI carries this out on some platform, \ + but there is no runner to do it", r.fix_id ); } } #[test] - fn exactly_the_four_known_fixes_are_auto() { - let auto: Vec<&str> = RECIPES - .iter() - .filter(|r| r.auto_applicable) - .map(|r| r.fix_id) + fn a_needs_argument_platform_names_the_argument() { + for (fix_id, os, class) in classes() { + let needs = RECIPES + .iter() + .find(|r| r.fix_id == fix_id) + .and_then(|r| r.platform(os)) + .and_then(|p| p.needs); + assert_eq!( + class == FixClass::NeedsArgument, + needs.is_some(), + "{fix_id} on {os}: the argument is the whole difference between this \ + class and AUTO, so it has to be named -- and nothing else may name one" + ); + } + } + + /// Pinned exactly, per platform. Promoting an entry to `AUTO` starts + /// changing machines that callers were told it only ever advised on, and + /// the two corrections below were both silent precisely because a flat + /// `bool` could not record the platform the claim was true on. + #[test] + fn only_these_entries_act_on_the_machine_and_only_on_these_platforms() { + let acts: Vec<(&str, &str, FixClass)> = classes() + .into_iter() + .filter(|(_, _, c)| matches!(c, FixClass::Auto | FixClass::NeedsArgument)) .collect(); assert_eq!( - auto, + acts, vec![ - "fix-2-unset-override", - "fix-4-render-group", - "fix-6-path", - "fix-9-igpu-dgpu" + // Linux reports and returns; only the Windows arm mutates. + ("fix-2-unset-override", "windows", FixClass::Auto), + ("fix-4-render-group", "linux", FixClass::Auto), + ("fix-6-path", "linux", FixClass::Auto), + ("fix-6-path", "windows", FixClass::Auto), + ("fix-6-path", "wsl", FixClass::Auto), + // Both arms short-circuit until `--device-index` names a target. + ("fix-9-igpu-dgpu", "linux", FixClass::NeedsArgument), + ("fix-9-igpu-dgpu", "windows", FixClass::NeedsArgument), ] ); } + #[test] + fn no_entry_claims_a_platform_twice() { + for r in RECIPES { + let mut seen: Vec<&str> = r.applies_on.iter().map(|p| p.os).collect(); + let count = seen.len(); + seen.sort_unstable(); + seen.dedup(); + assert_eq!( + seen.len(), + count, + "{}: a platform listed twice makes `platform()` order-dependent", + r.fix_id + ); + } + } + #[test] fn unknown_fix_id_returns_2() { let code = apply("fix-does-not-exist", &FixOptions::default()); @@ -1843,7 +2228,7 @@ mod tests { #[test] fn dry_run_never_mutates_and_returns_zero_for_auto_linux_fix() { - if !runtime_is_linux() { + if !recipe_applies_here("fix-2-unset-override") { return; } // fix-2 unset-override is print-only on linux (no mutation regardless); @@ -1858,16 +2243,15 @@ mod tests { #[test] fn fix_9_without_device_index_is_print_only_and_returns_zero() { + if !recipe_applies_here("fix-9-igpu-dgpu") { + // Refused at the OS gate here (WSL is its own family), which is a + // different assertion -- covered by the inapplicable-recipe test. + return; + } // Regression: the missing `--device-index` branch only prints the // query that identifies the dGPU, so it is a print-only preview and // must return 0 -- not the environment/OS code 3. A dry-run without the // argument must likewise succeed, since the runner never mutates. - // - // fix-9 does not apply on WSL (no per-device topology to collide over), - // where the correct answer is the OS refusal this test exists to rule out. - if !recipe_applies_here("fix-9-igpu-dgpu") { - return; - } for dry_run in [false, true] { let opts = FixOptions { dry_run, @@ -1885,11 +2269,13 @@ mod tests { fn print_only_fix_returns_zero() { // Pick a recipe that applies on THIS platform rather than naming a Linux // one: the assertion is about print-only recipes succeeding, and hunting - // for an applicable one keeps that meaningful on every lane instead of - // skipping wherever the hardcoded id happens not to apply. + // for an applicable one keeps it meaningful on every lane. let fix_id = RECIPES .iter() - .find(|r| !r.auto_applicable && r.applies_on.contains(¤t_os())) + .find(|r| { + r.platform(current_os()) + .is_some_and(|p| p.class == FixClass::PrintOnly) + }) .map(|r| r.fix_id) .expect("every supported platform has at least one print-only recipe"); let code = apply(fix_id, &FixOptions::default()); @@ -1960,39 +2346,62 @@ mod tests { } #[test] - fn format_flags_covers_every_flag_combination_and_both_auto_states() { - // Exhaustive over all 2^4 = 16 combinations of the 3 optional flags - // (sudo/reboot/relogin) x both auto_applicable states, so a wording - // regression on any one flag, or on the always-present auto/manual - // marker, fails here rather than only being visible by eyeballing - // `rocm fix`/`rocm diagnose` output. - for bits in 0..16u8 { + fn format_flags_covers_every_flag_combination_and_every_class() { + // Exhaustive over all 2^3 combinations of the optional flags + // (sudo/reboot/relogin) x all four classes, so a wording regression on + // any one flag, or on the always-present class marker, fails here + // rather than only being visible by eyeballing `rocm fix`/`rocm + // diagnose` output. The class replaced a bool: three of the four + // answers are "the CLI will not run it", for different reasons, and a + // bool could only say one of them. + for bits in 0..8u8 { let needs_sudo = bits & 1 != 0; let needs_reboot = bits & 2 != 0; let needs_relogin = bits & 4 != 0; - let auto_applicable = bits & 8 != 0; - - let mut expected = Vec::new(); - if needs_sudo { - expected.push("requires sudo"); - } - if needs_reboot { - expected.push("requires reboot"); + for class in [ + FixClass::Auto, + FixClass::NeedsArgument, + FixClass::PrintOnly, + FixClass::DiagnoseOnly, + ] { + let mut expected: Vec = Vec::new(); + if needs_sudo { + expected.push("requires sudo".to_owned()); + } + if needs_reboot { + expected.push("requires reboot".to_owned()); + } + if needs_relogin { + expected.push("requires re-login".to_owned()); + } + expected.push( + match class { + FixClass::Auto => "rocm fix can run it", + FixClass::NeedsArgument => { + "needs --device-index before `rocm fix` will run it" + } + FixClass::PrintOnly => { + "manual only (`rocm fix` will NOT run it automatically)" + } + FixClass::DiagnoseOnly => { + "no reliable fix (`rocm fix` will NOT change anything)" + } + } + .to_owned(), + ); + + let flags = format_flags( + needs_sudo, + needs_reboot, + needs_relogin, + class, + Some("--device-index"), + ); + assert_eq!( + flags, expected, + "sudo={needs_sudo} reboot={needs_reboot} relogin={needs_relogin} class={class:?}" + ); } - if needs_relogin { - expected.push("requires re-login"); - } - expected.push(if auto_applicable { - "rocm fix can run it" - } else { - "manual only (`rocm fix` will NOT run it automatically)" - }); - - let flags = format_flags(needs_sudo, needs_reboot, needs_relogin, auto_applicable); - assert_eq!( - flags, expected, - "sudo={needs_sudo} reboot={needs_reboot} relogin={needs_relogin} auto={auto_applicable}" - ); } } @@ -2282,15 +2691,68 @@ mod tests { #[test] fn exit_code_1_is_unreachable_by_construction() { - // apply() returns 1 only for an auto_applicable recipe with no runner. - // `auto_applicable_recipes_have_a_runner` keeps that impossible; assert - // the reachability argument here so reference.md's row 1 is accounted - // for rather than silently untested. + // apply() returns 1 only when a platform's class is Auto and the recipe + // has no runner (`Plan::MissingRunner`). `a_recipe_that_acts_has_a_runner` + // keeps that impossible; assert the reachability argument here so + // reference.md's row 1 is accounted for rather than silently untested. assert!( - RECIPES - .iter() - .all(|r| !r.auto_applicable || r.runner.is_some()), + RECIPES.iter().all(|r| { + !r.applies_on.iter().any(|p| p.class.applies_itself()) || r.runner.is_some() + }), "an auto-applicable recipe without a runner would make exit 1 reachable" ); } + + /// The four classes never render the same sentence. + /// + /// The bool this replaced could say two things, and the reason for keeping + /// four is that a user acts differently on each: wait for the CLI, supply an + /// argument, run the steps by hand, or stop looking for a fix. Two classes + /// that read alike would put us back where we started with a different type. + #[test] + fn every_class_says_something_different_about_what_will_happen() { + let rendered: Vec = [ + FixClass::Auto, + FixClass::NeedsArgument, + FixClass::PrintOnly, + FixClass::DiagnoseOnly, + ] + .into_iter() + .map(|c| { + format_flags(false, false, false, c, Some("--device-index")) + .pop() + .expect("format_flags always ends with the class marker") + }) + .collect(); + let mut unique = rendered.clone(); + unique.sort_unstable(); + unique.dedup(); + assert_eq!( + unique.len(), + rendered.len(), + "two classes render the same flag text, so a reader cannot tell them apart: {rendered:?}" + ); + } + + /// `rocm fix` and `rocm diagnose` name the same argument for the same entry. + /// + /// They render the flags line from two call sites, which is why + /// `format_flags` is shared at all. Passing `None` from one of them would + /// degrade its text to "an argument" while the other named the flag, and + /// nothing else in the suite compares the two. + #[test] + fn a_needs_argument_entry_names_the_argument_rather_than_describing_it() { + let named = format_flags( + false, + false, + false, + FixClass::NeedsArgument, + needs_on("fix-9-igpu-dgpu", "linux"), + ); + assert!( + named.iter().any(|f| f.contains("--device-index")), + "the catalog knows which argument fix-9-igpu-dgpu waits for, so the flags line \ + has to say it: {named:?}" + ); + } } diff --git a/docs/wsl.md b/docs/wsl.md index da9974fe7..ee3c279ce 100644 --- a/docs/wsl.md +++ b/docs/wsl.md @@ -209,8 +209,9 @@ larger and adding the matching `/etc/fstab` line inside the distro. Every WSL remedy is print-only. `rocm fix ` shows the commands and does not run them: they either install packages with `sudo`, edit loader configuration, or -belong to the Windows host, and none of that meets the bar the four -auto-applicable fixes clear. +belong to the Windows host, and none of that meets the bar an auto-applied fix +has to clear. On WSL the CLI carries out exactly one catalog entry itself, +`fix-6-path`, which is not one of the WSL entries. Two deliberate silences, so a report can be trusted: diff --git a/skills/rocm-doctor/reference.md b/skills/rocm-doctor/reference.md index be7179abb..d805ffee2 100644 --- a/skills/rocm-doctor/reference.md +++ b/skills/rocm-doctor/reference.md @@ -49,15 +49,25 @@ Apply a fix by id (run with no id to list). Exit codes: | 4 | attempted but the command failed | | 5 | user declined at the prompt | -Only four fixes are auto-applicable — `fix-2-unset-override`, -`fix-4-render-group`, `fix-6-path`, `fix-9-igpu-dgpu` — and the rest print their -plan for the user to run. Pass the **full** id (`rocm fix fix-2-unset-override`, -not `rocm fix fix-2`; a short id returns exit 2, unknown fix-id). - -**Auto-fix** in the catalog below means only what the CLI reports in -`rocm fix`'s listing: the CLI has a runner for it and will carry it out itself, -rather than printing a plan for the user. It is not a promise that the runner -mutates anything. +Three ids are ever auto-applicable — never four, and `fix-9-igpu-dgpu` is not +one of them (see below) — and which of the three depends on the **host**, +because `rocm fix`'s listing marks each entry for the machine you run it on, +never for every machine the id applies to: + +- **linux** — auto-applicable: `fix-4-render-group`, `fix-6-path`. +- **windows** — auto-applicable: `fix-2-unset-override`, `fix-6-path`. +- **wsl** — auto-applicable: `fix-6-path`. + +The rest print their plan for the user to run. Pass the **full** id +(`rocm fix fix-2-unset-override`, not `rocm fix fix-2`; a short id returns +exit 2, unknown fix-id). + +**Marker** in the catalog below is what `rocm fix`'s listing reports **on +bare-metal Linux** — AUTO means the CLI has a runner for the entry and will +carry it out itself there, rather than printing a plan for the user. It is not +a promise that the runner mutates anything, and — per the per-host list above — it is +not the same claim on Windows or WSL2 for the one entry whose behaviour +depends on more than the host alone. The ones that do mutate print the exact command, honor `--dry-run`, refuse on a non-interactive shell without `--yes`, and confirm first. **Two exceptions:** @@ -67,10 +77,10 @@ non-interactive shell without `--yes`, and confirm first. **Two exceptions:** not edit your dotfiles. So on Linux it never prompts, `--dry-run` has nothing to preview, and `--yes` is never read. Do not tell a Linux user a dry run previewed a change that was never going to happen. -- **`fix-9-igpu-dgpu` mutates only when you pass `--device-index N`.** Without - it — on Linux and Windows alike — the runner just prints the - `rocminfo`/`hipInfo` query that identifies which index is the discrete GPU - and returns 0: no prompt, nothing for `--dry-run` to preview, nothing +- **`fix-9-igpu-dgpu` needs `--device-index N` wherever it applies (Linux and + Windows) and is never marked AUTO.** Without the flag the runner just prints + the `rocminfo`/`hipInfo` query that identifies which index is the discrete + GPU and returns 0: no prompt, nothing for `--dry-run` to preview, nothing pinned. Run `rocm fix fix-9-igpu-dgpu --device-index N` (not the bare id) once you know N. @@ -82,32 +92,32 @@ by naming `wsl`, so the bare-metal Linux entries (the `amdgpu` module, `/dev/kfd`, the render group) stop applying there automatically rather than reporting confident nonsense. -| id | OS | Failure mode | Typical signal | Auto-fix | +| id | OS | Failure mode | Typical signal | Marker (bare-metal Linux) | | --- | --- | --- | --- | --- | -| `fix-1-arch` | linux/windows/wsl | GPU gfx target not in the framework's build arch list | `hipErrorNoBinaryForGpu`, `HSA_STATUS_ERROR_INVALID_ISA`, "invalid device function" | no | -| `fix-2-unset-override` | linux/windows/wsl | `HSA_OVERRIDE_GFX_VERSION` set on a GPU that now has a native wheel | page faults / `OUT_OF_REGISTERS`, override set in env | yes | -| `fix-3-rocm-kernel` | linux | ROCm + distro/kernel form an unsupported triple | ROCm installed but `amdgpu` not loaded; DKMS build failure | no | -| `fix-4-render-group` | linux | User not in `render`/`video` group (or `/dev/kfd` owned by the other group) | cannot open `/dev/kfd`, permission denied | yes | -| `fix-5-amdgpu-load` | linux | `amdgpu` module not loaded (or blacklisted) | "ROCk module is NOT loaded", blacklist entry, Secure Boot | no | -| `fix-6-path` | linux/windows/wsl | ROCm/HIP binaries not on PATH after install | `rocminfo: command not found`, `hipInfo` missing from PATH | yes | -| `fix-7-stale-repos` | linux | Stale/conflicting APT/DNF repos from prior installer runs | apt 404 `repo.radeon.com`, unmet deps, ≥2 ROCm repo files | no | -| `fix-8-wheel-rocm` | linux/windows/wsl | Framework wheel built for a different ROCm major than the system | `libamdhip64.so.X` / `amdhip64_X.dll` load failure | no | -| `fix-9-igpu-dgpu` | linux/windows | iGPU enumerated alongside dGPU, destabilising the runtime | APU + discrete AMD present, `HIP_VISIBLE_DEVICES` unset, crash/segfault | yes | -| `fix-10-container` | linux | Container can't see `/dev/kfd` or `/dev/dri/renderD*` | running in docker/podman, kfd/render devices missing | no | -| `fix-11-iommu` | linux | Multi-GPU hang with IOMMU enabled | ≥2 AMD GPUs, `iommu=` not `pt`, hang/deadlock/timeout | no | -| `fix-12-installer` | linux | `amdgpu-install` left a broken DKMS / repo state | dpkg half-configured, DKMS failed, `--accept-eula` | no | -| `fix-13-hip-sdk-missing` | windows | HIP SDK not installed | no HIP SDK under Program Files, `hipInfo` not recognized | no | -| `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" | no | -| `fix-15-msvc-redist` | windows | MSVC runtime missing (HIP DLLs can't load) | `vcruntime140.dll` / `vcruntime140_1.dll` missing | no | -| `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` | no | -| `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 | no | -| `fix-wsl-1-gpu-not-exposed` | wsl | `/dev/dxg` absent, so the distro cannot reach the GPU at all | no `/dev/dxg`; in a container, the device was never passed through | no | -| `fix-wsl-2-dxcore-missing` | wsl | `/usr/lib/wsl/lib` DXCore shims missing, so the runtime cannot reach the host driver | `/usr/lib/wsl/lib/libdxcore.so` missing (or the directory absent entirely) | no | -| `fix-wsl-3-rocdxg-missing` | wsl | ROCDXG, the ROCm-to-DXCore shim the WSL path runs on, is not installed | `librocdxg.so` not found under any ROCm install | no | -| `fix-wsl-4-rocdxg-not-linked` | wsl | `librocdxg` installed but not in the linker cache, so it is unloadable | `librocdxg` present on disk yet absent from `ldconfig -p` | no | -| `fix-wsl-5-distro-too-old` | wsl | Distro release below the floor the WSL path requires | distro release under the supported floor (e.g. Ubuntu 22.04, whose glibc 2.35 is below the 2.38 / `GLIBCXX_3.4.32` the engines need) | no | -| `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 | no | -| `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 | no | +| `fix-1-arch` | linux/windows/wsl | GPU gfx target not in the framework's build arch list | `hipErrorNoBinaryForGpu`, `HSA_STATUS_ERROR_INVALID_ISA`, "invalid device function" | print-only | +| `fix-2-unset-override` | linux/windows/wsl | `HSA_OVERRIDE_GFX_VERSION` set on a GPU that now has a native wheel | page faults / `OUT_OF_REGISTERS`, override set in env | print-only (auto on windows) | +| `fix-3-rocm-kernel` | linux | ROCm + distro/kernel form an unsupported triple | ROCm installed but `amdgpu` not loaded; DKMS build failure | print-only | +| `fix-4-render-group` | linux | User not in `render`/`video` group (or `/dev/kfd` owned by the other group) | cannot open `/dev/kfd`, permission denied | auto | +| `fix-5-amdgpu-load` | linux | `amdgpu` module not loaded (or blacklisted) | "ROCk module is NOT loaded", blacklist entry, Secure Boot | print-only | +| `fix-6-path` | linux/windows/wsl | ROCm/HIP binaries not on PATH after install | `rocminfo: command not found`, `hipInfo` missing from PATH | auto | +| `fix-7-stale-repos` | linux | Stale/conflicting APT/DNF repos from prior installer runs | apt 404 `repo.radeon.com`, unmet deps, ≥2 ROCm repo files | print-only | +| `fix-8-wheel-rocm` | linux/windows/wsl | Framework wheel built for a different ROCm major than the system | `libamdhip64.so.X` / `amdhip64_X.dll` load failure | print-only | +| `fix-9-igpu-dgpu` | linux/windows | iGPU enumerated alongside dGPU, destabilising the runtime | APU + discrete AMD present, `HIP_VISIBLE_DEVICES` unset, crash/segfault | needs-arg | +| `fix-10-container` | linux | Container can't see `/dev/kfd` or `/dev/dri/renderD*` | running in docker/podman, kfd/render devices missing | print-only | +| `fix-11-iommu` | linux | Multi-GPU hang with IOMMU enabled | ≥2 AMD GPUs, `iommu=` not `pt`, hang/deadlock/timeout | print-only | +| `fix-12-installer` | linux | `amdgpu-install` left a broken DKMS / repo state | dpkg half-configured, DKMS failed, `--accept-eula` | print-only | +| `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-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-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 | +| `fix-wsl-1-gpu-not-exposed` | wsl | `/dev/dxg` absent, so the distro cannot reach the GPU at all | no `/dev/dxg`; in a container, the device was never passed through | print-only | +| `fix-wsl-2-dxcore-missing` | wsl | `/usr/lib/wsl/lib` DXCore shims missing, so the runtime cannot reach the host driver | `/usr/lib/wsl/lib/libdxcore.so` missing (or the directory absent entirely) | print-only | +| `fix-wsl-3-rocdxg-missing` | wsl | ROCDXG, the ROCm-to-DXCore shim the WSL path runs on, is not installed | `librocdxg.so` not found under any ROCm install | print-only | +| `fix-wsl-4-rocdxg-not-linked` | wsl | `librocdxg` installed but not in the linker cache, so it is unloadable | `librocdxg` present on disk yet absent from `ldconfig -p` | print-only | +| `fix-wsl-5-distro-too-old` | wsl | Distro release below the floor the WSL path requires | distro release under the supported floor (e.g. Ubuntu 22.04, whose glibc 2.35 is below the 2.38 / `GLIBCXX_3.4.32` the engines need) | print-only | +| `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` diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index d33c22e74..cc2b0626d 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -276,16 +276,52 @@ Feature: Diagnosing failures and listing fixes # diagnose-05 only proves the manual/zero-optional-flags wording, because # PREVIEW_FIX_ID (fix-1-arch) needs none of sudo/reboot/re-login. The # sudo+re-login combination only exists on a fix gated to bare-metal Linux - # (fix-4-render-group), so it needs its own scenario -- but, like - # diagnose-14, it is deliberately not OS-gated: `print_recipe` runs before - # the fix's own platform gate (see `apply` in fix.rs), so the Flags: text - # under test renders identically regardless of which lane runs it. The step - # asserts only that printed text, never the exit code -- `fix-4-render-group` - # gates its own dry-run on host state ($USER, `usermod`/`sudo` on PATH), so - # unlike PREVIEW_FIX_ID its exit code is not guaranteed to be 0 everywhere. - @id:diagnose-fix-preview-states-required-flags - Scenario: diagnose-20 - Previewing a fix that needs sudo and a re-login says so, and that it's auto-applicable + # (fix-4-render-group), so it needs its own scenario. + # + # Unlike diagnose-14, this one IS OS-gated. `print_recipe` still runs before + # the fix's own platform gate (see `apply` in fix.rs), but the Flags: line it + # prints comes from `class_here()`, which looks up the catalog entry for the + # *running* host's OS. "AUTO" only renders where `fix-4-render-group` is + # actually `Auto` -- bare-metal Linux. Everywhere else (`applies_on` has no + # other member) `class_here()` falls back to PRINT-ONLY, the generic "this + # fix does not apply here" answer diagnose-11 already covers -- not a second, + # platform-specific behaviour worth asserting under this scenario's name. + # + # The step still asserts only the printed Flags: text, never the exit code -- + # `fix-4-render-group` gates its own dry-run on host state ($USER, + # `usermod`/`sudo` on PATH), so unlike PREVIEW_FIX_ID its exit code is not + # guaranteed to be 0 even on Linux. + @id:diagnose-fix-preview-states-required-flags @requires-os:linux @requires-bare-metal + Scenario: diagnose-20 - Previewing a fix that needs sudo and a re-login says so, and that it's auto-applicable here Given a user who has chosen a fix that needs sudo and a re-login When the user previews that fix without applying it Then the preview states that the fix requires sudo and a re-login And the preview states that the CLI can run it automatically + + # One entry behaves differently depending on the machine: it persists the + # change on Windows, and on Linux it only reports where the value is set, + # because the code that would write it takes no options and never does. + # The listing said "the CLI will run this" on both, so a user on Linux — and + # an agent reading the same listing — was told a change was coming that never + # came. Host-independent on purpose: the assertion is that the listing agrees + # with the machine in front of it, whichever machine that is. + @id:diagnose-fix-applicability-is-per-machine + Scenario: diagnose-21 - A fix that only explains itself here is not advertised as one the CLI will run + Given a fix the CLI carries out on one kind of machine and only explains on another + When the user asks the CLI which fixes it offers + Then that fix is shown as what it does on this machine + + # The other half of the same defect. This entry does have a fix and the CLI + # will carry it out, but not until it is told which device to pin; asked + # plainly it prints the query that identifies one and stops. It was marked as + # a fix the CLI applies, so the report of a change that never happened looked + # like success. + # @requires-bare-metal because the entry under test is scoped to bare-metal + # Linux and Windows. On WSL it is refused at the platform gate instead, which + # is a different contract with its own scenario — and the right one, since the + # catalog does not claim this remedy applies there. + @id:diagnose-fix-needing-an-argument-says-so @requires-bare-metal + Scenario: diagnose-22 - A fix that needs more information says what it needs and changes nothing + Given a user who has chosen a fix that cannot run until it is told what to act on + When the user asks the CLI to apply it without saying what to act on + Then the CLI names what it still needs and reports no change diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 2c59e6de1..ca1028dc7 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -34,8 +34,9 @@ const ENGINE_IMPORT_FIX_ID: &str = "fix-17-torch-dlpack"; const PREVIEW_FIX_ID: &str = "fix-1-arch"; /// A recipe that would really change the machine, used to prove the CLI asks -/// first. Of the four AUTO recipes this is the only one that reaches the -/// confirmation gate on a host with nothing installed: `fix-2-unset-override` +/// first. Of the entries the CLI carries out, this is the only one that +/// reaches the confirmation gate on a host with nothing installed: +/// `fix-2-unset-override` /// never calls it on Linux, `fix-4-render-group` exits early once the user is /// already in the groups, and `fix-6-path` exits early with "no ROCm install /// found". This one needs only `--device-index`, which the scenario supplies. @@ -93,15 +94,61 @@ const CATALOG_FIX_IDS: &[&str] = &[ "fix-19-shm-too-small", ]; -/// The fixes the CLI carries out itself. Every other entry only prints a plan. +/// The fixes the CLI carries out itself **on the host running the suite**. +/// /// Pinned exactly: a mode quietly promoted to AUTO would begin changing /// machines that callers had been told it only ever advised on. -const AUTO_APPLICABLE_FIX_IDS: &[&str] = &[ - "fix-2-unset-override", - "fix-4-render-group", - "fix-6-path", - "fix-9-igpu-dgpu", -]; +/// +/// Host-dependent because the catalog is. `fix-2-unset-override` persists the +/// change through `setx` on Windows but only reports on Linux, where its runner +/// takes no options and never writes. `fix-9-igpu-dgpu` is on neither list: it +/// acts only once `--device-index` names a target, and is marked NEEDS-ARG. +/// Both used to claim AUTO everywhere, which is the defect this pins. +fn auto_applicable_fix_ids() -> &'static [&'static str] { + if cfg!(windows) { + return &["fix-2-unset-override", "fix-6-path"]; + } + // WSL is its own catalog family, not "Linux with a flag", so `cfg!` cannot + // answer this -- it is a property of the running host. `fix-4-render-group` + // is bare-metal only and `fix-2-unset-override` only reports there, which + // leaves one entry the CLI actually carries out. + if e2e_cucumber::capability::host_capability().is_wsl { + return &["fix-6-path"]; + } + &["fix-4-render-group", "fix-6-path"] +} + +/// The one catalog entry whose behaviour splits by platform: it persists the +/// change through `setx` on Windows, while on Linux `run_unset_override_linux` +/// takes no options and only reports where the value is set. +const PLATFORM_SPLIT_FIX_ID: &str = "fix-2-unset-override"; + +/// What that entry does on the host running the suite. The listing has to agree +/// with the machine in front of it — claiming AUTO on Linux is what told users +/// a change was coming that never came. +const fn platform_split_marker_here() -> &'static str { + // Windows persists the change; Linux and WSL both run the arm that only + // reports, so both see PRINT-ONLY. + if cfg!(windows) { "AUTO" } else { "PRINT-ONLY" } +} + +/// The entry the CLI will carry out, but not until it is told which device to +/// pin. Asked plainly it prints the identifying query and stops. +const NEEDS_ARGUMENT_FIX_ID: &str = "fix-9-igpu-dgpu"; + +/// The argument it is waiting for. Named in the flags line, so a reader is not +/// left to find it in the notes. +const NEEDS_ARGUMENT_FLAG: &str = "--device-index"; + +/// The marker in a listing row — the contents of its first `[...]` group. +/// +/// Read rather than matched against a padded literal, so the assertions do not +/// break when a longer marker widens the column. +fn row_marker(line: &str) -> Option<&str> { + let open = line.find('[')?; + let close = line[open..].find(']')? + open; + Some(line[open + 1..close].trim()) +} /// A WSL distribution name no host will have. Deliberately not a plausible one: /// the scenario must fail for "this machine does not exist", never because the @@ -211,6 +258,16 @@ async fn user_named_unknown_fix(world: &mut E2eWorld) { world.model_name = Some("fix-does-not-exist".to_string()); } +#[given("a fix the CLI carries out on one kind of machine and only explains on another")] +async fn user_chose_platform_split_fix(world: &mut E2eWorld) { + world.model_name = Some(PLATFORM_SPLIT_FIX_ID.to_string()); +} + +#[given("a user who has chosen a fix that cannot run until it is told what to act on")] +async fn user_chose_fix_needing_an_argument(world: &mut E2eWorld) { + world.model_name = Some(NEEDS_ARGUMENT_FIX_ID.to_string()); +} + #[given("a user who has chosen a fix that would change the machine")] async fn user_chose_mutating_fix(world: &mut E2eWorld) { let rc_file = fix_rc_file(world); @@ -305,6 +362,17 @@ async fn user_lists_fixes(world: &mut E2eWorld) { world.cli_rc = Some(rc); } +#[when("the user asks the CLI to apply it without saying what to act on")] +async fn user_applies_fix_without_its_argument(world: &mut E2eWorld) { + let fix_id = world.model_name.clone().expect("no fix id set"); + // Deliberately no `--device-index`: the branch under test is the one that + // reports what is still needed instead of acting. + let (stdout, stderr, rc) = crate::run_rocm(world, &["fix", &fix_id]); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + #[when("the user previews that fix without applying it")] async fn user_previews_fix(world: &mut E2eWorld) { let fix_id = world.model_name.clone().expect("no fix id set"); @@ -466,10 +534,15 @@ async fn assert_markers_explained(world: &mut E2eWorld) { let output = world.cli_output.as_ref().expect("no fix list output"); // The markers were printed with no legend, so a reader could not tell // whether PRINT-ONLY meant "advisory" or "not implemented yet". - assert!( - output.contains("AUTO =") && output.contains("PRINT-ONLY ="), - "expected the listing to explain its AUTO/PRINT-ONLY markers:\n{output}" - ); + // Every marker the listing can print has to be explained, or the newer + // ones land in exactly the position PRINT-ONLY was in. + for marker in ["AUTO =", "NEEDS-ARG =", "PRINT-ONLY =", "DIAGNOSE-ONLY ="] { + assert!( + output.contains(marker), + "the listing prints markers it never explains; `{marker}` is missing \ + from the legend:\n{output}" + ); + } } #[then("the CLI always points to somewhere the problem can be reported")] @@ -902,17 +975,87 @@ async fn assert_auto_set_is_exact(world: &mut E2eWorld) { let output = world.cli_output.as_ref().expect("no fix list output"); let marked_auto: Vec<&str> = output .lines() - .filter(|line| line.contains("[ AUTO]")) + .filter(|line| row_marker(line) == Some("AUTO")) .filter_map(|line| CATALOG_FIX_IDS.iter().copied().find(|id| line.contains(id))) .collect(); // Exact, not "at least": a mode quietly promoted to AUTO would start // changing machines that callers were told it only ever advised. assert_eq!( - marked_auto, AUTO_APPLICABLE_FIX_IDS, + marked_auto, + auto_applicable_fix_ids(), "the set of fixes the CLI applies itself has changed:\n{output}" ); } +#[then("that fix is shown as what it does on this machine")] +async fn assert_platform_split_fix_is_listed_for_this_machine(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix list output"); + let fix_id = world.model_name.clone().expect("no fix id set"); + let row = output + .lines() + .find(|line| line.contains(&fix_id)) + .unwrap_or_else(|| panic!("the listing has no row for {fix_id}:\n{output}")); + assert_eq!( + row_marker(row), + Some(platform_split_marker_here()), + "{fix_id} is listed as something other than what it does on this machine. \ + The catalog is authoritative: if its behaviour here changed, change the \ + catalog and this expectation together — do not loosen the assertion.\n{row}" + ); +} + +#[then("the CLI names what it still needs and reports no change")] +async fn assert_missing_argument_is_named(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix output"); + // Exit 0, not an error code: nothing went wrong and nothing was attempted. + // A nonzero code would read as a failed fix rather than an unanswered + // question. + assert_eq!( + world.cli_rc, + Some(0), + "reporting what it still needs is not a failure:\n{output}" + ); + // The flags line specifically, not the output as a whole. The recipe's + // notes mention the argument either way, so a whole-output search passes + // even when the entry is marked as one the CLI applies outright -- which is + // exactly the defect, and an earlier version of this assertion missed it. + let flags = output + .lines() + .find(|line| line.starts_with("Flags:")) + .unwrap_or_else(|| { + panic!( + "no flags line, so nothing told the user the CLI is waiting on \ + `{NEEDS_ARGUMENT_FLAG}` rather than acting:\n{output}" + ) + }); + assert!( + flags.contains(NEEDS_ARGUMENT_FLAG), + "the flags line does not name `{NEEDS_ARGUMENT_FLAG}`, so the user is told \ + only that the fix is unavailable, not what would make it available:\n{flags}" + ); + // The whole defect was a report of a change that never happened, so the + // absence of that claim is the assertion -- but it has to name text the + // product can actually produce. This asserted the absence of "Applied", + // which appears nowhere in the CLI, so it held against every possible + // regression. `fix-9`'s Linux arm announces a real change by naming the + // file it appended to; the Windows arm by naming the value it persisted. + // Only text the CLI prints *after* acting. The catalog's own plan includes + // `setx HIP_VISIBLE_DEVICES `, which this path legitimately + // shows, so the placeholder command is not evidence of a change; the + // announcements below are printed only once a write has happened. + for claim in [ + "Appended to ", + "setx only takes effect in NEW shells", + "Plan: persist HIP_VISIBLE_DEVICES", + ] { + assert!( + !output.contains(claim), + "the CLI was not told what to act on, so it must not report having acted \ + ({claim:?}):\n{output}" + ); + } +} + #[then("the CLI lists the fixes it can apply")] async fn assert_lists_fixes(world: &mut E2eWorld) { assert_eq!(world.cli_rc, Some(0), "fix listing should exit 0"); diff --git a/tests/e2e-cucumber/tests/e2e/skill_steps.rs b/tests/e2e-cucumber/tests/e2e/skill_steps.rs index ba2de1be2..ccb7a43e2 100644 --- a/tests/e2e-cucumber/tests/e2e/skill_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/skill_steps.rs @@ -34,10 +34,10 @@ const KNOWN_SYMPTOM: &str = "HSA_STATUS_ERROR_INVALID_ISA"; /// route is populated either way, which is what the scenario using this checks. const UNMATCHED_SYMPTOM: &str = "the office printer keeps jamming on page three"; -/// The reference states the auto-applicable set twice: once as `yes` cells in -/// the catalog table, and once in prose above it. Both are parsed, so a rename -/// applied to the table and the CLI together still fails if the prose was -/// missed — and neither side is a constant restated in this file. +/// The reference states the auto-applicable set twice: once as marker cells in +/// the catalog table, and once in per-host prose above it. Both are parsed, so +/// a rename applied to the table and the CLI together still fails if the prose +/// was missed — and neither side is a constant restated in this file. const AUTO_APPLICABLE_PROSE: &str = "auto-applicable"; /// One catalog row, from either side of the comparison. @@ -47,8 +47,91 @@ struct Remediation { /// normalised, so a catalog that invents its own shorthand shows up as a /// mismatch instead of being quietly translated into agreement. os_scope: String, - /// Whether the CLI applies it itself, as opposed to printing a plan. - auto: bool, + /// The marker word this id carries: `AUTO`, `NEEDS-ARG`, `PRINT-ONLY`, or + /// `DIAGNOSE-ONLY` (no catalog entry uses the last one yet, but it is + /// recognised rather than silently dropped -- an unrecognised marker drops + /// its row from the map, which previously made `NEEDS-ARG` vanish fix-9 + /// entirely instead of failing on the mismatched marker). + marker: String, + /// A platform whose marker differs from [`Remediation::marker`], and what + /// it is there. `rocm fix`'s listing reports the marker for the machine it + /// runs on, never for every machine the id applies to, so an id whose + /// behaviour genuinely depends on more than the host needs this to stay + /// correct everywhere the suite runs (bare metal Linux, Windows, WSL2). + /// `fix-2-unset-override` is the only entry this applies to today: + /// PRINT-ONLY is the documented baseline (true on Linux and WSL), AUTO on + /// Windows only. Only ever set from the doc side; the CLI side already + /// reports the resolved, host-specific marker directly. + platform_override: Option<(String, String)>, +} + +impl Remediation { + /// The marker this row implies for `platform`, folding in the one + /// documented exception and the CLI's own out-of-scope fallback: an id + /// `rocm fix` lists but that does not apply on `platform` at all always + /// reports PRINT-ONLY there, whatever it is where it does apply (mirrors + /// `FixRecipe::class_here`'s `unwrap_or_default` in `crates/rocm-core`). + fn expected_marker(&self, platform: &str) -> &str { + if !self.os_scope.split('/').any(|p| p == platform) { + return "PRINT-ONLY"; + } + match &self.platform_override { + Some((override_platform, marker)) if override_platform == platform => marker, + _ => &self.marker, + } + } +} + +/// The `linux`/`windows`/`wsl` family the suite's own host-detection reports +/// for the machine this scenario is running on right now -- the same value +/// `crates/rocm-core`'s `current_os()` would compute, from the harness's +/// existing probe rather than a second one. +fn current_platform_family() -> String { + let cap = e2e_cucumber::capability::host_capability(); + if cap.is_wsl { + "wsl".to_owned() + } else { + cap.os_family.clone() + } +} + +/// Recognise a bare marker word in either casing the two sides use: the table +/// writes `auto`/`needs-arg`/`print-only`/`diagnose-only`, `rocm fix` prints +/// `AUTO`/`NEEDS-ARG`/`PRINT-ONLY`/`DIAGNOSE-ONLY`. Returns the CLI's casing, +/// so both sides compare equal literally once parsed. +fn normalise_marker(word: &str) -> Option<&'static str> { + match word.to_ascii_uppercase().as_str() { + "AUTO" => Some("AUTO"), + "NEEDS-ARG" => Some("NEEDS-ARG"), + "PRINT-ONLY" => Some("PRINT-ONLY"), + "DIAGNOSE-ONLY" => Some("DIAGNOSE-ONLY"), + _ => None, + } +} + +/// Splits a catalog cell into its base marker and an optional per-platform +/// override. Plain cells are just a marker word (`needs-arg`); a cell for an +/// id whose marker depends on more than the host also names the exception, +/// `print-only (auto on windows)`. Returns `None` for neither shape, which the +/// caller treats as an unrecognised cell. +fn parse_marker_cell(cell: &str) -> Option<(String, Option<(String, String)>)> { + let cell = cell.trim(); + let Some(paren_start) = cell.find('(') else { + return Some((normalise_marker(cell)?.to_owned(), None)); + }; + let base = normalise_marker(cell[..paren_start].trim())?; + let inside = cell[paren_start + 1..].trim_end_matches(')').trim(); + let mut words = inside.split_whitespace(); + let (Some(marker_word), Some("on"), Some(platform)) = + (words.next(), words.next(), words.next()) + else { + return None; + }; + let marker = normalise_marker(marker_word)?; + Some(( + base.to_owned(), + Some((platform.to_owned(), marker.to_owned())), + )) } fn reference_md_path() -> PathBuf { @@ -64,7 +147,7 @@ fn reference_md_path() -> PathBuf { /// Read the closed-catalog table out of the skill's reference doc. /// /// Rows look like: -/// `| `fix-1-arch` | linux/windows/wsl | | | no |` +/// `| `fix-1-arch` | linux/windows/wsl | | | print-only |` /// Only rows whose first cell is a backticked `fix-*` id are taken, which skips /// the header, the separator, and the exit-code table further up the file. fn parse_reference_catalog(md: &str) -> BTreeMap { @@ -93,12 +176,19 @@ fn parse_reference_catalog(md: &str) -> BTreeMap { // An unrecognised cell drops the row rather than aborting the parse, so // a reworded table surfaces as the scenario's own set diff — naming the // ids that went missing — instead of a panic from inside the reader. - let auto = match *cells.last().expect("row has cells") { - "yes" => true, - "no" => false, - _ => continue, + let Some((marker, platform_override)) = + parse_marker_cell(cells.last().expect("row has cells")) + else { + continue; }; - out.insert(id.to_owned(), Remediation { os_scope, auto }); + out.insert( + id.to_owned(), + Remediation { + os_scope, + marker, + platform_override, + }, + ); } assert!( !out.is_empty(), @@ -108,34 +198,31 @@ fn parse_reference_catalog(md: &str) -> BTreeMap { out } -/// The fix-ids the reference's prose names as auto-applicable. +/// The fix-ids the reference's prose names as auto-applicable on `platform`. /// -/// The sentence spans two lines and is delimited by em dashes: -/// `Only four fixes are auto-applicable — `fix-2-…`, `fix-4-…` — and the rest…` -/// Bounding on the dashes keeps the `rocm fix fix-2-unset-override` example -/// later in the same paragraph out of the set. -fn documented_auto_prose(md: &str) -> BTreeSet { - let joined = md.replace('\n', " "); - let Some((_, after)) = joined.split_once(AUTO_APPLICABLE_PROSE) else { +/// The per-host bullet list reads: +/// `- **linux** — auto-applicable: `fix-4-…`, `fix-6-…`.` +/// one bullet per platform, each ending the ids in backticks on that line. +fn documented_auto_prose_for(md: &str, platform: &str) -> BTreeSet { + let bullet_prefix = format!("- **{platform}**"); + let Some(line) = md.lines().find(|line| { + let line = line.trim(); + line.starts_with(&bullet_prefix) && line.contains(AUTO_APPLICABLE_PROSE) + }) else { panic!( - "reference.md no longer states which fixes are {AUTO_APPLICABLE_PROSE} in prose; \ - the catalog table alone cannot catch a rename that missed the prose" + "reference.md no longer states which fixes are {AUTO_APPLICABLE_PROSE} on \ + {platform} in prose; the catalog table alone cannot catch a rename that \ + missed the prose" ) }; - let (_, inside) = after - .split_once('—') - .expect("the auto-applicable sentence no longer opens with an em dash"); - let (list, _) = inside - .split_once('—') - .expect("the auto-applicable sentence no longer closes with an em dash"); - let ids: BTreeSet = backticked(list) + let ids: BTreeSet = backticked(line) .into_iter() .filter(|span| span.starts_with("fix-")) .map(str::to_owned) .collect(); assert!( !ids.is_empty(), - "no fix-ids parsed out of reference.md's auto-applicable sentence: {list:?}" + "no fix-ids parsed out of reference.md's {platform} auto-applicable bullet: {line:?}" ); ids } @@ -154,10 +241,13 @@ fn parse_fix_listing(stdout: &str) -> BTreeMap { }; // As above: a renamed marker drops the row, and the scenario reports it // as an id `rocm fix` no longer offers rather than as a parser panic. - let auto = match marker.trim() { - "AUTO" => true, - "PRINT-ONLY" => false, - _ => continue, + // Every marker the catalog's `FixClass` can print is recognised here -- + // `NEEDS-ARG` and `DIAGNOSE-ONLY` included -- so a reclassified entry + // shows up as a marker mismatch against the doc instead of silently + // disappearing from this map and being reported as an id `rocm fix` + // no longer offers at all. + let Some(marker) = normalise_marker(marker.trim()) else { + continue; }; let Some((os_scope, rest)) = rest.trim_start().trim_start_matches('[').split_once(']') else { @@ -171,7 +261,8 @@ fn parse_fix_listing(stdout: &str) -> BTreeMap { id.to_owned(), Remediation { os_scope: os_scope.trim().to_owned(), - auto, + marker: marker.to_owned(), + platform_override: None, }, ); } @@ -499,31 +590,37 @@ async fn assert_same_ids(world: &mut E2eWorld) { #[then("the skill and the CLI agree on which ones the CLI applies without help")] async fn assert_same_auto_set(world: &mut E2eWorld) { let (doc, cli) = both_sides(world); + // `rocm fix`'s listing marks each entry for the machine running it, not for + // every machine the id applies to -- `fix-2-unset-override` is AUTO on + // Windows and PRINT-ONLY everywhere else it applies, so the host this + // scenario runs on has to be part of what "the same set" means here. + let platform = current_platform_family(); for (id, offered) in &cli { let documented = doc.get(id).unwrap_or_else(|| { panic!( "`rocm fix` offers {id}, which skills/rocm-doctor/reference.md does not document" ) }); + let expected = documented.expected_marker(&platform); assert_eq!( - documented.auto, offered.auto, - "{id}: reference.md says auto-applicable={}, `rocm fix` says {}", - documented.auto, offered.auto + expected, offered.marker, + "{id} on {platform}: reference.md implies marker {expected}, `rocm fix` says {}", + offered.marker ); } - // The reference says it twice — `yes` cells above, prose below — and the - // loop only checked the cells. A rename applied to the table and the CLI - // together would leave the prose stale, and the prose is what an agent - // reads before it decides whether to offer to run a fix. - let prose = documented_auto_prose(reference(world)); + // The reference says it twice — marker cells above, per-host prose below — + // and the loop only checked the cells. A rename applied to the table and + // the CLI together would leave the prose stale, and the prose is what an + // agent reads before it decides whether to offer to run a fix. + let prose = documented_auto_prose_for(reference(world), &platform); let offered: BTreeSet = cli .iter() - .filter(|(_, r)| r.auto) + .filter(|(_, r)| r.marker == "AUTO") .map(|(id, _)| id.clone()) .collect(); assert_eq!( prose, offered, - "reference.md's prose names {prose:?} as auto-applicable, `rocm fix` offers {offered:?}" + "reference.md's {platform} prose names {prose:?} as auto-applicable, `rocm fix` offers {offered:?}" ); } diff --git a/tests/e2e-cucumber/tests/skill_reference.rs b/tests/e2e-cucumber/tests/skill_reference.rs index 9d5d53e15..8f6719d04 100644 --- a/tests/e2e-cucumber/tests/skill_reference.rs +++ b/tests/e2e-cucumber/tests/skill_reference.rs @@ -23,6 +23,13 @@ use std::path::{Path, PathBuf}; /// A catalog cell is any non-empty `/`-joined subset of these. const PLATFORM_ATOMS: &[&str] = &["linux", "windows", "wsl"]; +/// The marker words `rocm fix`'s listing prints (`FixClass::marker`, lower-cased +/// to match the table's style), plus `yes`/`no` -- recognised only so this +/// guard can name them explicitly as the vocabulary the class rewrite retired, +/// rather than reporting them as merely unrecognised. +const MARKER_WORDS: &[&str] = &["auto", "needs-arg", "print-only", "diagnose-only"]; +const RETIRED_MARKER_WORDS: &[&str] = &["yes", "no"]; + fn reference_md_path() -> PathBuf { Path::new(env!("CARGO_MANIFEST_DIR")) .join("..") @@ -32,10 +39,10 @@ fn reference_md_path() -> PathBuf { .join("reference.md") } -/// `(fix-id, os-scope cell)` for every catalog row, taken the same way -/// `skill_steps::parse_reference_catalog` takes them: a leading `|`, at least -/// five cells, and a backticked `fix-*` id first. -fn catalog_rows(md: &str) -> Vec<(String, String)> { +/// `(fix-id, os-scope cell, marker cell)` for every catalog row, taken the same +/// way `skill_steps::parse_reference_catalog` takes them: a leading `|`, at +/// least five cells, and a backticked `fix-*` id first. +fn catalog_rows(md: &str) -> Vec<(String, String, String)> { let mut rows = Vec::new(); for line in md.lines() { let line = line.trim(); @@ -50,7 +57,11 @@ fn catalog_rows(md: &str) -> Vec<(String, String)> { if !id.starts_with("fix-") { continue; } - rows.push((id.to_owned(), cells[1].to_owned())); + rows.push(( + id.to_owned(), + cells[1].to_owned(), + (*cells.last().expect("row has cells")).to_owned(), + )); } rows } @@ -73,9 +84,9 @@ fn catalog_os_scopes_use_the_cli_spellings() { ); let allowed: BTreeSet<&str> = PLATFORM_ATOMS.iter().copied().collect(); - let bad: Vec<&(String, String)> = rows + let bad: Vec<&(String, String, String)> = rows .iter() - .filter(|(_, scope)| { + .filter(|(_, scope, _)| { scope.is_empty() || scope.split('/').any(|atom| !allowed.contains(atom)) }) .collect(); @@ -89,3 +100,55 @@ fn catalog_os_scopes_use_the_cli_spellings() { shorthand would have to be translated -- which is how a dropped platform hides." ); } + +#[test] +fn catalog_markers_use_the_cli_vocabulary() { + // `skill_steps::parse_fix_listing` and `parse_reference_catalog` drop any + // row whose marker is unrecognised rather than panic, so a reclassified + // entry whose table cell still says `yes`/`no` -- the vocabulary the + // `FixClass` rewrite retired -- disappears from the comparison silently + // instead of failing on a marker mismatch. That is exactly the trap that + // let `fix-9-igpu-dgpu`'s `NEEDS-ARG` reclassification vanish from the + // skill-01 scenario's set comparison instead of showing up as a mismatch + // on skill-02. This guard runs in the ordinary `cargo test` set, with no + // built `rocm` binary needed, so the retired word is caught here first. + let md = std::fs::read_to_string(reference_md_path()) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", reference_md_path().display())); + let rows = catalog_rows(&md); + assert!( + !rows.is_empty(), + "no catalog rows found in {}", + reference_md_path().display() + ); + + // A marker cell is a bare word, or a word plus a per-platform exception -- + // `print-only (auto on windows)` -- for the one entry whose behaviour + // depends on more than the host. Only the base word is checked against the + // vocabulary; `parse_marker_cell` in skill_steps.rs owns the exception + // syntax itself. + let base_word = |cell: &str| -> String { + match cell.find('(') { + Some(i) => cell[..i].trim().to_owned(), + None => cell.trim().to_owned(), + } + }; + + let allowed: BTreeSet<&str> = MARKER_WORDS.iter().copied().collect(); + let retired: BTreeSet<&str> = RETIRED_MARKER_WORDS.iter().copied().collect(); + let bad: Vec<(&String, String)> = rows + .iter() + .map(|(id, _, marker)| (id, base_word(marker))) + .filter(|(_, word)| !allowed.contains(word.as_str())) + .collect(); + assert!( + bad.is_empty(), + "skills/rocm-doctor/reference.md marks these fixes with words `rocm fix` never prints: \ + {bad:?}.\n\ + A marker cell must be one of {MARKER_WORDS:?}, matching `FixClass::marker` in \ + crates/rocm-core/src/fix.rs (lower-cased). {} of those are `yes`/`no`, the vocabulary \ + the class rewrite retired -- replace with the CLI's own marker word.", + bad.iter() + .filter(|(_, word)| retired.contains(word.as_str())) + .count() + ); +}