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.

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>

EugeneKSiloAi · 2026-07-16T17:49:36Z

+        // carries a `/v1/models`-discovered model from `detect_local_chat`, and
+        // `OAuth` has no config. Discovery is gated inside the helper on
+        // `probe_ok` (an unreachable endpoint is never probed nor replaced) and
+        // on the absence of an explicit model (config precedence wins).


This is quite verbose, would suggest a small cleanup to keep comments at a reasonable size.

EugeneKSiloAi · 2026-07-16T17:58:02Z

@volen-silo lgtm once the tests and any other ci failures are taken care of. Approved.

Final refresh: main advanced 9830e57 -> 8308450 via #104 (decouple vLLM Prometheus scraping from enable_docker) and #97 (discover served model for configured chat endpoint). Clean auto-merge (runner.rs only), no conflicts. Preserves the startup-phase / Ready composition (InstanceStatus::Starting { phase }, PROTOCOL_VERSION = 2, phase-aware registry mapping, and the Starting -> Ready scrape-success promotion in runner.rs) alongside #104's scraping-decouple changes, plus the CodeQL test-path fix (rocmd tests use the CARGO_MANIFEST_DIR-rooted unique_test_root helper, not env::temp_dir()). Signed-off-by: Michael Roy <michael.roy@amd.com>

Final refresh: bring PR #92 up to main 8308450 (adds #104 decouple vLLM Prometheus scraping from enable_docker, and #97 discover served model for the configured chat endpoint). Clean auto-merge with no conflicts — merge-tree predicted clean and only crates/rocm-dash-tui/src/app/mod.rs was auto-merged (the #97 chat-endpoint change; scrollbar hit-testing code untouched). The scrollbar + braille spinner feature is unchanged: job_console.rs, ui/panel.rs and ui/spinner.rs are byte-identical to the prior branch head. 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

Merged

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

EugeneKSiloAi reviewed Jul 16, 2026

View reviewed changes

EugeneKSiloAi approved these changes Jul 16, 2026

View reviewed changes

michaelroy-amd added this pull request to the merge queue Jul 16, 2026

Merged via the queue into main with commit 8308450 Jul 16, 2026
19 of 21 checks passed

michaelroy-amd deleted the fix/tui-chat-model-discovery branch July 16, 2026 18:09

Navigation Menu

Uh oh!

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

fix(dash): discover served model for configured chat endpoint#97
michaelroy-amd merged 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!

EugeneKSiloAi Jul 16, 2026

Uh oh!

EugeneKSiloAi commented Jul 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 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!

EugeneKSiloAi Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

EugeneKSiloAi commented Jul 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants