feat(vllm): enhance OOM diagnostics and guidance in serve summary (eai-8059)

[r0x0r]

This pull request enhances GPU out-of-memory (OOM) diagnostics and user guidance for vLLM model serving, especially on shared or busy GPUs. It introduces actionable hints in both pre-launch warnings and post-failure summaries, ensuring users are clearly informed about how to adjust memory settings to avoid OOM errors. The changes also unify the messaging across CLI, engine logs, and documentation, and add comprehensive tests for these new behaviors.

Improvements to OOM diagnostics and user guidance:

• Added a clear, actionable hint (VLLM_GPU_MEMORY_UTILIZATION_HINT) that recommends adjusting --gpu-memory-utilization and/or targeting a less-busy GPU with --gpu <index>, shown both before launch (when low VRAM is detected) and after OOM failures in vLLM serves. [1] [2] [3] [4]
• Enhanced the logic to detect OOM errors in vLLM/PyTorch logs, including case-insensitive and signature-based matching, and surfaced the relevant memory knob hints only when applicable. [1] [2] [3] [4]

Integration and codebase updates:

• Updated collect_serve_notes and related serve summary logic to include vLLM-specific memory guidance, both pre-launch and post-failure, and ensured these notes are only shown when relevant. [1] [2] [3] [4]
• Ensured managed serve records retain log and manifest paths for accurate diagnostics after serve failures.

Documentation and testing:

• Expanded documentation to explain vLLM's memory reservation behavior and the recommended workarounds for OOM on shared/busy GPUs.
• Added extensive unit and end-to-end tests to verify OOM detection, note rendering, and user guidance in both CLI and engine logs. [1] [2] [3] [4] [5] [6]

These changes collectively improve the user experience by making OOM errors easier to diagnose and resolve, especially in shared GPU environments.<!--
Copyright © Advanced Micro Devices, Inc., or its affiliates.

SPDX-License-Identifier: MIT
-->

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows.

[volen-silo]

A few observations, mostly around the signature list and the stacking:

• vllm_log_shows_oom treats engine core initialization failed as an OOM signature, but vLLM emits that as the terminal wrapper for any EngineCore startup crash (unsupported arch, shm size, TP misconfig, missing weights). Being the last line, it reliably lands in the 80-line tail — so unrelated failures get reported as "the serve attempt ran out of GPU memory".
• The e2e scenario plants exactly that string as its OOM log, and a unit test asserts it must produce a hint — so the only end-to-end coverage never exercises a real OOM message.
• This branch removes the DRM sysfs VRAM fallback, the --gpu auto count derivation, their tests and the docs bullet — all added by its base feat(vllm): Tackle out of memory errors (EAI-8058) #251, which is still open. Intentional? auto_select_gpu_indices' doc still refers to that fallback.
• collect_serve_notes and append_oom_serve_note each emit the full hint verbatim, so a low-VRAM serve that then OOMs prints it twice.
• Setting log_path/manifest_path on the already-running branch makes the OOM note reachable when no launch happened — re-running serve against a starting service attributes that process's OOM to the new invocation.
• feat(diagnose): add the vLLM out-of-memory failure mode to the catalog (EAI-8060) #290 edits the same oom_utilization_hint and still calls log_tail_shows_oom, which this PR deletes.

[r0x0r]

Thanks for the detailed review — addressed all of these in b9316f3 (rebased onto the current gpu-out-of-memory tip):

