feat(diagnose): recognise the vLLM engine-startup import failure (EAI-8076) by r0x0r · Pull Request #374 · ROCm/rocm-cli

r0x0r · 2026-09-09T11:41:10Z

Summary

rocm serve --engine vllm can abort at engine startup with OSError: libtorch_cuda.so: cannot open shared object file (EAI-8012), and the closed catalog had no entry for it. Its highest-scoring answer to that symptom was fix-4-render-group at 45/100 — a false lead on a host whose /dev/kfd rocm examine itself reports as world-writable. A user following it runs usermod, logs out and back in, and is no closer.

This adds fix-17-torch-dlpack: a keyword table for the import signature (libtorch_cuda.so, torch_c_dlpack_ext, _optional_torch_c_dlpack) plus a PRINT-ONLY recipe carrying the remediation.

Before / after, same command, same host:

# before
#1 (fix-4-render-group)  [WEAK   score=45/100] User not in render/video group ...

# after
#1 (fix-17-torch-dlpack) [LIKELY score=50/100] vLLM engine start aborts on torch-c-dlpack-ext loading its CUDA variant
#2 (fix-4-render-group)  [WEAK   score=45/100] User not in render/video group ...

Why keyword-only

The fact that decides this failure is the torch version inside the managed runtime. Examination's framework probe imports torch from the ambient interpreter instead — on an affected host it reports framework: unknown with a torch import failed note — so the deciding fact is absent from the examination and no structural check can fire. Noted in the checker's doc comment so the next reader does not try to add a structural signal that cannot exist yet.

Discoverability, stated accurately

An earlier revision of this description claimed the entry was reachable "from a serve failure note". There is no such note — a service that died at startup renders logs: and restart: hints and never mentions diagnose. Corrected here and in the checker's doc comment, which now records the gap rather than papering over it.

What is true today: the only route from the symptom is the user pasting the error into rocm diagnose --symptom "<pasted error>". The recipe is additionally reachable by name — rocm fix lists it, rocm fix fix-17-torch-dlpack prints it — but that needs the id, not the symptom.

Closing the gap means adding a diagnose hint to the service-failure output. That changes a shared surface for every failed service regardless of engine or cause, needs its own scenario, and is a decision about rocm services rather than about this catalog entry — so it is deliberately not in this PR. Happy to open it as a follow-up if that is the preferred order.

libtorch_cuda.so alone is weighted at exactly MIN_SCORE_FOR_MATCH (50), deliberately: a ROCm build of torch ships libtorch_hip.so and never that file, so the token cannot mean anything else, and a user who pastes only the OSError line still has to clear the bar. torch_c_dlpack_ext alone stays at 45 — under the bar — because the extension appearing in a traceback says it is involved, not that it chose the wrong variant.

Wording stays conditional

The trigger conditions are narrow — a ROCm build of torch in the 2.4–2.9 range with torch-c-dlpack-ext present — so the entry states the condition before it advises anything, and the first note says outright that running vLLM on ROCm is not by itself a reason to apply it. The commands are confirm-then-act and use only commands this CLI has (rocm engines shell vllm, rocm engines install vllm --reinstall).

The command block says which shell each step runs in

This is the only recipe in the catalog whose steps span more than one shell, and both renderers print every line with the same $ prefix — so ordering alone said nothing about it. rocm engines shell vllm opens an interactive subshell, the two probes belong inside it, and the reinstall replaces the very environment that subshell is standing in, so it must not run there. A user pasting the block wholesale was left depending on terminal stdin buffering to land each line in the right place.

Now split into three labelled groups — your shell / inside the subshell / back in your own shell — with leaving the subshell an explicit exit rather than something to infer. Three rather than two because the reinstall was already implicitly outside the subshell and nothing said so. rocm engines shell has no non-interactive -- <cmd> form (it bail!s without a TTY), so collapsing this to one context would have meant inventing a command the CLI does not have.

