Skip to content

fix(dash): detect amd-smi through WSL DXG - #375

Open
michaelroy-amd wants to merge 1 commit into
mainfrom
fix/wsl-dashboard-amd-smi
Open

fix(dash): detect amd-smi through WSL DXG#375
michaelroy-amd wants to merge 1 commit into
mainfrom
fix/wsl-dashboard-amd-smi

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

Summary

  • accept readable /dev/dxg as the WSL transport for the amd-smi safety preflight
  • preserve the existing /dev/kfd guard and the protection against uninterruptible amd-smi hangs
  • rename KFD-specific test seams and make dashboard warnings transport-neutral
  • add @id:dash-rocm-29846-wsl-gpu-telemetry for live WSL dashboard coverage

Tracks ROCM-29846.

Why

The WSL amd-smi backend uses ROCDXG through /dev/dxg; /dev/kfd is correctly absent. The dashboard rejected the collector before invoking the working wrapper, producing zero GPUs and an unknown model.

This checks the transport directly rather than treating every WSL environment as safe: production invokes amd-smi only when /dev/kfd or /dev/dxg can be opened.

Verification

  • cargo test -p rocm-dash-collectors — 89 passed, 2 ignored
  • cargo test -p rocm-dash-daemon — 51 passed
  • cargo test --workspace --all-targets — 2485 passed, 8 ignored
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo fmt --all -- --check
  • python3 scripts/smoke_local.py
  • pre-push hooks, including signature/sign-off and Hawkeye license checks

cargo xtask e2e compiled and exercised the suite locally. The new WSL/GPU scenario was not applicable because this host reports wsl_rocdxg_missing; it will run on the e2e-wsl lane. The local run had six unrelated existing unexpected failures in dashboard throughput, dependency guard, diagnose dry-run, and examine default-engine scenarios.

Signed-off-by: Michael Roy <michael.roy@amd.com>
@michaelroy-amd
michaelroy-amd requested a review from a team as a code owner September 9, 2026 21:23

@siloteemu siloteemu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Automated review · pr-review-watcher · 1611f58

Summary

Widens the dash amd-smi pre-flight from /dev/kfd-only to /dev/kfd OR /dev/dxg so GPU telemetry works under WSL, renames the skip-preflight plumbing to match, and adds one unit test plus one @requires-wsl @requires-gpu cucumber scenario. Needs work — the new e2e step cannot pass on any host because it polls for a string the TUI never renders at the size the step sets. Verified: I read the full diff and the surrounding render/driver code, and independently confirmed the blocking finding (dock::gpu_wall is the sole producer of "GPUs · N", called only under dock::is_wide ≥180×45 at ui/mod.rs:83-85, while use_detail_size() sets 120×40 at tui_driver.rs:45-46); revert question — the unit test does discriminate (a KFD-only gpu_device_accessible makes it fail; confirmed by running cargo test -p rocm-dash-collectors --lib amd_smi, 11 passed), and the e2e step is not vacuous-pass but guaranteed-fail, so neither is the "passes either way" defect, though the unit test never covers the production call site's use of the DXG_DEVICE constant; sibling-PR interaction — no new WSL predicate is introduced, rocm_core::is_wsl_host (lib.rs:2357-2374) remains the single decider and is untouched, and this change moves in the opposite direction to the open detection-widening PR (it enables GPU reporting rather than suppressing a catalogue, and a native host inheriting WSL_DISTRO_NAME still passes here via /dev/kfd), so it neither compounds that risk nor the host-mutating driver-install PR; no prompt-injection content found in the diff; I relied on the reported CI (26 pass / 0 fail) for the full suite and Windows/GPU lanes, which is precisely why the blocking issue is invisible — the only lane that runs this scenario (e2e-wsl) is non-blocking per tests/e2e-cucumber/README.md:168-183. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

tests/e2e-cucumber/tests/e2e/dash_steps.rs:270-273 — the step's only load-bearing condition can never be true, so this scenario will time out and fail on every run, with or without the production fix. The step calls tui.use_detail_size() (line 264), which resizes the PTY to DETAIL_COLS=120, DETAIL_ROWS=40 (tests/e2e-cucumber/tests/e2e/tui_driver.rs:45-46, 273-290). The string "GPUs · N" is produced at exactly one place, crates/rocm-dash-tui/src/ui/dock.rs:74 inside gpu_wall, and gpu_wall has exactly one call site, crates/rocm-dash-tui/src/ui/mod.rs:85, reached only when dock::is_wide(area.width, area.height) is true — which requires cols >= 180 && rows >= 45 (dock.rs:32-33, 40-42). 120×40 fails both axes, so the wide triptych is never laid out and "GPUs · " never appears on screen. The loop at lines 266-284 therefore always exhausts default_timeout() and panics. This blocks because the scenario is the PR's only user-observable evidence for the fix (AGENTS.md §3 requires it), and it currently proves nothing — worse, it fails for a reason unrelated to the change, which will read as "the WSL fix doesn't work". Fix: add a wide-layout resize helper to tui_driver.rs (there is none today — use_detail_size is the widest, and it is too small) that sets at least dock::WIDE_COLS×dock::WIDE_ROWS, call it instead, and re-verify on the e2e-wsl lane; alternatively assert on a GPU signal the single-column layout actually renders.

Non-blocking

  • tests/e2e-cucumber/tests/e2e/dash_steps.rs:280!screen.contains("amd-smi unavailable") is inert: that text is rendered only by draw_gpus in crates/rocm-dash-tui/src/ui/tabs/hardware.rs:227-250, which this scenario never navigates to, and only when snap.gpus.is_empty() — a state the GPU-count condition already excludes. Drop it or navigate to the panel that shows it.
  • tests/e2e-cucumber/tests/e2e/dash_steps.rs:274"Node throughput ·" is emitted unconditionally by node_throughput_title (crates/rocm-dash-tui/src/ui/tabs/home.rs:131, and see its own test asserting "Node throughput · Unknown GPU" at home.rs:571), so only the paired !contains("Unknown GPU") carries signal; the bare-prefix check adds nothing.
  • crates/rocm-dash-collectors/src/amd_smi.rs:375-381 — the new test injects tempfile paths into gpu_device_accessible and never touches DXG_DEVICE or the call site at line 75-77; a typo in the constant (/dev/dxgkrnl is a plausible one) or a copy-paste passing KFD_DEVICE twice would ship green. One cheap line — assert_eq!(DXG_DEVICE, "/dev/dxg"), or calling gpu_device_accessible(Path::new(KFD_DEVICE), Path::new(DXG_DEVICE)) — closes it.
  • docs/ci-hardware-testing.md:72-74 — "the one scenario whose premise is a WSL host carries @requires-wsl" is now stale; this PR makes it two.
  • tests/e2e-cucumber/features/dash.feature:127 and dash_steps.rs:282 — the tracker reference sits in the @id and the panic message, inverting the convention one scenario above (which keeps a descriptive @id and puts the reference in a comment); the prefix used here is also the only one of its kind in the repo. Separately worth a line in the PR text: /dev/dxg being openable is weaker than WslSummary::rocdxg_ready (crates/rocm-core/src/lib.rs:2486-2489, which also needs dxcore/librocdxg/ldconfig), so dash can now report a GPU on a host rocm examine calls wsl_rocdxg_missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants