fix(dash): discover served model for configured chat endpoint by volen-silo · Pull Request #97 · ROCm/rocm-cli

volen-silo · 2026-07-10T10:55:18Z

Summary

The dash TUI chat now queries /v1/models at startup to discover the served model name when a chat endpoint is configured via OPENAI_BASE_URL / --chat-url but no model is set.
Why: the documented rocm serve … && OPENAI_BASE_URL=… rocm flow was broken — sending a message returned 404 Not Found: The model 'local-model' does not exist, making chat unusable out of the box.
Root cause: the managed-service and in-TUI detect-offer paths already discover the served model, but the startup env/CLI path fell straight through to resolve_llm_config's DEFAULT_CHAT_MODEL = "local-model" placeholder and built the agent with it. Accepting the consent prompt doesn't rebuild, so the placeholder reached the first request.
Fix: after resolving the config, when the endpoint is reachable and no explicit model was given, fetch /v1/models and adopt the served id (via the existing fetch_first_model / pick_first_model helpers), mirroring the managed-service path.

Non-obvious decisions

Gated on a successful TCP probe (probe_ok) so an unreachable configured endpoint never adds the 3s fetch timeout to startup.
An explicit --chat-model / config value always wins — discovery only fills the placeholder (enforced by the pure with_discovered_model helper).
Kept the local-model fallback when /v1/models can't be read — some endpoints ignore the model field or don't expose the listing.

Test plan

cargo test -p rocm-dash-tui --lib — 549 passed (3 new unit tests for with_discovered_model: placeholder replaced, explicit model preserved, failed discovery keeps fallback).
cargo clippy -p rocm-dash-tui --all-targets — clean.
Manual (deferred, needs a live engine): rocm serve Qwen/Qwen2.5-1.5B-Instruct --engine vllm, then OPENAI_BASE_URL=http://127.0.0.1:11435/v1 rocm, accept the endpoint in the Chat tab, send a message → expect a normal reply and no local-model 404.

Risk

Low — additive best-effort discovery on a single startup path; existing precedence and the neutral fallback are preserved.

A configured chat URL (OPENAI_BASE_URL / --chat-url) with no explicit model resolved to the "local-model" placeholder, which 404s on servers that register the model under its real id. Query /v1/models on startup to adopt the served model, mirroring the managed-service path. Gated on a reachable probe so an unreachable endpoint adds no fetch timeout to launch, and an explicit --chat-model/config value still wins. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

volen-silo · 2026-07-14T09:07:41Z

🔴 Automated review · pr-review-watcher · f8c2da1

Summary

Fixes the broken rocm serve … && OPENAI_BASE_URL=… rocm chat flow: when a chat endpoint is configured but no model is set, the startup path now queries /v1/models and adopts the served id instead of shipping the local-model placeholder that 404s. Verdict: Approve. Verified: cargo test -p rocm-dash-tui --lib passes (546 passed, 0 failed — incl. the 3 new with_discovered_model tests) and cargo clippy -p rocm-dash-tui --all-targets is clean on Rust 1.96/edition 2024; confirmed the discovery gate (!had_managed && probe_ok && args.chat_model.is_none()) never clobbers an explicit CLI/config model (args.chat_model derives from tui.chat_model, so a config-file model correctly suppresses it), never fires on a managed endpoint (which already discovers its own model), and safely no-ops when the resolved config is None (no-key OAuth path untouched). Blocking: 0 · Non-blocking: 3.

🚫 Blocking (must fix before merge)

None.

Non-blocking

crates/rocm-dash-tui/src/app/mod.rs:1606-1612 — the explicit_model arg to with_discovered_model is always None at this sole call site (the match guard already proved args.chat_model.is_none()), so the explicit_model.is_none() check is dead here; harmless and still useful as a pure unit-test seam, but could be simplified.
crates/rocm-dash-tui/src/app/chat.rs:306 — probe_ok is a 300ms TCP-only probe, so a reachable-but-slow endpoint can still stall the first frame up to the 3s fetch_first_model timeout; the PR description's "never adds the 3s fetch timeout" only holds for the unreachable case. Pre-existing/mirrored from the managed and local-detect paths (which fetch unconditionally), so strictly narrower than existing behavior — noting, not a regression.
crates/rocm-dash-tui/src/llm.rs:211 — discovery silently binds data[0].id with no user-visible signal of which model was picked; a one-line tracing log of the adopted id would aid debuggability when a multi-model server lists an unexpected entry first.

