Conversation
|
A few observations, mostly around the signature list and the stacking:
|
ee202f0 to
a97a39f
Compare
738cdf4 to
b9316f3
Compare
|
Thanks for the detailed review — addressed all of these in b9316f3 (rebased onto the current
All changes covered by unit tests; |
a910068 to
83dc552
Compare
|
Round 2. Items 1, 4 and 5 are genuinely fixed — I checked the mechanisms, not just the claims. The dedup works ( Blocking:
On #290: the claim that nothing defines Smaller things:
Checked and clean: log lifecycle ( Centralising the signature into |
|
Thanks for the Round-2 review — pushed Note could fire for a healthy Dead signature / narrowing regression. Inert
The 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 ( Smaller items: added the missing rocm-core test for Coordination with #290: once both land there are two OOM detectors ( |
|
@volen-silo — correction on the scenario-15 lane, because my Round-2 reply was wrong and CI called it out.
Proper fix ( Scenario 15 now runs on the blocking lane with the default device — Follow-up Note: the current red E2E tests (GPU) lane is unrelated — 9 scenarios fail identically with vLLM Your other two Round-2 items (restore the deleted |
|
Round 3. I re-verified every Round-2 item against the code at I also checked the new gate the other way, for over-narrowing: BlockingThe positive e2e scenario is still missing, and the infeasibility argument only covers half of it. You are right about the post-failure path. The pre-launch half is coverable today, with no new harness capability:
That is a fixture, not a new primitive. Nits (non-blocking)
Everything else I checked came back clean: the shared-constant centralisation means the engine hint and the CLI note cannot drift, |
|
Thanks for the Round-3 review — and for re-verifying the Round-2 items against the code rather than the claims. Blocking item — the positive scenario. You're right on both halves, so I've split them explicitly in the PR body (new "e2e coverage and the scenario gap" section), per §3's escape hatch and §2's body-vs-comment distinction:
Nits. Noted and deferred as non-blocking: the Two red checks (self-hosted GPU, readthedocs) fail identically on unrelated recently-merged PRs; the blocking GitHub-hosted mock lane runs and passes |
|
I read through this PR's diff (base The change is well-scoped and I didn't find anything blocking:
This looks safe to approve after the usual final checks — passing CI and a maintainer's own pass — since this is a diff-only read and doesn't replace running the test suite or verifying against real hardware. |
395ed9e to
e8e19e8
Compare
|
Rebased onto the updated One semantic merge decision worth flagging: the base branch had independently evolved The rest of the series replayed unchanged; |
|
Re-reviewed the update pushed since the last review (commit What's new since the last pass: it adds the positive e2e scenario the earlier review rounds flagged as missing (Scenario 16, I checked:
No findings from this pass. This looks safe to approve after the usual final checks — passing CI and a maintainer's own pass — since this is a diff-only read and doesn't replace running the test suite or verifying on real hardware. |
|
Pushed `5bd1bfc`: gated the `serve-oom-launch-memory-guidance` scenario on a new `@requires-oom-fault-injection` capability so the WSL2 self-hosted E2E lane skips it instead of failing. Why the lane failed: that scenario drives the test-only `e2e-oom-fault-injection` hook to fabricate a GPU-less OOM launch. The hook is compiled out of the shipping release binary. The self-hosted lanes test a prebuilt `ROCM_CLI_BINARY`, so The gate: `xtask e2e` now sets `ROCM_E2E_OOM_FAULT_INJECTION=1` only when it built the binary with the feature; the harness probe reads it into `HostCapability.oom_fault_injection`, and `resolve()` skips the tagged scenario when the hook is absent. This preserves the mock-lane coverage that gates every PR and cleanly skips the feature-less self-hosted lanes. Added unit tests for the resolver gate (both directions) and the xtask env signal. |
|
Re-reviewed the update pushed since the last review (commit What's new: the author gated the positive The fix:
I also re-read the full diff against base again (not just this delta) given how long this PR has been through review rounds: the OOM signature matcher, the This looks safe to approve after the usual final checks — passing CI (the self-hosted GPU/WSL lanes plus the mock lane) and a maintainer's own pass — since this remains a diff-only read. |
r0x0r
left a comment
r0x0r
left a comment
There was a problem hiding this comment.
The attribution reasoning here is the strongest part: gating the post-failure note on already_running == false and status == "starting" and the serve's own log_path is exactly the right defense against blaming this invocation for another process's OOM, and serve_failed_to_become_ready correctly keeps a healthy still-loading running service out of the failure path. The const false fault-injection seam compiling out of release builds is also clean.
One substantive question on precision: vllm_log_shows_oom is a case-insensitive substring scan for out of memory / outofmemory over the last 80 log lines, and the note fires whenever that substring appears in the tail of a failed-to-become-ready serve. It isn't anchored to the terminal/fatal error line. So a serve that died for an unrelated reason but whose tail happens to mention "out of memory" somewhere (a benign warning, a retried allocation that later succeeded, a model/path name containing the token) would be misreported as an OOM failure. The already_running/starting/own-log triad prevents cross-process misattribution, but not "wrong cause, same process." Is anchoring to the last matching line (the way oom_utilization_hint already picks the failing line) worth it here, or is substring-in-tail deemed precise enough in practice?
Smaller related point: the tail is read at 80 lines. If an OOM traceback is followed by more than 80 lines of shutdown/teardown noise, the signature scrolls out of the window and the note is silently withheld. Worth confirming 80 matches the engine's own DEFAULT_LOG_TAIL_LINES budget so the two surfaces don't disagree on what counts as "the tail."
|
Pushed Tail budget: the serve summary read a bare Anchoring vs substring-in-tail: deliberately keeping the whole-tail scan.
Meanwhile |
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Reviewed as the delta against gpu-out-of-memory (#251), so #251's own work isn't re-litigated here — I left that separately on #251.
The note logic is good and the fault-injection seam is a nice piece of design: feature-gated with a #[cfg(not(feature))] const fn -> false fallback, armed per-invocation by an env var, reported to the harness as a capability so the scenario skips rather than silently passes on a prebuilt binary. oom_memory_note is properly unit-tested and the reuse-vs-launch distinction it draws is exactly right.
Three things I'd want fixed before merge.
The reorder is the significant one. To decide reuse_existing, the new block runs load_managed_services (side-effecting — liveness probes, record writes), a full ResolveModel round-trip, and ensure_self_managed_engine_ready before the no-usable-GPU bail. The comment right below it still says "BEFORE preparing or launching any engine (no wasted engine download …)", and that's no longer true. It also drags resolve_gpu_indices from before the no-runtime bail to after it — and the GPU lane on this head is failing serve-absent-gpu-index-rejected with exactly the message that predicts:
Step panicked. Captured output: expected the absent GPU index to be reported unavailable, got:
Error: device_policy: gpu_required; no active ROCm runtime is configured; ...
I checked #251's GPU lane as the control — same runner, same base — and it doesn't fail that scenario (its failures are all serve-timeout/resolved model shaped). So this looks caused by the reorder rather than inherited.
Second: xtask/src/e2e.rs replaces the single --features value with rocm/e2e-oom-fault-injection, but main now passes rocm/e2e-test-hooks there — added by #249, which isn't an ancestor of this branch. On rebase those compete for one slot, and workflow_contract.rs asserts the literal --features rocm/e2e-test-hooks string, so a naive resolution either breaks the contract test or silently compiles out lemonade's failure seams.
Third, smaller: the negative OOM assertion uses a raw contains while both positive ones normalize whitespace — by the PR's own stated reason (80-col soft wrap), so Scenario 15 can pass without testing anything.
On process — this is stacked on #251's branch but isn't draft and has no Depends on #251 in the body. AGENTS.md §11 asks for both, and it matters here: about half of what a reviewer sees belongs to the other PR, and #284's delta relocates code #251 introduces. Worth considering whether the rocm-core placement should just land in #251 directly.
Leak scan is clean and the EAI-8059 reference is fine per AGENTS.md §2.
| .is_some(); | ||
| resolved_model = Some(probe); | ||
| } | ||
| // Fail fast under a GPU-required policy when the host has no usable AMD GPU, |
There was a problem hiding this comment.
This comment is now false, and the work it promises won't happen is exactly what runs above it.
"BEFORE preparing or launching any engine (no wasted engine download, and an actionable message instead of a late engine crash)" — but the reuse block at :4877-4897 already ran:
any_live_managed_service_for_engine→load_managed_services, which is side-effecting:refresh_from_engine_state,refresh_managed_service_runtime_liveness(HTTP readiness/inference probes with a 750ms timeout each), andrecord.write();- a full
ResolveModelengine round-trip; ensure_self_managed_engine_readyfor self-managed engines.
That last one is the expensive one. On a GPU-less host with lemonade selected, the user now sees "Preparing lemonade for GPU serving…" and potentially a multi-GiB install, and then gets told there's no usable AMD GPU. AGENTS.md §6's "preserve strict GPU-required behavior" is about the outcome, but the fail-fast part is what this comment is claiming and it's what's lost.
The !reuse_existing exemption itself is correct — nothing is launched when reusing, so the gate shouldn't fire. It's the cost of computing reuse_existing that's the problem.
A cheap ordering fix: gate the whole reuse block on usable_amd_gpu_indices() being non-empty (or None), so on a GPU-less host you fall straight through to the bail. Reuse can't be the right answer on a host with no GPU under gpu_required anyway. Alternatively determine reuse from a non-mutating read of the records and defer the ResolveModel/prepare work until after the gate.
Whichever way it goes, the comment needs to match — if the contract genuinely changed, say what the new one is.
There was a problem hiding this comment.
Fixed in 4a108b9 — you were right that the comment promised something the block above it had already spent.
The reuse pre-gate keyed on the engine alone, so a live service for a model this invocation can never reuse was enough to pull the ResolveModel round-trip and — the expensive one you named — ensure_self_managed_engine_ready, complete with Preparing lemonade for GPU serving... and a possible multi-GiB install, in front of the no-usable-GPU bail. It now keys on the model too (any_live_managed_service_for_model), so on a GPU-less host with a live lemonade service for some other model the block is skipped entirely and the bail is once again the first thing that runs.
On the two options you offered: gating the whole reuse block on usable_amd_gpu_indices() being non-empty would have been cheaper, but it breaks @id:serve-oom-memory-guidance (serve-20), which reaches the reuse short-circuit precisely on a GPU-less host. So I took the second one — decide reuse from the records and let only a plausible match pay for the engine round-trip. Matching uses service_model_names_match, the same lenient relation the service-listing surfaces already use, so a short-vs-canonical spelling still reaches the probe; the probe then still decides reuse authoritatively on the canonical id via existing_live_managed_service. Reuse detection is not narrowed in practice.
The residual, stated plainly: when a live service does match this engine and model, ensure_self_managed_engine_ready still runs before the bail. That is the case where this invocation is about to reuse and legitimately skip the bail, so the work is not wasted — and the comment now says exactly that instead of claiming an absolute.
Regression tests (apps/rocm/src/main.rs): reuse_pregate_skips_engine_work_for_an_unrelated_live_model — this is the one that fails before the fix, since the old function body had no model term at all and returned true for any live service of the engine; plus reuse_pregate_admits_the_same_model_including_a_short_spelling, reuse_pregate_still_keys_on_the_engine, and reuse_pregate_admits_the_model_id_the_oom_reuse_scenario_plants, which pins the literal id serve-20 plants so a matcher change cannot silently turn that scenario into a launch attempt on the one lane it runs on.
Verified on a Linux MI300X box: full workspace cargo test + clippy -D warnings + fmt --check, and cargo xtask e2e with serve-20 and serve-21 both passing. I could not run the self-hosted E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) lanes — that hardware is not available to me, so CI is the only authority for those two.
| env_id.as_deref(), | ||
| ); | ||
| let resolved_selection = validate_engine_selection_runtime(&paths, resolved_selection)?; | ||
| if !matches!(device_policy, DevicePolicy::CpuOnly) |
There was a problem hiding this comment.
resolve_gpu_indices moved from before the no-runtime bail to after it, and the GPU lane on this head is failing on exactly that.
On the base (:4873-4878 there), resolve_gpu_indices — the source of --gpu index N is out of range — ran ahead of the no active ROCm runtime is configured bail. Here it's at :4934, after. So a bad --gpu value now surfaces as an environment complaint instead of an argument complaint.
From run 33489993381 on c337c7c4:
Step panicked. Captured output: expected the absent GPU index to be reported unavailable, got:
Error: device_policy: gpu_required; no active ROCm runtime is configured; run `rocm runtimes list` ...
I pulled #251's GPU lane (run 33166192967, job 98832217235) as the control, since that lane has other unrelated problems and I didn't want to over-attribute. It does not fail serve-absent-gpu-index-rejected — its failures are all serve-timeout / missing-resolved model shaped. So the regression appears with this delta.
User-facing, argument validation before environment validation is the right order regardless: --gpu 99 is wrong no matter what runtime is active, and telling the user to go activate a runtime sends them down the wrong path.
The reorder that reuse needs shouldn't have to drag resolve_gpu_indices with it — gpu_vram_usage()/resolve_gpu_indices don't depend on resolved_model, so they can go back above the runtime bail.
There was a problem hiding this comment.
Fixed in 7c09129 (already on the branch when you filed this); re-verified against the current head rather than the claim.
resolve_gpu_indices now runs at apps/rocm/src/main.rs:5209, immediately after the no-usable-GPU pre-flight and before the no-runtime bail at :5211. So --gpu 99 is an out-of-range argument error again regardless of runtime state, and a GPU-less host still refuses with "no usable AMD GPU" first. Your point that argument validation belongs ahead of environment validation is the ordering that shipped.
Lane evidence, since this was a GPU-lane regression: serve-16 / @id:serve-absent-gpu-index-rejected ran and passed on E2E tests (GPU) at 83ecfaad, and it also passes in my own cargo xtask e2e run on a Linux MI300X box at the new head 4a108b9. It is not among that lane's current failures.
Thanks for pulling #251's run as a control before attributing it — that was the right call and it is what made the delta unambiguous.
| if binaries.build_release { | ||
| let status = Command::new(&cargo) | ||
| .args(["build", "--release", "-p", "rocm", "-p", "rocmd"]) | ||
| // `--features rocm/e2e-oom-fault-injection` compiles in the test-only |
There was a problem hiding this comment.
This replaces a --features value that main added, and there's a contract test asserting the old string.
On origin/main this call site is --features rocm/e2e-test-hooks (from #249, not an ancestor of this branch). Here it becomes --features rocm/e2e-oom-fault-injection, and apps/rocm/Cargo.toml in this diff declares only e2e-oom-fault-injection. git merge-tree origin/main pr/284 conflicts in apps/rocm/Cargo.toml, apps/rocm/src/main.rs, model_serving.feature, serving_steps.rs, and this file — and this one is literally the two feature strings competing for one slot.
The part that makes it more than a routine conflict: xtask/src/workflow_contract.rs:255-274 asserts every prebuilt E2E lane block contains the exact string
cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks
So a resolution that keeps only the new feature fails that test, and one that keeps only the old feature silently disables the new scenario (it degrades to a skip via the capability gate — a green run that tested nothing). Both failure modes are quiet in different ways.
On rebase this wants: a combined list (rocm/e2e-test-hooks,rocm/e2e-oom-fault-injection), e2e-test-hooks restored in apps/rocm/Cargo.toml, and the workflow_contract.rs expectation updated to match. Worth flagging in the PR text too, since whoever resolves the conflict may not know the contract test exists.
The configure_harness_env half is right — clearing the env var for a prebuilt binary rather than letting an inherited value through is the correct call.
There was a problem hiding this comment.
Addressed across 7c09129 and 4a108b9.
The overwrite is gone: xtask/src/e2e.rs:125 now passes a single space-separated value, "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection", so e2e-test-hooks (#249) survives alongside the new hook rather than losing the one --features slot to it. apps/rocm/Cargo.toml declares both.
On workflow_contract.rs:255-274: the assertion is a contains on cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks, and every prebuilt lane still spells exactly that, so it passes unchanged. But your framing exposed that its doc and failure message had become false — they claimed the asserted string matched what cargo xtask e2e builds for itself, which is now a superset. 4a108b9 rewrites both to describe it as the shared hook floor, and records why the fault-injection hook is deliberately absent from the prebuilt lanes: those scenarios carry @requires-oom-fault-injection, so a missing hook is a reported skip, not the silent green you were worried about. That closes the third failure mode — a stale comment nobody re-reads on rebase.
Your two other resolution notes are now moot for this branch: this PR targets gpu-out-of-memory, not main, and the base already carries the serve-NN scenario naming, so merge-tree no longer conflicts in model_serving.feature. I have put the combined-feature-list requirement in the PR body anyway, since whoever eventually rebases this onto main will hit exactly the collision you describe and should not have to rediscover the contract test.
Agreed on configure_harness_env — it env_removes on the prebuilt path rather than letting an ambient 1 through, and that is unchanged here.
| .expect("no interactive serve summary") | ||
| .screen_text(); | ||
| assert!( | ||
| !screen.contains("ran out of GPU memory"), |
There was a problem hiding this comment.
This negative assertion is whitespace-sensitive while both positive ones aren't, so Scenario 15 can pass vacuously.
assert_oom_launch_memory_guidance (:954) uses screen_without_whitespace(&screen).contains("ranoutofGPUmemory"), and the comment above it explains why: "The note is a long line the 80-column PTY wraps across grid rows … otherwise a soft wrap between 'GPU' and 'memory' would break a literal contains and mask a rendered note."
That reasoning applies identically here, in the direction that matters more. If the note were wrongly emitted on the reuse path and happened to wrap — which the positive step says is the normal case for this string — the raw contains wouldn't see it and the scenario would pass. So the regression guard is disarmed precisely when the note renders the way the code actually renders it.
screen_without_whitespace(&screen).contains("ranoutofGPUmemory") with the ! is the one-line fix, and it makes the pair symmetric.
The scenario itself is a good one — "don't blame this invocation for another process's OOM" is a real distinction and worth pinning.
There was a problem hiding this comment.
Fixed in 7c09129; verified against the current code, not the claim.
tests/e2e-cucumber/tests/e2e/serving_steps.rs:1218 is now:
!screen_without_whitespace(&screen).contains("ranoutofGPUmemory"),exactly the one-line fix you proposed, so the negative and the positive (:1272) are symmetric and the guard survives the soft wrap the positive step documents as the normal rendering. The comment above it now spells out why the whitespace collapse is load-bearing in the negative direction specifically — that a literal contains would have passed even if the note were wrongly emitted.
The scenario is now serve-20 (renumbered when the base landed serve-19); the @id: is unchanged. It ran and passed in cargo xtask e2e on a Linux MI300X box at 4a108b9, and it also runs on the blocking GitHub-hosted mock lane.
Thanks — that one really would have been a silently disarmed regression guard.
| /// `torch.cuda.OutOfMemoryError`, `hipErrorOutOfMemory` — while `out of memory` | ||
| /// catches the spaced runtime phrasing (`HIP out of memory`, `CUDA out of | ||
| /// memory`). An earlier `hip out of memory` entry was dead code, since any log | ||
| /// containing it already contains `out of memory`. |
There was a problem hiding this comment.
Two coupled issues with this detector and the note it drives.
The threshold contradicts #251's. This matches a bare out of memory substring anywhere in the tail. In the same stack, check_16_vllm_oom deliberately scores a bare "out of memory" at 25 — below MIN_SCORE_FOR_MATCH — with a comment explaining that it "is deliberately sub-threshold so this only claims the failure mode when the vLLM/HIP shape of the error is present," and gates the whole table behind VLLM_ANCHOR_PATTERN. Same failure class, same repo, two thresholds, and the looser one is the one driving user-facing serve output.
A managed-service log tail containing out of memory from any source — a kernel OOM-killer line, a dependency's log, an unrelated subprocess — produces the vLLM OOM note. The doc comment here is careful about excluding vLLM's generic wrapper line, which suggests the false-positive risk was on your mind; the bare-phrase case is the bigger one.
The guidance is wrong for the model-too-large case. The note only ever suggests --gpu-memory-utilization and --gpu <index>. Both help when the GPU is shared or busy. Neither helps when the model simply doesn't fit — and lowering the reservation there actively makes it worse, trading an earlier OOM for a later one.
#251 already gets this right: check_16_vllm_oom's summary says "Only lower --gpu-memory-utilization when the GPU is shared or already busy; on a GPU dedicated to this server it does not create room a too-large model needs." That caveat doesn't survive into the note this PR shows users, and --max-model-len isn't mentioned anywhere in the new guidance.
I'd reuse #251's anchored matcher rather than maintain a second looser one, and either carry the shared-vs-dedicated caveat into the note or have it point at rocm diagnose --symptom 'vllm: …', which already does the nuanced version. Pointing at diagnose is the lighter option, and it exercises the handoff the two PRs are building.
Small related note on the wording: the hint says --gpu-memory-utilization <0-1>, but parse_gpu_memory_utilization enforces (0, 1] — 0 is rejected.
There was a problem hiding this comment.
All three points fixed in 83ecfaa. Taking them in your order:
Threshold. You were right that maintaining a second, looser matcher for the same failure class was the bug, and that the looser one driving user-facing output made it worse. rocm_core::vllm_log_shows_oom no longer does a bare substring scan — it classifies each tail line with the same rule as check_16_vllm_oom, scoring the line as vllm: <line> against the shared MIN_SCORE_FOR_MATCH threshold (crates/rocm-core/src/lib.rs:7424). The tail is vLLM's own process output, so the checker's required VLLM_ANCHOR_PATTERN is supplied by context rather than dropped. Sub-threshold lines that used to fire now correctly do not: a kernel OOM-killer line, a bare out of memory, a bare HIP error: out of memory, a bare torch.OutOfMemoryError with no allocator corroboration. Genuine allocator failures still clear it. One helper feeds both the engine hint and the CLI note, so there is no second matcher left to drift.
Model-too-large guidance. Also fixed, and I took your lighter option and carried the caveat, since the note is what the user actually reads. oom_memory_note (apps/rocm/src/serve_summary.rs:219) now says that if the model simply does not fit, lowering the reservation will not help, and to serve a smaller or quantized model instead — then routes to rocm diagnose --symptom '<their actual failing line>' for the case-appropriate fix, which exercises the handoff you point out the two changes are building. --max-model-len is deliberately left to diagnose, which does the nuanced version, rather than duplicated into a fixed string here.
Wording. <0-1> is gone. The shared hint now reads "a fraction greater than 0 and at most 1", matching what parse_gpu_memory_utilization actually enforces on (0, 1].
Verified at 4a108b9 on a Linux MI300X box: full-workspace cargo test + clippy -D warnings, and cargo xtask e2e with diagnose-13 (@id:diagnose-vllm-oom-is-conditional), serve-20 and serve-21 all passing. I could not run the self-hosted E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) lanes — no such hardware available to me — so CI remains the only authority on those two.
| let Some(log_path) = log_path else { | ||
| return notes; | ||
| }; | ||
| // Read the same tail budget the engine's own OOM surfaces use |
There was a problem hiding this comment.
This comment claims an invariant that isn't enforced anywhere.
"Read the same tail budget the engine's own OOM surfaces use (rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES) … rather than drifting apart on independent magic literals" — but the vLLM engine's OOM surface reads STARTUP_FAILURE_LOG_TAIL_LINES, a local const … = 80 at engines/vllm/src/lib.rs:34, used at :1817 and :1823. That's precisely the independent magic literal the comment says has been eliminated.
They're both 80 today, so nothing is broken. But the comment (and the commit message) describes an enforced coupling, and a future change to DEFAULT_LOG_TAIL_LINES would silently desynchronize the two surfaces — with the comment still asserting they can't.
Pointing STARTUP_FAILURE_LOG_TAIL_LINES at the protocol constant is a one-liner and makes the claim true. Otherwise the comment should describe the intent rather than a guarantee.
There was a problem hiding this comment.
You were right, and this one was genuinely still open — the earlier response commits did not touch it. Fixed in 4a108b9.
engines/vllm/src/lib.rs:35 was still const STARTUP_FAILURE_LOG_TAIL_LINES: usize = 80;, the exact independent literal the serve-summary comment claimed had been eliminated. It now reads:
const STARTUP_FAILURE_LOG_TAIL_LINES: usize = DEFAULT_LOG_TAIL_LINES;DEFAULT_LOG_TAIL_LINES was already imported in that file, so it is the one-liner you described. I took your first option rather than weakening the comment to intent, because the coupling is the thing worth having: a change to the protocol budget now moves both surfaces, and append_oom_serve_note and oom_utilization_hint cannot silently disagree on what "the tail" of a failed launch means. The const carries a doc comment saying so, so the next person is not tempted to re-inline the literal.
Deliberately scoped to vLLM: engines/lemonade has its own STARTUP_FAILURE_LOG_TAIL_LINES alongside a local DEFAULT_LOG_TAIL_LINES = 200, and it is not part of the OOM surface this PR couples, so I left it alone rather than widen the change.
Verified at 4a108b9: full-workspace cargo test + clippy -D warnings + fmt --check on a Linux MI300X box, plus the vLLM engine's own tail-budget tests. Not verified on E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) — that hardware is unavailable to me.
| /// Environment variable `xtask e2e` sets to `1` when it compiled the binary | ||
| /// under test with the `rocm/e2e-oom-fault-injection` feature. Kept in sync with | ||
| /// the same name in `xtask::e2e`. | ||
| pub const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION"; |
There was a problem hiding this comment.
This constant is declared twice — here and in xtask/src/e2e.rs:24 — held together only by a "kept in sync" comment on each side.
A typo in either doesn't fail to compile and doesn't fail a test. xtask sets one name, the harness reads another, oom_fault_injection is false, @requires-oom-fault-injection maps to Expectation::Skip, and the scenario silently stops running. Green suite, zero coverage — the same failure mode the comment on probe_host_capability is careful about elsewhere.
Since the harness crate is already a build dependency relationship away, hoisting the constant into a shared crate would be cleanest. Failing that, a test in either crate asserting the two literals are equal costs three lines and closes it.
Minor, but this scenario is the only thing exercising the fault-injection path, so a silent skip is expensive.
There was a problem hiding this comment.
Fixed in 83ecfaa, and I took the cleaner of your two options.
ROCM_E2E_OOM_FAULT_INJECTION is now declared exactly once, in the e2e-report crate that both xtask and e2e-cucumber already depend on:
crates/e2e-report/src/lib.rs:30—pub const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION";xtask/src/e2e.rs:30—use e2e_report::OOM_FAULT_INJECTION_ENV;tests/e2e-cucumber/src/capability.rs:134—pub use e2e_report::OOM_FAULT_INJECTION_ENV;
so the two former copies are re-exports of one source and a typo is now a compile error rather than a silent oom_fault_injection = false. Since it is a single constant, the equality test you offered as the fallback is no longer needed — there is nothing left to hold in sync.
Your framing of the cost is what decided it: this is the only thing exercising the fault-injection path, so a @requires-oom-fault-injection scenario degrading to Expectation::Skip would have been a green suite with zero coverage of the PR's headline behavior. The resolver still has tests for both directions (hook present → run, hook absent → skip), and serve-21 ran and passed under cargo xtask e2e on a Linux box at 4a108b9, so the signal is reaching the harness for real rather than only in principle.
| # index-specific rejection can only be observed where a real device is present. | ||
| @id:serve-absent-gpu-index-rejected @requires-gpu @requires-os:linux | ||
| Scenario: 13 - Serving pinned to a GPU that does not exist is refused | ||
| When the user serves a model pinned to a GPU index that does not exist |
There was a problem hiding this comment.
These reuse scenario numbers that already exist on main.
origin/main's model_serving.feature has a Scenario: 15 at line 143 ("Serving with a temperature below zero is refused…"), another Scenario: 15 at 173 ("Selecting both a runtime and an environment at once…" — main already has a duplicate), and a Scenario: 16 at 184. This PR adds a third 15 and a second 16, in different hunks, so git merges them cleanly and nothing complains.
The cost is triage: a lane reports a scenario by name and number, and "Scenario 15 failed" stops identifying anything. Worth renumbering on rebase — or dropping the manual numbers in favor of the @id: tags the repo already uses everywhere, which is what the tooling actually keys on.
Both new scenarios read well otherwise; the positive/negative pair covers the distinction properly.
There was a problem hiding this comment.
Fixed — the collision is gone as of 83ecfaa, though partly by the base moving under us.
origin/main no longer carries bare Scenario: 15 / 16 headings; every scenario in model_serving.feature is now Scenario: serve-NN - ..., running to serve-18 on main. The base branch (#251) adds serve-19, and this PR's two are serve-20 and serve-21, so the numbering is unique and sequential across the file and feature_naming is satisfied. Verified against origin/main and origin/gpu-out-of-memory at the current head rather than from memory.
On dropping the manual numbers in favour of @id:: agreed that @id: is what the tooling keys on, and the two scenarios here carry stable ones (serve-oom-memory-guidance, serve-oom-launch-memory-guidance) that survived both renumberings untouched — I have quoted the @id: rather than the number everywhere in the PR body for exactly the triage reason you give. Removing the numeric prefixes wholesale is a rename across every feature file plus the feature_naming test, which is a bigger and unrelated change than this PR should carry; happy to raise it separately.
Both scenarios ran and passed under cargo xtask e2e on a Linux MI300X box at 4a108b9. Thanks for the note on the pair reading well — the positive/negative split was the thing earlier rounds spent the most time getting right.
e94ca5f to
e1adff1
Compare
c337c7c to
886a1ce
Compare
|
Thanks for the review. Rebased onto the updated #251 head and addressed the points; the branch is now two commits (the squashed feature + a review-response commit) and back in draft while it depends on #251.
Also renumbered the two OOM scenarios to 17/18 to sit alongside the runtime/env and Lemonade-recovery scenarios that landed on the base, and fixed a stale |
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
The engineering quality here is high, and the most important thing to say comes first: commit 0be2dde4 fixed a genuine security-shaped defect mid-review, and fixed it the right way.
Before that commit, untrusted text from a vLLM log tail was interpolated unescaped into the rocm diagnose --symptom '...' command that the serve summary prints for the user to copy and run, with ANSI/BEL sequences passing through to the terminal too. The fix promotes the guards into rocm-core so both command-building surfaces share one rule, documents why it rejects rather than escapes, and pins the exact rendered symptom in tests instead of merely asserting some fragment is absent. I checked the guard rather than taking it on trust: quotable_in_single_quotes rejects on ' or any char::is_control(). A POSIX single-quoted string can only be broken by ', so rejecting on that one character is sufficient rather than heuristic; ANSI, BEL and a lone \r are all caught as Cc; backtick and $(...) are inert inside single quotes. That is the right shape for this class of fix.
Freshness note: the head moved twice while I was reviewing. Everything below is re-resolved and re-anchored against the current head 4d5c8688.
That latest merge resolved what had been my largest concern. At the previous head this PR did not merge, and the conflict sat in the exact region it reorders: the base now carries #267, which computes visible_gpu_indices once in HIP-ordinal space and feeds it to a mask-aware validate_pinned_gpu_index, while this PR had replaced that with an inline usable_amd_gpu_indices() plus a validate_pinned_gpu_index_against variant carrying no masked-device test. Resolving toward this PR would have silently regressed #267 and let rocm serve --gpu N accept a device hidden by ROCR_VISIBLE_DEVICES/HIP_VISIBLE_DEVICES. I verified the merge went the other way: validate_pinned_gpu_index_against is gone, visible_gpu_indices is computed once and passed through, and all three of validate_pinned_gpu_index_rejects_masked_out_device, ..._absent_index_without_mask_reads_as_not_present and ..._prefers_visible_set_over_detected_count are present at this head. Good resolution; no action needed. Worth knowing for the record that the base independently implemented the same --gpu-before-runtime-bail ordering fix an earlier thread claimed as this PR's contribution, and did it mask-aware.
Six things still block, in my view.
1. The OOM note is destroyed rather than de-duplicated, in the feature's most likely trigger. collect_serve_notes pushes VLLM_GPU_MEMORY_UTILIZATION_HINT verbatim when the low-VRAM warning fires, and append_oom_serve_note then returns the notes unchanged if any of them contains that hint, before reading the log or calling oom_memory_note at all. What gets discarded is the entire note, not a duplicated fragment: the confirmation that this attempt actually ran out of GPU memory, the "if the model doesn't fit, lowering the reservation won't help, serve a smaller or quantized model" branch, and the rocm diagnose --symptom command with the real failing line. None of it was printed once, so "never printed twice" misdescribes what happens. Detail inline.
2. <0-1> is still in the shared hint, and the PR body says it isn't. The description states the range text "is corrected to 'a fraction greater than 0 and at most 1'". At this head it is not: the constant still reads --gpu-memory-utilization <0-1> while the parser rejects <= 0.0 and accepts 1.0. I traced how it came back and it is not carelessness, it is a merge resolution that reverted an earlier fix, which is exactly why it needs a regression pin. Detail inline.
3. A dead fallback branch in vllm_oom_diagnose_symptom, distinct from the reachable and correctly-tested quotable_in_single_quotes fallback next to it. Detail inline.
4. This stacked PR is not in draft. AGENTS.md line 241: "Keep stacked PRs in draft until dependencies merge upstream (i.e., this repo's main branch, not just local)." #251 is still OPEN (dffee3ea, BLOCKED), and this PR is isDraft: false. It was noted on 2026-09-03 that the PR was "back in draft"; it is not now.
5. Three required checks have never run on this PR. .github/workflows/codeql.yml triggers on pull_request: branches: [main] only. Because this PR targets gpu-out-of-memory, Analyze (actions), Analyze (python) and Analyze (rust) are simply absent from the rollup, and all three are in main's required-contexts list. I confirmed this directly against the current head rather than inferring it: repos/ROCm/rocm-cli/commits/4d5c8688.../check-runs returns zero check-runs whose name starts with Analyze. Two consequences: the merge gate is unsatisfiable while stacked, and no static security analysis has run against a head whose whole purpose was fixing a shell-quoting defect. This shares a remedy with item 4 -- returning to draft and waiting for #251 addresses both.
6. A duplicated extra_env loop weakens a credential safety net in the shared TUI driver, which backs every TUI-driven scenario. Detail inline.
Non-blocking observations are inline and marked as such: a measured detector gap for two real vLLM OOM messages (a real gap, but not a regression -- the pre-PR scanner missed them too), a misleading "cheap pre-gate" comment over a call that can take ~8.75s, OOM guidance being absent entirely from non-interactive output, a duplicated env-var literal, a hardcoded timeout bypassing the suite-wide one, an under-asserting step name, and an undocumented capability tag. I'd also like your read on a design question about e2e-test-hooks, raised inline as a question rather than a demand.
A fair amount checked out clean and is worth recording. The fault-injection gating is safe: e2e-oom-fault-injection is declared but in no default set, release.yml passes no --features, and in non-feature builds e2e_simulate_oom_launch is a const fn returning false that never reads the environment, so no attacker-controlled env var can reach a shipped binary. The --features "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection" single-argument form is correct, handed straight to Command and never shell-interpolated. The whitespace-symmetry fix is real and serve-20 is well-anchored: it asserts the reuse precondition before asserting absence and plants the exact allocator signature that would trip vllm_log_shows_oom, so deleting the already_running guard would genuinely fail it. No duplicated OOM logic remains against #251, and the engine hint and CLI note are mutually exclusive surfaces, so there's no double output. All commits are signed and carry sign-off.
Locally: cargo fmt --all --check passed; cargo clippy --workspace --all-targets -- -D warnings passed (with sources touched first to defeat the cache); cargo clippy -p e2e-cucumber --test e2e -- -D warnings passed; cargo nextest run --workspace gave 2713 passed / 3 failed, where all three are rocm-dash-tui/src/agent.rs token-cache-dir tests failing on Permission denied in my sandbox and reproduce identically at the merge base, so they're environmental and not yours.
On CI: the PR body's check table is stale. It lists E2E tests (GPU) as one of two red checks, and there is no check by that name. At this head the GitHub-hosted lanes are green except E2E tests and windows-build-and-test, which are still running, and the self-hosted lanes have not reported yet, so CI is not final. At the previous head E2E tests (Strix Halo, Ubuntu) was failing; I could not determine why (the run was still in progress and logs weren't available), so I am explicitly not attributing it to this PR.
| let already_hinted = notes | ||
| .iter() | ||
| .any(|note| note.contains(rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT)); | ||
| if already_hinted { |
There was a problem hiding this comment.
Blocking. This early return destroys the OOM note rather than de-duplicating it, in the feature's most likely trigger.
collect_serve_notes (line 5734) pushes rocm_core::VLLM_GPU_MEMORY_UTILIZATION_HINT verbatim when the pre-launch low-VRAM warning fires. already_hinted above then matches on .contains() of that same constant, so this returns notes unchanged before the log read at line 5864 and before oom_memory_note at line 5870 ever run.
What gets discarded is the whole note (apps/rocm/src/serve_summary.rs:213-236), not a duplicated fragment:
- the confirmation that this attempt actually ran out of GPU memory, as opposed to the pre-launch guess that it might;
- the "if the model doesn't fit, lowering the reservation won't help, serve a smaller or quantized model" branch, which the pre-launch hint does not contain;
rocm diagnose --symptom '<real failing line>'with the user's actual error.
Concretely: discrete GPU with amd-smi present, card already busy. rocm serve warns pre-launch, the user proceeds, vLLM OOMs with torch.OutOfMemoryError: HIP out of memory (which scores 95 and matches). The user sees only the generic pre-launch guess, with no confirmation it really did OOM and no diagnose command. Low-VRAM leading to OOM is precisely the causal chain this feature exists for, and it's the one path where the new note never appears.
The comment at 5848-5850 ("there is nothing to add, so skip the log read entirely") states the intent, but the intent doesn't hold: there is quite a lot to add.
Suggested direction: de-duplicate at composition level, so oom_memory_note omits the hint fragment when it is already present, rather than suppressing the entire note.
| } | ||
|
|
||
| #[test] | ||
| fn append_oom_serve_note_does_not_repeat_a_hint_already_in_the_notes() { |
There was a problem hiding this comment.
Related to the already_hinted early return above: this test asserts full equality (notes == pre_launch_notes), which cements the suppression as the specification. A fix that appends only the missing content -- the OOM confirmation, the doesn't-fit branch, and the diagnose command -- would fail this test even though it is the correct behaviour.
Worth reworking alongside the fix so the test pins "the hint text appears exactly once" rather than "nothing was added".
| @@ -7772,6 +7772,121 @@ pub const VLLM_GPU_MEMORY_UTILIZATION_HINT: &str = "vLLM reserves ~90% of the GP | |||
| collide with memory already in use. Lower the reservation with `--gpu-memory-utilization \ | |||
| <0-1>` (e.g. 0.5 for a small model), or target a less-busy GPU with `--gpu <index>`."; | |||
There was a problem hiding this comment.
Blocking. <0-1> is still here, and the PR description says it isn't.
The description states this text "is corrected to 'a fraction greater than 0 and at most 1'" because "<0-1> wrongly implied 0 was accepted". That correction is not present at this head -- the line above still renders --gpu-memory-utilization <0-1>, while parse_gpu_memory_utilization (apps/rocm/src/main.rs:5126-5142) rejects <= 0.0 and accepts 1.0. The real domain is (0, 1]. Flagging the description mismatch only so the claim and the code end up agreeing, not as a criticism of intent.
This constant prints in three places (pre-launch note, engine OOM hint, and the fix-16-vllm-oom summary), so a user who OOMs, reads the tool's own advice, and tries --gpu-memory-utilization 0 gets rejected by the same tool.
The history is worth knowing, because it explains why this needs a test rather than just an edit. 83ecfaad in this very branch fixed the range text. Base commit 78cdcef7 edited the same line from a pre-83ecfaad tree, and merge commits baeab319 and 9eaa54d1 resolved in the base's favour -- silently reverting the fix while keeping its 0.1 to 0.5 change. git blame now attributes this line to 78cdcef7.
The existing pin at crates/rocm-core/src/fix.rs:1555 only checks the worked example (0.5), not the range text, which is how a merge could eat the same fix twice without anything going red. Restoring the wording plus extending that pin to cover the range text would close it for good.
| .map(str::trim) | ||
| .find(|line| !line.is_empty() && vllm_log_shows_oom(line)) | ||
| .map_or_else( | ||
| || VLLM_OOM_CANONICAL_SYMPTOM.to_owned(), |
There was a problem hiding this comment.
Blocking (dead code). This VLLM_OOM_CANONICAL_SYMPTOM fallback is unreachable.
Both callers pre-gate on vllm_log_shows_oom(log_tail) and return early when it is false -- apps/rocm/src/serve_summary.rs:214 and engines/vllm/src/lib.rs:2236. The .find() inside this function then re-evaluates the byte-identical predicate over the same string. .any() returning true implies .find() finds something, independent of iteration order, so unwrap_or_else here can never fire. No test exercises it directly either.
Worth distinguishing from the other canonical-symptom fallback in the same change -- the one guarding quotable_in_single_quotes at serve_summary.rs:225 and engines/vllm/src/lib.rs:2254 is genuinely reachable and correctly tested. It's only this one that's dead.
Either drop it and make the invariant explicit (a debug assert or an expect naming the caller contract), or, if you'd rather the function be safe standalone, add a direct test that calls it with a non-OOM tail so the branch is real and pinned.
|
|
||
| // Per-child overrides last, so a scenario's explicit variable wins over | ||
| // the inherited/isolation environment. | ||
| for (key, value) in extra_env { |
There was a problem hiding this comment.
Blocking. This is a verbatim duplicate of the extra_env loop already at lines 201-203, and the duplication inverts a safety guarantee.
The pre-existing loop applies extra_env before the provider-credential env_remove calls at 208-215, so previously extra_env could never defeat the credential strip. This second loop runs after them, so now it can: an extra_env entry naming OPENAI_API_KEY, ANTHROPIC_API_KEY, OPENAI_BASE_URL or ROCMDASH_CHAT_API_KEY would survive and select a cloud backend for a journey meant to be deterministic local/mock chat.
It is redundant for every current caller, which by the repo's own rule makes it dead code; the concern is that it's dead code that quietly widens a hole. The new doc comment at 222-223 describes only this loop, so a future reader has no signal that an earlier one already did the job.
The blast radius is wide -- this helper backs every TUI-driven scenario. Simplest fix is to delete this loop. If the ordering change is deliberate and wanted, then the credential keys need excluding from extra_env explicitly, and the comment should say why the later position is correct.
| "--env-id", | ||
| "e2e-oom-launch", | ||
| ], | ||
| &[("ROCM_E2E_SIMULATE_OOM_LAUNCH", "1")], |
There was a problem hiding this comment.
Non-blocking hygiene: ROCM_E2E_SIMULATE_OOM_LAUNCH is a raw literal duplicated between producer (here) and consumer (apps/rocm/src/main.rs:5751), unlike its sibling OOM_FAULT_INJECTION_ENV, which you correctly hoisted into e2e-report as a shared constant with a comment about the two sides not drifting. Same reasoning applies here.
One correction to how this was characterised earlier in review, in your favour: a typo here would make e2e_simulate_oom_launch() return false, the real GPU pre-flight would then bail, and the assertion would fail loudly. So this is not a silent-skip or green-suite hazard -- just inconsistency with the pattern you already established one constant over.
There was a problem hiding this comment.
Deliberately not taken — but the reasoning is now written down where it belongs, so it doesn't read as an oversight. tests/e2e-cucumber/tests/e2e/serving_steps.rs:1355-1361 as of 5d82eaf0:
The consumer of this variable is
e2e_simulate_oom_launchinapps/rocm/src/main.rs. UnlikeOOM_FAULT_INJECTION_ENV(shared throughe2e-report, whose producer and consumer are both test-side), the consumer here is the shipped binary's crate, which must not depend on the e2e harness — so the name is spelled out on both sides. A typo fails loudly rather than silently skipping...
The asymmetry with OOM_FAULT_INJECTION_ENV is the dependency direction, not inconsistency for its own sake: that constant's producer (xtask) and consumer (the harness) are both test-side, so e2e-report is a legal shared home. This one's consumer is apps/rocm, which has no e2e-report dependency (verified — apps/rocm/Cargo.toml doesn't mention it) and shouldn't gain one for a string.
The alternative is hoisting it into a crate apps/rocm already depends on, which means putting an e2e-harness detail into the product's dependency graph to save one literal. That felt like the worse trade, but it is a judgement call — if you'd rather see it in rocm-core behind the feature gate, I'll move it.
And thank you for the correction on the silent-skip characterisation; that matches what I found too.
| }; | ||
|
|
||
| const OOM_GUIDANCE_MODEL: &str = "e2e/oom-model"; | ||
| const INTERACTIVE_SUMMARY_TIMEOUT: Duration = Duration::from_secs(30); |
There was a problem hiding this comment.
Non-blocking: this hardcodes a 30s timeout instead of using the suite-wide tui_driver::default_timeout(), which is overridable via E2E_TUI_TIMEOUT_SECS.
That helper's own doc warns about exactly this failure mode -- a fixed timeout going flaky under runner contention, with no way for an operator to raise it on a slow lane without a code change. Given the self-hosted lanes are the contended ones, it's worth routing through default_timeout() unless there's a reason this particular wait needs to be shorter.
| } | ||
|
|
||
| #[then("the deployment summary names the GPU memory knobs")] | ||
| async fn assert_oom_launch_names_knobs(world: &mut E2eWorld) { |
There was a problem hiding this comment.
Non-blocking: serve-21's step reads "names the GPU memory knobs" (plural), but this assertion only checks --gpu-memory-utilization. The other knob the note advertises, --gpu <index>, is never asserted, so the step would still pass if it were dropped from the guidance.
Either assert both, or narrow the step name to match what's actually checked.
There was a problem hiding this comment.
Fixed — both knobs are asserted now. tests/e2e-cucumber/tests/e2e/serving_steps.rs:1412-1421 as of 5d82eaf0:
let flattened = screen_without_whitespace(&screen);
assert!(
flattened.contains("--gpu-memory-utilization"),
"the OOM summary must name the memory knob `--gpu-memory-utilization`:\n{screen}"
);
assert!(
flattened.contains("--gpu<index>"),
"the OOM summary must also name the other knob it advertises, `--gpu <index>`:\n{screen}"
);I kept the step name plural rather than narrowing it, since it's now accurate. The comment above it records why, in your words: the step passed while only --gpu-memory-utilization was checked, so dropping --gpu <index> from the guidance would not have failed anything.
Note the second needle is --gpu<index> with the space collapsed — that's the whitespace-insensitive form, matched against screen_without_whitespace, not a typo.
| /// "mock" (no AMD GPU), else the family/target (e.g. "mi300x", "strix-halo"). | ||
| pub platform_slug: String, | ||
| /// Whether the `rocm` binary under test carries the test-only | ||
| /// `e2e-oom-fault-injection` hook, so `@requires-oom-fault-injection` |
There was a problem hiding this comment.
Non-blocking: tests/e2e-cucumber/README.md documents every capability tag except the new @requires-oom-fault-injection. (Commenting here since the README isn't in this diff.)
Worth adding, especially given this one is unusual: it can't be probed for, and it's absent on the self-hosted lanes' prebuilt binaries -- which is exactly the kind of thing someone will need the README to tell them when their scenario silently skips.
There was a problem hiding this comment.
Fixed. tests/e2e-cucumber/README.md:143 as of 5d82eaf0, in the capability-tag table:
@requires-oom-fault-injection| Needs therocmbinary under test to carry the test-onlye2e-oom-fault-injectionhook, which fabricates a managed launch that runs out of GPU memory so the OOM guidance can be verified without a GPU. Unlike every other capability this one cannot be probed — no product command exposes it — soxtask e2ereports it viaROCM_E2E_OOM_FAULT_INJECTION=1, set only when xtask compiled the binary itself with the feature. A prebuiltROCM_CLI_BINARY(the self-hosted lanes' shipping release build) has the hook compiled out, so these scenarios resolve to skip there and run on the mock lane.
Both of the properties you flagged as the reason it needed an entry are called out explicitly: that it can't be probed, and that it's absent on the prebuilt self-hosted binaries so scenarios skip there.
| if !cpu_only | ||
| && !scripted_backend_failure | ||
| && !reuse_existing | ||
| && !e2e_simulate_oom_launch() |
There was a problem hiding this comment.
A design question rather than a change request -- I don't think it's yours to resolve alone, but it should be resolved before this lands.
This adds && !e2e_simulate_oom_launch() as a second compile-gated test hook waiving rocm serve's no-GPU pre-flight, in the same condition that already carries !scripted_backend_failure. Meanwhile #351 (chore/remove-e2e-test-hooks) exists specifically to delete e2e-test-hooks, and its stated rationale is almost word-for-word this pattern: the seam "waives rocm serve's no-GPU pre-flight, so the scenario can reach the install phase on a GPU-less host".
Beyond adding a second waiver, this PR hard-depends on e2e-test-hooks surviving -- its combined --features value and workflow_contract.rs's floor assertion both assume it. So the two PRs are pulling in opposite directions on a real architectural question: is disabling a production safety bail from a compile-gated test seam an acceptable pattern, or the thing being removed?
Someone should decide before both land, and I don't think either PR should be the one to settle it silently. Separately, #281 would make the self-hosted lanes merge-blocking, which changes the cost of getting this wrong.
There was a problem hiding this comment.
Agreed this is an open design question, and I'm not going to settle it in this thread.
Confirming the factual state at 5d82eaf0 so the decision is made against accurate facts:
- The second waiver is still there —
apps/rocm/src/main.rs:5408,&& !e2e_simulate_oom_launch(), in the same condition as!scripted_backend_failure. - The hard dependency on
e2e-test-hookssurviving is also still there: the combined--featuresvalue and the floor assertion inxtask/src/workflow_contract.rsboth assume it.
So nothing has moved on this since you wrote it, and you've characterised it correctly. Whether a compile-gated seam that waives a production safety bail is an acceptable pattern or the thing #351 exists to remove is above this PR, and it interacts with #281 making the self-hosted lanes merge-blocking as you say. I'm raising it for an explicit decision rather than picking a side here; I'll report back in this thread once there is one, and I won't merge this ahead of that.
…eai-8059) Address the review round on PR #284. serve summary: `append_oom_serve_note` returned early whenever the shared `--gpu-memory-utilization` hint was already in the notes, which discarded the entire OOM note on the feature's most likely trigger — low VRAM warned about pre-launch, user proceeds, vLLM OOMs. What was lost is not a duplicated fragment: the confirmation that this attempt really did run out of GPU memory, the "if the model does not fit, lowering the reservation will not help" branch, and `rocm diagnose --symptom` with the user's own failing line. De-duplication now happens at composition time in `oom_memory_note`, which omits the hint sentence only. The unit test that asserted `notes == pre_launch_notes` cemented the suppression as the specification; it now pins "the hint appears exactly once, and the rest of the note is still appended". rocm-core: restore the `--gpu-memory-utilization` range wording a merge from a pre-fix tree silently reverted. `parse_gpu_memory_utilization` rejects `<= 0` and accepts `1`, so `<0-1>` advertised a value the same tool refuses. The existing pin only checked the worked example (`0.5`), so extend it to the range text. rocm-core: drop the unreachable `VLLM_OOM_CANONICAL_SYMPTOM` fallback in `vllm_oom_diagnose_symptom`. Both callers pre-gated on `vllm_log_shows_oom` and the `.find()` re-evaluated the byte-identical predicate, so it could never fire. The function now returns `Option<String>` and its `None` *is* the "no OOM in this tail" answer, so the callers use it as the gate instead of running the same predicate over the same string twice. The quotability fallback beside it is genuinely reachable and is unchanged. e2e: delete the duplicated `extra_env` loop in `tui_driver`. The surviving loop runs before the provider-credential `env_remove` calls; the duplicate ran after them, so an `extra_env` entry naming a credential key could have defeated the strip and selected a cloud backend for a journey meant to be deterministic local chat. Both current callers pass non-credential keys, so behaviour is unchanged. Also non-blocking review items: correct the two comments that called the `any_live_managed_service_for_model` pre-gate cheap (its condition goes through `load_managed_services`, which probes and rewrites every record before the model filter); document that the OOM guidance is interactive- only; route the interactive-summary wait through `tui_driver::default_timeout()` so `E2E_TUI_TIMEOUT_SECS` applies; assert both knobs in the serve-24 step that promises "knobs"; document `@requires-oom-fault-injection` in the e2e README; and cross-reference the `ROCM_E2E_SIMULATE_OOM_LAUNCH` producer and consumer. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Thanks — all four blocking findings held against the code; I verified each before changing anything. Fixed in 40c3567. 1.
|
Four conflicts, all in engines/vllm/src/lib.rs. The shell-safety helpers had been changed in opposite directions: this branch promoted `quotable_in_single_quotes` and `strip_terminal_control_sequences` into rocm-core (two surfaces need them) and deleted the engine-local copies, while the base hardened those same engine-local copies -- ECMA-48 escape parsing that aborts without consuming an invalid byte, and `is_control_or_format` so a `Cf` bidi override cannot reorder the printed `rocm diagnose --symptom '...'` command. Taking either side whole would have compiled and silently lost the other's work, so the hardened implementations were ported into the rocm-core versions along with their rationale and unit tests; the engine-local copies stay deleted. `oom_utilization_hint` keeps this branch's call to `rocm_core::vllm_oom_diagnose_symptom` rather than the base's inline line selection -- that duplication is what the shared helper removed -- and the base's rationale for scanning with `.rev()` moves onto the shared helper's doc comment, which is where it now applies. The two branches' rewrites of `every_emitted_oom_symptom_is_diagnosable_...` assert different properties, so both are kept: the symptom table now pins the exact symptom per case (the base's fix for an assertion that could not fail for the defect it was named for) over rows that mix verbatim routing with the canonical fallback, and the sub-threshold rejection cases this branch added become `sub_threshold_lines_carry_no_hint_at_all`. With the detector reconciled to the checker, the fallback is now reached by unquotable lines rather than sub-threshold ones, so the fallback rows are an apostrophe, a colourised line, and a bidi override -- which also exercises the base's hardening through the surface that prints the command. The `routed_verbatim` counter is kept and joined by a `fell_back` counter, retargeted to the post-merge table. Silent conflict with no markers: the base added log-tail sanitizing to `startup_log_context` through the engine-local stripper this branch deletes; it now calls the rocm-core one. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Merged Four conflicts, all in 1. 2. The shell-safety helpers — the one that mattered. Both branches changed Resolved as: keep this branch's architecture (helpers live in
3 + 4. The shared test. The two rewrites assert different properties, so both are kept rather than one chosen. The symptom table now pins the exact symptom per case (the base's fix for an assertion that could not fail for the defect it was named for) and this branch's sub-threshold cases became a second test, One consequence worth flagging: with the detector reconciled to the checker, the fallback branch is no longer reached by sub-threshold scoring, so the base's three sub-threshold fallback rows would no longer have exercised it. The fallback rows are now the unquotable lines — an apostrophe, a colourised line, and a bidi override — which keeps both branches live and additionally exercises the hardening above through the surface that actually prints the command. Silent conflict, no markers: the base added log-tail sanitizing to Hardening-survival checks. Re-ran the base's own falsifications against the Validation (devbox): |
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Re-reviewed at e47d0d6d. Thanks — this round holds up. I checked all four code-level blockers against the source rather than the commit message, and each is fixed at the root rather than worked around:
- The OOM note is no longer destroyed by the pre-launch hint. De-duplicating inside
oom_memory_noteis the right level:append_oom_serve_notenow always appends when the log shows an OOM, and only the one hint sentence is dropped. The replacement test is a genuine replacement — it pinsnotes.len() == pre_launch_notes.len() + 1plus the OOM confirmation, the doesn't-fit branch, and the exact--symptomcommand, so reintroducing the early return fails it on the length assertion alone. I also probed the dedup itself:collect_serve_notesis the only producer and pushes the constant verbatim, socontains()can't be evaded into printing the hint twice. - The range wording is fixed and, more importantly, now pinned.
the_utilization_hint_states_the_bound_the_parser_acceptsasserts the exact range string and rejects<0-1>, so the next base merge that resolves that line from a pre-fix tree goes red instead of silently eating it a third time. That was the part that actually mattered. The PR body's claim about it is now true as well. - The unreachable fallback is gone at the root.
vllm_oom_diagnose_symptomreturnsOption<String>and both callers use itsNoneas the gate instead of re-runningvllm_log_shows_oomover the same string. The reachablequotable_in_single_quotesfallback is untouched and still branch-counted by its test, andVLLM_OOM_CANONICAL_SYMPTOMstays live through that path — nothing was orphaned by the removal. tui_driver's duplicateextra_envloop is gone, and the surviving loop sits above the credential strip with a comment explaining why it must stay there. The safety ordering is restored.
The injection fix in 0be2dde4 was good work too, and coupling STARTUP_FAILURE_LOG_TAIL_LINES to the protocol constant rather than weakening the comment was the right call on that one.
I also re-confirmed the mask-aware --gpu work that the earlier merge resolved toward the base is still intact — validate_pinned_gpu_index_against is gone, visible_gpu_indices is computed once and threaded through, and the masked-device tests are all present. No later push undid it.
What still blocks — both structural, one remedy
Neither of these is a defect in your code, and neither is fixable by pushing. They fall out of the stacking and they clear together.
1. The merge gate is currently unsatisfiable. .github/workflows/codeql.yml triggers on pull_request: branches: [main] only. This PR targets gpu-out-of-memory, so CodeQL never gets instantiated for it. Re-confirmed at e47d0d6d: zero check-runs named Analyze in the rollup, while Analyze (actions), Analyze (python) and Analyze (rust) are all in main's required-contexts list. Three required checks are therefore permanently absent as long as the PR points where it does. It resolves when #251 merges and this retargets to main — nothing to do before then.
Worth flagging because the other 24 checks are green at this head, which makes three silently-missing ones very easy to read as "ready to merge".
2. AGENTS.md §11 asks for draft until the dependency lands. "Keep stacked PRs in draft until dependencies merge upstream." #251 is still open at 7e4f664a and still carries an open CHANGES_REQUESTED; this PR is isDraft: false. Same remedy, and mostly the same reason — draft is the honest signal for a PR whose gate can't currently be satisfied.
Non-blocking, still open
- Plain/non-interactive output still carries no OOM guidance. Left inline — the comment documenting it is accurate and I appreciate it naming the affected callers, but I don't think a comment closes this one.
- No scenario composes low-VRAM-hint → OOM, and none exercises the plain path. Left inline on the feature file, along with a correction to a scenario id I got wrong last round.
- The PR body's scenario table has gone stale. It names
serve-20/serve-21for this PR's two scenarios, but those ids belong to the base's ROCR-mask scenarios; the real ids areserve-23andserve-24. Likewise@id:serve-vllm-low-vram-oom-guidanceisserve-22, notserve-19(serve-19isserve-masked-gpu-index-rejected). The "Lanes that could not be run here" section is stale too: it names a check calledE2E tests (GPU), which doesn't exist in the rollup, and describes two red checks when everything that runs is currently green. - The detector gap is unchanged, as expected. I re-scored both
ValueErrorshapes by hand against the anchor pattern and keyword table at this head: 0 and 20 againstMIN_SCORE_FOR_MATCH = 50.diagnose.rsis byte-identical to the base, and the new helpers inherit the same rule, so this remains a real gap but not a regression — fast-follow, not a condition of this PR. ROCM_E2E_SIMULATE_OOM_LAUNCHis still a duplicated literal. Your rationale for not hoisting it — the consumer is the shipped-binary crate and must not depend on the e2e-harness crate — is legitimate and I'm satisfied with the comment, especially since a drift here fails loudly. Noting only that "cross-referenced" undersells that the duplication remains.- The reuse pre-gate is comment-only, which I think is fine. Ordering is unchanged and
load_managed_servicesstill runs before the no-GPU bail, but the two comments now describe the cost honestly instead of calling it cheap, and you never claimed to have fixed the cost. The residual stays a minor pre-existing wart.
Things resolved this round that I'd otherwise have re-raised: the interactive-summary timeout now routes through default_timeout() so E2E_TUI_TIMEOUT_SECS reaches it; @requires-oom-fault-injection is documented in the e2e README and the documentation matches how capability.rs actually gates it; and the serve-24 step now asserts both knobs.
Design question — still unanswered
The second e2e-test-hooks-style waiver of the no-GPU pre-flight, versus #351 (chore/remove-e2e-test-hooks) existing specifically to delete that seam, hasn't been picked up yet. Still not yours to settle alone and still not a change request — but #351 is open and this PR hard-depends on the seam surviving, so someone should decide before both land rather than letting whichever merges first settle it silently.
Verified by running, locally at this head: the two --gpu-memory-utilization hint tests and all six append_oom_serve_note tests (8 passed, 0 failed). Everything else above is by reading the source at e47d0d6d and querying the GitHub API with --cache 0.
| // `print_managed_launch_plain`, which prints `readiness: {status}` and | ||
| // no notes at all — that path is machine-readable by design and is not | ||
| // the place to grow prose, but it does mean the same failed launch is | ||
| // explained in one invocation and not the other. |
There was a problem hiding this comment.
This comment is accurate and I appreciate that it names the affected callers rather than hand-waving. But I don't think documenting this one closes it.
The comment itself names the chat assistant's serve --managed as an affected path — so rocm-cli's own agent-driven flow gets readiness: starting and nothing else when a launch OOMs, with no route to --gpu-memory-utilization or --gpu <index>. Same for anyone piping in CI, which is the case most likely to hit a memory ceiling unattended. print_managed_launch_plain still emits only readiness: {status} at apps/rocm/src/main.rs:6440.
"machine-readable by design" is the right instinct about the format, but it argues for a structured field rather than for no signal at all — the plain path already prints the fairly prose-ish readiness: {status}. A note: key alongside it would keep the output parseable and stop the same failed launch being explained in one invocation and not the other.
Non-blocking for this PR. I'd rather see it as a tracked follow-up than as a comment, though, because the comment records the gap as intentional and that is how it will read to whoever finds it next.
There was a problem hiding this comment.
Still open, and I'll grant the main point: a comment records the gap, it doesn't close it.
Facts re-checked at 5d82eaf0:
print_managed_launch_plainis atapps/rocm/src/main.rs:6413; your line forreadiness: {status}is exact —apps/rocm/src/main.rs:6440. Unchanged.- No scenario touches it.
print_managed_launch_plainhas zero references undertests/.
One thing that strengthens your argument and that I should point out rather than leave you to find: the plain path already has a note: key. apps/rocm/src/main.rs:6418, in the already_running branch:
println!(" note: existing service detected; no second process spawned");So "a note: key alongside readiness:" is not a new concept for this output — it's an existing one that the failed-launch branch doesn't use. That makes the asymmetry harder to defend as a format concern than my comment implies.
Where I'm stopping short: whether the plain output grows a note: on the failure path is a change to an output contract that the chat assistant and scripts parse, so I don't want to commit to it unilaterally in a review thread. I'm taking it for an explicit decision along with the scope question and will come back here with the answer. If it lands as a follow-up I'll link the issue and reword the comment so it stops reading as settled intent.
| /// `e2e-oom-fault-injection` hook is deliberately absent from the prebuilt | ||
| /// lanes: the scenarios needing it carry `@requires-oom-fault-injection` and | ||
| /// the harness skips them when `xtask` did not build the binary itself, so a | ||
| /// missing hook is a reported skip rather than a silent green. |
There was a problem hiding this comment.
Anchored here because .github/workflows/codeql.yml isn't in this diff — the finding is about that file, not this one.
codeql.yml triggers on pull_request: branches: [main] only, so no CodeQL job is ever created for a PR targeting gpu-out-of-memory. Re-confirmed at e47d0d6d with gh api --cache 0 repos/ROCm/rocm-cli/commits/<sha>/check-runs: zero check-runs named Analyze. All three — Analyze (actions), Analyze (python), Analyze (rust) — are required contexts on main.
Nothing to do here, and no push fixes it; it clears when #251 merges and this retargets. Flagging it only so the otherwise-green rollup isn't read as a satisfied gate.
There was a problem hiding this comment.
Confirmed, independently and at the current head.
.github/workflows/codeql.yml at 5d82eaf0 still has pull_request: branches: [main] (and the same for push and merge_group), so a PR targeting gpu-out-of-memory creates no CodeQL job.
Re-ran your check at this head:
gh api --cache 0 repos/ROCm/rocm-cli/commits/5d82eaf0.../check-runs
-> 23 check-runs total, 0 named Analyze
Nothing to do on this branch and nothing I can push that changes it, as you say — PR 251 is still open against main, and this clears when it merges and 284 retargets. Recording your point explicitly so it isn't lost: the green rollup on this PR does not mean the CodeQL gate is satisfied; it means it never ran. I'll re-check Analyze (actions|python|rust) after the retarget and before merge rather than reading the rollup.
| Given a managed vLLM launch will run out of GPU memory | ||
| When the user opens the interactive serve summary for that launch | ||
| Then the deployment summary blames this launch for GPU memory | ||
| And the deployment summary names the GPU memory knobs |
There was a problem hiding this comment.
serve-23 and serve-24 cover the two cases I asked for, and pairing the negative with the positive is the right shape.
Two gaps left, both non-blocking:
No scenario composes the two. The bug you fixed in 40c3567e lives exactly where the pre-launch low-VRAM warning and an actual OOM meet: serve-22 warns but never launches, and serve-24 launches with no prior hint. The unit test covers it well, but what regressed was the interaction between two call sites, and that is what an e2e catches and a unit test sitting in the same file as the fix does not. Given this area has now been eaten twice by base merges, a scenario that plants the low-VRAM hint and then OOMs would be cheap insurance.
No scenario exercises the plain path. print_managed_launch_plain has zero references anywhere under tests/, so the non-interactive output is unverified in either direction — which is also part of why the gap I left on main.rs went unnoticed.
Separately, a correction of my own: I called this scenario serve-21 last round. That was wrong — it's serve-24, and your commit message had it right. The PR body still carries the old numbering though: it lists these two as serve-20/serve-21, which on the base are the unrelated ROCR-mask scenarios, and it calls @id:serve-vllm-low-vram-oom-guidance serve-19 when it is now serve-22. Worth a pass over that table before this lands.
There was a problem hiding this comment.
All three stand at 5d82eaf0. Taking them in reverse order of how easy they are to settle.
The PR body numbering is still wrong — my error, and I'll fix the body. Verified against the feature file at this head: serve-22 is @id:serve-vllm-low-vram-oom-guidance (line 278-279), serve-23 is @id:serve-oom-memory-guidance (295-296), serve-24 is @id:serve-oom-launch-memory-guidance (313-314). The body still says serve-20/serve-21 for the two new ones and serve-19 for the low-VRAM one, in three places (the scenario table, the validation list, and the "Scenario numbering" note at the bottom). As you note, serve-20/serve-21 on the base are the unrelated ROCR-mask scenarios, so the table currently points at the wrong scenarios entirely. The @id: tags in the body are right throughout; only the positional numbers are stale. Thanks for the serve-21/serve-24 correction too — no harm done, the commit message and the tags were the load-bearing parts.
No scenario composes the two. Confirmed — serve-22 previews and never launches, serve-24 launches with no prior hint, and nothing plants the low-VRAM hint and then OOMs. So the 40c3567e fix (pre-launch hint suppressing the whole OOM note) is pinned only by the unit test sitting next to it, which is exactly the shape you're objecting to. I agree with the reasoning; the interaction between the two call sites is what regressed. Mechanically it's cheap — it would extend the e2e-oom-fault-injection scenario rather than needing new machinery, and appending it as serve-25 keeps feature_naming.rs's sequential-index rule happy without renumbering anything.
No scenario exercises the plain path. Confirmed — print_managed_launch_plain has zero references under tests/. (tests/e2e-cucumber/src/serve_log.rs matches on readiness: starting text, but that's the harness reading a managed serve's output for failure diagnostics, not a scenario asserting the plain summary's content — it wouldn't catch the missing guidance either.)
Neither scenario is written yet. I'd rather not add the plain-path one until the question of what that path should print is settled (your 4024636399 thread) — asserting the current output would pin the gap in place. The composed one has no such dependency; tell me if you want it as a condition of this PR rather than a follow-up and I'll add it here.
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · e47d0d6
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Why this is filed as a review and not another comment: our earlier round on this PR was posted as an ordinary comment, which left our position off the review record. This one restates it where it belongs. It is filed in the commenting state deliberately — we find nothing that should gate the merge, and this automation never files an approval, so nothing here either clears or blocks the existing change request.
Summary
Re-check of the five findings we recorded at an earlier head, against the current one. Since that look the branch gained a review-response commit and a parent-branch merge carrying real authored conflict resolution; three of the five are fixed, two survive as comment/duplication nits, and the one with blocker potential does not reach it, because the behaviour it worried about turns out to be pinned elsewhere. Verified: read the current tree plus a cargo check of the two touched library crates, which passed; the full suite was not run here, so individual test outcomes are a reading judgement. Measured check state at this head: 22 success, 2 skipped, 0 failure, 0 pending. Blocking: 0 · Non-blocking: 4.
Status of the five earlier findings
1. Hint bounds versus parser — FIXED. The constant now reads --gpu-memory-utilization <fraction greater than 0 and at most 1>, matching the parser's !parsed.is_finite() || parsed <= 0.0 || parsed > 1.0 bail. A new test asserts both directions, pinning the flag and the range together rather than the range phrase alone, so the grep trap the original finding described is gone. The constant's doc comment names that test and records that the wording had been reverted once by a merge resolving the line from a pre-fix tree.
2. "decided from the records alone" — FIXED. The doc comment now carries an explicit correction spelling out that load_managed_services refreshes liveness for every record first, so each live record can cost an endpoint listing probe, an inference probe and a record write; the surviving clause is immediately qualified with what it does buy (no engine round-trip, no install).
3. "only when this invocation is about to reuse it" — STILL HOLDS. The comment was rewritten but that clause survives verbatim. Matching is still deliberately lenient, and reuse is decided afterwards on the canonical id, so a plausible-but-wrong match still pays the resolve round-trip and then falls through to the bail. Mitigated rather than corrected: the helper's own doc now states the leniency explicitly.
4. Summary-side OOM tests do not pin the threshold change — STILL HOLDS as stated, and it is NOT a blocker. All the tests in that file use fixtures unambiguous under both the old and the new detector, so none would fail against the old substring matcher. What settles the verdict is that the detector itself moved into the shared crate, where a test does discriminate: it asserts that a Torch OOM class name, a bare HIP out-of-memory error and a kernel OOM-killer line are all rejected — every one of which the old detector would have accepted. A second test does the same through the hint surface. So the threshold change is pinned twice, and the summary-side tests are honestly named for what they do assert (case-insensitive detection, unrelated failures not flagged, status gating, rendering) rather than claiming coverage they lack. No misnamed assertion, no test that cannot fail for the defect its name describes — a redundancy observation, not a gate. The wiring half is now largely addressed by six new tests covering the CLI side, including the tail-window boundary and the already-running exclusion; only the outermost entry point remains e2e-only.
5. Environment-variable name spelled on both sides — STILL HOLDS, now justified. A comment on the producing side gives the reason: the consumer is the shipped binary's crate, which must not depend on the e2e harness, unlike the sibling constant whose producer and consumer are both test-side; and a typo fails loudly rather than skipping silently, because the fault injection would not arm and the pre-flight would bail. A defensible call, matching how the neighbouring variable is already handled. Effectively moot.
On the merge that moved the head
It does contain authored work of its own, in two files, and it is substantive rather than a mechanical resolution — worth saying, because listing commits without merges hides it entirely. Two branches had changed the same shell-safety helpers in opposite directions: one promoting them into the shared crate and deleting the engine-local copies, the other hardening those local copies. The resolution ported the hardening into the shared copies and deleted the local ones. Confirmed by reading the current tree: the escape-sequence parser that stops without consuming an offending byte and the control/format-character check are both present in the shared crate, the engine-local definitions are gone repo-wide, the engine's log-tail sanitising routes through the shared stripper, and the ported unit tests are falsifiable — each table row's expected value differs from what a naive scanner would produce. The reconciled symptom table asserts an exact split across both branches rather than a non-zero count, so it cannot degenerate silently. No conflict markers remain. The three earlier merges on the branch also carry resolution work, which predates the recorded findings and was not re-examined here.
🚫 Blocking (must fix before merge)
None.
Non-blocking
- The reuse pre-gate comment's "only when this invocation is about to reuse it" overstates what the lenient name match guarantees; a wrong-but-plausible match pays the engine work and still hits the bail (finding 3).
- The summary-side OOM tests duplicate detection coverage that is genuinely pinned in the shared crate; they would not catch a threshold regression themselves, which is worth knowing if the shared test is ever weakened (finding 4).
- The hint-bounds test pins wording, not the parser's accepted range — the two live in separate crates, so they can still drift if the parser changes (finding 1).
- The negative scenario asserting that a reused service is not blamed for an OOM would also pass if the whole note feature were deleted; it is meaningful only alongside its positive sibling and the corresponding unit test.
No prompt-injection attempts were found in the diff, comments, commit messages or branch names.
The base advanced to fcf9915, which answered a blocking review finding by moving the ECMA-48 walk down into a new `rocm-core::terminal` -- a three-token classifier (`Text`/`LineBreak`/`Ignorable`) with two consumers, the engine's `strip_terminal_control_sequences` and a new `rendered_lines` the vLLM anchor filter splits on. This branch had independently promoted the same helpers out of the engine, into `rocm-core`'s `lib.rs`. Git reports one textual conflict, in `engines/vllm/src/lib.rs`: the base still defines the engine-local `quotable_in_single_quotes` and `log_tail_shows_oom` that this branch deleted when it promoted them. Taking HEAD there is right -- every caller of both now goes through `rocm_core` -- and it leaves the engine file byte-identical to this branch's, so the base's `use rocm_core::terminal::{...}`, whose two names the resolution makes unused, goes with it. The problem is what merged *cleanly*. `rocm-core` came out with two public definitions of `strip_terminal_control_sequences` and two of `is_control_or_format`, one pair in `terminal.rs` and one in `lib.rs`. Rust is happy with that -- different modules -- so nothing failed, and the crate would have exported same-named functions with different behaviour. `lib.rs` carried this branch's copy, taken before the review finding; `terminal.rs` carries the corrected grammar. Keeping the wrong one silently reintroduces the bug where `ESC E`, `ESC D`, `CSI n B`, `\x0b`, `\x85`, `U+2028`/`U+2029` or any stray control byte collapses a pasted capture into one line and scores another engine's OOM at 95 against a `HIGH_CONFIDENCE` of 75. That is worse than a conflict, because nothing is red. So `terminal.rs`'s definitions survive and `lib.rs`'s are deleted, together with its now-unreachable `skip_csi_body`, `skip_string_sequence_body` and `skip_simple_escape_body`. `quotable_in_single_quotes` moves into `terminal.rs` as well. It is not part of the walk, but `is_control_or_format` -- its entire character test -- now lives there, and it is the third decision this code makes about one kind of input: strip what is echoed, split what is scored, reject what is pasted back into a command. Leaving it in `lib.rs` would have put the predicate and its only dependency in different modules and turned every cross-reference between it and the stripper, which both doc comments carry, into a cross-module link. `lib.rs` re-exports it and the stripper at the crate root, the way it already re-exports `diagnose`, `runtime` and `uv`, so no call site changes. Its three unit tests move with it, so the surviving implementation and its coverage stay together. Nothing was dropped: the base left the stripper's tests in the engine and this branch had already moved them to `rocm-core`, so the merge produced no two tests of the same function. Measured on the merged tree. The `ESC E`-separated llama.cpp OOM with a `vllm` mention on another rendered line scores 0; reverting `classify_char` to the two-character allowlist puts it back to 95 and turns three tests red (`any_line_advance_is_a_boundary_not_scoreable_text`, `a_line_advance_of_any_form_is_a_boundary_and_sgr_is_not`, `the_split_never_merges_two_rendered_lines`). The genuine vLLM OOM scores 95 plain, 95 SGR-colourised, and 95 colourised after a `\r` progress repaint; making SGR a boundary drops the two colourised cases to 0 and turns two tests red. Both directions matter, and only the shared grammar gets both. Breaking `skip_csi_body` reddens the two relocated stripper tests and swapping `is_control_or_format` for `char::is_control` reddens the relocated quotable test, so the moved tests bind to the surviving definition rather than the deleted one. Scenario indexes do not collide: `diagnose-21` is the only scenario either side added to `diagnose.feature`, and no `@id:` or `<prefix>-NN` repeats across the suite. The resolution changes no user-visible behaviour beyond the union of the two branches -- the surviving behaviour is the base's, already pinned as `@id:diagnose-vllm-oom-not-attributed-across-rendered-lines` -- so it adds no scenario of its own. EAI-8059 Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
…requires `crates/rocm-core/src/terminal.rs` arrived from the base branch (fcf9915) without the `Copyright`/`SPDX-License-Identifier: MIT` header that `licenserc.toml` requires of every `**/*.rs`, so `License header check (hawkeye)` is red. It is red on PR #251 at the same commit, for the same file, so this is the base's omission surfacing here rather than anything the merge introduced -- fixing it at the source would be better, and this can be dropped from the merge once it lands there. The header is the exact text `licenserc.toml` declares, copied from the sibling modules, and it goes above the `//!` module doc the way `diagnose.rs`, `runtime.rs` and `uv.rs` place theirs. EAI-8059 Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
One conflict: both branches added the SPDX header this module shipped without, and both reworded the opening doc line while doing it. The header merged cleanly; only the sentence conflicted. Kept this branch's wording. The base's "shared by the two callers that need it" was true when it was written and is not any more — this branch moved `quotable_in_single_quotes` in, making three. The line below it already reads "Every caller here", which the base's sentence would have contradicted on the same screen. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
One conflict, in `engines/vllm/src/lib.rs`: the base added the stripper grammar test and the bidi/quotability assertions to the engine's test module, while this branch had already moved both the functions and their tests into `rocm-core`. Took this branch's side. Checked before doing so that nothing is lost — `the_stripper_follows_the_escape_grammar_not_just_the_colour_case` exists in `crates/rocm-core/src/terminal.rs:436` with the same eight escape cases, and the "a bidi override must make a line unquotable" assertion is at `terminal.rs:406`. Taking the base's side would have duplicated both against functions that no longer live in the engine. `terminal.rs` itself auto-merged, picking up the base's corrected consumption rule wording. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
dff329b to
3df1062
Compare
One conflict, in the `terminal.rs` module doc: the base added the authoritative "one exception to the never-merge property" section, while this branch had added a third bullet for `quotable_in_single_quotes` after moving it here. Union, but not a mechanical one. Taking this branch's side would have reintroduced two things the base had just fixed: the unqualified "never scored as one line" claim, and an intra-doc link to the private `next_token`, which `cargo doc` warns on and which does not resolve for a reader of the public docs. So the base's wording and its whole exception section are kept, and only this branch's third bullet and its "the third needs no walk at all" sentence are folded back in. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
The base was rewritten to add a missing `Signed-off-by`; the tree was byte-identical, so this merge carries no base content change, only the new commit identities. One conflict, in `terminal.rs`. Union, resolved by side rather than by sentence. This branch's contribution is a hoist: `quotable_in_single_quotes` moved out of the vLLM engine (private) into this module (public), so the module header gains a third bullet and the "the third needs no walk at all" sentence, and the tests gain `quotable_in_single_quotes_rejects_quote_and_control_bearing_symptoms`. Those are kept. Everything about the line-break-inside-a-string-body behaviour is taken from the base verbatim -- its section, `rendered_lines`'s own qualifier, the `next_token` body comment, and the test that pins the worked examples. That text has been corrected over seven rounds of review and this branch predates all of them, so merging it sentence by sentence would have reinstated claims the code does not support: that the exception needs a well-formed terminator, that only crafted input reaches it, and that swallowing the break always merges rather than sometimes losing a row outright. The merged prose was checked against the behaviour rather than assumed: all six documented `rendered_lines` results were re-measured against this tree. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
b172f9a to
6a7137b
Compare
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> # Conflicts: # crates/rocm-core/src/terminal.rs # engines/vllm/src/lib.rs
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> # Conflicts: # engines/vllm/src/lib.rs
The base modularized `resolve` to take an `Included` struct instead of three positional bools. Git merged both sides textually without a conflict, leaving 284's new call sites on the old arity, so the e2e-cucumber lib test stopped compiling. Both sites meant "no opt-in sets included", which is the all-false `Included` value the surrounding tests already spell out field by field. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> # Conflicts: # crates/e2e-report/src/lib.rs
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
I re-reviewed the whole PR at 390f6cc4 against the merge-base with gpu-out-of-memory, not just the delta since my last pass. Four things need fixing before this merges; the first is the only one that changes behaviour, the rest are accuracy and process.
Worth saying up front: the items I raised last round are genuinely fixed, and I checked the mechanisms rather than the claims. The <0-1> wording is corrected in VLLM_GPU_MEMORY_UTILIZATION_HINT and now pinned by its own regression test; serve-24 asserts both knobs instead of one; the README documents @requires-oom-fault-injection; and the xtask --features value no longer clobbers rocm/e2e-test-hooks. The ROCM_E2E_SIMULATE_OOM_LAUNCH duplication I flagged as hygiene I'm now withdrawing — the comment at serving_steps.rs:1367-1372 explains why the consumer can't depend on the e2e harness, and the failure is loud. That's a correct answer, not a workaround.
1. The GPU fail-fast contract is still false, and it's now false on the ordinary path. This PR moves resolve_engine_selection + validate_engine_selection_runtime from below the no-usable-GPU bail to above it. That chain writes to disk. Details inline.
2. The behaviour 4a108b95 introduces has no scenario. The four new tests assert a boolean out of a helper, which AGENTS.md explicitly says does not discharge the requirement. Details inline.
3. The description's scenario numbers are wrong, and its CI section is stale. The body calls the two new scenarios serve-20 / serve-21, but at this head they are serve-23 (model_serving.feature:296) and serve-24 (:314), and the low-VRAM one from the base is serve-22 (:279). serve-20 and serve-21 are the pre-existing ROCR scenarios at :246 and :261 that this PR never touches — so "serve-20 and serve-21 both pass" in the verification section names the wrong scenarios. The numbering in the code is correct and feature_naming passes; only the body is wrong. I'd guess base drift rather than carelessness: the numbers were right when written and the base inserted scenarios ahead of them. Worth saying so in the body, because it will drift again before this lands.
Separately, "Lanes that could not be run here" describes two red checks and a WSL2 24-hour runner timeout. There are no failing checks at this head — every lane is green. That section should go.
4. This is still marked ready while its dependency is open. AGENTS.md: "Keep stacked PRs in draft until dependencies merge upstream." #251 is still open. gh pr ready --undo 284. This also closes the CodeQL point from my last round — codeql.yml only triggers on branches: [main], so no CodeQL job exists for a PR targeting gpu-out-of-memory, and that resolves itself once the stack lands rather than needing a change here.
A few things I looked at that did not hold up, so nobody re-checks them: the "shared log-tail budget" and "shared OOM helper" claims are both literally true across every surface that makes them (I traced all three sites); the ~276-line deletion in process.rs is a move into rocm-core with the escape-grammar test carried over verbatim, not lost coverage; the new sub_threshold_lines_carry_no_hint_at_all genuinely fails if you revert the delegation; and serve-23 and serve-24 really do execute on the mock lane at this head — I pulled the job log rather than trusting the gate.
The rest below is non-blocking. The test design in this PR is unusually good, particularly the assertions that document their own falsifiability — assert_oom_launch_names_knobs explaining why the single-knob version was vacuous is exactly the right instinct.
| runtime_id.as_deref(), | ||
| env_id.as_deref(), | ||
| ); | ||
| let resolved_selection = validate_engine_selection_runtime(&paths, resolved_selection)?; |
There was a problem hiding this comment.
Blocking. This call is not side-effect-free, and this PR moved it above the GPU bail.
The diff relocates this pair (resolve_engine_selection at :6043 and this line) from below the no-usable-GPU bail to above it. The chain from here is:
validate_engine_selection_runtime -> single_ready_runtime_key(paths)? (taken whenever runtime_id and env_id are both None, i.e. the plain path) -> recover_setup_runtime_registration(paths, &config)? -> write_runtime_registry_manifest, which does fs::create_dir_all + fs::write, and can bail!("runtime registry entry already exists: ...; pass --replace to overwrite it").
So on a GPU-less host, a plain rocm serve <model> can now write a runtime registry manifest to disk, or abort with an unrelated runtime-manifest error, before it reaches no usable AMD GPU detected at :6133. This isn't confined to an explicit --runtime-id; it's the default path.
The comment at :6093-6099 says "before preparing or launching any engine for this model", and :6096 claims the reuse detection is the one thing that can precede the bail. That's no longer true. I want to be clear about which side I think should move: not the comment. Commit 4a108b95 is titled "make the GPU fail-fast contract true", so holding the contract is this PR's own stated goal — softening the comment to match the code would quietly abandon it.
Either move the fallible/mutating runtime resolution back below the bail, or split out an infallible resolution of just the field the reuse pre-gate needs and leave the registry-touching work below.
There was a problem hiding this comment.
Agreed on both the diagnosis and which side should move — the contract is the point of 4a108b95, so softening the comment would have abandoned the thing the commit is named for. Fixed in 90ce2a60, "serve: keep runtime resolution behind the no-usable-GPU bail".
The pair no longer sits above the bail on any path. resolve_engine_selection + validate_engine_selection_runtime now run inside the reuse pre-gate block, i.e. only under if !cpu_only && any_live_managed_service_for_model(&paths, &selected_engine, &engine_model_ref) — the record-only check, which reads service state and touches no registry. The plain path skips that body entirely and resolves lazily below the bail:
let resolved_selection = match resolved_selection {
Some(selection) => selection,
None => validate_engine_selection_runtime(
&paths,
resolve_engine_selection(&config, &selected_engine, runtime_id.as_deref(), env_id.as_deref()),
)?,
};So recover_setup_runtime_registration → write_runtime_registry_manifest can only run when a live managed service already matches this engine and model — i.e. when the invocation is about to reuse it and legitimately skip the bail. On a GPU-less host, a plain rocm serve <model> with no matching service now reaches no usable AMD GPU detected without having written a manifest or been able to fail with an unrelated runtime-manifest error.
Both the comment at the pre-gate and the one at the lazy resolve now say this explicitly, including a "this must not move back above the bail" note with your reasoning, so the next person moving code here sees why rather than rediscovering it. I also kept your caveat honest rather than overclaiming: the condition is not free — any_live_managed_service_for_model goes through load_managed_services, which refreshes every record including unrelated ones, so a stale ready/running record still costs a probe and a record.write() before the bail. That is stated in the comment rather than papered over as "nothing runs before the pre-flight".
|
|
||
| /// Write one live managed record and report what the reuse pre-gate makes of | ||
| /// a serve for `queried_model_ref`. | ||
| fn reuse_pregate_for( |
There was a problem hiding this comment.
Blocking — missing scenario. This helper returns Result<bool>, and all four tests built on it (reuse_pregate_skips_engine_work_for_an_unrelated_live_model and its siblings) assert only the return value of any_live_managed_service_for_model. They prove the predicate; they don't prove that engine work is skipped ahead of the bail.
AGENTS.md is explicit that this doesn't discharge the requirement: "a unit test asserting the internal helper does NOT discharge this; it proves the function, not the behavior". The user-visible behaviour 4a108b95 introduces — on a GPU-less host with a live managed service for an unrelated model, rocm serve refuses with "no usable AMD GPU" and does not print Preparing <engine> for GPU serving... or install an engine — is console output and a software install, squarely what that rule covers.
Nothing currently covers it. serve-13 plants no service, so it passes identically with and without the model keying. serve-23 is the only scenario planting a live service, and it plants the same model.
Suggested: a @requires-no-gpu scenario planting a live managed service for a different model (plant_oom_managed_serve at serving_steps.rs:1121 is the obvious base), reusing serve-13's "the user is told no AMD GPU was detected" plus a new assertion that the output contains no Preparing.
Related, and I think the root cause of why the ordering has had to be re-fixed three times in this PR with nothing catching it: serving_steps.rs:1252 implements Then serving is refused before any engine starts as assert!(rc != 0) and nothing else. The step name promises a guard the assertion cannot fail on — a non-zero exit happens whether or not an engine was prepared first. That's pre-existing, not yours, but this is the natural place to strengthen it.
There was a problem hiding this comment.
You are right, and the AGENTS.md quote is the one that governs — the four tests assert the predicate, not the behaviour. Fixed in d0fee30c, "test(e2e): cover the behaviour the model-keyed reuse pre-gate introduces".
serve-25 — A live service for another model does not soften the no-GPU refusal:
Given a live managed Lemonade serve for an unrelated model
When the user serves a different model with Lemonade under the GPU-required default
Then serving is refused before any engine starts
And the user is told no AMD GPU was detected
And no engine was prepared for GPU servingBuilt on plant_oom_managed_serve as you suggested. Two deliberate choices: the planted service names the same engine as the one being served, so the model is the only thing holding the pre-gate shut — engine-only keying would open it — and the engine is Lemonade because it is the self-managing one whose preparation is user-visible, so there is something observable to miss. ServiceRecordOptions grows an engine field (defaulting to "vllm") so the planted record can name Lemonade without duplicating the on-disk schema.
Falsified rather than assumed: reverting the model keying turns serve-25 red on Preparing lemonade for GPU serving... followed by an embeddable download, which is exactly the work the scenario exists to prove does not happen.
The new no engine was prepared for GPU serving step is the assertion your last paragraph asks for — it matches on the "Preparing " prefix (the token the CLI owns, so a reworded sentence does not silently disarm it) against captured pipe output, with a comment noting why a plain contains is safe there and not on the PTY surfaces.
On the weak step — you are right that serving is refused before any engine starts is assert!(rc != 0) and nothing else, and that its name promises a guard it cannot fail on. That is why serve-25 carries the engine-preparation check as a separate step rather than relying on the existing one. I have deliberately not folded the check into the shared step in this PR: it is used by nine scenarios in model_serving.feature plus one in networking.feature, several of which refuse for non-GPU reasons (serve-14 negative temperature, serve-17 runtime-and-env), and newly constraining all of them is a change whose blast radius I would want a full suite run to justify rather than reasoning. Happy to make it here if you would rather it not wait — say the word and I will run the suite against it — otherwise I will file it as the follow-up, since the misleading step name is worth closing either way.
| // no notes at all — that path is machine-readable by design and is not | ||
| // the place to grow prose, but it does mean the same failed launch is | ||
| // explained in one invocation and not the other. | ||
| let notes = append_oom_serve_note( |
There was a problem hiding this comment.
Not a change request — flagging the tradeoff so it's visibly deliberate, since it's the one I raised last round and it's been documented rather than closed.
This sits inside if summary_mode, and summary_mode is background && stdout().is_terminal(). A scripted, CI, or assistant-driven run takes print_managed_launch_plain and gets no notes at all, so the same failed launch is explained in one invocation and silent in the other — including rocm-cli's own chat assistant path.
The comment above now states this plainly and argues the machine-readable path isn't the place to grow prose, and the PR body says "interactive deployment summary". That's disclosed rather than hidden, which is what I asked for. I'm not blocking on it. But it does mean the feature doesn't reach the caller most likely to hit an OOM unattended, so if that was meant to be addressed later, a follow-up issue would be worth filing now rather than relying on this comment to carry it.
There was a problem hiding this comment.
Agreed, and agreed it should not rest on a review comment to carry it. The asymmetry is real: summary_mode is background && stdout().is_terminal(), so a scripted, CI, or assistant-driven run takes print_managed_launch_plain and the same failed launch is explained in one invocation and silent in the other — including rocm-cli's own chat assistant path, which is plausibly the caller most likely to hit an unattended OOM.
Not changed here, for the reason the comment gives — the machine-readable path is not the place to grow prose, and widening it is a surface decision rather than a bug fix — but I will file the follow-up so it is tracked outside this thread rather than depending on someone rereading it. Thanks for pushing it from "documented" to "documented and owned"; those are not the same thing.
| || resolved_selection.env_id.is_some() | ||
| || engine_manages_own_runtime(&selected_engine)); | ||
| if can_resolve_model | ||
| && any_live_managed_service_for_model(&paths, &selected_engine, &engine_model_ref) |
There was a problem hiding this comment.
Non-blocking, and a genuine improvement over engine-only keying — just worth recording the bound.
service_model_names_match (main.rs:22415, outside this diff) is a bidirectional contains, so this pre-gate admits any overlapping name: a live service for qwen3-8b-instruct satisfies a query for qwen. On that path the ResolveModel round-trip and ensure_self_managed_engine_ready still run ahead of the GPU bail.
The body's claim that this uses "the same lenient relation the service surfaces already use" is true — I checked, it's one shared function with four call sites. I'd leave the matcher alone rather than tighten it, since tightening risks false negatives that break legitimate reuse. Just worth stating as a known bound rather than leaving it implied.
There was a problem hiding this comment.
Agreed, including the recommendation not to tighten it. service_model_names_match is a bidirectional contains shared by four call sites, so this pre-gate does admit any overlapping name — a live service for qwen3-8b-instruct satisfies a query for qwen, and on that path the ResolveModel round-trip and ensure_self_managed_engine_ready do run ahead of the GPU bail.
That is a genuine bound and worth stating rather than implying, and tightening the matcher would risk false negatives that break legitimate reuse — a worse failure than the one it would close. Recording it as a known bound is the right resolution; I would rather leave the shared relation alone than make reuse subtly stop working for the abbreviated model names people actually type.
| .expect("no interactive serve summary") | ||
| .screen_text(); | ||
| assert!( | ||
| screen.contains("already running"), |
There was a problem hiding this comment.
Non-blocking: whitespace-bearing needle on a rendered PTY.
"already running" contains a space, and this is matched against the raw 80-column screen, so a soft wrap between the two words breaks it. This PR makes the opposite argument two functions below — assert_no_oom_memory_guidance and assert_oom_launch_memory_guidance both route through screen_without_whitespace for exactly this reason, with the comment at :1337-1341 spelling it out.
The PR's own reasoning is the side that governs here. The failure mode is loud rather than vacuous (a wrap makes this fail, not silently pass), so it's robustness rather than a hole — but it should match its siblings: screen_without_whitespace(&screen).contains("alreadyrunning").
There was a problem hiding this comment.
You are right, and the PR's own argument two functions below is the side that governs. Fixed in d0fee30c — it now reads
screen_without_whitespace(&screen).contains("alreadyrunning"),matching its siblings assert_no_oom_memory_guidance and assert_oom_launch_memory_guidance, which route through the same helper for exactly the reason the comment at :1337-1341 spells out. Agreed the failure mode was loud rather than vacuous — a wrap made it fail, not silently pass — so this is robustness rather than a hole, but there is no reason for one needle on an 80-column PTY grid to be the odd one out.
|
|
||
| # Regression test (EAI-8059 review): re-issuing `serve` against an | ||
| # already-running managed service must never blame *this* invocation for | ||
| # whatever that other process's log contains, even when it carries a real |
There was a problem hiding this comment.
Non-blocking: this comment claims the planted OOM content is load-bearing, and it isn't.
"even when it carries a real OOM signature and the reused record is not yet 'ready'" reads as though the scenario would still pass if the note logic were wrong. It can't reach that code: the only site producing already_running: true (main.rs:6974) also sets log_path: None, so append_oom_serve_note returns at its let Some(log_path) = log_path else guard (main.rs:6602) regardless of what the planted log contains. The code governs, so the comment is the side to fix.
To be explicit, since this is the kind of finding that gets "resolved" the wrong way: keep the already_running || clause in the guard. It defends a cross-function invariant nothing else enforces, and the unit test at main.rs:31125 does discriminate it by passing already_running: true together with Some(log_path). Only the scenario comment overstates.
There was a problem hiding this comment.
Confirmed, and thank you for naming which side to fix — this is exactly the finding that gets "resolved" the wrong way.
The already_running || clause stays. It is still at main.rs:6645; I did not touch the guard. Your reasoning is the reason: it defends a cross-function invariant nothing else enforces, and the unit test does discriminate it by passing already_running: true together with Some(log_path) — a combination no production call site currently produces, which is the point.
Only the scenario comment was wrong, and d0fee30c rewrites it. The old wording, "even when it carries a real OOM signature and the reused record is not yet 'ready'", read as though the scenario would catch a broken note path; it cannot reach that code, because the only site producing already_running: true (main.rs:6974) also sets log_path: None, so append_oom_serve_note returns at its let Some(log_path) = log_path else guard regardless of what the planted log contains. The comment now says what serve-23 actually covers and names the unit test that discriminates the already_running clause, so the next reader is pointed at the real coverage instead of a claim the scenario cannot support.
| // Both counters, not just the verbatim one: a table that drifted until | ||
| // every row took the same branch would still satisfy every per-row | ||
| // assertion above, which is the vacuity this test was written to close. | ||
| assert_eq!( |
There was a problem hiding this comment.
Non-blocking: the failure message claims a sensitivity this table doesn't have.
"if a scoring or admissibility change moved a line across a boundary" — but no fixture row here sits near the scorer's boundary. The three fallback rows are rejected purely on admissibility (quotable_in_single_quotes: apostrophe, ANSI, bidi override), and the four verbatim rows carry both a class name and an anchor phrase, so they score far above threshold. A scorer-threshold regression would leave this (4, 3) split untouched.
The ruling isn't symmetric, so to be clear about which side to change: the descriptive comment at :1096-1101 is accurate and already defers the scoring boundary to sub_threshold_lines_carry_no_hint_at_all by name. It's only this failure message that overclaims. Narrowing it to admissibility and dropping "scoring" is the fix.
There was a problem hiding this comment.
Confirmed and fixed in 37247be0. You are right that the split is admissibility-only: the three fallback rows are rejected by quotable_in_single_quotes (apostrophe, ANSI, bidi override) and the four verbatim rows carry both a class name and an anchor phrase, so a scorer-threshold regression would leave (4, 3) untouched. The message claimed a sensitivity the table does not have.
Also taken: the descriptive comment at :1096-1101 is accurate and already defers the scoring boundary to sub_threshold_lines_carry_no_hint_at_all by name, so I left it alone and narrowed only the assertion message. It now reads:
the table must keep exercising both branches; if an admissibility change moved a line across the quotability boundary, re-pick the fixture rather than relaxing the expectation. Scoring is not what separates these rows — every row here is comfortably accepted by the detector, and the three fallbacks are fallbacks purely because they cannot be rendered as one single-quoted word.
"Scoring" is gone, and the message now tells a future maintainer where the real boundary is instead of sending them to the scorer.
| @@ -735,7 +739,7 @@ fn startup_log_context(log_path: Option<&Path>) -> String { | |||
| let hint = oom_utilization_hint(&summary); | |||
There was a problem hiding this comment.
Non-blocking and pre-existing — anchoring here because the lines themselves aren't in the diff. The code I mean is startup_log_context at :733-735.
let summary = log_path
.and_then(|p| summarize_startup_log_tail(p, STARTUP_FAILURE_LOG_TAIL_LINES).ok())
.unwrap_or_default();
if summary.is_empty() { return String::new(); }tail_lines collects BufReader::lines() into Result<Vec<_>, _>, which fails the entire read on the first non-UTF-8 byte in the tail window — not per-line skippable. .ok() then turns that into None and unwrap_or_default() into "", so both the "Last N lines of startup log" section and the OOM hint disappear with no indication a read even failed.
That matters here specifically because this is a killed process's log, where a truncated multi-byte sequence at a buffer boundary or binary noise in a crash dump is the normal case — the exact input class this feature exists to diagnose. The sibling surface does better: read_optional_tail_lines (main.rs:18405) renders <failed to read {label}: {error}> instead of vanishing.
I confirmed this is not a regression — no hunk in this PR touches tail_lines, summarize_startup_log_tail, or this error handling, and the new CLI-side oom_memory_note uses the safer reader, so it didn't inherit the bug. Not a condition of merging. But it sits inside the feature this PR is named for, so a follow-up making tail_lines lossy and surfacing a read failure rather than swallowing it would be worth filing.
There was a problem hiding this comment.
Verified, and thank you for checking that it is not a regression before raising it — that is the part I would have had to redo.
The mechanism is as you describe: tail_lines collects BufReader::lines() into Result<Vec<_>, _>, so the first non-UTF-8 byte anywhere in the tail window fails the whole read rather than skipping a line; .ok() turns that into None and unwrap_or_default() into "", and both the "Last N lines of startup log" section and the OOM hint vanish with no sign a read was attempted. And the input class is precisely the bad one — a killed process's log, where a multi-byte sequence truncated at a buffer boundary, or binary noise in a crash dump, is the normal case rather than the exotic one.
The sibling comparison is the convincing part: read_optional_tail_lines (main.rs:18405) renders <failed to read {label}: {error}> instead of disappearing, so the repo already has the better behaviour on the other surface and this one just never got it.
Not changing it here — no hunk in this PR touches tail_lines, summarize_startup_log_tail, or that error handling, and the new CLI-side oom_memory_note uses the safer reader so it did not inherit the bug — but it sits inside the feature this PR is named for, so I will file the follow-up to make tail_lines lossy and surface a read failure rather than swallow it.
| /// literal so this surface and the CLI's serve summary — which reads the same | ||
| /// protocol constant in `append_oom_serve_note` — cannot drift on what counts as | ||
| /// "the tail" of a failed launch. A change to the protocol budget moves both. | ||
| const STARTUP_FAILURE_LOG_TAIL_LINES: usize = DEFAULT_LOG_TAIL_LINES; |
There was a problem hiding this comment.
Non-blocking, follow-up rather than a change here — flagging because this line is what will mislead the next person.
This derivation and its comment ("A change to the protocol budget moves both") are correct for the two surfaces they describe; I verified both process.rs and append_oom_serve_note read rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES.
The hazard is next door: engines/lemonade/src/lib.rs:68 declares a local const DEFAULT_LOG_TAIL_LINES: usize = 200 — the same identifier as the protocol constant, with a different value — and :74 keeps STARTUP_FAILURE_LOG_TAIL_LINES: usize = 80 as a bare literal that matches the protocol value by coincidence, not derivation. So a maintainer who reads the comment here and then bumps the protocol budget will reasonably expect lemonade to follow, and it won't, silently.
Out of scope for this PR — you didn't change the protocol value, only added a derivation of it. Worth a follow-up to rename lemonade's local constant to something engine-scoped and either derive its startup budget or document why it's deliberately independent.
There was a problem hiding this comment.
Good catch, and the hazard is the shadowing rather than either value on its own. Confirmed: engines/lemonade/src/lib.rs:68 declares a local const DEFAULT_LOG_TAIL_LINES: usize = 200 — same identifier as rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES, different value — and :74 keeps STARTUP_FAILURE_LOG_TAIL_LINES: usize = 80 as a bare literal that matches the protocol value by coincidence rather than derivation.
So the trap is real and is aimed at a reader of my comment: someone who reads "A change to the protocol budget moves both", bumps the protocol constant, and reasonably expects lemonade to follow. It will not, and nothing will say so.
Out of scope here, as you say — this PR added a derivation of the protocol value, it did not change the value or touch lemonade — so I am filing the follow-up rather than widening the diff: rename lemonade's local constant to something engine-scoped so it cannot be mistaken for the protocol one, and either derive its startup budget from the protocol constant or state in a comment why it is deliberately independent. Either outcome is fine; what is not fine is the two being indistinguishable by name.
There was a problem hiding this comment.
Updating the coordinates: #457 ("Modularize engines/lemonade/src/lib.rs", merged to main as 58d66f0e a few hours after this thread) split that file into six modules, so the two line references above are stale. The hazard survived the split, and in one respect got harder to see:
engines/lemonade/src/lib.rs:43— stillconst DEFAULT_LOG_TAIL_LINES: usize = 200, still shadowingrocm_engine_protocol::DEFAULT_LOG_TAIL_LINES, which is80(crates/rocm-engine-protocol/src/lib.rs:15).engines/lemonade/src/process.rs:29—STARTUP_FAILURE_LOG_TAIL_LINES: usize = 80, still a bare literal matching the protocol value by coincidence rather than derivation.
They are now in different files, so a reader of process.rs sees 80 with no neighbouring constant to compare it against, and the accidental agreement is one file further from anyone who might notice it. For contrast, vllm came out of its own split reading the protocol constant properly (engines/vllm/src/state.rs:96 takes DEFAULT_LOG_TAIL_LINES from the import at :10), so lemonade is now the only engine with a local redefinition.
Nothing to do in this PR — still out of scope, and #457 is a no-behaviour-change refactor that neither introduced nor was obliged to fix this. Recording it so the follow-up I owe you points at live coordinates rather than pre-split ones.
| /// lanes: the scenarios needing it carry `@requires-oom-fault-injection` and | ||
| /// the harness skips them when `xtask` did not build the binary itself, so a | ||
| /// missing hook is a reported skip rather than a silent green. | ||
| fn assert_prebuilt_e2e_lanes_enable_test_hooks(workflow: &str, text: &str) { |
There was a problem hiding this comment.
Non-blocking. Anchoring on this function because the assertion I mean is :1958, outside the diff.
Nothing pins the release build's feature set. workflow_contract.rs has no references to release.yml at all, and the nearest guard asserts only !lifecycle.contains("e2e-test-hooks") — which does not match the substring e2e-oom-fault-injection, so a release build enabling only the new feature would pass that check.
Shipped binaries are clean today: I checked by hand that no workflow build of -p rocm passes e2e-oom-fault-injection, only xtask/src/e2e.rs. And this is the same posture the pre-existing e2e-test-hooks already had, so it's consistent rather than a new risk introduced here.
Since this PR adds a second test-only feature that must never ship, a generic assertion that release.yml's build step passes no --features at all would be cheap and would cover future ones too.
There was a problem hiding this comment.
Confirmed, and confirmed it is not a live exposure: workflow_contract.rs has no reference to release.yml at all, and the nearest guard asserts only !lifecycle.contains("e2e-test-hooks"), which does not match the substring e2e-oom-fault-injection — so a release build enabling only the new feature would pass it. I also re-checked your by-hand result independently: no workflow build of -p rocm passes e2e-oom-fault-injection; the only site that does is xtask/src/e2e.rs. Shipped binaries are clean today.
Your framing is the right one — this is the same posture e2e-test-hooks already had, so it is a pre-existing gap rather than a risk this PR introduces. But the observation that this PR adds a second test-only feature that must never ship is the argument for closing it generically instead of adding a second substring to chase: an assertion that release.yml's build step passes no --features at all covers both and every future one, and does not need updating when the next one appears.
I have not added it in this diff, only because an xtask contract assertion that newly constrains a release workflow is the kind of change I would rather land on its own, where a failure is unambiguous, than bury in a serve-path PR. Happy to add it here instead if you would prefer it not wait — otherwise it goes on the follow-up list with the others from this round.
There was a problem hiding this comment.
One detail here went stale overnight, and in a direction that could mislead whoever picks this up: #415 ("ci(e2e): auto-trigger self-hosted E2E matrix on release-branch push", merged to main as 6f7219a1) added 65 lines to xtask/src/workflow_contract.rs, and release.yml is now mentioned in that file.
The substance of your finding is unchanged, though — the mentions are prose, not coverage:
:792is a doc comment onself_hosted_workflow_fires_on_release_branch_push, and:804is that test's assertion message. Both refer torelease.ymlonly to explain when the self-hosted matrix should fire relative to cutting av*tag. The test itself readse2e-selfhosted.yml.- There is still no
read_workflow("release.yml")anywhere in the file — grepping for it as a workflow under test returns nothing, so nothing parses release.yml, let alone pins its feature set. - The nearest
--featuresguard is still the one you identified, and it is narrower than it looks:!lifecycle.contains("e2e-test-hooks")at:2012is scoped to the Windows lifecycle lane, not to release.yml, and still does not match the substringe2e-oom-fault-injection.
So a release build enabling only the new feature would still pass everything here. I am flagging the change only because "workflow_contract.rs has no references to release.yml at all" is now literally false, and someone grepping this thread later could reasonably conclude the gap had been closed when it has not.
Unchanged on my side: still not adding the assertion in this diff, for the reason above — an xtask contract assertion that newly constrains the release workflow should land where a failure is unambiguous rather than inside a serve-path PR. 284 still merges cleanly into 6f7219a1 (verified with git merge-tree, exit 0).
`4a108b95` set out to make the GPU fail-fast contract true, but the contract was still false on the ordinary path: this PR had also moved `resolve_engine_selection` / `validate_engine_selection_runtime` from below the bail to above it, and that chain is not side-effect-free. With neither `--runtime-id` nor `--env-id` given — the default — the second call takes `single_ready_runtime_key`, which calls `recover_setup_runtime_registration`, which can `create_dir_all` + `write` a runtime registry manifest and can fail with an unrelated runtime-manifest error. So a plain `rocm serve <model>` on a GPU-less host could mutate disk, or report a runtime-manifest problem, before reaching "no usable AMD GPU detected". Rather than soften the comment, move the code back. The reuse pre-gate is the only thing that legitimately precedes the bail, so make its record-only check (`any_live_managed_service_for_model`) the outermost condition and resolve the runtime selection *inside* it; every other path — including every no-GPU refusal — now resolves below the bail and reuses nothing. Reproduced with a `.rocm-cli-runtime.json` the setup root cannot parse: before, a masked-GPU `rocm serve` died with "failed to parse .../.rocm-cli-runtime.json"; after, it refuses with "no usable AMD GPU detected". Also records, on `append_oom_serve_note`, why its `already_running` clause stays even though the only producer of `already_running: true` reports `log_path: None`: the two guards assert different things, and the unit test discriminates the clause where the E2E scenario cannot. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
`4a108b95` changed what a user sees — on a GPU-less host with a live managed service for an *unrelated* model, `rocm serve` must refuse with "no usable AMD GPU" and must not print "Preparing <engine> for GPU serving..." or install an engine — and shipped only unit tests on `any_live_managed_service_for_model`. AGENTS.md is explicit that those prove the function, not the behaviour. serve-13 plants no service so it passes either way, and serve-23 plants the same model. serve-25 plants a live *Lemonade* service for a different model, then serves a Lemonade model: same engine, so the model is the only thing holding the pre-gate shut, and Lemonade because it is the self-managing engine whose preparation is user-visible. Falsified by reverting the model keying — the scenario goes red on "Preparing lemonade for GPU serving..." followed by an embeddable download. `ServiceRecordOptions` grows an `engine` field (default `"vllm"`) so the planted record can name Lemonade without duplicating the on-disk schema. Two review fixes alongside: - `the summary reflects the reused already-running service` matched "already running" against a raw 80-column PTY screen, where a soft wrap between the words breaks it. Route it through `screen_without_whitespace` like its two siblings. - serve-23's comment claimed the planted OOM content was load-bearing. It is not: the only site producing `already_running: true` also sets `log_path: None`, so `append_oom_serve_note` returns at that guard regardless. Say what the scenario actually covers and name the unit test that does discriminate the `already_running` clause. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
…lity The message blamed "a scoring or admissibility change" for a row crossing a branch boundary, but no fixture row sits near the scorer's threshold — all seven are comfortably accepted by the detector, and the three fallback rows fall back purely because they cannot be rendered as one single-quoted word (apostrophe, control bytes, bidi override). Pointing a future maintainer at scoring would send them to the wrong knob. The descriptive comment above the table already says this correctly and is unchanged. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> # Conflicts: # tests/e2e-cucumber/tests/e2e/serving_steps.rs
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
fbf5ff6 to
fe41981
Compare
|
🔴 Automated review · pr-review-watcher · fe41981 This automation never files a GitHub approval, so no approving review will SummaryThe PR adds vLLM out-of-memory (OOM) detection to the Reviewed: the whole change ( Verified:
Blocking: 0 · Non-blocking: 9. 🚫 Blocking (must fix before merge)None. Non-blocking
|
4 participants
Summary
Adds GPU out-of-memory (OOM) guidance to
rocm servefor vLLM: when a managedlaunch fails to become ready with a real allocator OOM signature in its own log,
the interactive deployment summary names the memory knobs (
--gpu-memory-utilization,--gpu <index>), says plainly that lowering the reservation does not help amodel that simply does not fit, and routes the user's actual failing line into
rocm diagnose --symptom '<line>'.Detection is deliberately narrow: the serve summary classifies each tail line with
the same rule as the
rocm diagnosevLLM-OOM checker (each line scored asvllm: <line>against the shared match threshold), so a bareout of memoryfroma kernel OOM-killer line, a dependency's log, or vLLM's generic
EngineCorewrapper never gets reported as memory exhaustion. One shared helper
(
rocm_core::vllm_log_shows_oom) feeds both the engine hint and the CLI note, sothe two surfaces cannot drift.
Behavior coverage
Scenarios this PR adds:
@id:)serve-oom-launch-memory-guidance(serve-24)@requires-oom-fault-injectionserve-oom-memory-guidance(serve-23)@requires-no-gpu@id:diagnose-vllm-oom-is-conditionaland@id:serve-vllm-low-vram-oom-guidance(
serve-19) come from the base branch (#251), not from this PR; this PR doesnot modify them.
The positive scenario cannot run a real launch on a GPU-less host, so the mock-lane
binary compiles in a test-only fault-injection hook (feature
e2e-oom-fault-injection,armed per child process by
ROCM_E2E_SIMULATE_OOM_LAUNCH) that fabricates exactlythe failed-launch state. The hook is absent from shipped binaries (
release.ymlnever passes the feature), so the scenario reports a skip on the self-hosted
(prebuilt-release) lanes rather than a silent green, and runs where
xtaskbuildsthe binary itself.
Verification, and what could not be run
Green at the pushed head on a Linux MI300X box:
cargo test --workspace --all-targetscargo clippy --workspace --all-targets -- -D warningscargo fmt --all --checkcargo xtask e2e— including this PR's own scenarios:serve-23(
serve-oom-memory-guidance) andserve-24(serve-oom-launch-memory-guidance)both pass, as does
serve-16(serve-absent-gpu-index-rejected), the laneevidence for the
--gpu-ordering fix below.CI. Every lane is green at this head; there are no failing checks.
83ecfaadchat-06"auto"tool choice needs--enable-auto-tool-choiceserve-01,serve-02resolved modellineserve-07,serve-08serve-13@requires-no-gpuscenario, forced onto a GPU host by the name filterserve-19serve-10,bench-04runtime-02Folder:line inexamineoutputE2E tests (Strix Halo, WSL2)is a 24h runner timeout — infrastructure, not anassertion failure.
Changes since the last review
--gpuvalidation ordering (blocking).resolve_gpu_indiceshad movedafter the no-runtime bail, so
--gpu 99surfaced as "no active ROCm runtimeis configured" instead of an out-of-range argument error, and
serve-absent-gpu-index-rejectedfailed on the GPU lane. Index validation nowruns immediately after the no-usable-GPU pre-flight and before any runtime or
engine work; a GPU-less host still refuses with "no usable AMD GPU" first.
serve-16passes on the GPU lane at the reviewed head.OOM detector threshold reconciled with
rocm diagnose. The serve-summarydetector used a looser bare-substring scan than feat(vllm): Tackle out of memory errors (EAI-8058) #251's
check_16_vllm_oom,which deliberately scores a bare
out of memorybelowMIN_SCORE_FOR_MATCH.Both now classify with the same rule through a single shared helper. The note
also carries the shared-vs-dedicated caveat and points at
rocm diagnoseforthe case-appropriate fix, and the
--gpu-memory-utilizationwording iscorrected to "a fraction greater than 0 and at most 1" (the parser enforces
(0, 1], so<0-1>wrongly implied0was accepted).GPU fail-fast contract made true. The comment above the no-usable-GPU bail
promised no engine work could precede it, while the reuse detection above it
ran a
ResolveModelround-trip and — for a self-managing engine — anensure_self_managed_engine_readythat can print "Preparing lemonade for GPUserving..." and install. The pre-gate keyed on the engine alone, so a live
service for an unrelated model, which this invocation can never reuse, pulled
that work in front of the bail. It now also keys on the model
(
any_live_managed_service_for_model), using the same lenient name relation theservice-listing surfaces already use so a short-vs-canonical spelling still
reaches the probe (which then decides reuse authoritatively on the canonical
id). The comment now states the real contract instead of an absolute one.
Regression tests:
reuse_pregate_skips_engine_work_for_an_unrelated_live_model,reuse_pregate_admits_the_same_model_including_a_short_spelling,reuse_pregate_still_keys_on_the_engine.Shared log-tail budget actually shared. The serve summary's comment claimed
it read the same budget as the engine's OOM surface, but
engines/vllm'sSTARTUP_FAILURE_LOG_TAIL_LINESwas an independent80literal. It is now derived from
rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES,so a change to the protocol budget moves both surfaces and the claim holds.
xtask feature collision.
cargo xtask e2ebuilds the mock-lane binary witha single
--features "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection"valueinstead of overwriting
e2e-test-hooks(fix(lemonade): retry interrupted backend setup #249).workflow_contract.rs'sprebuilt-lane assertion is a
containson--features rocm/e2e-test-hooksandstill holds; its doc and failure message now describe that string as the shared
hook floor (xtask builds a superset) rather than an exact match, and record
why the fault-injection hook is deliberately absent from prebuilt lanes.
Duplicated env constant.
ROCM_E2E_OOM_FAULT_INJECTIONwas declared twice,in
xtask/src/e2e.rsandtests/e2e-cucumber/src/capability.rs, held togetheronly by a "kept in sync" comment — a typo in either would have silently turned
the scenario into a skip. Hoisted into the shared
e2e-reportcrate bothalready depend on; the two former copies re-export the single source.
Negative assertion no longer vacuous. "does not blame this invocation for
GPU memory" collapses whitespace exactly like the positive checks, so it catches
a note that is wrongly rendered and soft-wrapped across the 80-column PTY grid
instead of passing against a literal substring that could never match.
Scenario numbering. The two OOM scenarios are
serve-23/serve-24atthis head. They were
serve-20/serve-21when written; the base has sinceinserted scenarios ahead of them, so the numbers drifted. The stable
@id:tags are unchanged and are what to match on — expect the numbers to drift
again before this lands.
Coordination
#290's diagnose catalog has merged, so
rocm diagnose's vLLM-OOM checker and thisPR's serve-summary detector now share one classification rule rather than being
two independent detectors.