• Generic "engine core initialization failed" signature: removed it from vllm_log_shows_oom. It really is vLLM's terminal wrapper for any EngineCore startup crash, not just OOM, so treating it as an OOM signature was wrong. Detection now only fires on the allocator-level signatures (torch.OutOfMemoryError, hip out of memory, out of memory).
• E2E coverage only exercising the wrapper string: the e2e scenario now plants a real allocator OOM signature (torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.) instead of the generic wrapper phrase, and unit tests were updated the same way.
• DRM sysfs fallback / --gpu auto count derivation removed: that was purely a stale-base artifact — this branch was still on top of an older point in gpu-out-of-memory than your feat(vllm): Tackle out of memory errors (EAI-8058) #251 fix commit. Rebased onto the current tip (a97a39f) and restored the gpu_vram_usage/gpu_vram_usage_amd_smi/gpu_vram_usage_sysfs split that got flattened by the rebase's merge resolution — verified with cargo test/clippy that the dispatcher and DRM fallback tests are back and passing.
• Duplicate hint when a low-VRAM serve then OOMs: append_oom_serve_note now skips adding the note if the shared VLLM_GPU_MEMORY_UTILIZATION_HINT text is already present in the notes collected pre-launch, so it's never printed twice.
• log_path/manifest_path on the already-running branch misattributing OOM: append_oom_serve_note now also takes already_running and bails out immediately when true, so re-running serve against a live/starting service never blames that invocation for whatever the other process's log contains. Added a regression test for this (append_oom_serve_note_ignores_an_already_running_services_log) and repointed the e2e scenario at this exact case (serve-oom-memory-guidance), asserting the summary does not show the OOM note when reusing an already-running service.
• feat(diagnose): add the vLLM out-of-memory failure mode to the catalog (EAI-8060) #290 touching the same oom_utilization_hint/log_tail_shows_oom: noted for awareness — nothing in this repo currently defines log_tail_shows_oom (this PR's function is vllm_log_shows_oom), so there's no conflict today, but that PR will need to reconcile with whichever version of this lands first.

All changes covered by unit tests; cargo test -p rocm --bin rocm, cargo clippy -p rocm -p rocm-core -p rocm-engine-vllm --all-targets -- -D warnings, and cargo fmt --check all pass locally.

[volen-silo]