Both copies of the plan carry it — the catalog recipe and the Fix the diagnosis attaches to its finding. Two shared assertions hold them: one checks the shell boundary is labelled in each, and one pins the two command blocks to each other byte for byte, so the block a user sees cannot drift from the block rocm fix prints. An earlier revision of this description claimed the single boundary assertion already guaranteed that; it did not — it checked three command strings and two label substrings, and the step-2 wording had in fact drifted between the copies. Both are now identical and the equality check keeps them so.

The surrounding prose is deliberately not identical, because FixRecipe has a rationale field that Fix has no counterpart for: the upstream-defect explanation diagnose carries as a note is printed by rocm fix out of rationale, so copying it into notes would print it twice. verify is byte-identical in both.

This does not fix the upstream defects (torch-c-dlpack-ext 0.1.5 still selects the CUDA variant on ROCm; tvm_ffi's optional-import guard still misses the OSError). It recognises the failure and names the remedy that moves the runtime off the versions with prebuilts.

Fix-id numbering

fix-16 is reserved by the vLLM out-of-memory entry on #251, so this takes 17. The id is a stable handle external tooling reproduces, so the two are kept distinct rather than one being renamed after it ships. The gap is commented in all three places a reader would meet it.

The AUTO set is unchanged — the new entry is PRINT-ONLY, so exactly_the_four_known_fixes_are_auto passes untouched. That assertion being untouched is the evidence that nothing was promoted to auto-applicable.

Test plan

Lanes not run locally: Windows (the checker is linux-only, so the entry is inert there) and WSL2 (no lane exists; the catalog is not run on that platform at all).

…-8076) `rocm serve --engine vllm` can abort at engine startup with `OSError: libtorch_cuda.so: cannot open shared object file`, and the catalog had no entry for it. Its best answer to that symptom was fix-4-render-group at 45/100 -- a false lead on a host whose /dev/kfd examine itself reports as world-writable, so the user ran usermod, logged out and back in, and was no closer. Add fix-17-torch-dlpack: a keyword table for the import signature and a PRINT-ONLY recipe. Keyword-only by necessity -- the fact that decides the failure is the torch version inside the managed runtime, and the framework probe reads the ambient interpreter instead, so nothing structural can fire until that probe targets the active runtime. The wording stays conditional throughout. The trigger is narrow (a ROCm build of torch in the 2.4-2.9 range with torch-c-dlpack-ext present), so the entry says so before it advises anything: running vLLM on ROCm is not by itself a reason to apply it. 17 rather than 16 because the vLLM out-of-memory entry reserves 16 on its own branch; the id is a stable handle, so the two are kept distinct rather than renamed after one of them ships. The AUTO set is deliberately unchanged -- the new entry is PRINT-ONLY, so `exactly_the_four_known_fixes_are_auto` still passes untouched, which is the evidence that nothing was promoted. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

jussielo-amd · 2026-09-09T12:27:38Z

Verified this end-to-end (built the branch, ran the new + existing tests, clippy/fmt clean, exercised rocm diagnose directly): the fix is technically sound — genuinely print-only, correctly Linux-gated, correctly outranks the reported fix-4-render-group false lead, and the remedy lines up with the existing pinned-torch-drift handling already in engines/vllm/src/lib.rs.

Two things worth addressing before merge:

Discoverability is overclaimed. The description says the entry is reached "through --symptom, or from a serve failure note." There is no such note anywhere in the codebase — rocm services list on a failed service only ever prints a restart: hint, never mentions diagnose. As written, a user only reaches this fix if they already know to run rocm diagnose --symptom "<pasted error>" manually. Either wire up a hint on service failure, or fix the PR description so it doesn't imply a discovery path that doesn't exist.
The command block mixes execution contexts with no visual separation. This is the only recipe in the catalog whose commands span two shells: rocm engines shell vllm opens an interactive subshell, and the next two lines are meant to run inside it — but all lines are rendered identically with a $ prefix, same as every other (fully sequential) recipe. The intent is explained in an inline comment, but a user who copy-pastes the whole block is relying on terminal stdin buffering to land the later commands in the right shell. Consider splitting this into two clearly labeled command groups.