…up (ROCm#100) * fix(dash): prefer a local chat server over the cloud default at startup Startup only single-probed DEFAULT_CHAT_BASE_URL (:8000) before falling back to the ChatGPT cloud OAuth flow, even though the manual 'd' detect path already knew how to find a local engine on any of its well-known ports (Lemonade :13305, vLLM :8000, rocm serve :11435) plus the managed-services registry. A server running on any port other than :8000 was invisible at startup and users landed in the cloud login flow instead. Reuse detect_local_chat() (registry-first, then the 3-port probe, then a best-effort model fetch) in the startup branch that already gated on "no explicit chat_url/env_url configured" — the same condition that previously ran only detect_managed_chat(). Parallelize llm::detect_local_endpoint()'s port probe (std::thread::scope) so a cold start with nothing listening still costs one PROBE_TIMEOUT (~300ms) instead of three, keeping startup latency bounded. Unmerged PR ROCm#97 also touches this startup branch (model discovery inside resolve_llm_config's own fallback when chat_model is None); this change is conflict-minimal — it swaps detect_managed_chat for detect_local_chat without touching the resolve_llm_config call itself. Relates to EAI-7347. Signed-off-by: Michael Roy <michael.roy@amd.com> * fix(chat): do not run local-detection swap when an api key is configured Startup local-engine detection returns a keyless detected_llm_config (api_key/auth_header forced to None). Firing it whenever no explicit URL was set — even when the user configured OPENAI_API_KEY / ROCMDASH_CHAT_API_KEY — silently dropped that credential and produced a 401 at request time. Gate detection on chat_api_key.is_none() as well (extracted into the pure should_detect_local_chat helper for unit testing): a configured key means "use my configured backend", so skip the swap and let resolve_llm_config carry the key through its normal precedence. Also skip the redundant fallback probe: when detection ran and found nothing it already probed the well-known vLLM :8000 port (== DEFAULT_CHAT_BASE_URL), so re-probing that same target just burned another PROBE_TIMEOUT on a no-server cold start; treat it as unreachable directly. Relates to EAI-7347. Signed-off-by: Michael Roy <michael.roy@amd.com> * fix(dash): guard well-known ports free in local-detect port-priority tests detect_local_chat_prefers_local_server_over_no_endpoint (and the sibling detect_local_endpoint_probes_rocm_serve_default_port) only bound the rocm-serve port and assumed the higher-priority Lemonade/vLLM ports were unreachable. On the Windows CI runner that isn't guaranteed, so an ambient listener on :13305 or :8000 made detection return that endpoint instead of rocm-serve's, failing the assertion. Both tests now bind-and-release the higher-priority ports first to confirm they're free before asserting on the rocm-serve fallback. Signed-off-by: Michael Roy <michael.roy@amd.com> * fix(dash): make local-detect priority tests port-independent The previous fix (bind-then-release the higher-priority ports before asserting) still flaked on the Windows CI runner: something answered on the Lemonade port during the probe despite the guard, most likely an ambient listener the test doesn't control. TCP-connect-based detection means any listener on a well-known port — even a bare guard listener held open, as the very first attempt at this fix did — registers as "reachable", so real ports can't be made deterministic from a test. Split the port-priority logic in llm.rs into probe_first_reachable, tested directly against OS-assigned ephemeral ports instead of the real well-known ports. Gave detect_local_chat an injectable probe seam (detect_local_chat_with_probe) so its own test can verify the no-managed-executor wiring without depending on any real network state. Signed-off-by: Michael Roy <michael.roy@amd.com> * fix(dash): widen latency margin in detect_local_endpoint timing test detect_local_endpoint_bounded_latency_when_nothing_listening's 2x PROBE_TIMEOUT margin was still too tight for the Windows CI runner (observed 624ms against a 600ms threshold). A true sequential-probing regression takes ~3x PROBE_TIMEOUT, so widen the margin to 4x to absorb scheduling jitter on slower CI machines while still catching that regression. Signed-off-by: Michael Roy <michael.roy@amd.com> * test(dash): strengthen local chat startup coverage Signed-off-by: Michael Roy <michael.roy@amd.com> --------- Signed-off-by: Michael Roy <michael.roy@amd.com>

Reconcile PR #97 (configured-endpoint served-model discovery) with main's PR #100 startup local-first detection (EAI-7347). Conflicts in crates/rocm-dash-tui/src/app/{chat.rs,mod.rs} resolved as a union that keeps both behaviors: - Preserve main's local-engine detection: should_detect_local_chat, StartupChatOutcome/startup_chat_outcome, detect_local_chat_with_probe, and the #89 Press-only overlay key gate (is_actionable_key). - Port PR #97's /v1/models discovery for a *configured* chat endpoint into a new helper, chat::discover_configured_chat_model, wired only on the StartupChatOutcome::Configured path. Startup matrix (verified by unit tests in app::chat::tests): - Configured URL + no explicit model + reachable -> adopt served /v1/models id. - Configured URL + explicit model -> config precedence, never overridden. - Configured URL + unreachable -> never probed, never replaced (no fetch timeout; an unreachable explicit CLI/env URL is left untouched). - Local detected / OAuth paths unchanged from main. RED before the port: configured_endpoint_adopts_served_model_when_no_model_set failed against the merged-main stub (got "local-model", expected served id); GREEN after wiring the helper. Full workspace tests, clippy -D warnings, and scripts/smoke_local.py all pass. Signed-off-by: Michael Roy <michael.roy@amd.com>

…ale chat_url recovery) Layer PR #105's stale persisted-URL recovery onto PR #97's configured-endpoint model discovery (which already includes current main). Manually resolve the chat.rs/mod.rs conflicts, preserving both behaviors: - PR #97: discover_configured_chat_model() adopts a served /v1/models id for a reachable configured endpoint that has no explicit model (probe_ok path). - PR #105 (EAI-7360): stale_chat_url_replacement() swaps a dead persisted tui.chat_url for a genuinely-different live local engine on the Configured + !probe_ok path, emits a "server changed" notice pointing at `/detect save`, and normalize_base_url() treats trailing slash and /v1 formatting as the same endpoint. Gated on chat_url.is_some() + no api_key/auth_header so env URLs and authenticated remote gateways are never silently replaced. The two paths are mutually exclusive on the Configured branch's probe_ok, so they compose without interference; resolve_llm_config's CLI>config>env>probe precedence is untouched. Matrix verified: no URL/model -> local detection; reachable configured -> /v1/models discovery; stale persisted -> live replacement + notice; unreachable env/explicit URL -> fail honestly; explicit auth -> no silent fallback. Signed-off-by: Michael Roy <michael.roy@amd.com>

volen-silo force-pushed the fix/tui-chat-model-discovery branch from 034e4e1 to f8c2da1 Compare July 10, 2026 10:58

volen-silo requested a review from fredespi July 10, 2026 11:04

This was referenced Jul 11, 2026

fix(dash): prefer a local chat server over the cloud default at startup #100

Merged

feat(chat): add /detect slash command for mid-session re-detection #102

Open

fix(chat): revalidate a stale persisted chat_url and fall back to a live endpoint #105

Open

michaelroy-amd enabled auto-merge July 16, 2026 02:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(dash): discover served model for configured chat endpoint#97

fix(dash): discover served model for configured chat endpoint#97
volen-silo wants to merge 2 commits into
mainfrom
fix/tui-chat-model-discovery

volen-silo commented Jul 10, 2026

Uh oh!

volen-silo commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

volen-silo commented Jul 10, 2026

Summary

Non-obvious decisions

Test plan

Risk

Uh oh!

volen-silo commented Jul 14, 2026

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants