fix(dash): detect amd-smi through WSL DXG - #375
Conversation
Signed-off-by: Michael Roy <michael.roy@amd.com>
siloteemu
left a comment
There was a problem hiding this comment.
🔴 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 bydraw_gpusincrates/rocm-dash-tui/src/ui/tabs/hardware.rs:227-250, which this scenario never navigates to, and only whensnap.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 bynode_throughput_title(crates/rocm-dash-tui/src/ui/tabs/home.rs:131, and see its own test asserting"Node throughput · Unknown GPU"athome.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 intogpu_device_accessibleand never touchesDXG_DEVICEor the call site at line 75-77; a typo in the constant (/dev/dxgkrnlis a plausible one) or a copy-paste passingKFD_DEVICEtwice would ship green. One cheap line —assert_eq!(DXG_DEVICE, "/dev/dxg"), or callinggpu_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:127anddash_steps.rs:282— the tracker reference sits in the@idand the panic message, inverting the convention one scenario above (which keeps a descriptive@idand 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/dxgbeing openable is weaker thanWslSummary::rocdxg_ready(crates/rocm-core/src/lib.rs:2486-2489, which also needsdxcore/librocdxg/ldconfig), so dash can now report a GPU on a hostrocm examinecallswsl_rocdxg_missing.
Summary
/dev/dxgas the WSL transport for theamd-smisafety preflight/dev/kfdguard and the protection against uninterruptibleamd-smihangs@id:dash-rocm-29846-wsl-gpu-telemetryfor live WSL dashboard coverageTracks ROCM-29846.
Why
The WSL
amd-smibackend uses ROCDXG through/dev/dxg;/dev/kfdis 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-smionly when/dev/kfdor/dev/dxgcan be opened.Verification
cargo test -p rocm-dash-collectors— 89 passed, 2 ignoredcargo test -p rocm-dash-daemon— 51 passedcargo test --workspace --all-targets— 2485 passed, 8 ignoredcargo clippy --workspace --all-targets -- -D warningscargo fmt --all -- --checkpython3 scripts/smoke_local.pycargo xtask e2ecompiled and exercised the suite locally. The new WSL/GPU scenario was not applicable because this host reportswsl_rocdxg_missing; it will run on thee2e-wsllane. The local run had six unrelated existing unexpected failures in dashboard throughput, dependency guard, diagnose dry-run, and examine default-engine scenarios.