Repository navigation
docs: propose which host-probing stack in rocm-core owns each fact - #944
Conversation
rocm examine reads several host facts through two separate stacks: the summary behind the text form (and the summary object of --json), and the examination behind the rest of --json and rocm diagnose. This adds a design note that, for each duplicate probe, names both implementations, how they differ today, what a user can see, a proposed surviving implementation and the order in which the pairs could converge. It proposes only; no code or behaviour changes. The note is linked from the module map. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
There was a problem hiding this comment.
🟡 Changes recommended
The documentation still understates fallback query counts, conflates shared implementations with reused observations, and omits observable PATH lookup differences and required scenarios from the convergence plan.
3 open issues
What changed in this PR
This docs-only PR proposes how rocm-core’s summary and examination stacks should converge on shared host-probing implementations. It changes no runtime behavior.
Changes:
- Compares six areas of duplication and proposes ownership, surviving implementations, and call direction.
- Outlines follow-up sequencing, characterization tests, and unresolved compatibility decisions, including coordination with #521 and #623.
- Links the proposal from the architecture inventory.
| File | Description |
|---|---|
| docs/host-probing.md | Adds the host-probing comparison and proposed convergence plan. |
| docs/architecture.md | Links the design note beside the relevant rocm-core modules. |
Review effort: Balanced
The note called the WSL check, the os-release parser, the Windows driver and the ldconfig lookup "each read once and called from the other stack". They share implementations, but ExamineSummary::gather and probe_wsl still run detect_wsl_summary separately, probe_wsl and probe_comgr add further ldconfig runs, and detect_distro_name is referenced from examine/ only by tests. Say so, and stop counting these as removed duplicates. Correct the Windows video-controller count: three queries is the usual case, and an absent summary inventory adds two more through the display gfx-target and driver-detail fallbacks. List it as a constraint on converging on one inventory. Replace the claim that the PATH lookup and subprocess runner changes leave output unchanged with the differences each one introduces (empty PATH entry, non-UTF-8 PATH, Windows candidate spellings, timeout path), and require a scenario or a stated reason for each. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
There was a problem hiding this comment.
🟡 Changes recommended
The proposal omits successful-exit capture loss from late descendant output and its required characterization test, and its CPU comparison plan does not account for WSL skipping CPU probing.
2 open issues
3 resolved since last review
Review effort: Balanced
…unner differences State which host (native Linux, WSL, native Windows, a Windows host inspecting a WSL distro) reads each fact with which stack, and say which hosts each characterisation test must cover. Record that on WSL the examination returns before reading the CPU, so cpu_model stays empty and a CPU comparison has to be scoped to non-WSL hosts or made an explicit behaviour change. Record that the runner convergence can shorten a successful capture: run_with_timeout drops output a descendant writes more than its drain grace after the child exited, where the older helpers wait for EOF. Add that and the other successful-run differences (read error, wait error, spawn failure, encoding, stderr, poll interval) to the runner proposal and the order, and require a late-descendant characterisation test. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
juhovainio
left a comment
There was a problem hiding this comment.
Docs-only change; I found nothing blocking. Approving with a few optional suggestions.
What I checked: the timeouts, poll intervals, DRAIN_GRACE, the uname handling, the CPU vendor logic and the CommandCapture fields against current sources all match the note. Links are relative and resolve, and the checks that ran are green. The E2E/GPU jobs skip on the docs-only path filter.
Non-blocking:
- Length: the ticket asks for a short note; this is ~190 dense lines and the decision per pair is buried mid-paragraph. A two-line summary per pair (survivor + biggest user-visible difference) would let a reader skip the host-by-host detail.
- Title vs. content: the title says "decides", but the note says it "decides nothing by itself" (a proposal). Something like "propose" would match.
- Derived counts: the 3 vs 5 video-controller queries and 4
ldconfigruns on WSL come from reading the code rather than a run. The note labels them as such, which is the right call. - Repetition: the same per-host facts recur across several sections.
- Snapshot: consider marking the note as a snapshot to retire once the convergence tickets close.
|
|
||
| # Host probing: which stack owns each fact | ||
|
|
||
| Status: proposal. This note changes no code and no behaviour. It decides nothing by itself: it lists the host facts that `rocm-core` reads twice (on the hosts each is read on, see "Which host reads which fact"), says how the two reads differ, and proposes which one should survive and in what order a later series of changes could remove the other. Each pair is meant to be a separate change with its own review. |
There was a problem hiding this comment.
The ticket asks for a short note. Consider adding a two-line decision summary per pair (survivor + biggest user-visible difference) near the top, so a reader can skip the host-by-host detail.
There was a problem hiding this comment.
Added a "Summary by pair" table right after the status and reading notes, in 383b828: one row per pair with the proposed survivor and the biggest user-visible difference, each linking to its section. Every cell repeats a claim already in the detailed section (no new claims), and I also collapsed a few per-host statements that the host table already makes (the WSL CPU and Windows kernel cases, the WSL GPU case, the native Linux and WSL name lookup, and the repeated PATH and runner differences in the order). The note did not get much shorter overall, because I did not drop any claim that appears only once, but the decision per pair is now readable from the top.
| - [`examine.rs`](../crates/rocm-core/src/examine.rs) — the host examination probe: the `Examination` document and its types, and `Examination::probe`, which runs the probes in order. The probes live in child modules under `examine/`, with `examine.rs` kept as the module root so the [`examine_driver_proptests.rs`](../crates/rocm-core/src/examine_driver_proptests.rs) `#[path]` attribute and the links to it stay valid: [`linux_gpus.rs`](../crates/rocm-core/src/examine/linux_gpus.rs) (Linux GPU enumeration from `lspci`, `rocminfo` and the kernel topology, with the marketing-name and APU classification they share), [`linux_driver.rs`](../crates/rocm-core/src/examine/linux_driver.rs) (kernel modules, device-node access and Secure Boot), [`rocm_install_probe.rs`](../crates/rocm-core/src/examine/rocm_install_probe.rs) (how ROCm was installed and which repository files configure it), [`library_copies.rs`](../crates/rocm-core/src/examine/library_copies.rs) (the loader library-copy search for comgr and the HIP runtime, and `comgr_matches_runtime`, which `diagnose/engine.rs` reads), [`framework.rs`](../crates/rocm-core/src/examine/framework.rs) (the PyTorch and llama.cpp probes), [`host_env.rs`](../crates/rocm-core/src/examine/host_env.rs) (the OS and distro, CPU, tracked environment variables, shared memory, container check and kernel log), [`wsl.rs`](../crates/rocm-core/src/examine/wsl.rs) (the WSL probe, including the remote probe that reads a named distribution from the Windows host, and the distro-floor check) and [`windows.rs`](../crates/rocm-core/src/examine/windows.rs) (the native Windows probes: the GPU query, the HIP SDK, the Adrenalin driver and the MSVC redistributable, plus `parse_dotted_version`, which `diagnose/windows.rs` reads). | ||
| - [`gfx_target.rs`](../crates/rocm-core/src/gfx_target.rs) — gfx-target identification and the TheRock package-family tables, split out of [`host_gpu.rs`](../crates/rocm-core/src/host_gpu.rs) so [`managed_runtime.rs`](../crates/rocm-core/src/managed_runtime.rs) can read the family mapping without importing the module that probes the host: `extract_first_gfx_token`, `normalize_therock_family`/`known_therock_families`, `gfx_target_has_no_rocm_build`, the AMD PCI-device-id and marketing-name tables behind `gfx_target_from_pnp_device_id`, and the parsers for the Windows display text and the kernel's gfx-target version. The module is private; `lib.rs` re-exports four of the public names (all but `gfx_target_from_pnp_device_id`) and `host_gpu.rs`, as before, re-exports all five. | ||
| - [`host_gpu.rs`](../crates/rocm-core/src/host_gpu.rs) — (ROCMAI-85, Phase 7a of ROCMAI-27's sequencing) GPU/host hardware detection — see the module's own doc comment for what it owns. Its leaf concerns are child modules under `host_gpu/`, with `host_gpu.rs` kept as the module root: [`capture.rs`](../crates/rocm-core/src/host_gpu/capture.rs) (running an optional tool under a timeout and the tool-on-`PATH` lookup), [`os_facts.rs`](../crates/rocm-core/src/host_gpu/os_facts.rs) (kernel version, distro name, CPU model and installed RAM), [`linux.rs`](../crates/rocm-core/src/host_gpu/linux.rs) (the sysfs, KFD and DRM probes), [`gpu_visibility.rs`](../crates/rocm-core/src/host_gpu/gpu_visibility.rs) (which AMD GPUs this process may use, from the KFD/DRM counts and the `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES` masks, plus `windows_amd_gpu_listing`, the native-Windows adapter bound `rocm serve --gpu <index>` is checked against), [`windows_inventory.rs`](../crates/rocm-core/src/host_gpu/windows_inventory.rs) (the Windows display-adapter inventory and its parsers, including the AMD-adapter count behind `windows_amd_gpu_listing`), [`wsl.rs`](../crates/rocm-core/src/host_gpu/wsl.rs) (WSL detection, the librocdxg chain and the Windows host driver as seen from the guest), [`render.rs`](../crates/rocm-core/src/host_gpu/render.rs) (the `rocm examine` text of an `ExamineSummary`, as a second `impl` block), [`diagnostics.rs`](../crates/rocm-core/src/host_gpu/diagnostics.rs) (the GPU detection diagnostics text) and [`engine_default.rs`](../crates/rocm-core/src/host_gpu/engine_default.rs) (the default serve engine, and the vLLM preference for the TheRock families that serve through it). | ||
| - [`host-probing.md`](host-probing.md) — a design note, not a module: [`host_gpu.rs`](../crates/rocm-core/src/host_gpu.rs) and [`examine.rs`](../crates/rocm-core/src/examine.rs) are two separate host-probing stacks that still read some of the same facts twice (kernel version, CPU model, the Windows GPU query, the marketing-name table, the `PATH` lookup and the subprocess runners). It proposes which of each pair should survive, how the two differ today and the order in which they could converge; it changes no code. |
There was a problem hiding this comment.
Optional: say the note is a snapshot to be retired when the convergence tickets close, so the inventory does not keep pointing at a stale design record.
There was a problem hiding this comment.
Done in 383b828: the entry now ends with "It is a snapshot, to be retired when the convergence tickets close", and the note itself says the same near the top, including that its entry in architecture.md goes with it.
…a snapshot Put the proposed survivor and the biggest user-visible difference for each pair at the top of the note, so a reader can skip the host-by-host detail. Collapse per-host statements that the host table already makes (the WSL CPU and Windows kernel cases, the WSL GPU case, the native Linux and WSL name lookup, and the repeated PATH and subprocess differences in the order). Say in the note and in its docs/architecture.md entry that it is a snapshot to be retired when the convergence tickets close. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
There was a problem hiding this comment.
🟡 Changes recommended
The documentation incorrectly treats NVIDIA as the only non-AMD inventory case and must clarify that rebuilding gpus[] also needs to account for existing Intel rows excluded by the current examination query.
1 open issue
Review effort: Balanced
| | | Summary | Examination | | ||
| |---|---|---| | ||
| | Symbols | `assemble_windows_inventory` over `pnputil`, `WINDOWS_VIDEO_CONTROLLER_INVENTORY_SCRIPT`, `WINDOWS_PNP_ENTITY_INVENTORY_SCRIPT`; plus the one-line query in `detect_windows_amd_display_driver` | `WIN_GPU_SCRIPT` in [windows.rs](../crates/rocm-core/src/examine/windows.rs) | | ||
| | Vendors | AMD only | AMD and NVIDIA (`has_nvidia_gpu`, non-AMD entries in `gpus[]`) | |

Closes ROCMAI-893.
Why this note exists
rocm examinereads several host facts through two separate stacks in rocm-core: the summary inhost_gpu.rs(the text form, and thesummaryobject of--json) and the examination inexamine.rs(the rest of--json,rocm diagnoseand Doctor reports).rocm examine --jsonruns both back to back, so the same fact can appear twice in one document from two separate reads, and the copies have drifted. Before anyone converges them, someone has to decide which copy of each pair should survive, because the surviving side decides which behaviour users keep. This adds a proposal for that decision asdocs/host-probing.md, linked from the rocm-core inventory indocs/architecture.md. The note is a snapshot, to be retired when the convergence tickets close, and says so at the top and in the architecture entry.What the note proposes
The top of the note has a table with one row per pair: the proposed survivor and the biggest difference a user can see today, so a reader can skip the host-by-host detail. Below it, for each duplicate it names both implementations as links, states how they differ today, says what a user can see, and proposes a survivor and a direction of calls. It first records that the imports already run one way (the examination calls into the summary, never the reverse) while stating plainly that shared code is not shared reads: on WSL
rocm examine --jsonstill runs the WSL summary twice andldconfig -pfour times, and the examination uses the os-release reader and parser directly rather thandetect_distro_name. It proposes the rule that follows from the imports: the summary stack owns how a fact is asked of the operating system, the examination owns the document and its JSON contract. Per pair: kernel version (detect_kernel_versionsurvives), CPU model (one reader returning model and vendor together), Windows GPU query (the summary's inventory survives, with NVIDIA and adapter names flagged as constraints), marketing name to gfx target (AMD_MARKETING_GFX_TARGETSsurvives), PATH lookup (thePATHEXT-aware behaviour survives under the namewhich), and subprocess runners (run_with_timeoutas the one bounded executor,CommandCaptureextended with a typed outcome rather than a second capture type).Verifying each claimed duplicate against
mainturned up three things the ticket text did not say. The Windows CPU read has three implementations, not two. The Windows GPU query has more than one counterpart on the summary side (pnputil, the video controller, the PnP entity and a one-line driver query), and--jsonasks for the video controller three times in the usual case and five when the summary's inventory is absent, because the display gfx-target and driver-detail fallbacks each query again. The two name tables are not equivalent: the examination's covers only some APU names and no discrete GPU, so an RX 7900 XTX with no HIP SDK has an emptygpus[].gfx_targetbesidesummary.detected_gfx_target: gfx1100in the same document, and the two disagree about Krackan (gfx1150versusgfx1152). Two more came from reading which host reaches which code. On WSL the examination returns from its WSL branch before the native Linux branch reads the CPU, socpu_modelis empty andcpu_vendorisunknownon every WSL host whilesummary.cpuis populated; the CPU change therefore does not reach WSL, and the note requires it to scope its comparison to non-WSL hosts or to make collecting CPU facts on WSL an explicit behaviour change. And the runner convergence is not only a timeout change:run_with_timeouttakes its buffer 200 ms after the child exits, so a successful capture can lose output a descendant writes later, where the older helpers wait for EOF; the note lists that with the other successful-run differences (read error, wait error, spawn failure, encoding, stderr, poll interval) and requires a late-descendant characterisation test. A table in the note says which host (native Linux, WSL, native Windows, a Windows host inspecting a WSL distro) reads each fact with which stack, and each follow-up item names the hosts its characterisation test must cover. Every claim read from the code is marked as verified; anything that depends on a real host is marked UNCONFIRMED.What it deliberately leaves open
How NVIDIA adapters are represented once the examination reads the inventory, whether
gpus[].nameshould come frompnputilor from WMI, which timeout wins for the kernel and CPU reads, whetherkernel_releaseshould be filled on Windows, whether the typed outcome sits onCommandCaptureor beside it, and whether the near copy of the candidate-spelling logic inapps/rocmjoins the PATH change. The note proposes; it does not mandate.Follow-up order
One pair per change, each starting with a characterisation test that is green against today's code, and none of them described as output-preserving: the PATH lookup change alters which tools are reported present (an empty
PATHentry, aPATHthat is not UTF-8, Windows candidate spellings), the CPU change leaves WSL unchanged or changes it on purpose, and the runner change alters the timeout path and can shorten a successful capture, so the note requires each such difference to get a scenario or a stated reason, and scopes the kernel and CPU comparison scenarios to the hosts where both sides are filled. The order is PATH lookup, kernel version, CPU model, marketing names (best done by finishing the open #521, which already proposes that convergence and conflicts withmain), the Windows GPU query, and last the subprocess runners (the bounded-join fix in #623 does not wait for it). Filing one ticket per pair is left for after this merges.Verification
Docs only. Ran: the docs-links check as CI runs it (
lychee --config lychee.toml './**/*.md', lychee 0.24.2) with 0 errors,cargo xtask lychee(exit 0), the repository's commit hooks (license header, markdown links, signing, whitespace, merge-conflict markers), a leak scan of the diff, a conflict-marker grep, and a check that no added line indocs/architecture.mdcarries a phase label. Not run:cargo test,cargo clippyand the smoke script, because no code is touched;cargo xtask manifest --check, because the manifest table covers Cargo dependencies and no manifest-relevant file changed. Nothing was executed on Windows. The WSL claim about the CPU was also checked by onerocm examine --jsonrun on a WSL2 host with a local debug build of unrecorded revision (cpu_modelempty,cpu_vendorunknown,summary.cpupopulated); every other claim is read from the code, and the runner differences were not run.User-visible behaviour
None. This is a docs-only change, so no scenario applies; the note cites the existing scenarios
examine-machine-readable-report,examine-both-forms-agree-on-gpuandexamine-windows-driver-version-date-and-ageas the ones later changes would extend.