Round 2. Items 1, 4 and 5 are genuinely fixed — I checked the mechanisms, not just the claims. The dedup works (VLLM_GPU_MEMORY_UTILIZATION_HINT collapses to one line, notes holds raw text at the dedup point, render_summary does no wrapping, so contains really fires), and the already_running bail-out loses nothing (ManagedLaunchReport.already_running has exactly two producers, start_managed_service's final Ok is always false, and load_managed_services refreshes liveness before the check).

Blocking:

• The note fires on status == "running", which the codebase documents as healthy. Both append_oom_serve_note and oom_memory_note gate on status == "ready", but status_for_readiness produces three values and the comment four lines above it describes "running" as "the engine is up and loading normally" — deliberately distinguished from "starting" so rocmd doesn't kill a slow-loading model. So a serve whose endpoint is up and whose model is still loading, with out of memory anywhere in its last 80 log lines, is told "the serve attempt ran out of GPU memory." Both rustdocs claim they only fire for a serve that "failed to become ready" — the code contradicts its own contract. Gate on "starting" or an explicit failure predicate, not != "ready".

• The positive e2e case now has no scenario at all. @id:serve-oom-memory-guidance is the only scenario touching OOM guidance in the whole feature directory, and it now asserts the note is absent. Round 1's point was that the positive scenario planted a fake OOM string; repointing it at an unrelated negative case rather than repairing it leaves the PR's headline user-visible behavior unit-tested only. 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." Keep the negative, add a positive.

• Round-1 item 3 is only partly fixed. The production code is byte-identical to a97a39f0 — gpu_vram_usage/_amd_smi/_sysfs, effective_gpu_count, validate_pinned_gpu_index, auto_select_gpu_indices, and that last one's doc comment does match its code. But auto_selection_uses_vram_row_count_when_amd_smi_count_is_unknown is still deleted with no replacement — it's the only test removed anywhere in this PR, and effective_gpu_count still falls back to vram.map(<[GpuVramUsage]>::len), so it's a live path with no selection-level coverage for detected: None + populated vram. Separately, the new comment at main.rs:16882 ("No GPU count from amd-smi (unavailable, or genuinely zero devices)") contradicts the unchanged comment eight lines above it in the same function, which explains that vram may be populated precisely when amd-smi is absent. Reaching count == 0 needs both sources to fail. Revert the comment edit and restore the test.

• docs/vllm.md:123 says "two ways", the list below it has three items and is byte-identical to the base. Residue of the reverted-then-restored sysfs bullet — the bullet came back, the count word didn't.

• Dead signature entry. "hip out of memory" contains "out of memory", so .any() can never reach it. Cleanest fix that also un-narrows detection: ["outofmemory", "out of memory"]. The old log_tail_shows_oom matched bare "outofmemory"; replacing that with "torch.outofmemoryerror" means torch.cuda.OutOfMemoryError (the .cuda. breaks contiguity) and hipErrorOutOfMemory no longer match on their own. Usually the spaced phrase is also in the tail so impact is limited, but neither commit message mentions the narrowing.

• The spawn_managed_engine_child hunk is now inert. log_path/manifest_path on the already-running branch has zero reachable consumer: the three reads in the tree are append_oom_serve_note (bails on already_running) and print_managed_launch_plain twice (returns before reaching them). The danger round 1 flagged was fixed elsewhere; the now-purposeless hunk was left in.

• @requires-gpu puts the new scenario on a lane that gates nothing. ci.yml:675 skips those on the blocking mock job and the self-hosted GPU lane is continue-on-error. It isn't gratuitous as written — main.rs:4857 bails GPU-required serves before the already-running short-circuit — but that check is skipped for --device cpu_only, and existing_live_managed_service matches on (engine, canonical_model_id) only, so device policy is irrelevant to the reuse this scenario depends on. --device cpu_only + @requires-no-gpu (as scenario 11 already does) would put it on the blocking lane. The step's own comment flags the mismatch: "allocates no GPU memory despite running on the GPU lane."

On #290: the claim that nothing defines log_tail_shows_oom is wrong — git show origin/oom-diagnostics-catalog:engines/vllm/src/lib.rs still defines it at 1846, calls it at 1832 and asserts on it at 2252. The conclusion holds anyway: merge-tree is clean, #290 edits only the format string two lines below the if. The real coordination issue is that once both land there are two independent OOM detectors — rocm_core::vllm_log_shows_oom and #290's KEYWORDS_VLLM_OOM/check_16_vllm_oom table. Whichever merges second should fold the checker onto the shared helper; worth saying so in the body now.

Smaller things:

• vllm_log_shows_oom lives in rocm-core but has zero tests there — near-identical assertions now sit in serve_summary.rs:510 and engines/vllm/src/lib.rs:2227. cargo test -p rocm-core alone covers none of it.
• assert_no_oom_memory_guidance also asserts screen.contains("already running"), which the Gherkin doesn't state. Good guard against a vacuous negative, but it belongs in the feature file as its own And.
• read_optional_tail_lines runs before the already_hinted check — swap them.
• oom_memory_note is a policy function with no rendering, in serve_summary.rs, while its sibling collect_serve_notes is in main.rs, stitched together three lines apart at the call site.

Checked and clean: log lifecycle (File::create truncates per launch, service_id is timestamp-unique, so no stale OOM can be attributed), the e2e plumbing is real rather than vacuous, no orphaned steps, no temp-file collision.

Centralising the signature into rocm_core::vllm_log_shows_oom so the engine error and the CLI summary can't drift is the right call — it's what makes #290's duplicate detector visible as a problem.

[r0x0r]

Thanks for the Round-2 review — pushed 1adc674 addressing it.

Note could fire for a healthy running (still-loading) service. Fixed. Introduced serve_summary::serve_failed_to_become_ready(status) (status == "starting") and gated both append_oom_serve_note and oom_memory_note on it, instead of status != "ready". A healthy running service is no longer treated as a failed launch. New test: append_oom_serve_note_is_withheld_for_a_healthy_still_loading_service.

Dead signature / narrowing regression. vllm_log_shows_oom signatures are now ["outofmemory", "out of memory"]. The old "hip out of memory" was dead (any log with it already contains "out of memory"), and the space-free "outofmemory" matches the allocator error types whatever module path they carry (torch.OutOfMemoryError, torch.cuda.OutOfMemoryError, hipErrorOutOfMemory). Added rocm-core coverage: vllm_log_shows_oom_matches_allocator_signatures_but_not_generic_failures (also asserts the generic EngineCore wrapper is not matched).

Inert spawn_managed_engine_child hunk / deleted test / misleading comment. Reverted the already-running branch's log_path/manifest_path change back to None, reverted the select_auto_gpu_index comment churn, and restored auto_selection_uses_vram_row_count_when_amd_smi_count_is_unknown.

docs/vllm.md said "two ways" but the list has three. Fixed to "three ways".

The @requires-gpu reuse scenario gated nothing. Moved @id:serve-oom-memory-guidance onto the blocking no-GPU mock lane: @requires-no-gpu @requires-os:linux, with --device cpu_only so the GPU-required pre-flight is skipped and the already-running reuse short-circuit is reached. It now gates every PR and allocates no GPU memory. Also split the "already running" assertion into its own explicit Gherkin Then step.

Positive e2e scenario — not added, and here's why. The negative case (reuse must not blame this invocation) is the one the harness can drive without a GPU, because it reuses a pre-planted live service rather than launching. A positive end-to-end (this invocation launches, the launch OOMs, the note renders) needs the mock engine to emit a real allocator OOM signature in a log it owns (already_running == false, status == "starting"), which the mock can't do today. The positive behavior is currently covered by unit tests (append_oom_serve_note_reads_the_failed_serve_log_and_names_the_knobs and serve_summary::oom_note_*). If you'd like it in e2e, I'll add a mock hook to inject a failed-launch OOM log and verify it on the Linux lane as a follow-up — flagging the gap rather than shipping an unverifiable scenario.

Smaller items: added the missing rocm-core test for vllm_log_shows_oom; reordered append_oom_serve_note so the cheap already_hinted check runs before the log read; the "already running" assertion is now explicit in the feature file.

Coordination with #290: once both land there are two OOM detectors (vllm_log_shows_oom here and the diagnose catalog there) — happy to fold them onto a shared signature source in a follow-up if you'd prefer.

[r0x0r]

@volen-silo — correction on the scenario-15 lane, because my Round-2 reply was wrong and CI called it out.

--device cpu_only does not skip the GPU pre-flight — it's rejected before either branch is reached. parse_device_policy refuses cpu/cpu_only outright under the strict no-CPU-fallback policy ("rocm serve requires ROCm GPU execution; CPU mode is not a fallback path"), so serve() bails at argument parsing, before both the GPU-required pre-flight and the already-running reuse short-circuit. That's exactly why @id:serve-oom-memory-guidance went red on the no-GPU lane in 1adc674. Both my earlier claim and the "cpu_only skips the pre-flight" assumption were incorrect.

Proper fix (c0f39cf): reuse detection now runs before the GPU pre-flight, at the correct layer. Reusing an already-running managed service launches nothing and pins no GPU, so it should never be blocked by the GPU-required check. I moved model resolution + reuse detection ahead of that bail, gated by a cheap any_live_managed_service_for_engine pre-check so the common launch path still fails fast before any engine work. When a live managed service for the engine already exists and the model ref resolves to it (existing_live_managed_service match), the GPU bail is skipped; otherwise everything is byte-identical — including scenario 11's no-GPU/no-runtime refusal, which still fires because no live service exists.

Scenario 15 now runs on the blocking lane with the default device — @requires-no-gpu @requires-os:linux, no --device flag. It reaches the reuse short-circuit on the no-GPU mock host purely because the planted live service is reused, gates every PR, and allocates no GPU memory. Feature/step comments updated to match.

Follow-up 395ed9e is a fixup! (clippy map_unwrap_or → is_ok_and on the new helper); it'll autosquash into c0f39cf on the final rebase.

Note: the current red E2E tests (GPU) lane is unrelated — 9 scenarios fail identically with vLLM RuntimeError: Failed to infer device type (the runner's HIP device wasn't visible), spanning serve/chat/benchmark/lemonade cases this change doesn't touch; the prior commit's GPU lane was green. Re-running it.

Your other two Round-2 items (restore the deleted auto_selection_uses_vram_row_count_... test + revert the main.rs:16882 comment; positive-launch e2e scenario) are tracked separately and not part of this lane fix.