diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 79b6ec5e6..9a3484c8c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1039,6 +1039,10 @@ jobs: if: needs.changes.outputs.rust == 'true' run: cargo xtask check-crate-edges + - name: Published Doctor catalog is current + if: needs.changes.outputs.rust == 'true' + run: cargo xtask catalog --check + # Surface the cache hit rate for the clippy compile so the effect is visible # in the logs, on the local-only path too — see build-and-test for why this # is gated on sccache being present rather than on the remote backend being diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index de20d2cb5..0af2f5148 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -171,6 +171,18 @@ repos: pass_filenames: false stages: [pre-commit] groups: [local-tools] # compiles xtask; CI covers it in the `clippy` job + - id: cargo-catalog + name: Published Doctor catalog is current + # The catalog the CLI ships and the file tools read have to be the same + # thing. Write form, like cargo-manifest: prek fails the commit when a + # hook rewrites a file, so a stale catalog is refreshed and re-staged + # rather than merely reported. + entry: cargo xtask catalog + language: system + files: '^(crates/rocm-core/src/fix\.rs|xtask/.*|doctor-catalog\.json)$' + pass_filenames: false + stages: [pre-commit] + groups: [local-tools] # compiles xtask; CI covers it in the `clippy` job - id: cargo-tpn name: THIRD_PARTY_NOTICES.txt is current # `--if-available` skips cleanly when cargo-about is absent or not the diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 8f2a34055..499620bfe 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -221,6 +221,9 @@ enum Command { /// For fix-9-igpu-dgpu: the discrete GPU index to pin. #[arg(long)] device_index: Option, + /// Emit the catalog as JSON for tooling. Only valid without a fix id. + #[arg(long)] + json: bool, }, /// Print the rocm-cli version, release tag or branch, and commit hash, /// plus the ROCm SDK and GPU driver this machine would use. @@ -2135,7 +2138,8 @@ fn dispatch(cli: Cli) -> Result<()> { yes, dry_run, device_index, - }) => fix(fix_id, yes, dry_run, device_index), + json, + }) => fix(fix_id, yes, dry_run, device_index, json), Some(Command::Version) => version(), Some(Command::Setup { command }) => setup(command), Some(Command::EngineServeHttp { @@ -3243,11 +3247,30 @@ fn show_prepared_report( } } -fn fix(fix_id: Option, yes: bool, dry_run: bool, device_index: Option) -> Result<()> { +fn fix( + fix_id: Option, + yes: bool, + dry_run: bool, + device_index: Option, + json: bool, +) -> Result<()> { let Some(fix_id) = fix_id else { - print!("{}", rocm_core::list_fix_recipes()); + if json { + print!("{}", rocm_core::catalog_manifest_json()?); + } else { + print!("{}", rocm_core::list_fix_recipes()); + } return Ok(()); }; + // Refused rather than ignored. "Apply this fix, as JSON" has no meaning, and + // quietly dropping the flag would let a caller believe it had asked for + // machine-readable output and got it. + if json { + anyhow::bail!( + "`--json` describes the whole catalog, so it cannot be combined with a fix id. \ + Run `rocm fix --json` to read the catalog, or `rocm fix {fix_id}` to apply this fix." + ); + } let opts = rocm_core::FixOptions { yes, dry_run, @@ -22974,6 +22997,7 @@ mod tests { yes: true, dry_run: false, device_index: None, + json: false, }), }; let result = super::dispatch(cli); diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index ff7e87177..18de40b5c 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -10,8 +10,9 @@ //! print the exact change, honor `--dry-run`, refuse on a non-interactive shell //! without `--yes`, and otherwise confirm before mutating anything. //! -//! Exit codes match `apply_fix.py`: `0` ok/dry-run/print-only, `2` unknown id, -//! `3` environment/OS not right, `4` a command failed, `5` user declined. +//! Exit codes are named in [`exit`] and published in the catalog manifest, so +//! the number, its meaning, and what a reader is told it means all come from one +//! place. use crate::examine::{run, which}; use crate::{runtime_is_linux, runtime_is_windows}; @@ -30,6 +31,30 @@ const QUERY_TIMEOUT: Duration = Duration::from_secs(8); /// apart in wording before they shared this constant. pub(crate) const COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED: &str = "Neither option is recommended over the other: which is right depends on which stack you mean to keep, and removing the wrong one breaks a working environment."; +/// The exit codes `apply` returns, named so the published manifest and the code +/// that produces them cannot come to disagree. +/// +/// These were written down only in the module comment above. A consumer reading +/// the manifest is being told what a number means, so the number and its meaning +/// have to come from one place -- a manifest that restated them by hand would be +/// the same defect this whole contract exists to remove, one level up. +pub mod exit { + /// Applied, previewed with `--dry-run`, printed, or listed. Nothing failed. + pub const OK: i32 = 0; + /// The CLI is inconsistent with itself -- a recipe it says it can carry out, + /// with nothing to carry it out with. Never a fault of the machine. + pub const INTERNAL: i32 = 1; + /// No such fix-id. + pub const UNKNOWN_ID: i32 = 2; + /// Real, but not for this machine: wrong platform, or a precondition the + /// host does not meet. Distinct from [`FAILED`], which means it was tried. + pub const NOT_APPLICABLE: i32 = 3; + /// Attempted, and a command it ran failed. + pub const FAILED: i32 = 4; + /// The user was asked and said no. + pub const DECLINED: i32 = 5; +} + /// Print a failure explanation to stderr, ignoring write failures (closed /// stderr, full disk) so an I/O error while explaining a failure can't itself /// panic the process. @@ -199,6 +224,10 @@ struct FixRecipe { needs_relogin: bool, verify: &'static str, notes: &'static [&'static str], + /// Where to read more about this failure. `None` for most entries today -- + /// an empty answer beats a guessed one, which would send a user somewhere + /// wrong with an air of authority. + doc_url: Option<&'static str>, /// Every operating system this applies on, and what it does on each. applies_on: &'static [Platform], runner: Option i32>, @@ -250,6 +279,9 @@ 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.", ], + doc_url: Some( + "https://rocm.docs.amd.com/projects/install-on-linux/en/latest/reference/system-requirements.html", + ), applies_on: PRINT_ON_LINUX_WINDOWS_AND_WSL, runner: None, }, @@ -270,6 +302,7 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "env | grep HSA_OVERRIDE_GFX_VERSION || echo OK_UNSET", notes: &[], + doc_url: None, applies_on: PRINT_ON_LINUX_AND_WSL_AUTO_ON_WINDOWS, runner: Some(run_unset_override), }, @@ -287,6 +320,9 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "lsmod | grep amdgpu && rocminfo | head -n 5", notes: &[], + doc_url: Some( + "https://rocm.docs.amd.com/projects/install-on-linux/en/latest/reference/system-requirements.html", + ), applies_on: PRINT_ON_LINUX, runner: None, }, @@ -300,6 +336,7 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: true, verify: "groups | tr ' ' '\\n' | grep -E '^(render|video)$' && rocminfo | head -n 5", notes: &[], + doc_url: None, applies_on: AUTO_ON_LINUX, runner: Some(run_render_group), }, @@ -321,6 +358,7 @@ 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.", ], + doc_url: None, applies_on: PRINT_ON_LINUX, runner: None, }, @@ -339,6 +377,7 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "rocminfo | head -n 5 && hipcc --version", notes: &[], + doc_url: None, applies_on: AUTO_ON_LINUX_WINDOWS_AND_WSL, runner: Some(run_path_export), }, @@ -357,6 +396,7 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "sudo apt update 2>&1 | tail -n 20", notes: &[], + doc_url: None, applies_on: PRINT_ON_LINUX, runner: None, }, @@ -378,6 +418,9 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "python -c \"import torch; print(torch.__version__, torch.version.hip, torch.cuda.is_available())\"", notes: &[], + doc_url: Some( + "https://rocm.docs.amd.com/projects/install-on-linux/en/latest/reference/system-requirements.html", + ), applies_on: PRINT_ON_LINUX_WINDOWS_AND_WSL, runner: None, }, @@ -400,6 +443,7 @@ 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.", ], + doc_url: None, applies_on: NEEDS_DEVICE_INDEX, runner: Some(run_hip_visible_devices), }, @@ -423,6 +467,7 @@ 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.", ], + doc_url: None, applies_on: PRINT_ON_LINUX, runner: None, }, @@ -442,6 +487,7 @@ const RECIPES: &[FixRecipe] = &[ needs_relogin: false, verify: "cat /proc/cmdline | grep -o 'iommu=\\w*'", notes: &[], + doc_url: None, applies_on: PRINT_ON_LINUX, runner: None, }, @@ -462,6 +508,7 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "If `apt autoremove --purge` warns it will remove unrelated packages, stop and resolve those by hand before continuing.", ], + doc_url: None, applies_on: PRINT_ON_LINUX, runner: None, }, @@ -481,6 +528,7 @@ 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.", ], + doc_url: Some("https://www.amd.com/en/developer/resources/rocm-hub/hip-sdk.html"), applies_on: PRINT_ON_WINDOWS, runner: None, }, @@ -500,6 +548,9 @@ 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: &[], + doc_url: Some( + "https://rocm.docs.amd.com/projects/install-on-windows/en/latest/install/install.html", + ), applies_on: PRINT_ON_WINDOWS, runner: None, }, @@ -519,6 +570,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.", ], + doc_url: Some("https://aka.ms/vs/17/release/vc_redist.x64.exe"), applies_on: PRINT_ON_WINDOWS, runner: None, }, @@ -566,6 +618,7 @@ 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.", ], + doc_url: None, applies_on: PRINT_ON_LINUX, runner: None, }, @@ -587,6 +640,7 @@ 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.", ], + doc_url: None, applies_on: PRINT_ON_WSL, runner: None, }, @@ -607,6 +661,7 @@ 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."], + doc_url: None, applies_on: PRINT_ON_WSL, runner: None, }, @@ -627,6 +682,7 @@ 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.", ], + doc_url: None, applies_on: PRINT_ON_WSL, runner: None, }, @@ -642,6 +698,7 @@ 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.", ], + doc_url: None, applies_on: PRINT_ON_WSL, runner: None, }, @@ -660,6 +717,7 @@ const RECIPES: &[FixRecipe] = &[ notes: &[ "Distros install side by side, so the current one can stay until the new one is set up.", ], + doc_url: None, applies_on: PRINT_ON_WSL, runner: None, }, @@ -679,6 +737,7 @@ 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.", ], + doc_url: None, applies_on: PRINT_ON_WSL, runner: None, }, @@ -698,6 +757,7 @@ 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.", ], + doc_url: None, applies_on: PRINT_ON_WSL, runner: None, }, @@ -728,6 +788,7 @@ const RECIPES: &[FixRecipe] = &[ COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED, "This describes the environment outside the CLI's managed runtimes. `rocm serve` puts a managed runtime's libraries first on purpose, so inside one the wheel copy wins by design and that is correct.", ], + doc_url: None, // Not `PRINT_ON_LINUX`: which copy the loader picks has nothing to do // with the amdgpu module, and the two copies collide on WSL2 just the // same. Print-only on both: neither way out can be chosen for the user. @@ -760,6 +821,7 @@ 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.", ], + doc_url: None, // 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. // Print-only on both because every remedy above needs sudo. @@ -981,6 +1043,108 @@ fn looks_like_a_diagnosis_position(value: &str) -> bool { !digits.is_empty() && digits.chars().all(|c| c.is_ascii_digit()) } +/// The version of the agreement between this binary and the tools that read its +/// catalog. +/// +/// Rules, following `ENGINE_RECIPE_CONTRACT_VERSION` in the engine protocol: an +/// **addition** keeps the version — a new entry, a new field, a doc URL filled +/// in. A **removal or a type change** raises it. A reader declares the minimum +/// it understands and stops below it rather than guessing. +pub const CATALOG_CONTRACT_VERSION: u32 = 1; + +/// What the CLI does with one entry on one platform. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ManifestPlatform { + pub os: String, + pub class: FixClass, + /// The argument [`FixClass::NeedsArgument`] is waiting for; absent otherwise. + #[serde(skip_serializing_if = "Option::is_none")] + pub needs: Option, +} + +/// One catalog entry, as published. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ManifestEntry { + pub id: String, + pub title: String, + /// Every platform this applies on, and what it does there. + /// + /// A list rather than one class for the entry: an entry can apply on Linux + /// as print-only and on Windows as auto, and flattening that is precisely + /// the defect the class model was introduced to remove. + pub platforms: Vec, + pub needs_sudo: bool, + pub needs_reboot: bool, + pub needs_relogin: bool, + /// Where to read more. Absent for most entries today; filling one in later + /// is an addition and does not raise the contract version. + #[serde(skip_serializing_if = "Option::is_none")] + pub doc_url: Option, +} + +/// The published description of the catalog. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct CatalogManifest { + pub contract_version: u32, + pub entries: Vec, + /// What each exit code from `rocm fix` means. + pub exit_codes: std::collections::BTreeMap, +} + +/// Describe the catalog for a machine to read. +/// +/// Derived from `RECIPES` on every call, never stored alongside it. The whole +/// point is that there is one catalog and one description of it; a second copy +/// maintained by hand is the drift this is meant to end. +#[must_use] +pub fn catalog_manifest() -> CatalogManifest { + CatalogManifest { + contract_version: CATALOG_CONTRACT_VERSION, + entries: RECIPES + .iter() + .map(|r| ManifestEntry { + id: r.fix_id.to_owned(), + title: r.title.to_owned(), + platforms: r + .applies_on + .iter() + .map(|p| ManifestPlatform { + os: p.os.to_owned(), + class: p.class, + needs: p.needs.map(ToOwned::to_owned), + }) + .collect(), + needs_sudo: r.needs_sudo, + needs_reboot: r.needs_reboot, + needs_relogin: r.needs_relogin, + doc_url: r.doc_url.map(ToOwned::to_owned), + }) + .collect(), + exit_codes: [ + ("ok", exit::OK), + ("internal", exit::INTERNAL), + ("unknown_id", exit::UNKNOWN_ID), + ("not_applicable", exit::NOT_APPLICABLE), + ("failed", exit::FAILED), + ("declined", exit::DECLINED), + ] + .into_iter() + .map(|(name, code)| (name.to_owned(), code)) + .collect(), + } +} + +/// The manifest as the published file holds it: pretty-printed, newline +/// terminated, so a catalog change reads as a reviewable diff rather than one +/// very long line. +/// +/// # Errors +/// When the manifest cannot be serialized, which would mean a type in it stopped +/// being serializable. +pub fn catalog_manifest_json() -> Result { + Ok(serde_json::to_string_pretty(&catalog_manifest())? + "\n") +} + /// List every fix-id (id, class on this machine, OS scope, title). /// /// The marker is what the entry does **here**, not everywhere: `fix-2` reports @@ -1107,7 +1271,7 @@ pub fn apply(fix_id: &str, opts: &FixOptions) -> i32 { } else { fail!("Run `rocm diagnose` to see which fix-id applies."); } - return 2; + return exit::UNKNOWN_ID; }; act_on(recipe, opts) } @@ -1177,26 +1341,27 @@ fn act_on(recipe: &FixRecipe, opts: &FixOptions) -> i32 { recipe.os_scope().join(", "), current_os() ); - 3 + exit::NOT_APPLICABLE } 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 + // could, which is name the problem. NOT_APPLICABLE would say "not + // applicable *here*", a 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 + exit::OK } 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`. + // carry it out with -> INTERNAL, not FAILED (which is reserved for + // "attempted, and 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 + exit::INTERNAL } Plan::PrintSteps => { println!("This fix is print-only (manual change required)."); @@ -1204,7 +1369,7 @@ fn act_on(recipe: &FixRecipe, opts: &FixOptions) -> i32 { if !recipe.verify.is_empty() { println!(" $ {}", recipe.verify); } - 0 + exit::OK } } } @@ -2045,6 +2210,7 @@ mod tests { needs_relogin: false, verify: "", notes: &[], + doc_url: None, applies_on: Box::leak(Box::new([on(current_os(), class)])), runner: None, } @@ -2233,6 +2399,97 @@ mod tests { /// 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 the_manifest_describes_every_entry_exactly_once() { + let manifest = catalog_manifest(); + let published: Vec<&str> = manifest.entries.iter().map(|e| e.id.as_str()).collect(); + let catalog: Vec<&str> = RECIPES.iter().map(|r| r.fix_id).collect(); + // Order too, not just membership: the published file is read as a diff, + // and a reordering that changed nothing would still churn review. + assert_eq!( + published, catalog, + "the manifest and the catalog must list the same entries in the same order" + ); + } + + #[test] + fn the_manifest_repeats_the_catalog_rather_than_reinterpreting_it() { + // The manifest exists so a reader does not have to parse prose. If it + // said anything the catalog does not, it would be a third copy to keep + // correct -- which is the defect, not the fix. + for entry in catalog_manifest().entries { + let recipe = find_recipe(&entry.id).expect("every published entry is in the catalog"); + let published: Vec<(&str, FixClass)> = entry + .platforms + .iter() + .map(|p| (p.os.as_str(), p.class)) + .collect(); + let actual: Vec<(&str, FixClass)> = + recipe.applies_on.iter().map(|p| (p.os, p.class)).collect(); + assert_eq!(published, actual, "{}: platform classes drifted", entry.id); + assert_eq!(entry.needs_sudo, recipe.needs_sudo, "{}", entry.id); + assert_eq!(entry.needs_reboot, recipe.needs_reboot, "{}", entry.id); + assert_eq!(entry.needs_relogin, recipe.needs_relogin, "{}", entry.id); + } + } + + #[test] + fn the_published_exit_codes_are_the_ones_apply_returns() { + // Reachable without a machine in a particular state, so they are worth + // pinning here: an unknown id and a recipe for another platform. + let manifest = catalog_manifest(); + assert_eq!( + apply("fix-does-not-exist", &FixOptions::default()), + manifest.exit_codes["unknown_id"], + "an unknown id must exit with the code the manifest publishes for it" + ); + assert_eq!( + apply(fix_id_for_another_platform(), &FixOptions::default()), + manifest.exit_codes["not_applicable"], + "a recipe for another platform must exit with the published code" + ); + // The map is a contract in itself: dropping a name would leave a reader + // unable to interpret a code it still receives. + for name in [ + "ok", + "internal", + "unknown_id", + "not_applicable", + "failed", + "declined", + ] { + assert!( + manifest.exit_codes.contains_key(name), + "the published exit-code map is missing `{name}`" + ); + } + } + + /// A catalog entry that cannot apply on the platform running the test. + fn fix_id_for_another_platform() -> &'static str { + RECIPES + .iter() + .find(|r| r.platform(current_os()).is_none()) + .map(|r| r.fix_id) + .expect("every platform has an entry belonging to a different one") + } + + #[test] + fn a_published_doc_url_is_a_url() { + // Most entries carry none, which is the honest answer until someone + // sources one. What must not happen is a value that looks like a link + // and is not, since that is what sends a reader somewhere wrong. + for entry in catalog_manifest().entries { + if let Some(url) = entry.doc_url { + assert!( + url.starts_with("https://"), + "{}: doc_url is not a URL: {url}", + entry.id + ); + } + } + } + #[test] fn only_these_entries_act_on_the_machine_and_only_on_these_platforms() { let acts: Vec<(&str, &str, FixClass)> = classes() diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 4bb8e43bd..dddd0a842 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -54,7 +54,11 @@ use examine::extract_rocm_version; pub use examine::{ Examination, FrameworkProbe, WSL_PLATFORM_NOTE, gfx_is_apu_family, probe_wsl_distro_from_host, }; -pub use fix::{FixOptions, apply as apply_fix, list_recipes as list_fix_recipes}; +pub use fix::{ + CATALOG_CONTRACT_VERSION, CatalogManifest, FixOptions, ManifestEntry, ManifestPlatform, + apply as apply_fix, catalog_manifest, catalog_manifest_json, exit as fix_exit, + list_recipes as list_fix_recipes, +}; pub use proc_lifecycle::{ IdentityState, KillScope, ProcessIdentity, TerminationOutcome, identity_state, process_start_ticks, terminate_verified, diff --git a/doctor-catalog.json b/doctor-catalog.json new file mode 100644 index 000000000..d37960451 --- /dev/null +++ b/doctor-catalog.json @@ -0,0 +1,390 @@ +{ + "contract_version": 1, + "entries": [ + { + "id": "fix-1-arch", + "title": "GPU gfx target not in framework arch list", + "platforms": [ + { + "os": "linux", + "class": "print-only" + }, + { + "os": "windows", + "class": "print-only" + }, + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false, + "doc_url": "https://rocm.docs.amd.com/projects/install-on-linux/en/latest/reference/system-requirements.html" + }, + { + "id": "fix-2-unset-override", + "title": "Unset HSA_OVERRIDE_GFX_VERSION", + "platforms": [ + { + "os": "linux", + "class": "print-only" + }, + { + "os": "windows", + "class": "auto" + }, + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-3-rocm-kernel", + "title": "ROCm/distro/kernel triple unsupported", + "platforms": [ + { + "os": "linux", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": true, + "needs_relogin": false, + "doc_url": "https://rocm.docs.amd.com/projects/install-on-linux/en/latest/reference/system-requirements.html" + }, + { + "id": "fix-4-render-group", + "title": "Add user to render/video groups", + "platforms": [ + { + "os": "linux", + "class": "auto" + } + ], + "needs_sudo": true, + "needs_reboot": false, + "needs_relogin": true + }, + { + "id": "fix-5-amdgpu-load", + "title": "Load amdgpu (and clear any blacklist)", + "platforms": [ + { + "os": "linux", + "class": "print-only" + } + ], + "needs_sudo": true, + "needs_reboot": true, + "needs_relogin": false + }, + { + "id": "fix-6-path", + "title": "Add the ROCm/HIP bin directory to PATH", + "platforms": [ + { + "os": "linux", + "class": "auto" + }, + { + "os": "windows", + "class": "auto" + }, + { + "os": "wsl", + "class": "auto" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-7-stale-repos", + "title": "Quarantine duplicate AMD repos", + "platforms": [ + { + "os": "linux", + "class": "print-only" + } + ], + "needs_sudo": true, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-8-wheel-rocm", + "title": "Reinstall the framework against the system ROCm/HIP major", + "platforms": [ + { + "os": "linux", + "class": "print-only" + }, + { + "os": "windows", + "class": "print-only" + }, + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false, + "doc_url": "https://rocm.docs.amd.com/projects/install-on-linux/en/latest/reference/system-requirements.html" + }, + { + "id": "fix-9-igpu-dgpu", + "title": "Hide the iGPU with HIP_VISIBLE_DEVICES", + "platforms": [ + { + "os": "linux", + "class": "needs-argument", + "needs": "--device-index" + }, + { + "os": "windows", + "class": "needs-argument", + "needs": "--device-index" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-10-container", + "title": "Re-launch the container with AMD devices passed through", + "platforms": [ + { + "os": "linux", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-11-iommu", + "title": "Add iommu=pt to the kernel command line", + "platforms": [ + { + "os": "linux", + "class": "print-only" + } + ], + "needs_sudo": true, + "needs_reboot": true, + "needs_relogin": false + }, + { + "id": "fix-12-installer", + "title": "Reset amdgpu-install state and reinstall", + "platforms": [ + { + "os": "linux", + "class": "print-only" + } + ], + "needs_sudo": true, + "needs_reboot": true, + "needs_relogin": false + }, + { + "id": "fix-13-hip-sdk-missing", + "title": "Install the AMD HIP SDK for Windows", + "platforms": [ + { + "os": "windows", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false, + "doc_url": "https://www.amd.com/en/developer/resources/rocm-hub/hip-sdk.html" + }, + { + "id": "fix-14-adrenalin-too-old", + "title": "Update the Adrenalin / kernel-mode driver", + "platforms": [ + { + "os": "windows", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": true, + "needs_relogin": false, + "doc_url": "https://rocm.docs.amd.com/projects/install-on-windows/en/latest/install/install.html" + }, + { + "id": "fix-15-msvc-redist", + "title": "Install the MSVC 2015-2022 runtime redistributable", + "platforms": [ + { + "os": "windows", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false, + "doc_url": "https://aka.ms/vs/17/release/vc_redist.x64.exe" + }, + { + "id": "fix-17-torch-dlpack", + "title": "Restore the engine's pinned torch (torch-c-dlpack-ext loads the CUDA variant)", + "platforms": [ + { + "os": "linux", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-wsl-1-gpu-not-exposed", + "title": "Expose the GPU to the WSL distro (/dev/dxg)", + "platforms": [ + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-wsl-2-dxcore-missing", + "title": "Restore the WSL DXCore libraries", + "platforms": [ + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": true, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-wsl-3-rocdxg-missing", + "title": "Install ROCDXG in the WSL distro", + "platforms": [ + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": true, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-wsl-4-rocdxg-not-linked", + "title": "Refresh the linker cache so ROCDXG is loadable", + "platforms": [ + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": true, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-wsl-5-distro-too-old", + "title": "Move to a distro release the WSL path supports", + "platforms": [ + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-wsl-6-host-driver-too-old", + "title": "Update the AMD driver on the Windows host", + "platforms": [ + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-wsl-7-wsl1", + "title": "Convert the distro from WSL 1 to WSL 2", + "platforms": [ + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-18-comgr-conflict", + "title": "Code object manager library does not belong to the active HIP runtime", + "platforms": [ + { + "os": "linux", + "class": "print-only" + }, + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": false, + "needs_reboot": false, + "needs_relogin": false + }, + { + "id": "fix-19-shm-too-small", + "title": "Raise the shared memory allowance", + "platforms": [ + { + "os": "linux", + "class": "print-only" + }, + { + "os": "wsl", + "class": "print-only" + } + ], + "needs_sudo": true, + "needs_reboot": false, + "needs_relogin": false + } + ], + "exit_codes": { + "declined": 5, + "failed": 4, + "internal": 1, + "not_applicable": 3, + "ok": 0, + "unknown_id": 2 + } +} diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index 8adde616e..a09b0d614 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -503,3 +503,24 @@ Feature: Diagnosing failures and listing fixes When the user asks the CLI to apply that fix Then the CLI explains that it will not make the change itself And the CLI offers both options without ranking them + + # The skill that drives this CLI states, in prose, which entries exist and what + # the CLI does with each. Every one of those is a copy of something the binary + # already knows, and a copy can go stale silently. This is the machine-readable + # form that replaces the copying — what a tool reads instead of parsing the + # listing meant for people. + @id:diagnose-fix-catalog-is-machine-readable + Scenario: diagnose-34 - A tool can read the catalog without parsing prose + When a tool asks the CLI for its catalog in machine-readable form + Then the catalog names every entry and what the CLI does with each + And it gives the meaning of every exit code the CLI can return + + # `--json` describes the whole catalog, so pairing it with one entry is a + # question with no answer. Refusing beats ignoring the flag: a caller that + # asked for machine-readable output and silently got something else has no way + # to notice. + @id:diagnose-fix-catalog-json-rejects-a-fix-id + Scenario: diagnose-35 - Asking to apply one fix in machine-readable form is refused + Given a user who has chosen a known fix + When the user asks the CLI to apply that fix in machine-readable form + Then the CLI refuses and explains that the two cannot be combined diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 468fbec51..488d0545b 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -1400,6 +1400,14 @@ async fn user_asks_whether_a_model_would_run(world: &mut E2eWorld) { world.cli_rc = Some(rc); } +#[when("a tool asks the CLI for its catalog in machine-readable form")] +async fn tool_reads_catalog(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm(world, &["fix", "--json"]); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + #[then("the model is never reported as ready")] async fn assert_model_is_never_ready(world: &mut E2eWorld) { assert_eq!( @@ -2195,3 +2203,102 @@ async fn assert_both_options_unranked(world: &mut E2eWorld) { depends on which stack the user means to keep:\n{output}" ); } + +#[when("the user asks the CLI to apply that fix in machine-readable form")] +async fn user_applies_fix_as_json(world: &mut E2eWorld) { + let fix_id = world.model_name.clone().expect("no fix id set"); + let (stdout, stderr, rc) = crate::run_rocm(world, &["fix", &fix_id, "--json"]); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + +#[then("the catalog names every entry and what the CLI does with each")] +async fn assert_catalog_is_complete(world: &mut E2eWorld) { + assert_eq!(world.cli_rc, Some(0), "reading the catalog is a query"); + let output = world.cli_output.as_ref().expect("no catalog output"); + let catalog: serde_json::Value = serde_json::from_str(output) + .unwrap_or_else(|e| panic!("the catalog is not machine-readable ({e}):\n{output}")); + + // Against the same pinned list the human listing is held to, so the two + // forms cannot come to describe different catalogs. + let ids: Vec<&str> = catalog["entries"] + .as_array() + .unwrap_or_else(|| panic!("no entries in the catalog:\n{catalog:#}")) + .iter() + .filter_map(|e| e["id"].as_str()) + .collect(); + assert_eq!( + ids, CATALOG_FIX_IDS, + "the published catalog and the documented one disagree" + ); + + // Per platform, not per entry: an entry can be applied on one platform and + // only explained on another, and a reader that could not see that would be + // told the wrong thing on one of them. + for entry in catalog["entries"].as_array().expect("entries") { + let platforms = entry["platforms"] + .as_array() + .unwrap_or_else(|| panic!("{} names no platforms:\n{entry:#}", entry["id"])); + assert!( + !platforms.is_empty(), + "{} applies nowhere, which cannot be right:\n{entry:#}", + entry["id"] + ); + for platform in platforms { + assert!( + platform["os"].is_string() && platform["class"].is_string(), + "{} does not say what it does on a platform it applies to:\n{platform:#}", + entry["id"] + ); + } + } + assert!( + catalog["contract_version"].is_number(), + "a reader cannot tell whether it understands this catalog:\n{catalog:#}" + ); +} + +#[then("it gives the meaning of every exit code the CLI can return")] +async fn assert_exit_codes_published(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no catalog output"); + let catalog: serde_json::Value = serde_json::from_str(output).expect("catalog parses"); + let codes = catalog["exit_codes"] + .as_object() + .unwrap_or_else(|| panic!("the catalog publishes no exit codes:\n{catalog:#}")); + // Naming them is the point: a caller that receives a 3 and cannot look it up + // is back to guessing, which is what the prose it replaces forced it to do. + for name in [ + "ok", + "internal", + "unknown_id", + "not_applicable", + "failed", + "declined", + ] { + assert!( + codes.contains_key(name), + "the published exit codes are missing `{name}`:\n{catalog:#}" + ); + } +} + +#[then("the CLI refuses and explains that the two cannot be combined")] +async fn assert_json_with_fix_id_refused(world: &mut E2eWorld) { + let stderr = world.cli_stderr.as_ref().expect("no stderr"); + assert_ne!( + world.cli_rc, + Some(0), + "a request with no answer must not look like it succeeded" + ); + assert!( + stderr.contains("--json"), + "the refusal has to name what was wrong with the request:\n{stderr}" + ); + // Nothing may be emitted that a caller could mistake for the catalog. + let stdout = world.cli_output.as_ref().map_or("", String::as_str); + assert!( + serde_json::from_str::(stdout).is_err(), + "a refused request still produced machine-readable output:\n{stdout}" + ); +} diff --git a/xtask/src/catalog.rs b/xtask/src/catalog.rs new file mode 100644 index 000000000..a3dd59958 --- /dev/null +++ b/xtask/src/catalog.rs @@ -0,0 +1,64 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Regenerate (or verify) the published Doctor catalog manifest. +//! +//! The manifest is what a tool reads instead of parsing the CLI's prose: every +//! entry, what the CLI does with each on each platform, and the meaning of every +//! exit code. It is rendered from the compiled catalog by +//! [`rocm_core::catalog_manifest_json`], so this never restates it -- the point +//! of the whole contract is that the catalog is described in exactly one place. +//! +//! The checked-in copy is a guard rather than the contract. Its value is that a +//! catalog change shows up in the pull request diff, in the form a consumer +//! reads, which is what makes deciding whether to raise `contract_version` a +//! deliberate act rather than something noticed afterwards. + +use std::fs; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result, bail}; + +use crate::paths; + +/// Published at the repository root, beside the other generated contract +/// artifacts (`MANIFEST.md`, `THIRD_PARTY_NOTICES.txt`). +const CATALOG_FILE: &str = "doctor-catalog.json"; + +fn catalog_path(root: &Path) -> PathBuf { + root.join(CATALOG_FILE) +} + +/// Regenerate (or, with `check`, verify) the published catalog manifest. +/// +/// # Errors +/// When the workspace root cannot be found, the file cannot be read or written, +/// or `check` is set and the file is out of date. +pub fn run(check: bool) -> Result<()> { + let root = paths::workspace_root()?; + let path = catalog_path(&root); + let rendered = + rocm_core::catalog_manifest_json().context("failed to render the catalog manifest")?; + + // Missing counts as out of date rather than as an error, so the first run + // and every later one behave the same way. + let current = fs::read_to_string(&path).unwrap_or_default(); + + if check { + if current != rendered { + bail!( + "{CATALOG_FILE} is out of date; run `cargo xtask catalog` to update it.\n\ + The catalog changed, so decide whether this is an addition (which keeps \ + contract_version) or a removal or type change (which raises it)." + ); + } + return Ok(()); + } + + if current != rendered { + fs::write(&path, rendered) + .with_context(|| format!("failed to write {}", path.display()))?; + } + Ok(()) +} diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 37b6d89bd..d7709fc39 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -12,6 +12,7 @@ mod affected; mod architecture_doc; +mod catalog; mod crate_edges; mod demos; mod e2e; @@ -109,6 +110,13 @@ enum Command { /// expected location per citation; see `citation_exists`'s doc comment /// in `xtask/src/architecture_doc.rs` for the full rule. CheckArchitectureDoc, + /// Regenerate the published Doctor catalog manifest from the compiled catalog. + Catalog { + /// Verify the published manifest is current without writing; exit + /// non-zero if it would change. + #[arg(long)] + check: bool, + }, /// Regenerate the Cargo dependency table in MANIFEST.md from `cargo metadata`. Manifest { /// Verify the table is up to date without writing; exit non-zero if it would change. @@ -257,6 +265,7 @@ fn run() -> Result<()> { Command::Affected { base } => affected::run(base)?, Command::CheckCrateEdges => crate_edges::run()?, Command::CheckArchitectureDoc => architecture_doc::run()?, + Command::Catalog { check } => catalog::run(check)?, Command::Manifest { check } => manifest::run(check)?, Command::Tpn { check,