…serve-note claim Review feedback on the fix-17-torch-dlpack entry. The command block is the only recipe in the catalog whose steps span more than one shell: `rocm engines shell vllm` opens an INTERACTIVE subshell, the two probes belong inside it, and the reinstall replaces the very environment that subshell is standing in, so it must not run there. Both renderers print every line with the same `$ ` prefix, so ordering said none of that -- a user pasting the block wholesale was depending on terminal stdin buffering to land each line in the right shell. Split it into three labelled groups (your shell / inside the subshell / back in your own shell) and make leaving the subshell an explicit `exit` rather than something the reader has to infer. Three rather than two because the reinstall was already implicitly outside the subshell and nothing said so. Both copies of the plan carry it -- the catalog recipe and the `Fix` the diagnosis attaches to its finding -- and one shared assertion holds them both, so the copy a user sees cannot drift from the copy `rocm fix` prints. Scenario diagnose-14 covers the printed block end to end; it is not OS-gated because the plan is printed before the fix's own platform gate, so both lanes exercise the same text. Separately, the checker's doc comment claimed the entry was reachable "from a serve failure note". No such note exists: a service that died at startup renders `logs:` and `restart:` hints and never mentions `diagnose`. Correct the claim rather than invent the path -- adding a diagnose hint to the service-failure output changes a shared surface for every failed service regardless of cause, and belongs in its own change. The comment now says what is true, names `rocm fix` as the by-id route, and records the gap so the next reader does not have to rediscover it. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

r0x0r · 2026-09-09T13:12:20Z

Both points addressed in 56923ee.

1. Discoverability was overclaimed — corrected rather than wired up. You were right that no serve failure note exists: a service that died at startup renders logs: and restart: hints and never mentions diagnose. The claim is gone from the PR description and from the doc comment on check_17_torch_dlpack_cuda_variant, which now states what is actually true (the only route from the symptom is rocm diagnose --symptom "<pasted error>"; the recipe is separately reachable by id through rocm fix) and records the gap so the next reader does not have to rediscover it.

I chose the description over wiring the hint because the hint is a change to rocm services, not to this catalog entry: it would alter output for every failed service regardless of engine or cause, needs its own scenario and its own decision about which surfaces carry it (text render, tool-result text, TUI), and AGENTS.md §11 asks for one logical change per PR. Say the word and I will open it as a follow-up — the site is the else branch in render_services_text where the restart: hint is emitted.

2. The command block now says which shell each step runs in. Split into three labelled groups — step 1 of 3, in YOUR shell / step 2 of 3, INSIDE the subshell step 1 opened / step 3 of 3, back in YOUR OWN shell — with leaving the subshell an explicit exit instead of something the reader has to infer.

Three rather than two because the reinstall was already implicitly outside the subshell and nothing said so; it replaces the very environment that subshell is standing in, so running it there is actively wrong. rocm engines shell has no non-interactive -- <cmd> form (it bail!s without a TTY), so collapsing this to a single context would have meant inventing a command the CLI does not have.

Both copies of the plan carry it — the FixRecipe in fix.rs and the Fix the diagnosis attaches to its finding — and one shared assertion, assert_engine_shell_boundary_is_labelled, holds them both, so the copy a user sees cannot drift from the copy rocm fix prints. It checks the boundary is ordered (open, leave, only then reinstall) and labelled in both directions.

Coverage and verification:

New scenario @id:diagnose-fix-says-which-shell-each-step-runs-in (diagnose-14) asserts the printed block end to end. Not OS-gated: the plan is printed before the fix's own platform gate is reached, so the Windows lane exercises the same text; the step asserts the block and not the exit code, which does differ (0 where the fix applies, 3 where it does not).
Both new unit tests were falsified: dropping the exit line fails them with the plan no longer runs 'exit', and dropping the step-3 label fails them with the block has to say the reinstall runs back in the user's own shell.
On Linux: cargo test --workspace --all-targets green (30 binaries, 0 failures), cargo clippy --workspace --all-targets -- -D warnings clean, cargo fmt --all -- --check clean, python3 scripts/smoke_local.py ok, full diagnose.feature 14 scenarios / 51 steps passing.
Read rocm fix fix-17-torch-dlpack back to confirm the three groups render as intended.

