Repository navigation
Conversation
…tic ROCm claim ROCMAI-550: on a GPU with no ROCm/HIP build (e.g. gfx90c), `rocm examine` and `rocm engines` kept claiming the embedded Lemonade engine runs "ROCm llama.cpp backend" even though it actually falls back to Vulkan. `rocm install sdk`'s refusal for such a GPU didn't name the actual next step, and neither `rocm serve`'s deployment summary nor `rocm services list` showed which backend/ device an engine actually selected. - add `GpuSupportLevel` (RocmHip/VulkanOnly/Unknown) in rocm-core, derived from the existing `normalize_therock_family`/`gfx_target_has_no_rocm_build` primitives - make the Lemonade engine note in `rocm examine` and `rocm engines` GPU-aware instead of a hardcoded ROCm claim - `rocm install sdk`'s unsupported-GPU errors now suggest `rocm serve <model>` - surface the engine's actual backend/device (read live via the existing `Healthcheck` RPC, never guessed) in `rocm serve`'s deployment summary and `rocm services list` Most of the ticket's `rocm examine`/`install sdk` complaints for gfx90c were already fixed by #648 (ROCMAI-542); this covers what was left. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged two assertions as cleartext logging of `api_key`. Those two use `base_summary()`, whose key is `None`, so the literal instances are false positives — but the pattern is not. `render_summary` prints the key by design, and `api_key_client_config_shown_only_when_present` sets a real one, so a failure there copied `endpoint-secret` into CI logs, which are retained and widely readable. Assert on the real text, print a redacted copy. Covers all twelve sites that interpolated a rendered summary, not just the two CodeQL picked. Production output is unchanged: the key is still shown once, and the test pinning that still asserts the exact string. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
CodeQL's two What CodeQL flagged ( What it was actually pointing at. Fix: assert on the real text, print a redacted copy. A small test-only Production output is untouched: the key is still shown once, and the test that pins the exact string still asserts Verified on Linux at |
There was a problem hiding this comment.
🟡 Changes recommended
Unknown GPUs remain misreported, authenticated Lemonade devices are omitted, and required observable-output coverage is incomplete.
7 open findings
What changed in this PR
Updates GPU/backend reporting to reflect detected support and live engine state.
Changes:
- Adds GPU support classification and dynamic Lemonade messaging.
- Reports live backend/device details in serving output.
- Improves unsupported-SDK guidance and test coverage.
| File | Description |
|---|---|
crates/rocm-core/src/host_gpu.rs |
Adds GPU support tiers. |
crates/rocm-core/src/lib.rs |
Exports support APIs. |
apps/rocm/src/main.rs |
Adds dynamic notes and live device reporting. |
apps/rocm/src/serve_cmd.rs |
Fetches devices for deployment summaries. |
apps/rocm/src/serve_summary.rs |
Renders the device row. |
apps/rocm/src/therock.rs |
Adds serving remediation guidance. |
apps/rocm/src/providers.rs |
Updates summary construction. |
tests/e2e-cucumber/features/model_serving.feature |
Extends GPU serving scenarios. |
tests/e2e-cucumber/tests/e2e/serving_steps.rs |
Adds device-reporting assertion. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| match level { | ||
| rocm_core::GpuSupportLevel::VulkanOnly => { | ||
| "default embedded Lemonade server; this GPU has no ROCm/HIP support, so it runs on the Vulkan llama.cpp backend" | ||
| } | ||
| _ => "default embedded Lemonade server with ROCm llama.cpp backend", | ||
| } |
There was a problem hiding this comment.
You were right and this is now fixed in f54a8d2f. The earlier round added engines_inventory_does_not_claim_vulkan_for_an_unrecognized_gpu and its two siblings, which pinned that Unknown must not claim Vulkan — but they said nothing about the ROCm side, and the _ => catch-all kept printing "with ROCm llama.cpp backend". That is exactly the stale claim this PR exists to remove, surviving on precisely the hosts where the CLI knows least.
lemonade_engine_note (apps/rocm/src/main.rs:2748) now matches GpuSupportLevel exhaustively, with no catch-all, so a future variant has to be classified rather than silently inheriting the ROCm wording:
VulkanOnly— confirmed no ROCm build, names the Vulkan backend.RocmHip— keeps the ROCm wording.Unknown— claims no backend: "default embedded Lemonade server; runrocm examineto see which llama.cpp backend this GPU uses".
Not the Vulkan wording for Unknown either, for the reason the type exists: a detection gap is not a confirmed absence. Pointing at rocm examine keeps the row actionable, and matches what unsupported_requested_target_message already does on the other branch where the host is unknown.
The two Unknown tests now assert all three facts — no Vulkan claim, no ROCm claim, and a next step — at apps/rocm/src/examine_report_text.rs:2608 and apps/rocm/src/main.rs (codex_bridge_engine_inventory_does_not_claim_vulkan_for_an_unrecognized_gpu). Falsified on the Linux box by restoring the catch-all: both go red.
| let response: HealthcheckResponse = engine_request( | ||
| Some(paths), | ||
| engine, | ||
| EngineMethod::Healthcheck, | ||
| &HealthcheckRequest { | ||
| service_id: service_id.to_owned(), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Verified against the current code and fixed in f54a8d2f — this was still live, not stale.
Confirmed the mechanism you described: engines/lemonade/src/state.rs::inference_verified passes rocm_engine_protocol::resolve_endpoint_api_key() to its probe, which reads ROCM_SERVE_API_KEY_FILE from the environment. fetch_service_device supplied only AppPaths, so for a public or --require-api-key service the engine's own /v1/models probe answered 401, listed/ready came back false, the device degraded to "unknown", and the row vanished for exactly the services that are protected.
Fix: the key file is threaded per call, since the key is per service id.
engine_request_with_scoped_env(apps/rocm/src/engine_ipc.rs:152) takes anendpoint_key_filebeside the existingenv_root. For the built-in path it is scoped bywith_scoped_builtin_engine_envinside the sameBUILTIN_ENGINE_ENV_LOCKcritical section as the other vars, so the env-mutation contract (xtask/src/env_mutation_contract.rs) still holds; for an external plugin it is set on the childCommand.fetch_service_device(:305) passesendpoint_keys::endpoint_key_file_if_present(paths, service_id)—Nonefor a loopback service, which has no key file and needs none.engine_request_with_env_rootdelegates, so no existing call site changed.
Your pointer to the restart path was the right precedent: services_text.rs already re-threads the same file onto the engine child it spawns.
Not verified against a live protected server — the dev box has no usable GPU telemetry, so there is no managed service to probe there. This is wiring verified by compilation and the existing gates, not an observed 401 turning into a device row.
| the Vulkan backend can still be used. \ | ||
| Try `rocm serve <model>` — it runs on Lemonade's Vulkan backend instead." |
There was a problem hiding this comment.
Confirmed and fixed in f54a8d2f. @juhovainio and @nowycondro reached the same defect from the configured-default_engine side; your recipe-selection angle is the other half of it, and the fix covers both.
unsupported_host_gpu_message (apps/rocm/src/therock.rs:3607) now pins the engine, which is the first of your two suggestions:
Try
rocm serve <model> --engine lemonade- it runs on Lemonade's Vulkan backend instead.
I did not take the second (describe backend selection conditionally). Conditioning means resolve_family reading RocmCliConfig and the model recipe to say which engine would win — a config load and a recipe resolution added to an install-path error message, to produce a sentence that is still only correct for the inputs it happened to check. --engine out-ranks every source in select_serve_engine, so pinning makes the claim unconditional instead of conditional, which is the stronger guarantee for less machinery.
One correction to your description, for the record: a recipe's preferred_engines is ranked below the host-GPU preference in select_serve_engine, not above it — the override that genuinely out-ranks the host is the configured default_engine. Either way the bare command could not be promised, so the conclusion stands.
Test: unsupported_gpu_error_pins_the_engine_a_configured_default_could_override (therock.rs:8399), which asserts the pinned form and the absence of the bare one still carrying the promise — necessary because the pinned form contains the bare one as a prefix.
Your note about line 3479 refers to unsupported_requested_target_message; that one was already fixed in 16ccfb3a and now points at rocm examine instead of predicting a backend.
| /// The lemonade engine note shown in `rocm examine` and `rocm engines`, adjusted | ||
| /// for whether this host's detected GPU can actually use the ROCm llama.cpp | ||
| /// backend. Both call sites have a live `GpuSupportLevel` in scope; other |
There was a problem hiding this comment.
Judging this one partly answered and partly not reachable, with the reasons below rather than a silent skip.
Answered. The two notes are no longer unit-only in the sense you meant: engines_inventory_* (apps/rocm/src/examine_report_text.rs:2550-2628), examine_engine_inventory_* and codex_bridge_engine_inventory_* / engines_mcp_tool_* now drive the real renderers for all three support levels, including the Unknown expectation you asked for — which this push strengthened to assert that Unknown claims no backend rather than merely not claiming Vulkan (see the main.rs:2837 thread).
Not reachable as a scenario, and this is the honest part. A Gherkin assertion on the ROCm-versus-Vulkan wording needs a host whose GPU is detected and confirmed to have no ROCm build — a Renoir/Cezanne Vega APU. detect_host_gpu_summary reads real sysfs/WMI with no test override, and every lane reports a ROCm-capable GPU (platform=mi300x, Strix Halo, W7900). A scenario written for the Vulkan branch would be permanently skipped on every lane, which reads greener than it is.
What a lane can assert is the negative, and that is now pinned: runtime-19 checks that the --family gfx90c refusal points at rocm examine and does not contain rocm serve, on a runner that is ROCm-capable. Proving the absence of a false prediction is the half a lane can actually reach.
The device: row — the other observable this PR adds — is now asserted end-to-end, including its value and agreement between rocm serve and rocm services list; see the model_serving.feature and serving_steps.rs threads.
| assert!(message.contains("no ROCm SDK build"), "{message}"); | ||
| assert!(message.contains("`gfx90c`"), "{message}"); | ||
| assert!(message.contains("Vulkan"), "{message}"); | ||
| assert!(message.contains("rocm serve <model>"), "{message}"); |
There was a problem hiding this comment.
Standing by the existing answer here rather than adding a scenario, and stating the reason explicitly rather than leaving it in another thread: the full write-up is in the PR description section "Why the rocm install sdk remediation sentence is unit-covered and not a scenario", and in the reply to @juhovainio's thread on the same hunk.
Short version: unsupported_host_gpu_message is only reachable from resolve_family after the --family, ROCM_CLI_THEROCK_FAMILY and managed-runtime doors are all closed, and then only when detect_host_gfx_target() returns a target normalize_therock_family does not cover. That needs a real Renoir/Cezanne Vega APU on the runner, and detect_host_gfx_target reads sysfs/WMI with no test override. runtime-19 drives the --family branch precisely because that is the one door a lane can open — you are right that it does not prove the detected-host path, and nothing in CI can.
What did change in f54a8d2f is the remediation itself: it now names the engine, rocm serve <model> --engine lemonade, because a configured default_engine could otherwise make the sentence false (see @nowycondro's and @juhovainio's threads). The two unit tests you point at are joined by a third, unsupported_gpu_error_pins_the_engine_a_configured_default_could_override (apps/rocm/src/therock.rs:8399), which pins the engine-pinned form and the absence of the bare one — the bare command is a prefix of the pinned one, so a presence-only assertion could not tell them apart.
| When the user sends a chat completion request | ||
| Then the response contains a model reply | ||
| And the response identifies the correct model | ||
| And the services list reports the engine's actual backend device |
There was a problem hiding this comment.
Correct, and fixed in f54a8d2f. The scenarios ran rocm serve in a Given and then asserted only on a later services list, so the deployment summary's own device: row — half of what this PR adds — had no live assertion anywhere.
Two changes:
E2eWorldgainsserve_summary(tests/e2e-cucumber/tests/e2e.rs), set byserve_and_waitandserve_gpu_modelon the successful attempt. Deliberately notcli_output: that field belongs to each scenario's When step, and overwriting it from a Given would change what every existing Then on these scenarios reads.assert_services_list_reports_device(serving_steps.rs:1491) now cross-checks the two surfaces: if the summary printed adevice:row it must equal the oneservices listreports (they read the sameHealthcheckRPC), and if it did not, the summary must not have saidready— so the row's absence has to be explained by a launch that was still starting, not by the row being dropped.
The assertion is two-sided rather than unconditional on purpose. rocm serve only asks the engine once the launch already reported ready, and a managed launch can legitimately return while the weights are still loading — which is exactly why the Given then polls /v1/models. An unconditional "summary must contain device:" would have been flaky on a slow cold start rather than a real check.
Caveat on where this actually runs: serve-07 is xfail'd on lemonade+linux (EAI-7423), so on that lane no step of it is a live assertion. serve-05 (the vLLM canary) is what exercises this on every PR.
| assert!( | ||
| block | ||
| .lines() | ||
| .any(|line| line.trim_start().starts_with("device:")), |
There was a problem hiding this comment.
Fair, and fixed in f54a8d2f. The step checked only that a line starts with device:, so an empty, hardcoded or wrong value all passed.
assert_services_list_reports_device (tests/e2e-cucumber/tests/e2e/serving_steps.rs:1491) now parses the value and holds it to three independent facts:
- Shape.
parse_device_row(:1453) requires<backend> gpuor<backend> gpu <n>with nothing trailing and a numeric ordinal — the shape every engine reports. An empty or malformed value fails here. - Backend against the engine that served. The step reads the
engine:row from the same services-list block and checks the backend against it:lemonademay reportrocmorvulkan;vllmandaimare ROCm-only, sovulkanfrom either means the value did not come from the engine that served.unknownfails for all three — it is the engines' own "no state for this", whichfetch_service_deviceis supposed to drop rather than print. That is yourrocm gpu …vsvulkan gpu …distinction, enforced rather than assumed. - Agreement with the serve summary — see the thread on
model_serving.feature.
Note the shape check only became meaningful with the engine fix in this same push: engines/vllm used to report the literal "rocm_gpu", which parse_device_row rejects. It now reports rocm gpu <n> like Lemonade.
What I could not do is assert the expected backend for a given host — that needs a Vulkan-only GPU, and no lane has one. The check is "consistent with what this engine can select", not "equal to what this host must select".
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed this with a review-code fan-out pass plus a separate Standards/Spec pass against Jira ROCMAI-550. The core design is good: GpuSupportLevel is derived from existing detection primitives rather than adding new probing, the device: row is read live from each engine's own Healthcheck RPC rather than guessed from the launch-time GPU plan, and the PR description is refreshingly honest about what it couldn't verify end-to-end (both GPU canaries fail on this box for pre-existing, unrelated environment reasons, clearly disclosed) and what it checked by hand instead.
Requesting changes on one gap: the rocm install sdk remediation text added in therock.rs (unsupported_host_gpu_message/unsupported_requested_target_message now suggest rocm serve <model> as the next step) is a second user-observable CLI text change in this same PR, but only the device: row change got a Gherkin scenario. AGENTS.md §3 asks for a scenario or an explicit justification for user-observable output changes - the PR's test plan explicitly discloses the gap for the e2e device scenario (good), but says nothing about this one at all, so it reads as simply uncovered rather than a disclosed, accepted gap.
One more thing worth a look, not blocking: builtin_codex_bridge_engine_inventory (via builtin_engine_inventory's static table) still unconditionally claims lemonade runs "with ROCm llama.cpp backend" - the exact stale claim this PR exists to fix. The new lemonade_engine_note helper's own doc comment calls this out by name as a deliberate scope choice (no live GPU probe in front of the codex-bridge inventory), so this may be intentional - but it's the kind of thing worth confirming explicitly rather than leaving implicit, since it's the same wording this PR is removing from two other call sites.
| The GPU is detected but is not supported by ROCm; \ | ||
| the Vulkan backend can still be used." | ||
| the Vulkan backend can still be used. \ | ||
| Try `rocm serve <model>` — it runs on Lemonade's Vulkan backend instead." |
There was a problem hiding this comment.
This is a second user-observable CLI text change in this PR (alongside the device: row), but it has no Gherkin scenario and no justification note in the PR description - unlike the device: row change, whose missing e2e coverage is explicitly disclosed in the test plan as a known, accepted gap. Right now this one just reads as uncovered. Worth either a scenario asserting rocm install sdk's refusal message on an unsupported GPU names rocm serve <model>, or an explicit note in the PR description on why unit-test coverage (which does exist, in the two tests just below this hunk) is sufficient here.
There was a problem hiding this comment.
Went with the PR-description note rather than a scenario, and the reason is stronger than "a scenario costs runtime": a scenario for this branch cannot run on any lane we have. Full write-up is now a dedicated section in the PR description ("Why the rocm install sdk remediation sentence is unit-covered and not a scenario"); the short version:
unsupported_host_gpu_message is reached from resolve_family only after all three earlier doors are closed — no --family, no ROCM_CLI_THEROCK_FAMILY, no managed runtime — and then only when detect_host_gfx_target() itself returns a target normalize_therock_family does not cover and gfx_target_has_no_rocm_build confirms. That needs a real Renoir/Cezanne Vega APU on the runner. Every self-hosted lane reports platform=mi300x os=linux, and detect_host_gfx_target reads real sysfs/WMI with no test override, so the branch cannot be made to fire in e2e. A scenario written for it would be permanently skipped, which reads greener than it is.
What a lane can drive is the sibling branch, and that is now asserted — including the negative you found in your other thread: runtime-19 checks the --family gfx90c refusal points at rocm examine and does not contain rocm serve, precisely because that lane is ROCm-capable. So the one door a lane can open is covered by proving the absence of the prediction rather than enforcing it.
Holding this sentence up: the two unit tests just below (therock.rs:7681, therock.rs:7807), plus the fact that the claim is not a bare string pin — gfx_target_has_no_rocm_build is what routed the user here, and selects_vulkan_when_rocm_unsupported in engines/lemonade/src/backend_alignment.rs is what makes llama.cpp actually pick vulkan once rocm reports unsupported.
One wording change you should know about, since the exact text is what those unit tests sit next to: I merged main into this branch (it was 36 commits behind) and that brought in #683's whole-tree ASCII output contract, which this branch predates. xtask's every_literal_that_can_reach_a_redirectable_sink_is_ascii flagged this literal — and only this one out of everything the branch adds — for the em dash. It is now an ASCII hyphen:
Try `rocm serve <model>` - it runs on Lemonade's Vulkan backend instead.
Both unit tests match on rocm serve <model>, so they are unaffected. Merge commit is daed78d5; full gate re-run there on MI300X: 4178 tests passed / 0 failed, both clippy gates clean, cargo fmt --all --check clean.
| /// places that print this note statically (the dashboard TUI, `rocmd`'s own | ||
| /// catalog) have no host probe in front of them and keep the unconditional | ||
| /// ROCm wording, since they describe the engine in the abstract. | ||
| const fn lemonade_engine_note(level: rocm_core::GpuSupportLevel) -> &'static str { |
There was a problem hiding this comment.
This helper's own doc comment discloses that builtin_codex_bridge_engine_inventory/builtin_engine_inventory (main.rs ~2726-2747) keep the unconditional "ROCm llama.cpp backend" wording for lemonade, since that call site has no live GPU probe in front of it. That may be the right scope call, but it's worth confirming explicitly - it's the exact stale claim about ROCm support this PR exists to fix in the other two call sites, just surfacing through a third one (the codex bridge inventory) that's silently left out.
There was a problem hiding this comment.
You were right that this needed to be decided explicitly rather than left in a doc comment, and on looking at it I don't think the scope call it described actually held. Fixed in 49e23e3 instead of justified.
The old comment claimed the bridge inventory could not be host-aware because it is "the static catalogue". That isn't what blocked it — both call sites can see the host:
build_codex_bridge_snapshot(main.rs:2706) already gathers theExamineSummarycarryingdetected_gfx_targetandcompatible_therock_family, the exact two inputsgpu_support_leveltakes. It now hoists that gather and passes the level down, so the engine rows cost no detection the snapshot wasn't already paying for.- The bridge's
enginestool (main.rs:8123) takes the same cheapdetect_host_gpu_summaryprobe thatrocm enginesalready uses, via a newhost_gpu_support_levelhelper.
So there was no call site here that genuinely could not answer the question, which is what made "left alone deliberately" the wrong reading. As you put it, this is the same stale ROCm claim ROCMAI-550 exists to remove, surfacing through a third user-facing surface — a bridge client renders summary to the operator.
Only the summary string moves. Field set, field names and the installed_binary: true beside it are untouched, so the bridge's JSON shape is unchanged — this isn't the contract change the old comment warned about.
Two places still print the note statically, and that is now stated as deliberate rather than implied: the dashboard TUI's engine-manager pane renders a hard-coded descriptive list with no probe behind it, and rocmd's catalog in apps/rocmd/src/common.rs answers for whichever host a client asks about, not the one it runs on. Either becoming host-aware is its own change.
Gates on the Linux box at 49e23e3: 439 crates compiled, 3852 tests passed / 0 failed, clippy --workspace --all-targets -D warnings clean, and clippy --locked -p e2e-cucumber --test e2e -D warnings clean.
…l is produced The gfx90c refusals in `therock.rs` gained a `rocm serve <model>` remediation sentence, a second user-observable output change in this PR with no scenario behind it. AGENTS.md §3 wants remediation advice asserted where the message is really printed, not only against the formatter. runtime-19 already opens the one door of the two a lane can reach -- naming the target with `--family` -- so the assertion goes there. The detected-host door needs a Vega APU no runner has and stays unit-covered, as its existing comment already records. Also names the codex-bridge engine inventory at both the static table and the bridge helper as a deliberate scope boundary, instead of only in `lemonade_engine_note`'s doc comment where a reader of the table would miss it. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Thanks — both points checked against the code before acting. Pushed Blocking: the One correction to be fair to the test plan: it did list "unit tests for the I chose coverage over disclosure, because it turned out the gap was smaller than either of us assumed. §3 also wants remediation advice tied to the behaviour, not just the string, and that part genuinely cannot run here, so the step's doc comment names what does prove it rather than pinning the wording alone: That leaves one genuine gap, now disclosed in the test plan rather than inherited silently: Non-blocking: the codex-bridge inventory. Confirmed, and deliberate — with one correction to the premise. The On the substance, I considered making it support-aware — |
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Re-reviewed from scratch at the current head (this PR has moved substantially since my last review, which GitHub auto-dismissed). This version delivers what ROCMAI-550 asks for: a GpuSupportLevel classification, a GPU-aware Lemonade engine note, an install-sdk remediation sentence, and live device reporting (via the Healthcheck RPC, not guessed) in the serve summary and services list. I grepped the repo for the stale "ROCm llama.cpp backend" claim this ticket is about; the only remaining occurrences are the 4 locations this PR's own new doc comments document as a deliberate, explained exclusion - nothing stale slipped through. CI is fully green across all 24 checks, nothing flaky triggered.
One real issue, left as an inline comment below: the new remediation sentence added to unsupported_requested_target_message makes an unconditional claim about which backend the host will use, but that function fires on a rejected requested override (a --family flag or a stale ROCM_CLI_THEROCK_FAMILY env value) independent of the host's actual GPU. Your own new test in this same diff (cli_family_overrides_a_stale_env_value_naming_an_unsupported_gpu) demonstrates exactly this kind of override/real-host mismatch - which means on a host that actually has a ROCm-capable GPU, this message would tell the user rocm serve runs on Vulkan when it would really run on ROCm/HIP. The identical sentence is correct in the sibling unsupported_host_gpu_message, since that path only fires when host detection itself found no ROCm build, so it's describing a confirmed fact about this host.
Non-blocking, for your awareness:
host_gpu.rs'sExamineSummary::rocm_support_linehand-rolls the same classification the newgpu_support_level()function now exists to express, instead of calling it - currently equivalent, but a future change to one could silently desync from the other.- README.md/docs/testing.md/docs/manual-testing.md aren't updated for the new
device:lines in the serve summary and services list text output. - The lemonade e2e device-reporting Then-step is appended after assertions that are xfailed on that lane (EAI-7423), so it doesn't actually run there yet; only the vLLM canary scenario reliably exercises the new device-reporting path in CI today.
render_services_jsondoesn't get the newdevicefield that the text view does, so--jsonand text output diverge in informational content.
| That GPU is not supported by ROCm; the Vulkan backend can still be \ | ||
| used. Nothing was installed and no other family was substituted." | ||
| used. Nothing was installed and no other family was substituted. \ | ||
| Try `rocm serve <model>` — it runs on Lemonade's Vulkan backend instead." |
There was a problem hiding this comment.
This sentence is added to the requested-target rejection path, which fires independent of the host's real GPU (e.g. a stale ROCM_CLI_THEROCK_FAMILY env value naming an unsupported target on a host that actually has a ROCm-capable GPU - exactly the scenario your own new test cli_family_overrides_a_stale_env_value_naming_an_unsupported_gpu exercises). On such a host, rocm serve <model> would actually run on ROCm/HIP, not Vulkan, making this an incorrect claim rather than a confirmed fact. The identical sentence in unsupported_host_gpu_message above is fine because that path only fires when host detection itself returned the no-build target. Worth either dropping this sentence from the requested-target message, or rephrasing it to not assert a specific backend when the host's actual support level is unknown at this point.
There was a problem hiding this comment.
Confirmed and fixed in 16ccfb3 — you were right, and the precondition difference is exactly as you described.
Checking it before changing anything: unsupported_requested_target_message is raised from the two guards at the top of resolve_family (the --family branch and the ROCM_CLI_THEROCK_FAMILY branch), both of which sit above detect_managed_therock_family and detect_host_gfx_target. No host probe has run at that point. unsupported_host_gpu_message is only reachable further down, after detect_host_gfx_target() returned a target that normalize_therock_family didn't cover and gfx_target_has_no_rocm_build confirmed — so there the sentence is a fact about this host.
What I chose: reworded rather than dropped, to Run rocm examine to see what this host itself supports.
I considered gating it on the real GpuSupportLevel, but getting one there means calling detect_host_gfx_target() inside a branch deliberately placed ahead of all detection — it would contradict the function's own doc comment ("nothing was detected here"), and the existing test comment on requesting_gfx90c_by_name_fails_instead_of_installing_another_family explicitly treats reaching detection as the failure signal. Dropping it outright was the safe option but loses the next step, so pointing at rocm examine seemed strictly better: true on every host, still actionable, and it's the command this PR just taught to report the real support level.
One thing your review surfaced that I think matters more than the wording: runtime-19 was asserting the bug. It drives --family gfx90c, so it goes through this exact function, and its new Then was checking for rocm serve <model> + Vulkan. The CI runner it passes on reports platform=mi300x os=linux gpu=true effective_engine=vllm — a fully ROCm-capable host. So the false claim was being pinned green on precisely the kind of machine where it's false. That step now asserts the rocm examine pointer and the negative (!output.contains("rocm serve")), so the lane now proves the absence of the prediction instead of enforcing it.
Evidence the test fails without the fix — reverted just the message string on the devbox, left the tests alone:
test therock::tests::requested_target_error_does_not_claim_which_backend_serving_will_use ... FAILED
test therock::tests::requested_target_error_names_where_the_target_came_from ... FAILED
the requested target says nothing about this host's GPU, so the refusal must not
predict what `rocm serve` would run on: ... Try `rocm serve <model>` — it runs on
Lemonade's Vulkan backend instead.
Restored, both pass. The new test also asserts the positive side on the sibling path (unsupported_host_gpu_message keeps rocm serve <model> + Vulkan), so it pins the asymmetry rather than banning the sentence outright — if someone later copies it back across, that test is what catches it.
`unsupported_requested_target_message` is raised from the `--family` and `ROCM_CLI_THEROCK_FAMILY` branches at the top of `resolve_family`, before any host probe runs, so the rejected target says nothing about the GPU in this machine. It had borrowed its sibling's closing sentence promising that `rocm serve <model>` runs on Lemonade's Vulkan backend -- true in `unsupported_host_gpu_message`, which is only reached once host detection itself returns a target with no ROCm build, but false here: a stale env value naming gfx90c refuses just the same on a host whose own GPU is fully ROCm-capable, and there serving would run on ROCm/HIP. Point at `rocm examine` instead. It is true on every host, it is still a next step, and it is the command this PR just taught to report the real support level. runtime-19 was asserting the wrong half of this: it drives `--family gfx90c`, so it was pinning the Vulkan-serving claim green on exactly the ROCm-capable runners where it was false. It now asserts the examine pointer and the negative. Also route `ExamineSummary::rocm_support_line` through `gpu_support_level` rather than re-deriving the same classification, and document the serve summary's `device:` row in README and the manual-testing GPU check. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Thanks for the re-review — the blocking finding was correct and is fixed in 16ccfb3. Details on the fix and the falsification evidence are in-thread; summary plus the four non-blocking items here. Blocking — requested-target path claimed a backend it never checked. Verified the precondition difference at the source before touching it: both Non-blocking:
Verification (devbox,
No new assertion interpolates a rendered summary, so no new |
…tory The codex-bridge engine inventory kept the unconditional "with ROCm llama.cpp backend" wording that ROCMAI-550 removed from `rocm examine` and `rocm engines`, so a bridge client still rendered a ROCm claim to the operator on a host that cannot use it. Both call sites can answer the question: the snapshot already gathers the ExamineSummary carrying the two inputs, and the `engines` tool takes the same cheap host probe `rocm engines` uses. Only the `summary` string changes; the field set and the bridge's JSON shape are untouched. The dashboard TUI pane and rocmd's own catalog keep the static wording deliberately -- the pane has no probe behind it, and rocmd answers for whichever host a client asks about, not the one it runs on. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Resolves the one conflict, apps/rocm/src/main.rs, by taking main's post-#667 file (chat/MCP-bridge extraction removed 185 functions) and re-applying this branch's 23 hunks onto it. One hunk follows its function: `run_internal_mcp_call` now lives in apps/rocm/src/chat.rs, so the `engines` tool's call to `builtin_codex_bridge_engine_inventory` is updated there and `host_gpu_support_level` is imported from the crate root. Verified by construction: the `fn` name sets of main, this branch and the resolution comm out to resolved == main's set plus exactly the five functions this branch adds (`fetch_service_device`, `host_gpu_support_level`, `lemonade_engine_note` and the two new examine tests), with nothing of main's dropped. Also converts this branch's one new non-ASCII production literal to ASCII for the output contract #683 added, which this branch predates: the em dash in `unsupported_host_gpu_message`'s "Try `rocm serve <model>`" sentence becomes a hyphen, matching how #683 converted the rest of the tree. `xtask`'s `every_literal_that_can_reach_a_redirectable_sink_is_ascii` named that literal and only that one; everything else this branch adds with non-ASCII is a comment or lives under the exempt `tests/` prefix. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed the GPU-support-level messaging work (ROCm/HIP vs Vulkan-only vs Unknown) across host_gpu.rs, the codex-bridge/MCP snapshot, rocm engines/rocm examine, and the install-sdk refusal paths.
The core idea is sound: a 3-tier GpuSupportLevel replaces a static "ROCm llama.cpp backend" claim with one that reflects what the host actually supports, and it's backed by real e2e coverage (serve-05/06/07, runtime-19) plus an honest EAI-7423 xfail scoped correctly to effective_engine=lemonade, os=linux.
One thing worth flagging before this merges: the install-sdk refusal messages are not symmetric between the host-detected path (unsupported_host_gpu_message, adds "Try rocm serve <model>") and the explicit --family/env path (unsupported_requested_target_message, points at rocm examine instead) — but that's deliberate and well-documented/tested here (requested_target_error_does_not_claim_which_backend_serving_will_use), so just calling it out as intentional, not a problem.
Blocking:
- New GPU-aware behavior on the codex-bridge/MCP surface (
host_gpu_support_level, the new branch inbuiltin_codex_bridge_engine_inventory, and the"engines"MCP tool call site inchat.rs) has zero test coverage.append_examine_engine_inventory(rocm examine) got solid new tests for bothVulkanOnlyandRocmHip, but neitherrender_engine_inventory_text_with_paths(rocm engines) nor the codex-bridge path exercise the same branch directly — see inline comment.
Non-blocking:
- Stale doc reference in
host_gpu.rs— see inline comment. - The
lemonadenote-substitution logic (if *engine == "lemonade" { lemonade_engine_note(...) }) is now duplicated three times acrossrender_engine_inventory_text_with_paths,append_examine_engine_inventory, andbuiltin_codex_bridge_engine_inventory. Not blocking, but a shared helper would avoid the three call sites drifting independently later.
Ticket scope: Confirmed against ROCMAI-550 (epic ROCMAI-517) — the change matches the ticket's intent (stop asserting a static ROCm claim, report real support level instead). One review pass raised that docs/rocm-docs/, docs/ux-guidelines.md, doctor skill files, and --help text might still carry the old unconditional claim; a follow-up repo-wide grep came back clean on all of those. Flagging the discrepancy rather than silently picking one — worth a quick independent grep before merge if you want to be certain, but I did not find the stale string anywhere outside what's already fixed in this diff.
CI: The PR was just updated with a merge of main (resolving an earlier merge conflict cleanly — verified independently, chat.rs's call site already correctly passes host_gpu_support_level(Some(paths))). Most checks are still pending against the new merge commit as of this review; nothing has failed so far.
| /// `Unknown` deliberately covers both "nothing detected" and "a gfx target we | ||
| /// don't recognize" rather than being called `Unsupported` — see the doc | ||
| /// comment on [`gfx_target_has_no_rocm_build`] and | ||
| /// `GFX_TARGETS_WITHOUT_ROCM_SUPPORT` in `diagnose.rs` for why conflating |
There was a problem hiding this comment.
This doc comment points to GFX_TARGETS_WITHOUT_ROCM_SUPPORT in diagnose.rs, but that constant doesn't exist anywhere in the repo (checked via a full-repo grep). Either the name changed before this landed or it was never added — worth fixing so the doc doesn't send readers chasing a symbol that isn't there.
There was a problem hiding this comment.
Confirmed and fixed in bce633d6. GFX_TARGETS_WITHOUT_ROCM_SUPPORT appears exactly once in the repo — in this doc comment. diagnose.rs has no such constant (its consts are the KEYWORDS_* tables, MIN_SCORE_FOR_MATCH, SHM_MIN_BYTES, etc.), and nothing matching GFX_TARGETS* or *WITHOUT_ROCM* exists anywhere else either. It was never a real symbol, not a rename.
The sentence needed the second reference because gfx_target_has_no_rocm_build alone doesn't carry the "confirmed" half — that comes from its conjunction with normalize_therock_family. So rather than point at a different file, the doc now names the conjunct that actually establishes it:
Only that predicate's conjunction with [
normalize_therock_family] establishes the "confirmed" half; an unrecognized target satisfies neither conjunct, so it is a gap in what this catalog knows rather than a fact about the card.
Both links are now intra-module rustdoc links, so a future rename breaks the doc build instead of rotting silently.
| /// The same two inputs `rocm examine` derives it from, read through | ||
| /// `detect_host_gpu_summary` rather than the full examine gather — exactly what | ||
| /// `render_engine_inventory_text_with_paths` (i.e. `rocm engines`) already does. | ||
| fn host_gpu_support_level(paths: Option<&AppPaths>) -> rocm_core::GpuSupportLevel { |
There was a problem hiding this comment.
This function (and the new GPU-aware branch it feeds into builtin_codex_bridge_engine_inventory, plus the "engines" MCP tool call site in chat.rs) has no test coverage. append_examine_engine_inventory got examine_engine_inventory_names_vulkan_for_a_gpu_with_no_rocm_support / ..._keeps_rocm_note_for_a_rocm_capable_gpu for the equivalent rocm examine branch — this surface (and render_engine_inventory_text_with_paths's own rocm engines output) should get the same treatment before merge, since it's the same user-facing correction (no more asserting ROCm support on a Vulkan-only host) just reached through the MCP bridge instead of the CLI.
There was a problem hiding this comment.
You're right, and the gap was mine — I pushed 49e23e30 earlier today without tests. Fixed in bce633d6.
Both surfaces were untestable for the same reason: the branch's input is a live sysfs probe, and no CI runner has a GPU that is detected and confirmed to have no ROCm build. So each now takes the level the way append_examine_engine_inventory already did — the caller probes once, the renderer is handed the answer:
render_engine_inventory_text_for_host(paths, host_default_engine, level)— split out ofrender_engine_inventory_text_with_paths, which still does the onedetect_host_gpu_summarycall and derives both arguments from it, so the*marker and the note cannot disagree about the same host.internal_mcp_engines_tool_result(level)— split out of the"engines"match arm inchat.rs. Same text, sameengineskey, same entries.
Six new tests mirroring yours, plus three on the probe wiring:
| test | proves |
|---|---|
engines_inventory_names_vulkan_for_a_gpu_with_no_rocm_support |
rocm engines names the Vulkan backend, drops the ROCm claim, and leaves vLLM's row alone |
engines_inventory_keeps_rocm_note_for_a_rocm_capable_gpu |
a ROCm host keeps the ROCm wording and is not told it's on Vulkan |
engines_inventory_does_not_claim_vulkan_for_an_unrecognized_gpu |
Unknown is a detection gap, not a confirmed absence |
codex_bridge_engine_inventory_names_vulkan_… / …_keeps_rocm_note_… / …_does_not_claim_vulkan_… |
the same three on CodexBridgeEngine.summary, with installed_binary and vLLM's row asserted unchanged so the JSON shape is pinned too |
engines_mcp_tool_names_vulkan_… / …_keeps_rocm_note_… |
asserted on structuredContent.engines[].summary — the exact string an MCP client renders |
engines_mcp_tool_answers_for_the_host_it_runs_on |
the tool reaches the inventory through the live probe, not a level nailed to a constant |
gpu_support_level_for_host_gpu_reads_a_vulkan_only_summary_as_vulkan_only (+ two) |
covers host_gpu_support_level's derivation |
Falsified every one — five mutations on the devbox, each run confirmed non-zero test count:
engine_inventory_note'sengine == "lemonade"→false: 3 new tests red (engines_inventory_names_vulkan…,codex_bridge_…_names_vulkan…,engines_mcp_tool_names_vulkan…), plus yourexamine_engine_inventory_names_vulkan….lemonade_engine_note'sVulkanOnly =>→RocmHip =>: 6 new tests red — both directions on all three surfaces.VulkanOnly =>→Unknown =>: 4 new tests red, including both…_does_not_claim_vulkan_for_an_unrecognized_gpu, so theUnknownarm has teeth.gpu_support_level_for_host_gpuarguments transposed:gpu_support_level_for_host_gpu_reads_a_vulkan_only_summary_as_vulkan_onlyred. The twoOption<String>fields are positional and same-typed, so that mutation compiles clean and classifies exactly the Vulkan-only host asRocmHip— it was the one genuinely silent failure mode here."engines"arm hard-coded toGpuSupportLevel::VulkanOnly:engines_mcp_tool_answers_for_the_host_it_runs_onred.
On your duplication point: I did extract the helper — engine_inventory_note(engine, catalog_note, level) — and all three renderers now route through it. The tests deliberately drive the renderers and never that helper, per your #703 note; mutation 1 above is the proof that going through it is what they're actually checking.
One honest limitation: host_gpu_support_level(Some(paths)) at the call site is covered only by engines_mcp_tool_answers_for_the_host_it_runs_on, which can catch a hard-coded level that disagrees with the runner but not one that happens to render identically to it (Unknown and RocmHip share wording). The discriminating coverage for a real Vulkan-only host stays with the e2e lane.
Gates on the devbox, all green: cargo test --workspace --all-targets --no-fail-fast (rc 0, 35 suites, all 12 new tests present), cargo clippy --workspace --all-targets -- -D warnings (rc 0), cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings (rc 0), cargo fmt --all clean.
… and the bridge `append_examine_engine_inventory` got tests for both support levels when ROCMAI-550 landed; the other two surfaces carrying the same correction did not. `rocm engines` and the codex-bridge inventory (including its `engines` MCP tool) could have gone back to asserting ROCm support on a Vulkan-only host with nothing red. Both were untestable for the same reason: the branch's input is a live sysfs probe, and no CI runner has a GPU that is detected *and* confirmed to have no ROCm build. So each surface now takes the level the way `append_examine_engine_inventory` already did -- the caller probes once, the renderer is given the answer. `render_engine_inventory_text_for_host` and `internal_mcp_engines_tool_result` are those renderers; neither changes what is printed. The three copies of the lemonade substitution collapse into `engine_inventory_note`, so the surfaces cannot drift. The new tests drive the renderers, not that helper: asserting on it would restate the condition rather than prove a surface prints it. `gpu_support_level_for_host_gpu` reads the two `HostGpuSummary` fields once instead of at each call site. They are same-typed positional arguments, so a transposed call site would have classified a Vulkan-only host as `RocmHip` and still compiled; it now has a test. Also drops the dangling `GFX_TARGETS_WITHOUT_ROCM_SUPPORT` reference from the `GpuSupportLevel` doc comment -- no such constant exists in `diagnose.rs` or anywhere else, so the doc pointed readers at nothing. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Addressed the blocking item and both non-blocking ones in On your third point — the independent grep you asked for. I ran it and it agrees with yours: no stale unconditional claim survives outside this diff.
Specifically on the four places the earlier pass worried about:
So: nothing to fix, second pair of eyes agrees. Also leaving Re-requesting your review. |
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Re-reviewed at the current head (bce633d6) after the prior review (at daed78d5) was superseded by new commits.
Blocking
apps/rocm/src/therock.rs(unsupported_host_gpu_message): the new closing sentence always promises Lemonade's Vulkan fallback viarocm serve. Butselect_serve_enginelets a configureddefault_engineoverride (e.g.vllm) win regardless of host GPU support, so a user on an unsupported-ROCm GPU with such a config gets a message promising a Vulkan fallback thatrocm servewon't actually take. Please condition the message on the engine that will actually be selected, or soften it to not name a specific backend unconditionally.
Non-blocking
apps/rocm/src/main.rs(render_services_text/fetch_service_device): the live per-serviceHealthcheckRPC on eachrocm services listrender has no visible timeout bound. The in-code comment explains why this reads live instead of from the persistedManagedServiceRecord(avoiding staleness) — that part's fine — but worth confirming there's an upstream timeout inengine_request, otherwise a hung/slow third-party engine plugin could stall the whole listing.- The
gpu_support_levelderivation (rocm_core::gpu_support_level(detected_gfx_target, compatible_therock_family)) is duplicated verbatim betweenbuild_codex_bridge_snapshotandexamine_human_report; an analogous helper to the PR's owngpu_support_level_for_host_gpuwould avoid the duplicated positional-argument call. - Ticket scope (ROCMAI-550) nuance: the ticket's literal wording asks for a three-tier model ("full ROCm/HIP support, Vulkan only, or unsupported"), but this ships two confirmed tiers plus
Unknownfor detection gaps rather than a confirmed-unsupported verdict. Seems like a deliberate, defensible choice to avoid false-negative claims, but flagging since it's a wording deviation from the ticket. - The PR's description implies the stale "ROCm llama.cpp backend" claim has been fully purged; confirmed true in README/docs/rocm-doctor, but
apps/rocmd/src/common.rs:186(rocmd_engine_inventory) still hard-codes that exact phrase and is untouched by this PR. May be intentionally out of scope (rocmd answers for an arbitrary queried host, not necessarily the one it runs on) but it's a live remaining instance worth a look.
CI / mergeability
CI is green aside from the known Strix Halo Windows self-hosted download-infra flake (repo.amd.com 403) and its consolidated-report rollup failure — not a regression. Separately, the PR currently shows mergeable=CONFLICTING and will need a rebase/merge-conflict resolution regardless of this review.
| The GPU is detected but is not supported by ROCm; \ | ||
| the Vulkan backend can still be used." | ||
| the Vulkan backend can still be used. \ | ||
| Try `rocm serve <model>` - it runs on Lemonade's Vulkan backend instead." |
There was a problem hiding this comment.
This sentence is unconditional, but select_serve_engine can select a configured default_engine override (e.g. vllm) regardless of host GPU support. On an unsupported-ROCm GPU with such a config, rocm serve won't actually take the Vulkan/Lemonade path this message promises. Consider conditioning this on the engine that will actually be selected, or softening the wording.
There was a problem hiding this comment.
Confirmed and fixed in f54a8d2f — you and @nowycondro landed on the same defect from two directions, and you were both right. select_serve_engine (apps/rocm/src/serve_cmd.rs) ranks explicit --engine first, then configured_default, and only then the host preference, so a host that reaches unsupported_host_gpu_message can still have rocm serve <model> select vLLM.
I pinned the engine rather than conditioning the wording on config. apps/rocm/src/therock.rs:3607 now reads:
Try
rocm serve <model> --engine lemonade- it runs on Lemonade's Vulkan backend instead.
The reason for pinning over conditioning: resolve_family would have to read RocmCliConfig to condition, which adds a config load to an install-path error and still only covers the configured-default source — the recipe-preferred-engine source (which @copilot-pull-request-reviewer raised separately) would remain. An explicit --engine out-ranks every selection source in that function, so the sentence is true whatever the config and whatever the recipe says.
New test at apps/rocm/src/therock.rs:8399, unsupported_gpu_error_pins_the_engine_a_configured_default_could_override. It pins both halves — the engine-pinned command, and the absence of the bare one still carrying the promise. Falsified on the MI300X box by reverting the message to the bare command: that test and requested_target_error_does_not_claim_which_backend_serving_will_use both go red (the latter because it asserts the sibling's positive side).
| // actually reports right now. Only meaningful once the engine is ready | ||
| // to answer, same gate `rocm serve`'s summary uses. | ||
| if record.status == "ready" | ||
| && let Some(device) = fetch_service_device(paths, &record.engine, &record.service_id) |
There was a problem hiding this comment.
Non-blocking: this issues a live Healthcheck RPC per listed service with no visible timeout bound here. The comment above explains why it reads live rather than from the persisted record (good reasoning), but worth confirming engine_request enforces a timeout upstream — otherwise a slow/hung third-party engine plugin could stall the whole rocm services list render.
There was a problem hiding this comment.
Checked it, and the answer to "worth confirming engine_request enforces a timeout upstream" is no, it does not — so this needed a fix, not a confirmation. engine_request_with_env_root (apps/rocm/src/engine_ipc.rs) ends in a plain child.wait() with no deadline; there is no *_TIMEOUT anywhere in that module. I had half-written a doc comment claiming a timeout before I went and read it, which is a good argument for your asking.
What is bounded is the engine side, and only for the built-ins: rocm serve's three engines take the in-process fast path, and every endpoint probe they make is timeout-bounded (engines/vllm's HEALTHCHECK_TIMEOUT_MS, engines/lemonade's rocm_core::INFERENCE_PROBE_TIMEOUT). An external plugin is a spawned child and is genuinely unbounded.
Fixed in f54a8d2f by bounding the aggregate at the call site rather than per call — fetch_service_devices (engine_ipc.rs:365) under SERVICE_DEVICE_PROBE_BUDGET (:345, 10s). Once the budget is spent the remaining services simply get no device: row. render_services_text (services_text.rs:441) goes through it.
Two things I deliberately did not do, both stated in the doc comment rather than left implicit:
- Not parallelised.
engine_requestroutes the built-ins throughwith_scoped_builtin_engine_env, which takes a process-wide mutex and mutates process-globalROCM_CLI_*env vars for the duration. Concurrent probes would serialise on that lock anyway and racing them would be unsound. - The budget is checked between probes, not inside one. A single external plugin that never exits still blocks. Bounding that means a detached thread, and abandoning a worker that holds the env lock mid-probe would leak mutated process env into whatever ran next — worse than the problem. That is the pre-existing behaviour of every engine call site in the CLI and this does not make it worse.
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
Automated review (medium/high-effort pass). 3 findings posted inline, ranked by severity: (1) a correctness issue where unsupported_host_gpu_message's new "Try rocm serve <model>" sentence can be false when the user has a configured default_engine override; (2) a documentation/behavior mismatch where vLLM's actual device string (rocm_gpu) doesn't match the format the PR documents (rocm gpu 0/rocm gpu <n>); (3) a minor efficiency note that rocm services list now does live per-service network healthchecks where it previously only read local state. No other blocking issues found — the core GpuSupportLevel plumbing, the rocm serve/services list device-row gating, and the refactor of append_examine_engine_inventory/render_engine_inventory_text_for_host all look correct and consistently wired.
| The GPU is detected but is not supported by ROCm; \ | ||
| the Vulkan backend can still be used." | ||
| the Vulkan backend can still be used. \ | ||
| Try `rocm serve <model>` - it runs on Lemonade's Vulkan backend instead." |
There was a problem hiding this comment.
This sentence unconditionally promises that rocm serve <model> "runs on Lemonade's Vulkan backend", but this branch (unsupported_host_gpu_message) only confirms the host GPU has no ROCm build — it says nothing about which engine rocm serve will actually pick. Per select_serve_engine (apps/rocm/src/serve_cmd.rs:60-78), a configured_default_engine (set via rocm config set-default-engine, which is never validated against the host's GPU) outranks the host's engine preference. So a user who previously ran rocm config set-default-engine vllm (e.g. after moving a config from a ROCm box, or by mistake) would have rocm serve <model> attempt vllm on this Vulkan-only host and fail under the project's strict no-CPU-fallback policy — directly contradicting this message's promise.
Notably, the sibling message unsupported_requested_target_message in this same PR deliberately avoids making this exact claim (see its doc comment a few lines below) because it "never looked at this host" — but unsupported_host_gpu_message did look at the host's GPU, just not at the user's configured engine override, so the same class of false-promise risk applies here too. The new unit tests (therock.rs ~7677, ~7807) only pin the string "rocm serve <model>", not that running it actually succeeds via Lemonade under a configured-engine override.
Failure scenario: host is gfx90c (Vulkan-only, confirmed via detect_host_gfx_target), rocm_cli_config.default_engine == Some("vllm"). rocm install sdk (no --family) hits this branch and tells the user to "Try rocm serve <model>" promising Vulkan/Lemonade; rocm serve <model> instead selects vllm (configured default wins) and fails since there's no ROCm GPU.
There was a problem hiding this comment.
Confirmed and fixed in f54a8d2f. Your failure scenario is exact, including that rocm config set-default-engine never validates against the GPU — I checked select_serve_engine (apps/rocm/src/serve_cmd.rs) and configured_default does sit above the host-GPU preference, so rocm serve <model> on a gfx90c host with default_engine = "vllm" attempts vLLM and dies under the no-CPU-fallback policy while this message promised Lemonade.
apps/rocm/src/therock.rs:3607 now pins the engine:
Try
rocm serve <model> --engine lemonade- it runs on Lemonade's Vulkan backend instead.
An explicit --engine out-ranks every other selection source in that function, so this is true for every config and for a recipe whose preferred_engines would otherwise win — which closes the second half of the same hole that a config-conditional message would have left open.
Your point that the sibling message deliberately avoids the claim is why I did not just drop the sentence here: the asymmetry is real (this branch did look at the host) and worth keeping, it was only over-claiming the engine, not the backend. The doc comment above the function now says exactly that, and the new test unsupported_gpu_error_pins_the_engine_a_configured_default_could_override (therock.rs:8399) pins it against the configured-default regression specifically.
On your note that the old tests only pinned the string "rocm serve <model>": true, and that is why the new test asserts the negative too (no bare rocm serve <model> followed by the promise), not just the presence of the longer form — the longer form contains the shorter one as a prefix, so a presence-only assertion could not tell them apart.
| - The engine uses ROCm GPU execution. The deployment summary's `device:` row | ||
| reports what the engine actually selected, so check it by reading that row | ||
| rather than inferring it from the absence of a fallback warning: expect | ||
| `rocm gpu <n>`. `rocm services` shows the same `device` while it runs. |
There was a problem hiding this comment.
This tells the manual tester to expect rocm gpu <n> in the new device: row for a vLLM/ROCm run, matching the format documented in README.md:680 (rocm gpu 0) and the doc comment on DeploymentSummary.device (serve_summary.rs:59). But vLLM's own Healthcheck handler (engines/vllm/src/state.rs:60-66) hardcodes the device string as the literal constant "rocm_gpu" (underscore, no GPU index) whenever state.is_some():
let device = if state.is_some() {
"rocm_gpu"
} else {
"unknown"
};fetch_service_device (apps/rocm/src/main.rs) displays response.device verbatim with no reformatting. So for the vLLM engine — the only engine that reports a rocm* device — the actual printed line will read device: rocm_gpu, not device: rocm gpu 0 as this doc (and the README) describe. A tester following this doc literally would flag a false failure, or a reader would be misled about the real format. This is also the serve-05 vLLM canary the PR added device:-row e2e coverage for, and the PR's own test-plan notes it could not actually exercise that step live due to an unrelated environment gap — so this mismatch was never observed end-to-end.
There was a problem hiding this comment.
You are right about the mismatch and I fixed the code rather than the doc, in f54a8d2f. One correction to the attribution, which does not change the finding: the section at that line is ## 3. Lemonade GPU Verification (rocm serve qwen --engine lemonade ...), so the doc's own command is Lemonade, not vLLM. But your underlying claim is exactly right and the mismatch was real in two places:
engines/vllm/src/state.rshardcoded the literal"rocm_gpu", whichfetch_service_deviceprints verbatim. README.md'srocm gpu 0and this doc'srocm gpu <n>described a format vLLM never produced.engines/aim/src/state.rs(new onmainsince this PR opened) had the same literal.- Even for Lemonade,
reported_device(None, "rocm")returns a bare"rocm gpu", sorocm gpu <n>was not guaranteed there either.
What changed:
engines/vllm/src/state.rs:87gains areported_devicemirroring Lemonade's:"rocm gpu {index}", falling back to"rocm gpu".write_running_statenow persistsgpu_indices(introspection only, never read back to relaunch) so the ordinal is available. Test:reported_device_matches_the_shape_lemonade_reports(state.rs:759).engines/aim/src/state.rs:43reports"rocm gpu". No ordinal: AIM's state records nogpu_indices(the container gets the device list), so claiming one would be inventing it — stated in the comment rather than left implicit.docs/manual-testing.md:286-293now describes the real contract:rocm gpu <n>, or a barerocm gpuwhen no ordinal was pinned, and says the first word is the half that decides the check. README.md's example is now accurate for every engine.
Side effect worth flagging: main's every_advised_command_parses guard reads a backticked `rocm gpu 0` as advice to run a command. That gate was failing on this branch after the merge — six findings across README.md, docs/manual-testing.md and the two engines. They are now NOT_INVOCATIONS entries with reasons (apps/rocm/src/advised_commands.rs), which is the escape hatch that guard documents.
I could not verify the printed row on real hardware: the dev box has no amd-smi/rocm-smi and no usable GPU telemetry, so this is unit-level plus the strengthened e2e step, not an observed device: line.
| // actually reports right now. Only meaningful once the engine is ready | ||
| // to answer, same gate `rocm serve`'s summary uses. | ||
| if record.status == "ready" | ||
| && let Some(device) = fetch_service_device(paths, &record.engine, &record.service_id) |
There was a problem hiding this comment.
Minor efficiency/behavior-change note: before this PR, rocm services list (render_services_text) only read the locally persisted ManagedServiceRecords — pure local I/O. Now, for every record with status == "ready", it calls fetch_service_device, which issues a live engine Healthcheck RPC. For the Lemonade engine this RPC itself performs real HTTP requests to the running server's /v1/health and (on a miss) /v1/models endpoints (engines/lemonade/src/process.rs::query_loaded_model_endpoint), each with its own multi-second timeout, on every invocation (the latch only short-circuits the separate "has inference been verified" check, not this listed query).
So rocm services list --all with several ready services now makes that many live network round-trips to each server before printing anything, and a single slow/stalled/unresponsive server can noticeably delay what used to be an instant, purely-local listing command. This is probably an acceptable tradeoff for the new device: info, but worth confirming it's intentional — e.g. whether services list should cap total wait time or run these checks concurrently rather than sequentially per record.
There was a problem hiding this comment.
Confirmed and bounded in f54a8d2f. Your reading of the cost is right: render_services_text was pure local I/O, and the row made it one live Healthcheck RPC per ready record, sequentially, each with its own multi-second engine-side timeout. @juhovainio raised the same surface from the timeout angle, and the answer to his question turned out to be that engine_request enforces no deadline at all — engine_request_with_env_root ends in a bare child.wait().
What changed: fetch_service_devices (apps/rocm/src/engine_ipc.rs:365) runs the probes under one shared SERVICE_DEVICE_PROBE_BUDGET (:345, 10s). Once spent, the remaining services get no device: row rather than each adding its own wait. So rocm services list --all is now flat in the number of services instead of linear in it, and the row is informational so dropping it is strictly better than a long hang. services_text.rs:441 is the call site.
On your "run these concurrently" suggestion — I looked and it is not available: engine_request routes the three built-in engines through with_scoped_builtin_engine_env, which takes a process-wide mutex and mutates process-global ROCM_CLI_* env vars for the duration of the call. Concurrent probes would serialise on that lock anyway, and racing them would be unsound rather than merely useless. A cap was the option actually on the table, which is why I took it. The doc comment on fetch_service_devices says this so the next person does not re-derive it.
One honest limit, also in the doc comment: the budget is checked between probes, not inside one, so a single external plugin that never exits still blocks. Bounding that needs a detached thread, and abandoning a worker that holds the env lock mid-probe would leak mutated process env into the next operation. That is a worse trade than the one it fixes, and it is pre-existing behaviour of every engine call site in the CLI.
|
🤖 Automated review (Claude Code) · bce633d This supplements my review posted above (PR review #5655784868) with the required format. Findings (COMMENT only, non-blocking severity noted per item):
What I read: the full diff for this PR at bce633d, plus the surrounding non-diff context for What I could not verify: no Rust toolchain was available in this sandbox, so none of this was confirmed by No other correctness issues found beyond what's listed above and what's already been raised in the existing review thread. 🤖 Generated with Claude Code |
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · bce633d
Review
Full independent review of the whole PR diff (base d4e7c75 → head bce633d6, 14 files, +856/-53), using a two-worker fan-out plus direct source verification of every surviving finding. Outcome: needs work.
Intent: implements most of ROCMAI-550 ("clear messages for AMD GPUs that ROCm doesn't support but Vulkan does") — adds a GpuSupportLevel classification, GPU-aware Lemonade engine wording in rocm examine/rocm engines/the codex-bridge inventory, an install-sdk remediation sentence, and live backend/device reporting in rocm serve's summary and rocm services list. The ticket is still "In Progress" and the PR does not claim to close it; its own scope note says part of the ticket (gfx90c detection/rocm install sdk wording) was already covered by a prior PR (#648), which is consistent with what the ticket itself asks for.
Blocking
-
GpuSupportLevel::Unknownstill falls through to the stale "ROCm llama.cpp backend" claim —apps/rocm/src/main.rs:2831-2838(lemonade_engine_note): the match has an explicitVulkanOnlyarm and a catch-all_arm returning"default embedded Lemonade server with ROCm llama.cpp backend"._catches bothRocmHip(correct) andUnknown(wrong) — and perhost_gpu.rs,Unknowncovers both an unrecognized gfx target and no AMD GPU detected at all. So on a host with no AMD GPU,rocm examine,rocm engines, and the codex-bridge/MCP "engines" tool all still unconditionally print the exact claim this PR (ROCMAI-550) exists to remove. The one test covering this case,engines_inventory_does_not_claim_vulkan_for_an_unrecognized_gpu, only asserts the negative (!output.contains("no ROCm/HIP support")) and never asserts the positive ROCm claim is absent, so this passes CI undetected.
Confidence 90 · logic · Fix: add an explicitGpuSupportLevel::Unknownarm with wording that doesn't assert either backend (e.g. "GPU support not yet determined"), and extend the existing test to assert the ROCm string is also absent. -
unsupported_host_gpu_message's new remediation sentence can be false under a configured engine override —apps/rocm/src/therock.rs:3463: unconditionally appends"Try \rocm serve ` - it runs on Lemonade's Vulkan backend instead."Butselect_serve_engine(apps/rocm/src/serve_cmd.rs:60-109) checks a configureddefault_engine*before* any host-GPU preference (lines 73-78 return early on a non-empty configured value; the host-GPU branch at line 80 is only reached if that's unset). So a user on an unsupported-ROCm-GPU host with e.g.default_engine = "vllm"configured gets a message promising a Vulkan/Lemonade fallback thatrocm servewill not actually select. The sibling message (unsupported_requested_target_message) had this exact class of bug fixed on this same branch (commit16ccfb3) with the stated reasoning "only reached once host detection itself confirmed no ROCm build" — that reasoning holds for host detection, but doesn't account forselect_serve_engine's config-override precedence, so the asymmetry fix was incomplete. Confidence 85 · logic · Fix: condition the sentence on whatselect_serve_engine` will actually pick (or soften it to not name a specific backend unconditionally, the way its sibling message was changed to do). -
Two CI checks are currently failing:
E2E tests (Strix Halo, Windows, current)andE2E consolidated report (self-hosted)(re-verified live against headbce633d6immediately before this review). This review could not fetch the job logs (network egress to the Actions API was blocked in this review environment), so the root cause is not independently confirmed from here — the job names suggest a Windows self-hosted hardware lane and its rollup, which has been attributed elsewhere to download-infrastructure flakiness unrelated to this diff, but that should be reconfirmed by someone who can read the logs rather than assumed clean.
Confidence 100 (checks are red, verified live) · process · Fix: confirm cause, re-run if infra-flake, or fix if real.
Non-blocking
-
rocm services list --json(and the MCPservicestool) never gain the newdevicefield —ManagedServiceRecord(crates/rocm-core/src/service_record.rs:29) is serialized verbatim for JSON (render_services_json,apps/rocm/src/main.rs:6834) and carries nodevicefield; only the text renderer calls the newfetch_service_device. A scripting/MCP consumer gets strictly less information than a human reading text output now does.
Confidence 85 · architectural · Fix: either adddeviceto the JSON record (live-fetched, not persisted) or document the asymmetry. -
No test exercises the positive render path of the new
device:line, and the one e2e assertion that does touch it is weak —tests/e2e-cucumber/tests/e2e/serving_steps.rs'sassert_services_list_reports_deviceonly checks that a line starting withdevice:exists; it never checks the value. This is currently safe only becausefetch_service_device(apps/rocm/src/main.rs:10323) already filters out empty/"unknown"values before the CLI ever writes the label — an invariant the test itself doesn't pin, so if that filter weakened, this step wouldn't catch it. On the unit side,serve_summary.rs'sdevice_row_shown_only_when_knowntest only covers "shown when known" / "absent by default" — nothing seeds a real ready-state Healthcheck response end-to-end and asserts the exact rendered value.
Confidence 75 · test-coverage · Fix: add a test that seeds a concrete device string and asserts it appears verbatim. -
vLLM's actual
devicestring doesn't match the documented format —engines/vllm/src/state.rs:64hardcodes the literal"rocm_gpu"(no GPU index, underscore not space), while this PR's own doc comment (serve_summary.rs), README.md, and docs/manual-testing.md all document the shape as"rocm gpu 0"/"rocm gpu <n>"(matching Lemonade's{backend} gpu {index}format inengines/lemonade/src/state.rs:60-64). Not a functional bug — the string is still truthful — but this data was never user-facing before this PR, and now it is, inconsistently.
Confidence 80 · mechanical · Fix: either have vLLM's healthcheck report an index-bearing device string to match, or adjust the documented examples to not imply one exists for vLLM. -
gpu_support_level(...)is called with raw positional args, duplicated at two sites —build_codex_bridge_snapshot(apps/rocm/src/main.rs:2715) andexamine_human_report(apps/rocm/src/main.rs:~5097) both callrocm_core::gpu_support_level(examine.detected_gfx_target.as_deref(), examine.compatible_therock_family.as_deref())directly instead of through a single-call helper analogous to the PR's owngpu_support_level_for_host_gpu. Both arguments areOption<&str>, so a future transposition at either site would compile and silently misclassify.
Confidence 75 · mechanical · Fix: add anExamineSummary::gpu_support_level(&self)(or similar) and route both call sites through it. -
apps/rocmd/src/common.rs:186(rocmd_engine_inventory) still hardcodes the unconditional claim — "embedded Lemonade server with ROCm llama.cpp backend", with noGpuSupportLevelparameter and no comment in this file explaining the exemption. The only stated rationale for excludingrocmdlives in a doc comment on a different function in a different file ("the still-host-blind consumers ... keep the static row"), but README.md describesrocmdas "the background telemetry daemon used by the dashboard" that's auto-started locally — which is in tension with treating it as answering for an arbitrary, not-necessarily-local host. May be intentional, but it's unverified from the code as it stands.
Confidence 70 · logic · Fix: add a comment at the rocmd call site itself stating the rationale, or make it GPU-aware too if it in fact usually runs on the host it describes. -
rocm services list's text render now makes a live, synchronous, unparallelized RPC per listed service (fetch_service_device→engine_request,apps/rocm/src/main.rs:6730-6804) where it previously only read local disk state. Each call is individually timeout-bounded, so it won't hang indefinitely, but total latency now scales linearly with the number of concurrently-ready services being listed.
Confidence 70 · architectural · Fix: consider parallelizing the per-service fetch, or note the latency tradeoff is accepted.
Decisions for the author
GpuSupportLevelships three tiers (RocmHip/VulkanOnly/Unknown) rather than the ticket's literal "full ROCm/HIP support, Vulkan only, or unsupported" — tradeoff. Collapsing "unrecognized gfx target" and "no GPU detected" intoUnknowninstead of a confirmedUnsupportedavoids a false-negative claim on a detection gap, and is well-documented inhost_gpu.rs's doc comments with a dedicated test guarding the distinction. Worth the author explicitly confirming this reading of the ticket's "unsupported" tier is the intended scope, since it is a visible wording deviation from the ticket text.
Positive signals
gpu_support_level/gpu_support_level_for_host_gpu(crates/rocm-core/src/host_gpu.rs) are genuinely a single source of truth with a dedicated test (gpu_support_level_for_host_gpu_reads_a_vulkan_only_summary_as_vulkan_only) specifically guarding against the two positionalOption<&str>arguments being silently transposed.lemonade_engine_note(apps/rocm/src/main.rs:2831) is a single shared helper reused by all three GPU-aware call sites (rocm examine,rocm engines, codex-bridge) rather than duplicated inline per site, so the three renderers can't independently drift apart on the parts it does cover.serve_summary.rs'sredacted()test helper (masksapi key: ...before interpolating a rendered summary into a panic message) is a thoughtful, specific guard against leaking secrets into retained CI logs.- The PR description's test-plan section is unusually candid about what wasn't verified end-to-end (both GPU canaries failing locally for disclosed, pre-existing environment reasons) rather than silently claiming full coverage.
Deployment notes
- The PR currently shows
mergeable: CONFLICTING/mergeStateStatus: DIRTYagainst currentmain(re-verified live immediately before posting this review) — it needs a rebase/conflict resolution before it can be merged, independent of the findings above. - This surfaces a live per-service network RPC inside
rocm services listandrocm serve's summary that did not exist before (see the latency non-blocking item above); operators scripting against these commands at scale should be aware the command's cost profile changed.
What this covered
Read the complete diff (base d4e7c75194d1643eacc3b1bda33afe10766def73 → head bce633d6e143d72ee8a147bb189f7dbbd72552f0, all 14 changed files) directly for crates/rocm-core/src/host_gpu.rs, crates/rocm-core/src/lib.rs, apps/rocm/src/serve_cmd.rs, apps/rocm/src/serve_summary.rs, apps/rocm/src/therock.rs, README.md, and docs/manual-testing.md, including the functions each diff touches, their callers, and surrounding tests. apps/rocm/src/main.rs, apps/rocm/src/chat.rs, apps/rocm/src/providers.rs, and the four e2e test files were covered by two independent fan-out workers; every finding either worker reported above was independently re-derived and confirmed against the source by this review before inclusion (exact line reads, not taken on the workers' word). Also independently grepped the whole repository tree at head for the literal phrase "ROCm llama.cpp backend" to check for other remaining stale-claim sites. Checked live CI status and PR mergeability twice (once mid-review, once immediately before posting); could not fetch Actions job logs (network egress to the Actions API is blocked in this review sandbox) so the two failing checks' root cause is reported but not independently confirmed. Did not run cargo test/cargo clippy locally (no Rust build performed in this review session) — all findings are from static reading of the diff and surrounding code, not execution. Retrieved the named ticket (ROCMAI-550) directly from the tracker to check the description's claims against it, rather than from the PR body alone.
This review's own live-state check (required before reviewing, to avoid duplicating prior coverage) necessarily surfaced the existing review thread on this PR as a side effect of querying gh pr view --json reviews; this review did not treat any of that thread's conclusions as given — every finding reported above was re-derived from the source code directly (function bodies, call sites, and tests read and traced independently) before being included. Not reconciled against that thread beyond that.
Resolves the conflicts with main's `main.rs` modularization (Phase 6f,
ROCMAI-208/213/214/215) and addresses the outstanding review findings in
the same pass, because several of the fixes land in code this merge had
to relocate.
Conflict resolution. Every large hunk had an empty `origin/main` side:
main moved the code out of `main.rs`, it did not delete it. The HEAD
block was dropped and this branch's additions re-applied in the module
main moved them to.
- `examine_human_report`, `render_engine_inventory_text_with_paths` /
`_for_host` and `append_examine_engine_inventory` ->
`examine_report_text.rs`, with the seven `append_examine_engine_inventory`
test call sites and the five relocated inventory tests.
- `render_services_text`'s `device:` row -> `services_text.rs`, with the
`!rendered.contains("device:")` assertion that pins the `"unknown"` case.
- `fetch_service_device` -> `engine_ipc.rs`, beside the `engine_request`
it calls; `serve_cmd.rs` imports it from there.
- The codex-bridge inventory stayed in `main.rs`, so its three tests
went back into `main.rs`'s test module.
All 21 test functions this branch adds are present and run; `engine_inventory()`
now carries a third engine (`aim`) from main and the renderers handle it
unchanged. No scenario ids were added by this branch (it only appends
`Then` steps to serve-05, serve-07 and runtime-19), so nothing needed
renumbering.
Review findings.
- `unsupported_host_gpu_message` pins `--engine lemonade` instead of
suggesting a bare `rocm serve <model>`: a configured `default_engine`
out-ranks the host preference in `select_serve_engine`, so the bare
command could attempt vLLM on a GPU with no ROCm build.
- `lemonade_engine_note` now matches `GpuSupportLevel` exhaustively.
`Unknown` no longer inherits the ROCm claim through a catch-all; it
asserts no backend and points at `rocm examine`.
- `fetch_service_device` hands the engine the service's 0600 key file via
`ROCM_SERVE_API_KEY_FILE`, so a public / `--require-api-key` service
does not 401 its own healthcheck and lose the row.
- `rocm services list` probes under one aggregate budget
(`fetch_service_devices`), so the row cannot turn a local-only render
into an unbounded wait.
- `engines/vllm` and `engines/aim` report the device in the same
`<backend> gpu [<n>]` shape `engines/lemonade` does, instead of the
literal `"rocm_gpu"` that README.md and `docs/manual-testing.md` never
described.
- The `services list` e2e step parses the device value, checks it against
the engine that served, and cross-checks it with the `rocm serve`
deployment summary.
Verified on the Linux MI300X box: `cargo fmt --all --check`, `cargo
clippy --workspace --all-targets -- -D warnings`, `cargo clippy --locked
-p e2e-cucumber --test e2e -- -D warnings` and `cargo test --workspace
--all-targets --no-fail-fast` all rc 0, 4823 tests passed / 0 failed.
Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Merged
|
| this branch's content | new home |
|---|---|
examine_human_report's gpu_support_level derivation, render_engine_inventory_text_with_paths → _for_host split, append_examine_engine_inventory's new parameter, both engine_inventory_note call sites |
apps/rocm/src/examine_report_text.rs |
render_services_text's device: row |
apps/rocm/src/services_text.rs |
fetch_service_device |
apps/rocm/src/engine_ipc.rs (beside the engine_request it calls); serve_cmd.rs imports it from there |
builtin_codex_bridge_engine_inventory, engine_inventory_note, lemonade_engine_note, host_gpu_support_level |
stayed in main.rs (main did not move them) |
Tests were the thing most at risk here, since three of the dropped blocks were test modules. Relocated and verified present by name in the final tree, all 21 of them:
examine_report_text.rs— the sevenappend_examine_engine_inventorycall sites, plusexamine_engine_inventory_names_vulkan_…/…_keeps_rocm_note_…and the threeengines_inventory_….services_text.rs— the!rendered.contains("device:")assertion insiderender_services_text_lists_live_services_by_default_and_all_on_request, which pins that an engine reporting"unknown"prints no row.main.rs— the threecodex_bridge_engine_inventory_….chat.rs,serve_summary.rs,crates/rocm-core/src/host_gpu.rs,therock.rsauto-merged; their tests were checked by name too.
Other merge notes: engine_inventory() now carries a third engine (aim) from main, which the renderers handle unchanged. No scenario renumbering was needed — this branch adds no scenarios, only Then steps to the existing serve-05, serve-07 and runtime-19. I checked every feature file for duplicate ids against origin/main: none.
Findings addressed
The engine promise was wrong (@juhovainio, @nowycondro, @copilot-pull-request-reviewer — three threads, one defect). unsupported_host_gpu_message promised rocm serve <model> runs on Lemonade/Vulkan, but select_serve_engine ranks a configured default_engine above the host preference, and rocm config set-default-engine never validates against the GPU. apps/rocm/src/therock.rs:3607 now pins the engine — rocm serve <model> --engine lemonade — which out-ranks every selection source, so the sentence holds for any config and any recipe. New test therock.rs:8399 pins the pinned form and the absence of the bare one (the bare command is a prefix of the pinned one).
Unknown still asserted ROCm support (@copilot-pull-request-reviewer). lemonade_engine_note (main.rs:2748) now matches GpuSupportLevel exhaustively instead of folding Unknown into a _ => ROCm arm. Unknown claims no backend and points at rocm examine; it does not get the Vulkan wording either, because a detection gap is not a confirmed absence. Both Unknown tests now assert all three facts.
The healthcheck was unauthenticated (@copilot-pull-request-reviewer). Verified the mechanism: the engines read ROCM_SERVE_API_KEY_FILE for their own /v1/models probe, so a public / --require-api-key service 401'd its own healthcheck and the row vanished for exactly the protected services. engine_request_with_scoped_env (engine_ipc.rs:152) now threads the service's 0600 key file — inside the same BUILTIN_ENGINE_ENV_LOCK critical section for the built-in path, on the child Command for an external plugin.
services list cost and timeouts (@juhovainio, @nowycondro). The answer to "does engine_request enforce a timeout upstream" is no — it ends in a bare child.wait(). fetch_service_devices (engine_ipc.rs:365) now runs the probes under one SERVICE_DEVICE_PROBE_BUDGET (10s), so the cost is flat in the number of services instead of linear. Not parallelised, and the reason is in the doc comment: with_scoped_builtin_engine_env takes a process-wide mutex and mutates process-global env, so concurrent probes would serialise anyway and racing them would be unsound. The budget is checked between probes, not inside one — a hung external plugin still blocks, which is pre-existing CLI-wide behaviour and is stated rather than implied.
The documented device: format did not exist (@nowycondro). engines/vllm hardcoded the literal "rocm_gpu" and engines/aim (new on main) did the same, while README.md and docs/manual-testing.md described rocm gpu <n>. Fixed on the code side: vLLM now reports rocm gpu <n> (persisting gpu_indices for the ordinal) and AIM reports rocm gpu, both matching Lemonade's shape; the docs now also describe the ordinal-less form, which Lemonade could already produce. One attribution correction in that thread: the section is the Lemonade one, not vLLM — the finding stands regardless.
The e2e assertions had no teeth (@copilot-pull-request-reviewer, two threads). assert_services_list_reports_device now parses the value (<backend> gpu [<n>], nothing trailing), checks the backend against the engine that served (vulkan from vllm/aim fails; unknown fails everywhere), and cross-checks it against the rocm serve deployment summary via a new E2eWorld::serve_summary. The summary check is two-sided — equal if present, and if absent the summary must not have said ready — because a managed launch can legitimately return before the weights load.
Judged already handled, not silently skipped: the two Gherkin-coverage threads. A scenario for the Vulkan-only branch needs a Renoir/Cezanne Vega APU on a runner and detect_host_gfx_target has no test override, so it would be permanently skipped — the reasoning is in the PR description and restated in each thread. runtime-19 asserts the half a lane can reach (the negative).
Failing checks
Both were on the stale 2026-10-17 run and neither is this PR's:
- E2E tests (Strix Halo, Windows, current) —
repo.amd.comreturned HTTP 403 fortherock-dist-windows-gfx1151-7.14.1.tar.gzafter 6 attempts, so the Lemonade backend install failed androcm engines installexited 1. Transient upstream artifact availability.main's own run on11a60691(today) has this lane green. - E2E consolidated report (self-hosted) — strictly downstream of the above:
lane e2e-gpu-strix-windows ran (needs result: failure) but uploaded no e2e-gpu-strix-windows-report. The lane died before writing a results dir, so the guard that every lane contributes an artifact fired. It appears "only on this PR" because only this PR had the Windows lane fail; there is nothing to fix here, and a fresh run on this head should decide it.
Verification — Linux MI300X box, exit codes captured
Darwin cannot build this workspace (engines/lemonade: st_mode is u16 there), so everything was run remotely against the exact merged tree (git diff origin/main..HEAD applied onto origin/main; the patch sha256 matches the pushed commit byte for byte).
cargo build --workspace --all-targets rc 0
cargo fmt --all --check rc 0
cargo clippy --workspace --all-targets -- -D warnings rc 0
cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings rc 0
cargo test --workspace --all-targets --no-fail-fast rc 0 4823 passed / 0 failed
All 21 of this branch's test functions were confirmed ... ok by name in that log, including every relocated one.
Falsification, on the box, each run confirming a non-zero test count:
lemonade_engine_note'sUnknownarm restored to the ROCm string →engines_inventory_does_not_claim_vulkan_for_an_unrecognized_gpuandcodex_bridge_engine_inventory_does_not_claim_vulkan_for_an_unrecognized_gpured.unsupported_host_gpu_messagereverted to the barerocm serve <model>→unsupported_gpu_error_pins_the_engine_a_configured_default_could_overrideandrequested_target_error_does_not_claim_which_backend_serving_will_usered.
Both restored, all green again.
Also caught by the merge: main's new every_advised_command_parses guard reads a backticked `rocm gpu 0` as advice to run a command, and was failing on this branch with six findings. They are now NOT_INVOCATIONS entries with reasons in apps/rocm/src/advised_commands.rs — the escape hatch that guard documents.
Push hygiene: pushed with SKIP=cargo-clippy,cargo-test because those two pre-push hooks cannot compile on Darwin. That is legitimate only because both were really run on the Linux box first, with the results above. Nothing else was skipped — verify-commits (signature + DCO), license-headers, cargo fmt, cargo clippy (e2e harness + steps), lychee and the manifest/catalog/TPN hooks all ran and passed locally. No --no-verify, no force-push.
What I could not verify
The dev box has no amd-smi/rocm-smi and no usable GPU telemetry despite KFD nodes, so none of the real-hardware reporting this PR is about was observed there: no live device: row, no Vulkan-only host exercising the VulkanOnly branch, and no protected service proving the API-key fix turns a 401 into a device row. Those remain unit-level plus the strengthened e2e steps, which is why the e2e assertions got the attention they did in this round.
… step failing a correct run Review round 3 on #678. - The `device:` e2e step failed a legitimately correct run. Lemonade reports `unknown` until its own inference probe returns, and the serve summary reads the device once right after `ready`, so the row can be absent on a correct implementation. Only serve-07 being xfail'd on lemonade+linux hid it. The step now tolerates the absence and the window is documented rather than asserted away. - `rocm examine` grew a `rocm_support` line for RocmHip and Unknown with no scenario asserting it. examine-31 covers it, deliberately untagged so each lane's own hardware is the coverage: the claim is cross-checked against `detected_gfx_target` and `compatible_therock_family` from the same report, so "full ROCm/HIP support" on the mock lane or "not supported by ROCm" on an MI300X both fail. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Two conflicts, both from main's ongoing module splits. `examine_report_text.rs`: main replaced the hand-written marker/`writeln!` pair with `write_marked_row`. Took main's call shape and kept this branch's `engine_inventory_note(..., gpu_support_level)`, which is the whole point of the row here. `therock.rs`: main moved the family-resolution cluster into `therock/family.rs`, so both conflict hunks had an empty main side — the code moved rather than went away. This branch's three contributions moved with it: the `--engine lemonade` pin on `unsupported_host_gpu_message`, the `rocm examine` pointer on `unsupported_requested_target_message`, and the two tests pinning the asymmetry between them (plus the extra assertion on `unsupported_gpu_error_names_the_gpu_and_does_not_suggest_families`). Note on numbering: this branch's new `examine-31` is correct for its own file position, and so is #447's. They collide only when the second of the two merges, which is when the loser renumbers to examine-32 — renumbering either now would fail `feature_naming` on its own branch. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · cabe979
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.
Review — no blocking findings
Full review of the whole change (base c39ee0c6d...head cabe9795b, 24 files, +1832/-90).
Implements the ROCMAI-550 scope the PR body states is left after #648 landed the gfx90c detection/refusal-wording half; not the whole ticket (the ticket's three-tier ask is covered by GpuSupportLevel, deliberately keeping Unknown rather than a hard "unsupported" — a documented, reasoned design choice, not an omission).
Blocking
None
Non-blocking
main.rsdoc comment cites the wrong file for the one deliberately-static engine catalog —apps/rocm/src/main.rs:2775(inlemonade_engine_note's doc comment)
The comment says the dashboard pane and "rocmd's own catalog" live in "apps/rocmd/src/common.rs". That file exists (102 lines) but contains no engine-inventory/lemonade content at all; the actual static catalog with the literal"with ROCm llama.cpp backend"is inapps/rocmd/src/host_snapshot.rs:159. Verified directly:git show pr/678:apps/rocmd/src/common.rs | grep -c "ROCm llama"→ 0; same againsthost_snapshot.rs→ 1. A reader following this citation to confirm the PR's scope boundary (which files intentionally keep the old wording) lands in the wrong file.
Confidence 100 · mechanical · Fix: change the citation toapps/rocmd/src/host_snapshot.rs.gpu_support_level'sRocmHiparm ignores whether a GPU was even detected, so the type permits a state that would render a self-contradictory message —crates/rocm-core/src/host_gpu.rs:157-165(match),:1980-1984(rendering)
match (detected_gfx_target, compatible_therock_family) { (_, Some(_)) => RocmHip, ... }discards the first element entirely. Ifcompatible_therock_familywere everSome(_)whiledetected_gfx_targetisNone,rocm_support_line()'sRocmHiparm renders"GPU detected (<unknown>) with full ROCm/HIP support"— asserting detection while admitting the identity is unknown, the exact shape of false claim this PR exists to eliminate. This is not reachable today: the single production constructor (ExamineSummary::gather_scoped,host_gpu.rs:1896) always derivescompatible_therock_familyasdetected_gfx_target.as_deref().and_then(normalize_therock_family), so the invariant holds by convention at that one call site, not by the type (ExamineSummary's fields are allpub, and a pre-existing, unrelated test fixture athost_gpu.rs:5076already hand-constructs the otherwise-"impossible" combination for a different assertion, proving the type allows it). The parallelVulkanOnlyarm has a comment acknowledging its own unreachable fallback; this arm's identically-shapedunwrap_or("<unknown>")fallback has no such comment, suggesting the case wasn't considered.
Confidence 70 · logic/type-design · Fix: match(Some(_), Some(_)) => RocmHipand fold(None, Some(_))intoUnknown(or bundle the two facts into a type that can only be constructed together), so the invariant is enforced rather than merely convention.
Decisions for the author
rocm services list(and the chatservicestool) is no longer a pure local read; probes are sequential, not parallel — tradeoff
A liveHealthcheckRPC per ready service is now made to fill thedevice:row, capped in aggregate at 10s (documented worst case ~18s with the built-in engines' own timeouts) rather than per-probe, because the built-in-engine call path takes a process-wide mutex and mutates process-wide env vars for its duration, making concurrent probes unsound. This is unusually well reasoned and tested in-code (injected clock/probe tests pin the exhaustion branch precisely), but it is still a new, growing-with-engine-count latency floor on a command that used to be instant. Confirm the ~18s worst case is acceptable for every caller ofrender_services_text(CLI and chat tool alike) as more engines/services are added.- No compile-time guarantee ties a future 4th engine to the shared
format_reported_device— non-blocking-improvement
Today all three engines (aim, lemonade, vllm) route their device string throughrocm_engine_protocol::format_reported_device, consistently. Nothing stops a future engine crate from hand-rolling its own device string again (the exact drift this PR fixes — vLLM/AIM previously emitted bare"rocm_gpu"). A shared trait or constructor that forces the formatter's use would make the old mistake a compile error rather than a convention. unsupported_host_gpu_message's remediation pins--engine lemonadeas a hand-written literal, not derived fromselect_serve_engine's actual ranking — non-blocking-improvement
The doc comment and a dedicated regression test (unsupported_gpu_error_pins_the_engine_a_configured_default_could_override) correctly explain and guard why the bare command would be unsafe under a configureddefault_engine, but the message text and the engine-selection ranking logic remain two independently-maintained sources of truth rather than one computed from the other (AGENTS.md §3's own stated preference for the latter). Not blocking because the regression is actively tested today.- The dashboard TUI engine-manager pane and
rocmd's own catalog keep the old unconditional "with ROCm llama.cpp backend" wording indefinitely — tradeoff
Explicitly scoped out and justified in-code (no probe behind the TUI pane;rocmdanswers for an arbitrary remote host it doesn't necessarily run on) rather than an oversight. Confirm this is the accepted long-term boundary for ROCMAI-550 and not a gap expected to close soon — these two surfaces will keep making the exact claim the other three were just corrected for.
Positive signals
- Every match over the new
GpuSupportLevelin consuming code (rocm_support_line,lemonade_engine_note) is exhaustive with no wildcard arm, so a future fourth variant is a compile error rather than a silent fold into the wrong tier — exactly the bug class the PR's own history (visible in this branch's commit messages) shows was caught and fixed mid-review once already. - New/changed tests consistently assert both the new wording's presence and the old/wrong wording's absence (e.g. the three codex-bridge inventory tests, the two
therock.rsrefusal-message tests), which is the stronger bar: a reverted substitution or a swapped match arm would fail these, not just produce an uninformative pass. SERVICE_DEVICE_PROBE_BUDGET's exhaustion branch is tested via an injected clock and probe rather than a real 10-second sleep, and the test explicitly asserts the later services were never probed at all (not merely unreported) — the harder and more correct half of that claim.- The e2e step for the new
device:row (assert_services_list_reports_device) cross-checks two independent surfaces (therocm servesummary androcm services list) for the same value rather than asserting presence alone, polls for the documented "not ready yet" window instead of failing on first look, and constrains the allowed backend per engine (vulkanis impossible fromvllm/aim). - The two "install sdk" refusal messages' subtle cross-branch requirement (only the detected-host door may name a backend; the requested-target door must not) is precisely pinned by tests in both directions, including the negative assertion, which is the actual regression a prior review round caught.
Deployment notes
rocm services listand the chatservicestool now make live IPC calls as part of rendering and are no longer pure local-state reads; worst-case wall time is documented at ~18s with the built-in engines, flat in the number of services (not growing per-service beyond the shared budget).- No data migration needed:
gpu_nameis additive to engine state files and explicitly tolerant of legacy files that lack the key (tested).
What this covered
Every file in the diff read completely, grouped by coupling: (1) crates/rocm-core/src/host_gpu.rs + lib.rs + crates/rocm-engine-protocol/src/lib.rs; (2) apps/rocm/src/examine_report_text.rs + chat.rs; (3) apps/rocm/src/serve_cmd.rs + serve_summary.rs + services_text.rs + engine_ipc.rs + advised_commands.rs; (4) apps/rocm/src/therock.rs + main.rs + providers.rs; (5) engines/{aim,lemonade,vllm}/src/state.rs + engines/lemonade/src/device_policy.rs; (6) the e2e feature/step files + README.md + docs/manual-testing.md — plus the surrounding/caller code and relevant git history for each (c39ee0c6d…cabe9795b). Targeted repo-wide greps closed the loop on "exactly 3 call sites corrected, 2 deliberately left static" and on whether any fixture elsewhere still encoded the pre-PR unconditional wording (two fixtures do, both verified to represent a ROCm-capable host, where that wording is still correct).
Fan-out was partially degraded. Groups (1) and (2) above ran as independent background subagents. The shared subagent pool was saturated (a harness-level capacity limit, not specific to any file or worker) for groups (3)-(6) and for the two source-reading passes (agent-instruction adherence against AGENTS.md; history/blame); one retry per failure hit the same limit, so — per the method's fallback — I ran those six passes myself in the driver context rather than dropping them, and they are not independent of each other or of my own cross-checking of groups (1)/(2)'s findings. Design-review (Step 7) questions were reasoned through directly rather than fanned out to dedicated workers.
CI was read, not re-derived: all 25 checks green at cabe9795 (clippy, full workspace test suite, e2e, CodeQL, the ASCII-output-contract and markdown-link jobs among them).
Blind-exposure disclosure: retrieving basic PR metadata (head/base SHA, title) incidentally returned the full PR description before Step 9, since the GitHub API does not separate them. The description is unusually detailed, including prior-review framing, specific file/line citations, and stated design rationale. None of it was used to seed or shape the findings above — both citation/reachability claims quoted from it were independently re-verified against the code (the common.rs claim turned out wrong per the first finding above; the "exactly 3 call sites" and "4178 tests passed" claims were corroborated via grep and the live CI run respectively, not taken on faith). It was deliberately read and weighed only at Step 9, below.
Step 9: the description matches the diff — every itemized test-plan claim I spot-checked (the three-tier unit tests, the per-surface engine-note tests, the two refusal-message tests, the two e2e scenario additions, the two stated test-plan gaps) is actually present in the code, and the two explicitly-stated coverage gaps (the detected-host refusal door; the lemonade e2e device step on a GPU-less sandbox) are reasoned and match what the code structure shows (no override exists to drive detect_host_gfx_target in a test). No blocking finding survived to retire against it.
The one mechanical listing of existing reviews (to find the current HEAD's state and avoid double-posting) showed only logins/commits/timestamps, no review body text. Not reconciled against any prior review discussion otherwise — per the method, that content was not read.
🤖 Generated with Claude Code
main split `host_gpu.rs` into a directory module plus `gfx_target.rs`, so every conflict hunk had an empty main side — the code moved rather than went away. This branch's additions move with it: - `GpuSupportLevel`, `gpu_support_level` and `gpu_support_level_for_host_gpu` join the family tables in `gfx_target.rs`, which is where the predicate they build on (`gfx_target_has_no_rocm_build`) now lives. - `pinned_gpu_name` and its naming helpers stay in `host_gpu.rs`, next to the probes they guard. - `WindowsExamineInventory::amd_display_count` moves to `host_gpu/windows_inventory.rs`. - The exhaustive `rocm_support_line` replaces the VulkanOnly-only version main carried in `host_gpu/render.rs`. That is the point of the change: every support level gets a line, because `lemonade_engine_note` sends Unknown hosts to `rocm examine` and silence there is not an answer. All 13 tests this branch added to `host_gpu.rs` were relocated with their subjects and are verified present and passing, not dropped with the moved region. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Both blocking items done, plus the multi-GPU naming point. Pushed as 1. The 2. Note Multi-GPU MergeThis needed the largest re-home of the four PRs. Two of The repo's own mojibake guard (ROCMAI-543) also caught em dashes in the VerificationLinux MI300X box, merged tree applied onto
On the two failing checks you flagged: Still unverified, and I would rather say so: the box has no |
Both blocking items addressed in 53b5cdb: the device: step no longer fails a correct run (lemonade reports unknown until its inference probe returns; the absence is now tolerated and documented), and examine-31 pins the rocm_support line, untagged so each lane's own hardware is the coverage and cross-checked against detected_gfx_target/compatible_therock_family. Also fixed the multi-GPU gpu_name point rather than documenting it: pinned_gpu_name reports a name only on a single-AMD-GPU host and withholds it under a visibility mask. Re-requesting your review.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
I read through the whole diff for this one. The approach looks sound: the support level derives from the existing family checks, and Unknown stays distinct from a confirmed no-ROCm claim. I'm requesting changes for two things that need to be sorted before merge, plus a handful of smaller points inline.
Needs fixing before merge
- The PR is conflicting and CI has not run on this head. The only check present is readthedocs, so the "4178 passed / clippy clean" claims are unverified by CI. Please merge or rebase main, push, and drive the gates (tests, clippy, e2e lanes, commit signatures) to green.
- The PR body no longer matches the diff. It quotes the install-sdk sentence without
--engine lemonadeand cites tests attherock.rs:7681/7807, but the message and its tests now live intherock/family.rs. The rationale for pinning--engine lemonade(a configured default engine outranks the host preference) is also missing from the body. Please rewrite it from the final diff.
Smaller points (inline)
- The new
GpuSupportLevelitems sit aftermod tests, which I expectclippy::items_after_test_moduleto flag under-D warnings. I did not run clippy, so please confirm. - The
Unknownadvice may not clear the condition it is printed for, and the examine-31 step is nearly vacuous. rocm services list(and the chatservicestool) now do live healthcheck RPCs, about 18s worst case under the env lock. Please confirm this is the intended tradeoff.- serve-07 is xfail'd on lemonade+linux, so no lane live-asserts the Vulkan device path this change is about.
Questions for you
- Is
Unknownan acceptable third tier in place of "unsupported"? - Is it fine that the
device:row onrocm serveis optional and often absent for slow lemonade starts, withrocm services listas the reliable place to see it? - This is a large PR (31 files). The codex-bridge/MCP change and the
pinned_gpu_name/ multi-GPU naming work could land separately if you prefer.
I did not build or run tests locally; this is a read-through of the diff.
| /// unrecognized target satisfies neither conjunct, so it is a gap in what this | ||
| /// catalog knows rather than a fact about the card. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum GpuSupportLevel { |
There was a problem hiding this comment.
GpuSupportLevel and the two functions below it are appended after mod tests; I expect clippy::items_after_test_module (style, CI runs -D warnings) to flag this. Moving them above #[cfg(test)] mod tests avoids it.
| // do is let an engine decide and then read back what it chose - | ||
| // which is the `device:` row this same ticket added, and the only | ||
| // thing on this host that can actually settle the question. | ||
| GpuSupportLevel::Unknown => format!( |
There was a problem hiding this comment.
This line recommends rocm serve <model> --engine lemonade for a host where no AMD GPU was detected, and says it "reports the backend it picked on its device: row". With no GPU, serving may refuse (no CPU fallback), so the advice can be a dead end. Also, the serve summary row is documented in this PR as often absent for lemonade, so the line promises something the summary frequently won't print. Consider pointing at rocm services list for the row, and not suggesting serve when nothing was detected. The tests here pin the wording only, not that the advice works.
| let output = world.cli_output.as_ref().expect("no command was run"); | ||
| let line = rocm_support_line(output).expect("no rocm_support line"); | ||
| assert!( | ||
| line.contains("rocm "), |
There was a problem hiding this comment.
line.contains("rocm ") is satisfied by every variant of the support line, so this step can't fail when the advice is wrong. Assert the specific command for the level (rocm install sdk vs rocm serve <model> --engine lemonade).
| // same gate `rocm serve`'s summary uses, and capped in aggregate by | ||
| // `fetch_service_devices` so a long list (or one unresponsive server) | ||
| // cannot turn what used to be a local-only render into a long wait. | ||
| let devices = fetch_service_devices( |
There was a problem hiding this comment.
rocm services list (and the chat services tool) now do a live healthcheck per ready service: ~18s worst case, executed under the process-wide builtin-engine env lock. Previously this was an instant local read. Is this the intended tradeoff versus persisting the device at launch or probing opt-in?
| The GPU is detected but is not supported by ROCm; \ | ||
| the Vulkan backend can still be used." | ||
| the Vulkan backend can still be used. \ | ||
| Try `rocm serve <model> --engine lemonade` - it runs on Lemonade's \ |
There was a problem hiding this comment.
The PR body still quotes this sentence without --engine lemonade and cites tests at therock.rs:7681/7807; please update the body to match this final text and test location.
| .serve_summary | ||
| .as_deref() | ||
| .expect("the Given step must have recorded the serve summary"); | ||
| if let Some(summary_device) = device_row_value(summary) { |
There was a problem hiding this comment.
This comparison only runs when the summary printed a device row, and serve-07 is xfail'd on lemonade+linux, so no lane live-asserts the Vulkan/lemonade device path. Worth stating the gap in the PR text and naming the lane that will exercise it once the xfail lifts.
main moved the codex-bridge snapshot cluster into `bridge_snapshot.rs`, so that hunk had an empty main side again. This branch's contributions move with it: `builtin_codex_bridge_engine_inventory` takes the host's `GpuSupportLevel` and routes lemonade's row through `engine_inventory_note`, and `build_codex_bridge_snapshot` hoists the `ExamineSummary` out of the struct literal to derive it. The `host_gpu_support_level` helper stays at the crate root, where `chat.rs`'s two MCP `engines` call sites reach it. Three smaller ones: the `examine_report_text.rs` import takes both sides; main changed the serve helper to return its stdout while this branch records it on the world, so it now does both; and runtime-19's new `Then` step sits alongside main's new runtime-20/21. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · 53b5cdb
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.
Review — no blocking findings
DEGRADED REVIEW (see What this covered). Full review of the whole change (d7faa84e...53b5cdb9, 31 files, +2533/-102). Implements ROCMAI-550 (ticket could not be retrieved here — no Jira tool available to this automation; judged only against the PR description, which this review found to be an accurate account of the diff, including its disclosed gaps).
Blocking
None.
Non-blocking
-
Stale file reference in a new doc comment —
apps/rocm/src/main.rs:2784
The newlemonade_engine_notedoc comment says the remaining host-blind static engine catalogue lives in`apps/rocmd/src/common.rs`. Verified by reading both files:apps/rocmd/src/common.rshas no such catalogue; the staticrocmd_engine_inventory()function this comment is describing actually lives inapps/rocmd/src/host_snapshot.rs:159. A misleading pointer for the next person who goes looking.
Confidence 85 · mechanical · Fix: change the path in the doc comment toapps/rocmd/src/host_snapshot.rs. -
rocm services list --jsonstill cannot report the device this PR added —apps/rocm/src/services_text.rs(render_services_json, unchanged by this diff) vs.render_services_text(changed)
The newdevice:row is wired into the human-text renderer only.render_services_json(its own doc comment: "Emits theManagedServiceRecords verbatim...rocm remotedeserializes them back into the same type") never callsfetch_service_devicesandManagedServiceRecordcarries no device field, so the machine-readable form of the exact same command — androcm remote's control channel, which reads this JSON — can never learn the backend/device info this PR's stated goal is to surface. Unlike the TUI-pane/rocmd-catalog scope boundary (which this PR documents explicitly on two functions), nothing in the diff states this as a deliberate boundary.
Confidence 70 · architectural · Fix: either extendManagedServiceRecord/the JSON output with the device, or add a comment onrender_services_jsonstating this is an intentional, scoped-out gap (as was done for the TUI/rocmd case).
Decisions for the author
None beyond what's already a Non-blocking item above.
Positive signals
GpuSupportLevel's three-tier classification (RocmHip/VulkanOnly/Unknown) is matched exhaustively everywhere it's consumed (lemonade_engine_note,rocm_support_line) rather than through a_ =>catch-all, so a future fourth variant becomes a compile error at every call site instead of silently inheriting the wrong wording — this is exactly the class of bug (Unknownsilently reading as ROCm-supported) the PR is fixing.- The new live
HealthcheckRPC paths (fetch_service_device,fetch_service_devices) are carefully budget-bounded (SERVE_DEVICE_PROBE_BUDGET,SERVICE_DEVICE_PROBE_BUDGET) with the budget-exhaustion and retry branches unit-tested via injected clocks/probes, sorocm services listcannot have its wall time grow unbounded with a hung engine — a real risk given this used to be pure local I/O. format_reported_deviceis shared across all three independently-implemented engines (aim, lemonade, vllm), each with its own unit test pinning the exact shared shape, so the three cannot drift into disagreeing about the same row — verified by reading all threereported_deviceimplementations.- Backward compatibility for the new
gpu_namestate field is explicitly tested in all three engines (a state file written before this change, with nogpu_namekey, still renders a cleanbackend/backend gpu <n>row rather than a dangling noun or a panic).
Deployment notes
- This head does not merge cleanly into the current
main. GitHub reportsmergeable_state: dirty, and this was independently reproduced here withgit merge-tree origin/main pr-678-head, which shows real conflicts in 6 files (README.md,apps/rocm/src/examine_report_text.rs,apps/rocm/src/main.rs,apps/rocm/src/serve_summary.rs,docs/testing.md,tests/e2e-cucumber/features/runtime_setup.feature,tests/e2e-cucumber/tests/e2e/serving_steps.rs). A rebase/merge is needed before this can land. - No CI has run against the current head (
53b5cdb9) yet. The most recent completed CI/CodeQL runs are againstcabe9795, which is two commits and one conflict-resolution merge behind the current head. This is stated as a verification gap, not assumed to be covered — per this method's rule that an unchecked class is reported rather than assumed clean.
What this covered
DEGRADED REVIEW: the planned multi-worker fan-out (5 coupled-file code-review passes plus the 4 source-reading passes: agent-instruction adherence, history, prior-changes, code-comment compliance) could not be run. Every Agent spawn attempt failed with "Concurrent subagent limit reached" across three separate waves over several minutes (the pool was saturated by unrelated subagents not spawned by this task and never drained below capacity), including a final single-agent-only attempt. Per the review method's rule for this case, the review proceeded single-context instead, and that is disclosed here rather than presented as a complete fan-out.
What was actually read in that single context: the complete diff d7faa84e...53b5cdb9 (all 31 files, with 10 lines of context per hunk — effectively the whole of each changed region) via a local read-only clone (git fetch of pull/678/head, never checked out); the full current AGENTS.md (502 lines) to check the PR's own section citations (§3 scenario-coverage/remediation rule, §5 four-surface doc rule) against its actual content — both citations were verified accurate and the diff was checked against both rules; an independent repo-wide grep for the stale "with ROCm llama.cpp backend" claim and for non-ASCII characters in newly-added non-comment lines (none found outside documented exemptions); and targeted reads of surrounding/untouched code to verify specific claims (e.g. that --engine lemonade is a real, accepted rocm serve flag value; that rocmd's static catalogue's true location; that render_services_json doesn't carry device data).
Not run as independent passes: the dedicated history (git log/git blame) archaeology and prior-review-comment-pattern passes were not performed as their own separate sweep beyond what surfaced naturally while reading the diff; a dedicated per-file-group adversarial pass by a fresh reader (the main value a real fan-out adds) did not happen. Recommend re-running this review with fan-out available.
Not reconciled against any discussion on this PR — this review was produced blind to the existing review thread, per the method's rule, and a prior nowycondro review exists on this PR at an older commit (cabe9795) that is not reconciled with here.
🤖 Generated with Claude Code
|
Merged
Three smaller ones: the Verified on a Linux MI300X box with the merged tree applied onto No blocking reviews outstanding on this one. |
#284 landed, so `serve_summary.rs` and `docs/testing.md` now carry its OOM tests and prose alongside this branch's device-row ones; both kept. main also added the stop-confirmation contract to the README and the serve `--help` text, which sits beside this branch's device-line paragraph rather than replacing it. `engine_manages_own_runtime` moved to `engine_install_env`, and `rocm-core` gained `windows_amd_gpu_listing` next to this branch's `pinned_gpu_name` re-export. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Both blocking items done. Pushed as 1. Conflicting, and CI had not run. Merged Linux MI300X box, merged tree applied onto You were right to discount the earlier "4178 passed" claim — it had no CI behind it. These numbers are from a run I can point at, and GitHub's own gates are now running on this head. 2. The PR body no longer matched the diff. Rewritten and verified against the live body. It now carries the The smaller points
Your three questions
Still not verified by anythingThe box has no |
Both blocking items addressed in 5309936: main is merged (19 behind; #284 landed meanwhile) so the branch is mergeable and the gates have run — clippy, e2e-target clippy, 76 test suites and fmt all exit 0 on a Linux MI300X box, and GitHub's checks are running on this head. The PR body is rewritten from the final diff and verified: it carries the --engine lemonade rationale and replaces the stale therock.rs:7681/7807 citations with test names, since main split therock.rs into a directory module. GpuSupportLevel now lives in gfx_target.rs, so items_after_test_module is clean. The Unknown-advice and examine-31 points are left open rather than claimed fixed. Re-requesting your review.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
One blocking issue and a few smaller ones, details inline.
Blocking
- The
rocm serve--helpdoc comment lost the tail of a sentence in the rewrite (main.rs), so help now reads "...signals the server process directly and exitsrocm services." and the documented exit-0 behavior is gone.
Worth addressing
- The
rocm examine"unknown" remediation promises thatrocm serve ... --engine lemonadereports adevice:row, while the PR's own docs say that row can be absent. The named command may not clear the condition. - The install-sdk Vulkan message is covered only by unit tests; no lane has a Vulkan-only host. Please add a scenario or call out the gap in the PR text.
rocm servecan now wait up to ~14s for the device row, androcm services list/ the chatservicestool up to ~18s. Please confirm that is acceptable.- The PR carries several changes beyond ROCMAI-550's scope list (endpoint-key env threading, the
redacted()test refactor, the multi-GPU name/visibility-mask logic). Splitting them out, or at least listing them in the PR body, would make this easier to review. HealthcheckResponse.devicestrings changed (rocm_gpu->rocm,vulkan gpu->vulkan); I could not check from the diff whether anything else consumes that field, so please grep for consumers.- Two mechanical nits: an orphaned doc comment in main.rs and items placed after
mod testsin gfx_target.rs.
CI: the Strix Halo E2E lane fails on serve-40 ("device state NOT drained", "llama-server failed to start"), which looks like a GPU-lane flake rather than something this change touches; a rerun should confirm. MI300X and W7900 lanes were still pending when I looked and are the ones that exercise the new serve/examine device assertions.
This was a read of the diff only; I did not build or run the tests locally.
| /// to detach and leave the server running, or Ctrl-C to stop it. If that stop answers | ||
| /// that it could not confirm the server exited, it says so on stderr and exits with an | ||
| /// error; if the stop itself fails, it says why on stderr, signals the server process | ||
| /// directly and exits |
There was a problem hiding this comment.
The rewritten rocm serve doc comment drops the end of the old sentence: "...exits 0, noting on stderr if that signal fails too. Inspect or stop servers later with" is gone, so --help now reads "signals the server process directly and exits rocm services." That is a broken sentence and it loses the documented exit-0 behavior. Please restore the clause.
| bare | ||
| } | ||
|
|
||
| /// This host's GPU support level from the cheap host probe, for the call sites |
There was a problem hiding this comment.
This paragraph documents host_gpu_support_level, but it now sits above engine_inventory_note; host_gpu_support_level (declared further down) has no doc. Looks like an edit accident, please move it.
| GpuSupportLevel::Unknown => format!( | ||
| " rocm_support: unknown - {}, so this build can confirm neither \ | ||
| ROCm support nor its absence; `rocm serve <model> --engine \ | ||
| lemonade` reports the backend it picked on its `device:` row\n", |
There was a problem hiding this comment.
This line says rocm serve ... --engine lemonade "reports the backend it picked on its device: row", but the PR's own docs say the serve summary can omit that row until the engine verifies. The named command may therefore not clear the "unknown" state. Consider pointing at rocm services list, or soften to "may report". Only the unit test pins this string; examine-31 does not check the claim against serve behavior.
| } | ||
| } | ||
|
|
||
| /// How well this host's GPU is supported, for messages that tell the user what |
There was a problem hiding this comment.
GpuSupportLevel and the two functions are defined after mod tests; please move them above the test module.
| The GPU is detected but is not supported by ROCm; \ | ||
| the Vulkan backend can still be used." | ||
| the Vulkan backend can still be used. \ | ||
| Try `rocm serve <model> --engine lemonade` - it runs on Lemonade's \ |
There was a problem hiding this comment.
This is the ticket's main remediation message, but only unit tests pin the string. No lane has a Vulkan-only host, so nothing shows that rocm serve <model> --engine lemonade works there. Either add a mocked-host scenario or state the gap in the PR text.
| // bounded (`engine_ipc::SERVE_DEVICE_PROBE_BUDGET`) and `None` is a | ||
| // supported outcome — the summary then has no `device:` row, and | ||
| // `rocm services list` shows it once the engine can say. | ||
| let device = (report.status == "ready") |
There was a problem hiding this comment.
The ~6s retry window plus an 8s probe can add up to ~14s to rocm serve when the engine still reports unknown, which is most likely on the Vulkan/lemonade host the ticket targets, and the row may still be absent afterwards. Is that tradeoff intended, versus no retry and relying on rocm services list?
…ge cut Review round on #678. - The `rocm serve` `--help` doc lost the tail of a sentence when I combined this branch's device-line paragraph with main's stop-confirmation text: it read "...signals the server process directly and exits `rocm services`." and the documented exit-0 behaviour was gone. Restored from main's wording. - `GpuSupportLevel` and the two `gpu_support_level*` functions were appended after `gfx_target.rs`'s `mod tests`. Moved above it. - main added `freeform_prefix_with_quote` and the runtime-22 steps to `runtime_steps.rs` next to this branch's examine-pointing assertion; both kept. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Blocking item fixed, plus the two nits and your grep. Pushed as The truncated Mechanical nits — both fixed. The The ones I am answering rather than fixing
CI: your read of the Strix Halo serve-40 failure matches mine. It is Verification on the Linux MI300X box with the merged tree: Worth noting the |
Blocking item fixed in 74a1496: the serve --help sentence I truncated in an earlier merge is restored from main's wording, including the exit-0 behaviour. GpuSupportLevel moved above mod tests in gfx_target.rs. Grepped the HealthcheckResponse.device consumers as asked — nothing else reads it; the remaining rocm_gpu hits are DeviceDescriptor.kind and capability flags, untouched. The unknown-remediation wording, the Vulkan-lane gap, the latency budgets and the out-of-scope list are answered in the comment rather than silently closed; the wording fix specifically is NOT in this push. Re-requesting your review.
…he row Review round on #678. The note promised that `rocm serve <model> --engine lemonade` reports the backend on its `device:` row, while this PR's own docs say that row can be absent: the serve summary reads the device once, right after `ready`, and lemonade answers `unknown` until its own inference probe returns. The advice now names `rocm services list`, which shows the row once it is known, so the command can actually clear the condition it is printed for. The reasoning is recorded beside the arm rather than left implied. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
|
Fixed the unknown-tier wording I flagged as outstanding last round. Pushed as You were right that the promise was stronger than the behaviour. The note said It now reads: serve the model with that command and read the backend from the Verified on the Linux MI300X box with the merged tree: |
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Thanks for this. The support-level line, the device row read from the live engine health, and the install-sdk remediation all follow the ticket. I found three things to fix before merge, plus the state of the branch.
Needs fixing
- The
rocm serve --helptext is corrupted (a clause was dropped, probably in a merge), so it now ends "...exitsrocm services." - The no-GPU
rocm_supportadvice namesrocm serve <model> --engine lemonade, which the PR's own help text says refuses on a host with no usable AMD GPU. The named command has to actually clear the condition, and the test should assert that behavior rather than the wording. GpuSupportLeveland its functions sit after thetestsmodule ingfx_target.rs, which I expect to fail clippy with-D warnings. I didn't run clippy, so please confirm.
Branch state: the PR is currently conflicting with main and no tests, clippy or e2e have run on this head. Please rebase and get the full gate green; I haven't been able to verify any of it in CI.
Non-blocking
- A misplaced doc comment at
main.rs:2363, andservices listcan now block for up to ~18s (inline). - The PR is broader than the ticket: retry/budget loops, API-key env plumbing, GPU-name and visibility-mask logic, new engine state keys, and a change to the vLLM/AIM
Healthcheck.deviceshape. I'd suggest splitting it (support-level messages / device row / GPU naming) to make review and the engine-protocol impact easier to check. - The dash TUI engine pane and the
rocmdcatalog still carry the static "ROCm llama.cpp backend" claim. The code comments say that's deliberate, but the ticket asks for all surfaces to be checked. - No live lane exercises the Vulkan device output (serve-07 is xfail on lemonade+linux), so that path is covered by unit tests only.
I also considered the Unknown tier (instead of "unsupported") and pinning --engine lemonade in the remediation; both are reasoned in the PR and fine with me.
| /// to detach and leave the server running, or Ctrl-C to stop it. If that stop answers | ||
| /// that it could not confirm the server exited, it says so on stderr and exits with an | ||
| /// error; if the stop itself fails, it says why on stderr, signals the server process | ||
| /// directly and exits |
There was a problem hiding this comment.
The serve --help sentence is cut off: the clause "0, noting on stderr if that signal fails too. Inspect or stop servers later with" was dropped, so it now reads "...signals the server process directly and exits rocm services." Looks like a merge-resolution artifact; please restore it.
| // do is let an engine decide and then read back what it chose - | ||
| // which is the `device:` row this same ticket added, and the only | ||
| // thing on this host that can actually settle the question. | ||
| GpuSupportLevel::Unknown => format!( |
There was a problem hiding this comment.
For the no-GPU-detected case this advises rocm serve <model> --engine lemonade, but per the new serve --help text, serve refuses under the default gpu_required policy on a host with no usable AMD GPU. So the named command doesn't clear the condition it's printed for. Keep the serve advice for the detected-but-unrecognised case and give the no-detection case different advice (driver / rocm doctor), and assert the behavior in the test rather than pinning the wording (the e2e step only checks for rocm ).
| /// unrecognized target satisfies neither conjunct, so it is a gap in what this | ||
| /// catalog knows rather than a fact about the card. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum GpuSupportLevel { |
There was a problem hiding this comment.
GpuSupportLevel and the two functions are added after #[cfg(test)] mod tests (which starts around line 554). That will likely trip clippy::items_after_test_module under -D warnings; please move them above the test module.
| bare | ||
| } | ||
|
|
||
| /// This host's GPU support level from the cheap host probe, for the call sites |
There was a problem hiding this comment.
The doc comment for host_gpu_support_level got merged into the front of engine_inventory_note's doc, and host_gpu_support_level itself has none. Please split them.
| // same gate `rocm serve`'s summary uses, and capped in aggregate by | ||
| // `fetch_service_devices` so a long list (or one unresponsive server) | ||
| // cannot turn what used to be a local-only render into a long wait. | ||
| let devices = fetch_service_devices( |
There was a problem hiding this comment.
rocm services list (and the chat services tool) now does up to ~18s of synchronous engine RPCs for a read-only listing. It's bounded and documented, but consider persisting the device at launch, or probing only on request.
Summary
ROCMAI-550: on a GPU with no ROCm/HIP build (e.g. gfx90c),
rocm examineandrocm engineskept claiming the embedded Lemonade engine runs "ROCm llama.cpp backend" even though it actually falls back to Vulkan.rocm install sdk's refusal for such a GPU didn't name the actual next step, and neitherrocm serve's deployment summary norrocm services listshowed which backend/device an engine actually selected.Scope note: most of the ticket's
rocm examine/rocm install sdkcomplaints for gfx90c were already fixed by #648 (ROCMAI-542), which merged with full gfx90c PCI-ID detection, arocm_support:line inrocm examine, and a refusal message pointing at Vulkan inrocm install sdk. README/docs/rocm-docs//doctor-skill files/docs/ux-guidelines.mdwere also checked and do not contain the stale "ROCm llama.cpp backend" claim — no changes needed there. This PR covers what was actually left:Add
GpuSupportLevel(RocmHip/VulkanOnly/Unknown) inrocm-core::host_gpu, derived from the existingnormalize_therock_family/gfx_target_has_no_rocm_buildprimitives (no new detection logic).Make the Lemonade engine note in
rocm examine'sengine_inventory:section,rocm engines, and the codex-bridge engine inventory GPU-aware instead of a hardcoded ROCm claim.rocm install sdk's unsupported-GPU error (the detected-host branch) now suggestsrocm serve <model> --engine lemonadeas the concrete next step.The engine is pinned deliberately. This branch knows the host has no ROCm build, which is not the same as knowing which engine
servewould pick:select_serve_engineranks an explicit--enginefirst, then a configureddefault_engine, and only then the host's own preference. A user who once ranrocm config set-default-engine vllm— never validated against the GPU — would have the bare command attempt vLLM on a host with no ROCm and fail under the no-CPU-fallback policy. Naming the engine makes the sentence true whatever the config says, because the explicit flag outranks every other selection source. Pinned byunsupported_gpu_error_pins_the_engine_a_configured_default_could_override.Surface the engine's actual backend/device in
rocm serve's deployment summary androcm services list, read live via the engine's existingHealthcheckRPC (HealthcheckResponse.device, e.g."vulkan gpu 0"/"rocm gpu 0") rather than guessed from the pre-launch GPU selection — this RPC already existed and was already populated by both engines, just never called fromapps/rocm.Design note
The ticket asks for three support tiers including "unsupported". Kept
Unknowninstead of a hard "unsupported" claim for an undetected/unrecognized GPU, matching the existing documented discipline inhost_gpu.rs/diagnose.rs(gfx_target_has_no_rocm_build's doc comment,GFX_TARGETS_WITHOUT_ROCM_SUPPORT): conflating "confirmed no ROCm build" with "unrecognized" would turn a detection gap into a false claim about the hardware.Why the
rocm install sdkremediation sentence is unit-covered and not a scenarioRaised in review: this is the PR's second user-observable CLI text change, so it owes either a Gherkin scenario or a stated reason. The reason, stated:
A scenario for it cannot run on any lane we have. The sentence lives in
unsupported_host_gpu_message, whichresolve_familyreaches only after all three earlier doors are closed — no--family, noROCM_CLI_THEROCK_FAMILY, no managed runtime — and only whendetect_host_gfx_target()itself returns a target thatnormalize_therock_familydoes not cover andgfx_target_has_no_rocm_buildconfirms has no build. That needs a real Renoir/Cezanne Vega APU on the runner. Every self-hosted lane is ROCm-capable (platform=mi300x os=linux), anddetect_host_gfx_targetreads real sysfs/WMI with no test override, so there is no way to make this branch fire in e2e. A scenario written for it would be permanently skipped, which is worse than no scenario.The adjacent branch that can be driven is already asserted, including the negative.
runtime-19(@id:runtime-install-sdk-refuses-unsupported-gpu-target) drives--family gfx90c, which lands inunsupported_requested_target_message, and now asserts the refusal points atrocm examineand that it does not containrocm serve— precisely because that lane runs on a ROCm-capable host where predicting a Vulkan backend would be false. So the one door a lane can open is covered, and it is covered by proving the absence of the prediction rather than enforcing it.What does hold the sentence up.
unsupported_host_gpu_messageis pinned byapps/rocm/src/therock/family.rs's unit tests —unsupported_gpu_error_names_the_gpu_and_does_not_suggest_families,unsupported_gpu_error_pins_the_engine_a_configured_default_could_override, andrequested_target_error_does_not_claim_which_backend_serving_will_use(mainsplittherock.rsinto a directory module during this PR's life, so the oldtherock.rs:7681/7807line references are gone), and the claim the sentence makes is not a bare string pin:gfx_target_has_no_rocm_buildis what routed the user into this branch, andselects_vulkan_when_rocm_unsupportedinengines/lemonade/src/backend_alignment.rsis what makes llama.cpp actually pickvulkanoncerocmreports unsupported.Wording note: merging
mainbrought in #683's whole-tree ASCII output contract, which this branch predates. The em dash in this sentence is a non-ASCII character in a user-facing literal, so it is now an ASCII hyphen —Try \rocm serve ` - it runs on Lemonade's Vulkan backend instead.The two unit tests above match onrocm serve `, so they are unaffected.Test plan
GpuSupportLevel/gpu_support_level(gfx90a → RocmHip, gfx90c → VulkanOnly, unrecognized/undetected → Unknown)rocm examine,rocm enginesand codex-bridge call sitesrocm install sdknext-step suggestion on both refusal branchesdevicerow inrocm serve's deployment summary (shown only when known; "unknown" from the engine is treated as not-known)rocm services listomits thedevice:line when no real engine state backs the recordthe services list reports the engine's actual backend device) to the existing per-PR GPU canariesserve-05(vLLM) andserve-07(Lemonade) intests/e2e-cucumber/features/model_serving.feature, per this repo's requirement that user-observable CLI output changes need scenario coverage, not just unit tests@id:runtime-install-sdk-refuses-unsupported-gpu-target(runtime-19,tests/e2e-cucumber/features/runtime_setup.feature) asserting the refusal points atrocm examineand does not predict a backend. Verified green on a Linux box (cargo xtask e2e -- -n "runtime-19"), and falsified by reverting the message string, which fails exactly that step.runtime-19is ungated, so every lane runs itunsupported_host_gpu_message) stays unit-covered only, for the reason set out in the section above — it needs a Renoir/Cezanne Vega APU to fire, no runner has one, anddetect_host_gfx_targetreads real sysfs/WMI with no override to fake a targetbuiltin_codex_bridge_engine_inventoryno longer reports lemonade's static "with ROCm llama.cpp backend". Both of its call sites can see the host —build_codex_bridge_snapshotalready gathers theExamineSummarycarrying the two inputs, and the bridge'senginestool takes the same cheap host proberocm enginesdoes — so it takes aGpuSupportLeveland substituteslemonade_engine_note. Onlysummarychanges; the field set and the JSON shape are untouched. The still-host-blind consumers (the dashboard TUI pane,rocmd's own catalog) keep the static row, and the boundary is stated at the static table and at the bridge helperserve-07fails because the sandbox is missinglibdrm_amdgpu.so.1for Lemonade's bundled binary, andserve-05fails on a broken vLLM wheel (ImportError: cannot import name 'current_platform' from 'vllm.platforms'). Both are pre-existing environment gaps on that box, not introduced by this diff — this PR touches neitherengines/vllmnorengines/lemonade. CI runs on dedicated hardware without these gaps, so it is the authoritative verification for this scenario perAGENTS.md§8.main(36 commits behind, including ROCMAI-88: extract chat/MCP-bridge function bodies (2/3) #667's extraction of 185 functions out ofapps/rocm/src/main.rsand fix(output): replace non-ASCII punctuation with ASCII in CLI output text #683's ASCII output contract). The one conflict,apps/rocm/src/main.rs, was resolved by taking main's post-extraction file and re-applying this branch's 23 hunks onto it, with theengines-tool hunk followingrun_internal_mcp_callintoapps/rocm/src/chat.rs. Verified by construction: thefnname sets of main, this branch and the resolution comm out toresolved == main ∪ {the five functions this branch adds}, nothing of main's droppedcargo test --workspace --all-targets --no-fail-fast(4178 passed, 0 failed),cargo clippy --workspace --all-targets -- -D warnings(clean),cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings(clean),cargo fmt --all --check(clean).xtask'severy_literal_that_can_reach_a_redirectable_sink_is_ascii,every_help_text_in_the_command_tree_is_asciiandscenario_names_are_indexed_sequentially_per_featureall ran and passedrocm examine,rocm engines list,rocm services liston that same ROCm-capable box, confirming the ROCm-path wording is unchanged there (as expected — no gfx90c hardware available to additionally smoke-test the Vulkan-only wording by hand, covered by unit tests instead)🤖 Generated with Claude Code
Where things live after main's refactors
mainhas reorganised several of the files this PR touches while it was open, so for review:apps/rocm/src/therock/family.rs(wastherock.rs)GpuSupportLevel,gpu_support_level,gpu_support_level_for_host_gpu:crates/rocm-core/src/gfx_target.rspinned_gpu_nameand the GPU-naming helpers:crates/rocm-core/src/host_gpu.rsrocm_supportline:crates/rocm-core/src/host_gpu/render.rsWindowsExamineInventory::amd_display_count:crates/rocm-core/src/host_gpu/windows_inventory.rsapps/rocm/src/bridge_snapshot.rsreported_device:engines/lemonade/src/device_policy.rs(wasstate.rs)Known limits, stated rather than implied
pinned_gpu_namereports a name only when the host has exactly one AMD GPU (and no visibility mask), because every name probe answers for the host's primary adapter; on a multi-GPU host a named row would be specific and wrong. The restriction is unit-tested; no lane has two AMD GPUs to prove it in situ.rocm services listand the chatservicestool now make live engine RPCs, ~18s worst case in total under the probe budget, however many servers are listed. A server that does not answer in time simply has nodeviceline.