Repository navigation
fix(serve): stop warning about VRAM capacity on unified-memory APUs - #196
Conversation
|
I reviewed this diff ( A few things I checked specifically since this touches serve-path logic:
I also looked at the two CI failures on this run:
This looks safe to approve pending a green re-run of those jobs. Note this is a diff-only read — it doesn't replace CI or a maintainer's own pass. |
b5ec05f to
a9b7c33
Compare
On Strix Halo and other AMD APUs, `rocm serve --gpu auto` warned "GPU 0 has only 1.6 GiB of 4.0 GiB free; serving may fail on VRAM" on a machine with 122 GiB of shared RAM, where serving then succeeded. An APU has no private VRAM. amd-smi reports the fixed BIOS carveout as total_vram, while the allocator serves the model out of GTT-backed system RAM, so the free-fraction check was thresholding a pool the engine does not use: it fires on healthy machines and would stay quiet on a genuinely starved one. Withholding it removes false positives without adding false negatives, because that number never predicted serve failure on these parts. Reuse the existing `gfx_is_apu_family` predicate rather than adding a gfx1150..1153 list that would drift from it; promoting it to `pub` also gives the classification a single definition across the doctor and serve. Suppression is gated on the host reporting exactly one GPU. The predicate classifies a part while host detection yields one target for the whole machine, so on an APU+dGPU laptop an unguarded verdict could silence a legitimate warning about the discrete card. Closes EAI-7369. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
a9b7c33 to
64f620f
Compare
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
rominf
left a comment
There was a problem hiding this comment.
Reviewed at 6c3e293. No blocking issues. Flagging two things and a few smaller notes — this is currently the only review covering the current head.
The post-approval delta is benign
The raw b5ec05f..6c3e293 diff looks alarming (46 files) but it spans a rebase: b5ec05f sat on 04f568b8, head sits on ce923d42. Comparing the PR's own patch at each base isolates the real delta, and it is entirely mechanical — a parameter reorder forced by collect_serve_notes gaining an engine_flag_note argument from main, two pre-existing test call sites gaining a trailing None (both pass gpu_vram = None, so the warning path short-circuits either way), and a cargo fmt reflow. Zero new logic; the predicate, the guard and the three new tests are byte-identical to what was approved.
The detection predicate holds up
Since a wrong predicate here silently suppresses a genuine out-of-VRAM warning on a discrete card — much worse than the warning it removes — I checked it against the target space rather than trusting the description. gfx1100/1101/1102 → false, gfx1103 and all gfx115x → true, feature suffixes such as :sramecc+:xnack- handled, and the gfx115/gfx110 prefixes diverge at their sixth character so no string can match both — branch ordering is provably irrelevant rather than accidentally safe. The fallback is safe in the right direction: !gfx_target.is_some_and(gfx_is_apu_family) leaves the warning in place for an unknown or absent target, and nothing in the chain collapses "unknown" into "UMA".
One thing the existing approval genuinely does not cover, and it isn't code
The rebase pulled in a project rule that postdates the approval. AGENTS.md §3 — "User-observable behavior needs a scenario, not only a unit test… a unit test asserting the internal helper does NOT discharge this" — landed on main as f551677f, which is not an ancestor of b5ec05f but is an ancestor of 6c3e293.
This PR removes a warning line from rocm serve stdout, which is squarely user-observable, and ships unit tests only. It takes a fourth path beyond the rule's three sanctioned outs, arguing the only lane that could run such a scenario is already xfail'd. I checked the factual claims behind that and they're all true rather than overstated: no feature file asserts this warning, the xfails genuinely cover the serve scenarios on the relevant lane, and the natural extension point would panic inside wait_for_model before reaching any new assertion. It reads as sound judgement — but it is a deviation from a brand-new rule and deserves an explicit maintainer nod rather than passing silently.
Worth adding, if anyone does ask for a scenario: on a host whose unified-memory carve-out is large, the free-VRAM fraction is healthy and the warning wouldn't fire anyway, so a scenario asserting "no warning appears" would pass even without this fix. Getting a non-vacuous scenario needs a host configured with a small carve-out, which is a harder ask than it sounds.
Non-blocking
- Top residual risk: the whole fix hinges on
amd-smireporting exactly one GPU row on Strix Halo. If it reports two,gpu_count != 1short-circuits and the bug simply isn't fixed. The PR is honest that gfx1151 hardware verification wasn't run, and unit tests structurally cannot validate this — worth confirming on the reporting user's machine. - The dashboard still shows the same misleading signal:
crates/rocm-dash-tui/src/ui/dock.rs:118andui/tabs/hardware.rs:417compute a VRAM percentage from the same carve-out, so a Strix Halo will still display a near-full gauge in the TUI after this lands, and the two surfaces will disagree. Pre-existing and out of scope, but it is the same bug on another surface. - MI300A isn't covered —
gfx942is reported by both MI300X (discrete) and MI300A (unified), so MI300A keeps the spurious warning. Not fixable from the gfx string alone and it fails in the safe direction, but worth a line in thegfx_is_apu_familydoc comment, which now enumerates its consumers without noting this exception. - The deferred UMA-appropriate capacity check has no tracking issue, so the gap exists only in this PR's prose. The user-visible consequence is worth stating: on these machines there is now no memory signal at all before serve, and actual RAM exhaustion surfaces only as an engine OOM.
apus_verdict_does_not_silence_a_second_gpu's comment frames the test as verifying per-ordinal attribution, butvram_capacity_is_meaningfulnever inspectsgpu_indices— the guard is row-count-only, so&[1]vs&[0]changes nothing in the path under test.
Reusing gfx_is_apu_family rather than adding a parallel gfx list, and keeping gpu_low_memory_warning pure with the policy decision isolated in vram_capacity_is_meaningful, both made this change easy to audit — and the gpu_count != 1 guard is the detail that makes it safe rather than reckless.
tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No EAI-7369 rows exist; this warning has never had e2e coverage.)Problem
On Strix Halo and other AMD APUs,
rocm serve --gpu autowarns:…on a machine with 122 GiB of shared RAM, where serving then proceeds without trouble. Reported from a gfx1151 laptop (Ryzen AI Max+ 395, 128 GB, Ubuntu 24.04).
Root cause
An APU has no private VRAM.
amd-smireports the fixed BIOS carveout — often ~4 GiB — astotal_vram, while the allocator actually serves the model out of GTT-backed system RAM.gpu_low_memory_warningthresholds a free fraction of that carveout, so on a unified-memory part it is measuring a pool the engine does not use.That makes the warning uninformative in both directions: it fires on healthy machines, and it would stay quiet on one that is genuinely out of memory. Withholding it removes false positives without introducing false negatives, because the number it thresholds never predicted serve failure on these parts.
Approach
Reuse the existing
gfx_is_apu_familypredicate rather than adding agfx1150..1153list beside it. It already covers everygfx115xpart plusgfx1103+, correctly excludes the discretegfx1100/1101/1102neighbours that share thegfx110xprefix, and tolerates a trailinggcnArchNamefeature suffix. Promoting it topubalso keeps one definition of "this part is integrated" shared between the doctor and serve, instead of a second list that would drift.gpu_low_memory_warningis unchanged and still pure. A newserve_gpu_low_memory_warningwraps it for the two serve call sites, andvram_capacity_is_meaningfulholds the decision as a separate pure function.HostGpuSummarythatservemay already hold. Host GPU detection can probe sysfs/WSL, andservedeliberately gates its own call on whether engine selection needs it — an eager lookup here would regress that.Scope
AUTO_FREE_VRAM_FRACTIONis shared between this warning and--gpu autoselection. Only the warning path changes here; touching the constant or the shared path would silently alter which GPU gets picked.Suppressing rather than re-basing the warning on system RAM is deliberate. A capacity check against shared RAM needs a real free-memory signal and its own threshold, and on a unified-memory box the carveout is over 10% used essentially always — so porting the 90% rule across would nag on every serve. That is separable work.
On a hybrid APU + discrete host the current behaviour is preserved, including the false positive when the APU is the selected device. Fixing that properly means resolving a gfx target per amd-smi ordinal, which is more machinery than this warrants.
Testing
cargo fmt --all -- --checkandcargo clippy --workspace --all-targets -- -D warningsclean.cargo test --workspace --all-targets --no-fail-fastpasses apart from three pre-existing failures unrelated to this change and on untouched files: tworocm-coreproc_lifecycle::tests::tree_*tests that fail on WSL2 for process-tree signalling reasons, andtherock::tests::extracting_the_sdk_archive_removes_it, which flakes onmainitself.New unit tests cover suppression on
gfx1151, retention on a busy discretegfx1100, the idle-discrete baseline, and the hybrid two-GPU case.gfx1153was missing from the APU predicate's contract test and is now asserted.Not run locally: verification on real gfx1151 hardware — this host has no AMD GPU, and the behaviour depends on what
amd-smireports on actual unified-memory silicon. The unit tests pin the decision logic against synthetic telemetry; confirmation on hardware is still outstanding.No e2e scenario asserts this warning today, and the serve scenarios on the Strix Halo Ubuntu lane are currently xfail'd under EAI-7423, so a new e2e there would land in a known-failing lane rather than protect this fix.
Closes EAI-7369.