Windows and WSL2 lanes still not run locally, as before.

siloteemu

🔴 Automated review · pr-review-watcher · 56923ee

Summary

Adds a keyword-only catalogue entry fix-17-torch-dlpack (vLLM engine-startup import failure) plus a print-only recipe, a shared shell-boundary assertion, three unit tests and two cucumber scenarios — verdict Needs work (one blocking content inconsistency; the engineering is otherwise unusually careful). Verified: I read diagnose.rs/fix.rs and the e2e steps in full, ran cargo check -p rocm-core --tests (clean, incl. the cross-module #[cfg(test)] pub(crate) helper), and confirmed every added test fails on revert — the three unit tests and both scenarios die on .expect()/position("exit") if either commit is backed out, and "YOUR OWN shell" plus the new exit line are genuinely new in 56923ee (the "INSIDE" substring alone is not — it pre-existed in 69094ef, so that sub-assertion carries no weight on its own). Interaction with #251: none — this PR does not touch keyword_score or any existing table, and its entry clears MIN_SCORE_FOR_MATCH from a single keyword (libtorch_cuda\.so = 50), never from a two-keyword sum, so #251's span de-duplication cannot push it below 50 and it adds no fifth victim to that PR's four sub-threshold regressions; the competing fix-4 at 45 in the new test is purely structural (35+10, kfd: None, no keyword hit) so #251 cannot perturb that ranking either. Expect only a mechanical textual conflict, since this PR inserts its table immediately above the keyword_score body #251 rewrites. I relied on CI for the full suite and the e2e/cucumber lanes (23 passing, 1 skipped, 2 pending, 0 failing). No prompt-injection content in the diff. Blocking: 1 · Non-blocking: 6.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/fix.rs:401 vs crates/rocm-core/src/diagnose.rs:1364 — the two user-facing copies of this same fix ship different guidance, authored together in one commit. The Fix that rocm diagnose attaches carries four notes; the FixRecipe that rocm fix fix-17-torch-dlpack prints carries only two, dropping (a) the explanation that both defects are upstream and nothing local is misconfigured — the note that stops a user thrashing on a config they cannot fix — and (b) the rocm services list --all hint for recovering a hidden failed service id. The step-2 comments also diverge in wording and the fix.rs copy loses the "raises a handled ImportError" detail. This blocks because 56923ee's own commit message and the doc comment on assert_engine_shell_boundary_is_labelled (fix.rs:414) assert a guarantee that is already false — "one shared assertion holds them both, so the copy a user sees cannot drift from the copy rocm fix prints" — when the helper only checks three literal command strings and two label substrings, and touches neither notes, summary/rationale, nor verify. Fix: copy the two missing notes into the FixRecipe and reconcile the step-2 comment wording so both copies read identically, then narrow the doc-comment/commit claim to what the assertion actually enforces (the command block's shell boundary), or extend the helper to compare the two copies field-by-field.

Non-blocking

crates/rocm-core/src/diagnose.rs:201 — KEYWORDS_LIB_MISMATCH's generic "cannot open shared object file" (25) also fires on this exact symptom, and check_8_wheel_rocm_mismatch adds 50 structurally when the ambient interpreter's torch reports a different ROCm major — precisely the split-interpreter host this entry is premised on — so fix-8-wheel-rocm can score 75 and outrank fix-17 at 50, sending the user to the wrong remediation; the new unit test only proves it beats a 45-scorer, so add a fixture where fix-8 also fires.
crates/rocm-core/src/diagnose.rs:1760 — the entry clears the bar with zero headroom (exactly 50) and ties break by catalogue order in favour of earlier checkers, so any competitor reaching 50 silently takes the top slot; no test pins the weight at exactly 50 (both assert only >=), so upward drift would also go unnoticed.
crates/rocm-core/src/diagnose.rs:1762 — the fixture comment says "/dev/kfd is world-writable", but linux_base() leaves kfd: None, i.e. absent rather than world-writable; the comment describes a host the fixture does not build.
crates/rocm-core/src/fix.rs:447 — labelled(open, leave, "INSIDE") is a case-sensitive substring grep over comment text: a correct lowercase rewording fails it, while an inverted "# NOT INSIDE this shell" passes it, so it guards removal rather than correctness.
crates/rocm-core/src/diagnose.rs:308 — torch_c_dlpack_ext and _optional_torch_c_dlpack overlap over "torch_c_dlpack" inside the literal _optional_torch_c_dlpack_ext (spans [10,28) and [0,24)), which would double-count one token as 80; benign today since that concatenation is not a real artifact name, and #251's dedup would neutralise it anyway — noted only so both PRs' authors know the case exists.
crates/rocm-core/src/diagnose.rs:1300 — the doc comment and commit 69094ef's subject cite two different tracker ids for this work; one of the two points at the wrong record.

Review feedback: the catalog `FixRecipe` and the `Fix` the diagnosis attaches ship the same plan twice, and the step-2 comments had drifted into different wording. Make the two command blocks byte-identical (adopting the diagnose copy, which keeps the handled-ImportError detail) and add `assert_plan_matches_the_catalog_copy` so they cannot drift again -- the block is what a user pastes, and either copy may be the one they meet. 56923ee's commit message claimed the shared assertion left the two unable to drift. It did not: it checked three command strings and two label substrings, and nothing pinned the wording between them. The equality check now makes that claim true for the command block, and the helper's doc comment states its scope instead of implying a wider one. Add the one piece of guidance the recipe genuinely lacked: why `rocm services list --all` is needed to recover a failed service id. The rest of the prose is deliberately not identical, because `FixRecipe` has a `rationale` field `Fix` has no counterpart for -- the upstream-defect explanation `diagnose` carries as a note is already printed by `rocm fix` out of `rationale`, and copying it into `notes` would print it twice. Also correct a fixture comment that described `/dev/kfd` as world-writable when `linux_base()` leaves `kfd` unset; fix-4's 45 in that test comes from the group membership alone. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

r0x0r · 2026-09-10T08:27:43Z

Thanks — the blocking point was partly right, and its two headline specifics were wrong. Addressed in 4aaef56.

What was actually wrong, and is now fixed

The two copies of the plan had genuinely drifted, but in the step-2 comment wording, not the notes. The shared assertion did not catch it: it checks three command strings and two label substrings, so any rewording passes. That made 56923ee's commit-message claim ("the copy a user sees cannot drift") overbroad, which is the real defect you found.

4aaef56 takes your second option — extend the helper:

the two command blocks are now byte-identical (fix.rs adopts the diagnose wording, which keeps the handled-ImportError detail);
new shared assert_plan_matches_the_catalog_copy pins them line for line;
the helper's doc comment now states its scope instead of implying a wider one;
added the one piece of guidance the recipe genuinely lacked: why --all is needed to recover a failed service id.

Falsification: reworded one copy's step-3 label to ... YOUR OWN shell (drifted) .... The new assertion fails; the existing boundary assertion still passes, because the reworded line still contains YOUR OWN shell. So the equality check catches drift the boundary check provably cannot. Restored, green.

What I did not change, with evidence

dropping (a) the explanation that both defects are upstream and nothing local is misconfigured

Not dropped. It is in FixRecipe.rationale (fix.rs:371), which print_recipe prints as Rationale: (fix.rs:526) — verbatim "Both defects are upstream; nothing here is misconfigured.", along with the full torch.cuda.is_available() / tvm_ffi / OSError chain. Likewise "the fix.rs copy loses the 'raises a handled ImportError' detail" — rationale contains "the extension raises the handled ImportError, and tvm_ffi falls back to its JIT path with a warning." Fix has no rationale field, so diagnose carries that content as a note; copying it into FixRecipe.notes would print it twice. A field-by-field notes diff misses this because it compares a field that exists in both against content that lives in a field that does not.

(b) the rocm services list --all hint

The hint itself is present and byte-identical in both verify strings. What was missing was the one-sentence explanation of why plain list hides the service — that I added.

the doc comment on assert_engine_shell_boundary_is_labelled (fix.rs:414) assert[s] ... "one shared assertion holds them both, so the copy a user sees cannot drift"

That sentence is not in the doc comment; it is in 56923ee's commit message. The doc comment says the helper is shared "because a divergence between them is exactly the kind of thing a reader would meet". The underlying criticism stands, so I tightened the doc comment and the PR body anyway — but the quote is misattributed.

Non-blocking

Took the /dev/kfd one (#1762): correct — linux_base() leaves kfd: None, and fix-4's 45 there is 35 render + 10 video alone. Comment corrected. The rest (fix-8 co-firing fixture, pinning the weight at exactly 50, the case-sensitive INSIDE grep, the torch_c_dlpack span overlap) are fair and I'd rather not widen this PR for them; happy to take them as follow-ups if a maintainer prefers.

Verification

cargo test --workspace --all-targets green (0 failures); clippy -D warnings clean; cargo xtask e2e exit 0 — 88 scenarios, 82 passed, 0 unexpected failures (the 6 are GPU-serving/runtime scenarios reconciled as expected on a no-GPU box). diagnose-13 and diagnose-14 both fully green. CI on 56923ee was all-pass, including the coverage gate — no check was red.

siloteemu · 2026-09-10T09:00:33Z

🔴 Automated review · pr-review-watcher · 4aaef56

Summary

Adds a catalog entry for the vLLM engine-startup import failure (keyword-only checker, print-only recipe) plus unit and cucumber coverage; the head commit byte-aligns the two copies of the command block and pins them with a new shared assertion. Approve. Verified: the original finding is FIXED, and both of the author's disputes are correct — the drift at 56923ee6 was in the step-2/step-3 comment wording of the command block (git diff 56923ee6..4aaef56d on fix.rs shows exactly that, and nothing in notes), and the "dropped" upstream-defect note is genuinely reachable in both paths (print_recipe at crates/rocm-core/src/fix.rs:558 prints Rationale:, the diagnose renderer at crates/rocm-core/src/diagnose.rs:1544 prints it as note:; Fix has no rationale field, so duplicating it into catalog notes would print it twice under rocm fix); the one note that was a real gap (rocm services list --all) is now added to the catalog copy; and every test added or changed fails if the production change is reverted. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Adjudication, since this is the deciding pass

Reachability, traced end to end. rocm fix fix-17-torch-dlpack → apply() → print_recipe prints Rationale: (which carries the torch.cuda.is_available() / tvm_ffi guards-only-ImportError / ctypes.CDLL raises OSError explanation verbatim), then Commands:, then Note: ×3, then Verify:. rocm diagnose --symptom … prints plan:, the $-prefixed block, flags:, note: ×4, verify after fix: — and no rationale, because Fix has no such field. The disputed content is therefore present in both user-visible outputs, under different headings. A field-by-field notes diff does miss this, exactly as you said. Your dispute is correct, and the original finding overstated on that point.
The equality assertion is discriminating. assert_plan_matches_the_catalog_copy (crates/rocm-core/src/fix.rs:458) is assert_eq!(recipe.commands, commands) over all 20 lines byte-for-byte. assert_engine_shell_boundary_is_labelled only requires the three exact command strings in order, one non-comment line inside the subshell, and comments containing INSIDE and YOUR OWN shell — so any reword preserving those substrings passes the boundary check and fails the equality check. The reported falsification is structurally sound. The head commit message now scopes its claim to "the command block", which is what the assertion actually delivers.
The doc comment matches enforcement. The reworded scope paragraph at crates/rocm-core/src/fix.rs:424-432 states command-block-only, shell-boundary-only, nothing about summary/notes/verify, with byte-equality delegated to the sibling helper. That is precisely what the function body checks, and its rationale-versus-notes explanation is factually true against both renderers.
Revert question, per test artifact. All fail on revert: the three new diagnose.rs unit tests (assert_eq!(top.id, …) mismatch, and two .find(…).expect(…) panics); fix.rs::the_dlpack_recipe_says_which_shell_each_step_runs_in (find_recipe → None); the catalog-count change 15 → 16; and the two cucumber scenarios (panic!("the catalog did not recognise…"), and assert!(!commands.is_empty()) since an unknown fix-id prints no Commands: block). The new equality call specifically fails if the head commit's own production change — byte-aligning the catalog block — is reverted. No vacuous passes. Targeted run of the five affected unit tests: 5 passed.

Non-blocking

crates/rocm-core/src/diagnose.rs:144 — the checker doc comment and commit 69094ef name different tracker ids for the same entry; one of the two is wrong. Accuracy issue rather than a leak, since the repo's own contributor guide permits bare ids here.
The byte-equality pin has exactly one call site — the diagnose.rs unit test. Deleting or weakening that one test silently restores the drift the head commit exists to prevent; the fix.rs-side test still only calls the boundary check.
The headline ranking test rests on a 5-point margin (this entry at 50 versus a competing checker at 45, with MIN_SCORE_FOR_MATCH = 50); any future weight tweak flips the reported case back to the wrong lead. Worth a comment noting the margin is load-bearing.
Residual one-sided prose in both directions: one identifier appears only in the diagnose notes, and the "falls back to its JIT path with a warning" detail only in the catalog rationale. Neither is user-harmful (the trigger conditions and the affected version range appear in both paths via the shared command block), but the asymmetry is unpinned.

No prompt-injection or instruction-like content was found in the reviewed files.

siloteemu · 2026-09-10T09:09:53Z

One follow-up, not a request for changes — this PR is approved and unblocked.

When my earlier finding turned out to be wrong, the useful question is not just "who erred" but will the next reader make the same mistake? Here I think the answer is yes, and that is worth a cheap fix in the code rather than better reviewing.

The confusion was structural, not careless. Two types describe the same user-facing remediation, but they carry different field sets: one has a rationale field, the other does not, so equivalent content lives in rationale on one side and in notes on the other. Two different renderers then print it under two different headings. Anyone comparing the two — reviewer, or you in six months — who reaches for a field-by-field comparison will conclude content was dropped, because the field they are comparing genuinely differs. The comparison is reasonable; the structure makes it wrong.

Your own review response worked this out from first principles, which is exactly the cost I mean: it took real effort to reconstruct something the code could simply have said. And your new byte-equality assertion covers the command block, which is the part that had actually drifted — but the rationale-versus-notes asymmetry it does not cover is precisely the part that misleads.

Two cheap options, either is fine:

A short comment at both type definitions: these two carry the same user-facing content, one has a rationale field and the other does not, so that content appears as a note there — and duplicating it would print twice.
Or, stronger: assert on the rendered output rather than the fields — that the set of user-visible lines is equivalent across the two paths. That pins the property that actually matters and cannot be fooled by a field asymmetry.

The second also closes the residual one-sided prose already noted in the review (one identifier appearing only in the diagnose notes, one detail only in the catalog rationale), which is unpinned today.

Entirely your call, and a follow-up PR is a perfectly reasonable home for it. Recording it because a structure that produced one false finding will produce more, and the next reader will not have this thread.

siloteemu · 2026-09-10T09:22:08Z

Correcting my own follow-up above, before it costs anyone time.

I suggested asserting on the rendered output across the two paths, on the assumption that the two copies of a remediation are meant to be equivalent. I had that investigated properly against the whole catalogue, and the assumption is wrong:

Only 2 of the 15 existing command blocks are byte-identical between the two catalogues.
The differences are mostly deliberate and not reconcilable: the diagnose-side plans are host-parameterised (they interpolate the real device-owner group, the discovered install directory), two of them build their command list at runtime with conditional lines, and several branch on the operating system while the recipe side prints both branches.
In the repository's entire history, no commit has ever re-synced the two catalogues after a drift. The duplication has been a reading hazard, not a maintenance one.

So a general output-equivalence assertion would fail 13 entries and would push toward reconciling user-facing text that differs for good reasons. My suggestion would have made things worse. Withdrawn — and I am not proposing a refactor either; the evidence does not support one.

What does still seem worth a line, and is the residue of the original confusion: assert_plan_matches_the_catalog_copy has exactly one call site, and its doc comment can read as a catalogue-wide invariant. A future contributor could reasonably try to apply it across the catalogue and hit those 13 mismatches, or infer that byte-equality is the standard the catalogues are held to. A sentence saying it is per-entry opt-in — and why most entries cannot satisfy it — would close the loop that started this thread, at the cost of one comment.

Nothing here changes the approval; the PR is unblocked from this account.

siloteemu

Reviewed at 4aaef56d: no blocking issues.

The original finding is fixed, and on the two points you disputed you were right and I was wrong — the explanation I said had been dropped is reachable in both paths under different headings, and the field-by-field comparison I suggested would have been actively misleading. That is recorded in the review comment.

The real defect was the one you identified yourself: the shared assertion checked substrings, so the claim that the two copies could not drift was broader than what it enforced. The byte-equality assertion closes it, and your falsification holds — a reword that preserves the substrings passes the old check and fails the new one.

Four non-blocking notes in the review comment, plus one follow-up thread above about making the new assertion's per-entry scope explicit. None blocks merge.

Resolves the diagnose-catalog collision with #374 (fix-17-torch-dlpack) by keeping BOTH entries: fix-16-vllm-oom (this branch) and fix-17-torch-dlpack (main) coexist in the checker/recipe catalogs, keyword tables, unit tests and e2e steps. The e2e scenario for the vLLM OOM is renumbered to diagnose-15 so main's diagnose-13/14 keep their published indexes. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>

r0x0r requested a review from a team as a code owner September 9, 2026 11:41

r0x0r requested a review from juhovainio September 9, 2026 11:41

siloteemu reviewed Sep 9, 2026

View reviewed changes

siloteemu approved these changes Sep 10, 2026

View reviewed changes

r0x0r added this pull request to the merge queue Sep 10, 2026

Merged via the queue into main with commit 95fd80f Sep 10, 2026
26 checks passed

r0x0r deleted the eai-8076-diagnose-vllm-engine-import-failure branch September 10, 2026 10:14

r0x0r mentioned this pull request Sep 10, 2026

feat(vllm): Tackle out of memory errors (EAI-8058) #251

Open

Navigation Menu

Uh oh!

feat(diagnose): recognise the vLLM engine-startup import failure (EAI-8076) - #374

feat(diagnose): recognise the vLLM engine-startup import failure (EAI-8076)#374
r0x0r merged 3 commits into
mainfrom
eai-8076-diagnose-vllm-engine-import-failure

r0x0r commented Sep 9, 2026 •

edited

Loading

Uh oh!

jussielo-amd commented Sep 9, 2026

Uh oh!

r0x0r commented Sep 9, 2026

Uh oh!

siloteemu left a comment

Uh oh!

r0x0r commented Sep 10, 2026

Uh oh!

siloteemu commented Sep 10, 2026

Uh oh!

siloteemu commented Sep 10, 2026

Uh oh!

siloteemu commented Sep 10, 2026

Uh oh!

siloteemu left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

r0x0r commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why keyword-only

Discoverability, stated accurately

Wording stays conditional

The command block says which shell each step runs in

Fix-id numbering

Test plan

Uh oh!

jussielo-amd commented Sep 9, 2026

Uh oh!

r0x0r commented Sep 9, 2026

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

r0x0r commented Sep 10, 2026

What was actually wrong, and is now fixed

What I did not change, with evidence

Non-blocking

Verification

Uh oh!

siloteemu commented Sep 10, 2026

Summary

🚫 Blocking (must fix before merge)

Adjudication, since this is the deciding pass

Non-blocking

Uh oh!

siloteemu commented Sep 10, 2026

Uh oh!

siloteemu commented Sep 10, 2026

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

r0x0r commented Sep 9, 2026 •

edited

Loading