From e7f9b7f4db21f0bfdeaac4ab2b78eb30209fede1 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Fri, 11 Sep 2026 09:23:29 +0000 Subject: [PATCH] fix(diagnose): stop claiming the CLI will apply fixes it only reports `auto_applicable` was a flat bool meaning "rocm fix will carry this out", and it was wrong for two entries: - fix-2-unset-override on Linux. `run_unset_override_linux` takes no FixOptions and never writes: it reads the live override, scans the shell rc files, and reports where it is set. Only the Windows arm mutates, and WSL runs the Linux arm. - fix-9-igpu-dgpu with no --device-index. Both arms print the query that identifies the discrete GPU and return without acting. Both were marked auto-applicable, so a user -- and an agent reading the same output -- was told a change was coming that never came, and got exit 0 to confirm it. The repo already knew: two test names and a step comment say "print-only" for exactly these cases. Only the type disagreed. Replace the bool with a per-platform class (AUTO, NEEDS-ARG, PRINT-ONLY, DIAGNOSE-ONLY). Scope and class are held together because they are one fact -- fix-2 applies on Linux as print-only and on Windows as auto, which a flat flag could not say. The class says what happens to the machine, not whether a runner runs, so fix-2 keeps its Linux rc-file report instead of falling back to a generic "copy the commands above". diagnose no longer restates applicability per checker. It reads the class off the catalog in one place, keyed on the examined machine's platform family. The two copies had already drifted -- fix-9 was auto-applicable in the catalog and not in the Linux arm of its own checker -- with nothing comparing them. Keyed on the family rather than os_family because WSL reports linux there while being its own catalog family: reading os_family would look up every WSL recipe under the wrong platform and report them all as print-only. The seven WSL recipes carry the class directly, all print-only, and fix-6-path is the only entry the CLI carries out on that platform. DIAGNOSE-ONLY ships with no member. Reading all 24 entries found none that qualifies: every one has a real fix, and the print-only ones are so because carrying the fix out needs sudo, a reboot, a reinstall or a download -- not because no fix exists. The class still ships now rather than when a member turns up: `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, so shipping the full set from the start means that variant is already something callers have to tolerate. README and `rocm fix --help` now say so explicitly, so neither promises output the CLI cannot currently produce. That left the branch unreachable from any test. Split the id lookup in apply from what it does with the recipe, and the decision from the messages, so the class gate can be driven with a recipe the catalog does not contain. The decision is now an exhaustive match, which turns an arm deleted here into a compile error rather than a silent fall-through -- Unfixable and PrintSteps both exit 0, so the exit code alone could not tell them apart, and a disabled guard would have kept its own test green. A tripwire test asserts no entry uses the class, so the first real member fails it and says what to do next. format_flags, which exists so rocm fix and rocm diagnose word the same entry the same way, now takes the class instead of the bool. Three of the four answers are "the CLI will not run it", for different reasons, and a bool could only carry one of them. diagnose reads the argument name from the catalog beside the class, so a needs-argument entry names --device-index from either command rather than one saying "an argument". docs/wsl.md said none of the WSL remedies meets the bar "the four auto-applicable fixes" clear. There is no set of four once applicability is per-platform, and leaving the count there would recreate on the one user-facing page the wrong claim this change removes everywhere else. The same stale "four auto-applicable" count and two other claims the class rewrite invalidated survived uncaught in three more places: fix.rs's own WSL-recipes comment (now redundant with PRINT_ON_WSL's corrected doc comment, so dropped), diagnose_steps.rs's MUTATING_FIX_ID comment (still said "of the four AUTO recipes" about an entry this PR reclassified to NEEDS-ARG), and a companion-test comment in diagnose.rs naming a test this PR renamed and a flags-line example fix-9 no longer renders. Reworded all three to match current behaviour. The shared-memory checker and all seven WSL checkers still hand-set `auto_applicable: false,` in their own `Fix` literals instead of leaving it at `Fix::default()`, so eight of the twenty-eight sites were still restating what the catalog alone was supposed to decide. Converted the remaining eight -- the catalog already overwrites the field regardless, so this changes nothing a user or agent can observe -- and added a test that fails if any checker's `Fix` literal sets `auto_applicable` again, so `take_applicability_from_the_catalog` stays the only place that can, instead of a checker being able to quietly disagree with it a second time the way fix-9 once did. Three surfaces still described the old two-value system after the class landed, and one test could not fail. The `rocm fix` help said fixes are marked AUTO or PRINT-ONLY, which is a two-value description of something that now has four. fix-9's own note ended "despite being marked AUTO" -- true of the flat flag, and precisely the overstatement this change exists to remove. The e2e step asserting the CLI reports no change checked that its output does not contain "Applied". That string appears nowhere in the product, so the assertion held against every possible regression. It now names text the CLI prints only after it has acted, and deliberately not the catalog's own plan, which includes a setx line this path legitimately shows. diagnose-20 (fix-preview-states-required-flags) asserted the Flags: line `rocm fix fix-4-render-group --dry-run` prints, on the premise that `print_recipe` runs before the fix's own platform gate so the text renders identically on every lane. That held for the flat bool; it stopped holding here, because the Flags: line now comes from `class_here()`, which looks up the catalog entry for the *running* host's OS. fix-4-render-group is `applies_on: AUTO_ON_LINUX` only, so bare-metal Linux renders "AUTO" and every other host falls back to PRINT-ONLY -- the generic "does not apply here" answer diagnose-11 already covers, not a second platform-specific behaviour worth asserting under this scenario's name. Gated the scenario `@requires-os:linux @requires-bare-metal`, matching the sibling scenarios for this same fix-id (diagnose-08, -15, -16), and corrected the stale comment. Verified against the Strix Halo WSL2 and Windows CI logs (both failed on this exact assertion) and reproduced/fixed locally by running the real cucumber harness (not the `-n` scenario filter, which bypasses the tag-based skip resolution) against this repo's own WSL2 host. This branch forked before `skills/rocm-doctor/` landed on main. Rebasing onto current main surfaced the gap review caught: the skill's catalog table and `rocm fix`'s own listing used to agree on a flat yes/no per id, and the class rewrite breaks that for any id whose class is not the same on every platform it applies to -- `fix-2-unset-override` (print-only on Linux/WSL, auto on Windows) and `fix-9-igpu-dgpu` (reclassified from AUTO to NEEDS-ARG outright). `skill_steps.rs`'s marker comparison is now host-aware: it reads the catalog's base marker plus an optional per-platform override, derives what `rocm fix` should report on the host the scenario is actually running on (folding in the CLI's own out-of-scope-means-PRINT-ONLY fallback), and compares that against the real listing instead of a flat bool. A silent trap in the obvious fix -- mapping an unrecognised marker to "drop the row" would have made fix-9 vanish from the comparison instead of failing on a mismatch -- is closed by recognising all four markers explicitly. `skills/rocm-doctor/reference.md` gained the same per-host story in prose (which platform's auto set is which) and in the catalog table (every cell now spells the marker word the CLI prints, not a yes/no this change retired). A new `tests/skill_reference.rs` guard, run in the ordinary `cargo test` set with no built `rocm` binary needed, catches a cell still using the retired vocabulary before the cucumber suite would. Three smaller review corrections in the same spirit: `diagnose.rs`'s `os_family` parameter renamed to `platform_family` to match both its own doc comment and its call site, which both already said "family, not os_family"; the regression test comparing `diagnose`'s applicability against the catalog now reads `fix::class_on` instead of a removed flat accessor, same claim, current API; and diagnose-20's title now says "auto-applicable here" rather than a bare "auto-applicable", matching the per-host story introduced above. A trailing comment on fix-17's `rationale` string (explaining the commands block below it) was glued to the end of that string by an earlier edit that deleted the line between them, leaving the rest of the comment floating detached from what it describes. Moved onto its own lines directly above `commands:`, where rustfmt will keep it. Signed-off-by: Eugene Volen --- README.md | 19 +- apps/rocm/src/main.rs | 12 +- crates/rocm-core/src/diagnose.rs | 224 +++-- crates/rocm-core/src/fix.rs | 900 +++++++++++++----- docs/wsl.md | 5 +- skills/rocm-doctor/reference.md | 86 +- tests/e2e-cucumber/features/diagnose.feature | 54 +- .../e2e-cucumber/tests/e2e/diagnose_steps.rs | 173 +++- tests/e2e-cucumber/tests/e2e/skill_steps.rs | 187 +++- tests/e2e-cucumber/tests/skill_reference.rs | 77 +- 10 files changed, 1302 insertions(+), 435 deletions(-) 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() + ); +}