diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 6ff9dcd74..a1ef94f23 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -1011,6 +1011,13 @@ 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)." ) }; + // Marked auto_applicable below, but `rocm fix fix-9-igpu-dgpu` still needs + // --device-index to actually make the change: without it, both the Linux + // and Windows runners only print the query that finds the index and + // change nothing (see README's --device-index caveat). + let note = format!( + "{note} Without --device-index, `rocm fix` only prints this query and makes no change, despite being marked AUTO." + ); 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(), @@ -1038,7 +1045,10 @@ 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(), - auto_applicable: false, + // Matches the `fix-9-igpu-dgpu` FixRecipe in fix.rs (auto_applicable: + // true, runner: run_hip_visible_devices) -- `rocm fix` can already + // carry this out on Linux, so the report must not claim otherwise. + auto_applicable: true, verify: "HIP_VISIBLE_DEVICES=1 python -c \"import torch; print(torch.cuda.device_count())\"".to_owned(), notes: vec![note], ..Fix::default() @@ -2184,22 +2194,13 @@ pub fn render_report_text(report: &DiagnoseReport, top: usize) -> String { for c in &fix.commands { let _ = writeln!(out, " $ {c}"); } - let mut flags = Vec::new(); - if fix.needs_sudo { - flags.push("sudo"); - } - if fix.needs_reboot { - flags.push("reboot required"); - } - if fix.needs_relogin { - flags.push("re-login required"); - } - if fix.auto_applicable { - flags.push("rocm fix can run it"); - } - if !flags.is_empty() { - let _ = writeln!(out, " flags: {}", flags.join(", ")); - } + let flags = crate::fix::format_flags( + fix.needs_sudo, + fix.needs_reboot, + fix.needs_relogin, + fix.auto_applicable, + ); + let _ = writeln!(out, " flags: {}", flags.join(", ")); for n in &fix.notes { let _ = writeln!(out, " note: {n}"); } @@ -2869,6 +2870,110 @@ mod tests { !note.contains("usually the higher-numbered"), "note must not repeat the old wrong gfx-number heuristic: {note}" ); + assert!( + note.contains("Without --device-index") && note.contains("despite being marked AUTO"), + "note must warn that fix-9 is a no-op without --device-index: {note}" + ); + } + + #[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. + let mut e = linux_base(); + e.has_apu = true; + e.has_discrete_amd = true; + e.gpus = vec![ + Gpu { + gfx_target: "gfx1103".to_owned(), + is_amd: true, + is_apu: Some(true), + ..Gpu::default() + }, + Gpu { + gfx_target: "gfx1100".to_owned(), + is_amd: true, + is_apu: Some(false), + ..Gpu::default() + }, + ]; + let report = diagnose(&e, "torch crashes with a segfault"); + let hit = report + .matched + .iter() + .find(|d| d.id == "fix-9-igpu-dgpu") + .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" + ); + + let text = render_report_text(&report, report.matched.len()); + let lines: Vec<&str> = text.lines().collect(); + let id_line = lines + .iter() + .position(|l| l.trim_start() == "id: fix-9-igpu-dgpu") + .expect("fix-9-igpu-dgpu should appear in the rendered report"); + let flags_line = lines[id_line..] + .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. + assert_eq!( + flags_line.trim_start(), + "flags: rocm fix can 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. + let mut e = linux_base(); + e.iommu_kernel_param = "on".to_owned(); + e.gpus = vec![ + Gpu { + is_amd: true, + ..Gpu::default() + }, + Gpu { + is_amd: true, + ..Gpu::default() + }, + ]; + let report = diagnose(&e, ""); + let text = render_report_text(&report, report.matched.len()); + let lines: Vec<&str> = text.lines().collect(); + let id_line = lines + .iter() + .position(|l| l.trim_start() == "id: fix-11-iommu") + .expect("fix-11-iommu should appear in the rendered report"); + let flags_line = lines[id_line..] + .iter() + .find(|l| l.trim_start().starts_with("flags:")) + .expect("fix-11-iommu should have a flags: line"); + assert_eq!( + flags_line.trim_start(), + "flags: requires sudo, requires reboot, manual only (`rocm fix` will NOT run it automatically)", + "rendered flags: line for fix-11-iommu: {flags_line}" + ); } #[test] diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 600cc543a..ed32e2d12 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -762,6 +762,45 @@ pub fn list_recipes() -> String { out } +/// 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 (known example: +/// `fix-5-amdgpu-load`'s `needs_reboot`). 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. +// 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 +// `clippy::fn_params_excessive_bools` is separately allowed below. +#[allow(clippy::fn_params_excessive_bools)] +pub(crate) fn format_flags( + needs_sudo: bool, + needs_reboot: bool, + needs_relogin: bool, + auto_applicable: bool, +) -> Vec<&'static str> { + let mut flags = Vec::new(); + if needs_sudo { + flags.push("requires sudo"); + } + if needs_reboot { + flags.push("requires reboot"); + } + 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 +} + fn print_recipe(r: &FixRecipe) { println!("Fix: {} -- {}", r.fix_id, r.title); println!("OS scope: {}", r.applies_on.join(", ")); @@ -772,22 +811,13 @@ fn print_recipe(r: &FixRecipe) { println!(" $ {c}"); } } - let mut flags = Vec::new(); - if r.needs_sudo { - flags.push("requires sudo"); - } - if r.needs_reboot { - flags.push("requires reboot"); - } - if r.needs_relogin { - flags.push("requires re-login"); - } - if !r.auto_applicable { - flags.push("manual only (this command will NOT run it)"); - } - if !flags.is_empty() { - println!("Flags: {}", flags.join(", ")); - } + let flags = format_flags( + r.needs_sudo, + r.needs_reboot, + r.needs_relogin, + r.auto_applicable, + ); + println!("Flags: {}", flags.join(", ")); for n in r.notes { println!("Note: {n}"); } @@ -1644,4 +1674,41 @@ mod tests { assert_eq!(apply("#1", &FixOptions::default()), 2); assert_eq!(apply("bogus", &FixOptions::default()), 2); } + + #[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 { + 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"); + } + 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}" + ); + } + } } diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index d860d0324..d33c22e74 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -20,6 +20,7 @@ Feature: Diagnosing failures and listing fixes When the user asks the CLI to diagnose that symptom Then the CLI reports a likely cause with a suggested fix And every reported cause comes with a command that applies it + And every reported cause states its remediation flags @id:diagnose-always-offers-a-way-forward Scenario: diagnose-02 - Diagnosing any failure always gives the user a way to escalate @@ -40,12 +41,17 @@ Feature: Diagnosing failures and listing fixes And each fix indicates whether the CLI can apply it automatically And the listing explains what those indicators mean + # This scenario exercises `rocm fix --dry-run` (print_recipe's `Flags:` + # line), not the `rocm diagnose` report itself -- see diagnose-01's "states + # its remediation flags" step for the equivalent `flags:` line on that + # surface. @id:diagnose-fix-dry-run-changes-nothing Scenario: diagnose-05 - Previewing a fix explains the change without making it Given a user who has chosen a known fix When the user previews that fix without applying it Then the CLI describes what the fix would change And nothing on the machine is changed + And the preview states plainly that this fix is manual only @id:diagnose-fix-unknown-id-rejected Scenario: diagnose-06 - Asking for a fix the CLI does not know is refused clearly @@ -266,3 +272,20 @@ Feature: Diagnosing failures and listing fixes When the user asks the CLI to diagnose that machine Then the CLI refuses and explains that it could not reach that machine And no diagnosis of this machine is reported + + # 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 + 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 diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 153257e86..fb6d2da6a 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -201,6 +201,11 @@ async fn user_chose_known_fix(world: &mut E2eWorld) { world.model_name = Some(PREVIEW_FIX_ID.to_string()); } +#[given("a user who has chosen a fix that needs sudo and a re-login")] +async fn user_chose_fix_needing_sudo_and_relogin(world: &mut E2eWorld) { + world.model_name = Some(COMMAND_FAILURE_FIX_ID.to_string()); +} + #[given("a user who names a fix the CLI does not offer")] async fn user_named_unknown_fix(world: &mut E2eWorld) { world.model_name = Some("fix-does-not-exist".to_string()); @@ -421,6 +426,40 @@ async fn assert_every_cause_has_a_command(world: &mut E2eWorld) { ); } +#[then("every reported cause states its remediation flags")] +async fn assert_every_cause_has_flags(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no diagnose output"); + // `flags:` is the line `render_report_text` builds from + // `crate::fix::format_flags` -- the same helper `rocm fix `'s `Flags:` + // line uses, so the same flag values render as the same text from either + // command. This is the only scenario that exercises that line through the + // real `rocm diagnose` rendering surface rather than through `rocm fix + // --dry-run`. Assert the shape (present once per cause, ending in + // the always-on auto/manual marker) rather than a specific fix-id's exact + // flags: the top match is environment-dependent, and a shared vocabulary + // doesn't guarantee diagnose and the fix.rs catalog agree on the + // underlying values for a given fix-id (known drift: fix-5-amdgpu-load's + // needs_reboot). + let causes = output.lines().filter(|l| l.contains("score=")).count(); + assert!(causes > 0, "no scored causes to check:\n{output}"); + let flag_lines: Vec<&str> = output + .lines() + .filter(|l| l.trim_start().starts_with("flags:")) + .collect(); + assert_eq!( + flag_lines.len(), + causes, + "each of the {causes} causes needs its own flags: line:\n{output}" + ); + for line in &flag_lines { + assert!( + line.contains("rocm fix can run it") + || line.contains("manual only (`rocm fix` will NOT run it automatically)"), + "expected the auto/manual marker on the flags: line:\n{line}" + ); + } +} + #[then("the listing explains what those indicators mean")] async fn assert_markers_explained(world: &mut E2eWorld) { let output = world.cli_output.as_ref().expect("no fix list output"); @@ -911,6 +950,42 @@ async fn assert_describes_change(world: &mut E2eWorld) { ); } +#[then("the preview states plainly that this fix is manual only")] +async fn assert_preview_states_manual_only(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix preview output"); + assert!( + output.contains("Flags: manual only (`rocm fix` will NOT run it automatically)"), + "expected a bare manual-only Flags: line for {PREVIEW_FIX_ID}, with no \ + sudo/reboot/re-login flags ahead of it:\n{output}" + ); +} + +// Exercises COMMAND_FAILURE_FIX_ID (fix-4-render-group: needs_sudo + +// needs_relogin + auto_applicable), the only catalog entry that combines sudo, +// re-login, and AUTO in one recipe -- so it is the one place that can prove +// `format_flags` renders more than one optional flag, and the auto-applicable +// line, from a real `rocm fix --dry-run` invocation. Deliberately checked +// as one line, not three separate `contains`, so a regression that reordered +// the flags (e.g. put re-login before sudo) would also be caught. +#[then("the preview states that the fix requires sudo and a re-login")] +async fn assert_preview_states_sudo_and_relogin(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix preview output"); + assert!( + output.contains("Flags: requires sudo, requires re-login,"), + "expected sudo and re-login flags, in that order, for \ + {COMMAND_FAILURE_FIX_ID}:\n{output}" + ); +} + +#[then("the preview states that the CLI can run it automatically")] +async fn assert_preview_states_auto_applicable(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix preview output"); + assert!( + output.contains("rocm fix can run it"), + "expected the auto-applicable flag text for {COMMAND_FAILURE_FIX_ID}:\n{output}" + ); +} + #[then("nothing on the machine is changed")] async fn assert_no_mutation(world: &mut E2eWorld) { // A dry-run must not write MANAGED STATE. It may still create incidental