fix(dash): chat /v1 404 + Esc closes a running serve modal - #68
Conversation
rominf
left a comment
There was a problem hiding this comment.
Both bugs are real and fixed at the correct layer — nice, focused change. One blocking item plus a couple of things to tighten before this is ready.
Blocking: the 404 fix has no regression test
probed_default_used_when_nothing_configured_but_reachable asserts r.base_url == DEFAULT_CHAT_BASE_URL, which is tautological — both sides are the same constant. I reverted the /v1 suffix locally and the entire llm test suite still passed, so nothing guards the actual fix. The Esc change is well covered, but the headline bug needs a test that fails on revert, e.g.:
assert!(DEFAULT_CHAT_BASE_URL.ends_with("/v1"), "Rig appends chat/completions to base_url");or assert the resolved base_url ends in /v1.
Please address
- Scope of the Esc change is wider than the description says. The summary lists 6 overlays, but
on_console_keyis the shared seam for 12. The unlisted ones include onboarding: Esc on a running onboarding step now closes the whole onboarding wizard (onboarding.rs:180,*ob = None). It's consistent with whatqalready did, but it's an unmentioned behavior change in a guided flow — worth calling out so it's a deliberate decision. (For what it's worth, I checked every caller'sUnhandledarm and none branch on Esc, so nothing screen-specific gets silently intercepted.) - Stacked PR isn't a draft, and CI hasn't run. It's correctly noted as stacked on #67 and targets
retire-legacy-tui, but it's marked ready with no checks reported yet, so the test/clippy results are local-only at this point.
Nit
- The doc comment says the new value "Mirrors ...
VLLM_ENDPOINT/LEMONADE_ENDPOINT", but those uselocalhostwhile this uses127.0.0.1— they share the/v1suffix and port, not the host.127.0.0.1is the right pick (unambiguous IPv4 loopback, and it matches the live test inagent.rs); just consider softening the wording to "matches the/v1convention of ...".
Address review on #68: - probed_default_used_when_nothing_configured_but_reachable only asserted base_url == DEFAULT_CHAT_BASE_URL, which is tautological w.r.t. the /v1 fix. Add an assertion that the *resolved* base_url ends in /v1 — it fails when the suffix is reverted (verified), so it actually guards the 404 fix. - Soften the const doc: VLLM_ENDPOINT / LEMONADE_ENDPOINT share the /v1 convention and port, not the host (they use localhost, this uses the unambiguous IPv4 loopback 127.0.0.1). Signed-off-by: Michael Roy <michael.roy@amd.com>
|
Thanks @rominf — all three addressed in a1fa5f7 + a description update.
On the CI/draft note: it targets |
711d3ae to
43ce97b
Compare
… job console
Two dash chat usability fixes surfaced when serving a model then chatting:
1. 404 on chat completions. The Rig OpenAI client builds the request URL as
`base_url + "/" + "chat/completions"`, so `base_url` must end in `/v1`.
Every endpoint carried it (VLLM_ENDPOINT, LEMONADE_ENDPOINT, managed
`endpoint_url`) except the probed default `DEFAULT_CHAT_BASE_URL`, so a
vLLM probed on :8000 was POSTed to `/chat/completions` and answered
`404 {"detail":"Not Found"}`. Add the `/v1` suffix.
2. Serve modal felt stuck. A managed serve blocks for up to 45s in its HTTP
readiness wait; during that window the job console is Running, where Esc/Enter
were swallowed and only `q` closed — but the footer advertised only Ctrl+C, so
the user had no visible exit. Esc now closes the overlay while a job runs (the
job keeps running in the background); the footer says so. Finished-job Esc/Enter
still dismiss back to the screen body. Enter stays inert while running to avoid
accidental dismissal.
Signed-off-by: Michael Roy <michael.roy@amd.com>
Address review on #68: - probed_default_used_when_nothing_configured_but_reachable only asserted base_url == DEFAULT_CHAT_BASE_URL, which is tautological w.r.t. the /v1 fix. Add an assertion that the *resolved* base_url ends in /v1 — it fails when the suffix is reverted (verified), so it actually guards the 404 fix. - Soften the const doc: VLLM_ENDPOINT / LEMONADE_ENDPOINT share the /v1 convention and port, not the host (they use localhost, this uses the unambiguous IPv4 loopback 127.0.0.1). Signed-off-by: Michael Roy <michael.roy@amd.com>
a1fa5f7 to
c44eb10
Compare
* fix(dash): add /v1 to default chat base url + let Esc leave a running job console
Two dash chat usability fixes surfaced when serving a model then chatting:
1. 404 on chat completions. The Rig OpenAI client builds the request URL as
`base_url + "/" + "chat/completions"`, so `base_url` must end in `/v1`.
Every endpoint carried it (VLLM_ENDPOINT, LEMONADE_ENDPOINT, managed
`endpoint_url`) except the probed default `DEFAULT_CHAT_BASE_URL`, so a
vLLM probed on :8000 was POSTed to `/chat/completions` and answered
`404 {"detail":"Not Found"}`. Add the `/v1` suffix.
2. Serve modal felt stuck. A managed serve blocks for up to 45s in its HTTP
readiness wait; during that window the job console is Running, where Esc/Enter
were swallowed and only `q` closed — but the footer advertised only Ctrl+C, so
the user had no visible exit. Esc now closes the overlay while a job runs (the
job keeps running in the background); the footer says so. Finished-job Esc/Enter
still dismiss back to the screen body. Enter stays inert while running to avoid
accidental dismissal.
Signed-off-by: Michael Roy <michael.roy@amd.com>
* fix(dash): guard the /v1 default with a revert-proof test + soften doc
Address review on #68:
- probed_default_used_when_nothing_configured_but_reachable only asserted
base_url == DEFAULT_CHAT_BASE_URL, which is tautological w.r.t. the /v1
fix. Add an assertion that the *resolved* base_url ends in /v1 — it fails
when the suffix is reverted (verified), so it actually guards the 404 fix.
- Soften the const doc: VLLM_ENDPOINT / LEMONADE_ENDPOINT share the /v1
convention and port, not the host (they use localhost, this uses the
unambiguous IPv4 loopback 127.0.0.1).
Signed-off-by: Michael Roy <michael.roy@amd.com>
---------
Signed-off-by: Michael Roy <michael.roy@amd.com>
Summary
Two dash chat usability bugs hit when serving a model and then chatting:
Chat completions 404 (
completion error, http error, 404 detail not found).The Rig OpenAI client builds the request URL as
base_url + "/" + "chat/completions",so
base_urlmust end in/v1. Every endpoint carried it (VLLM_ENDPOINT,LEMONADE_ENDPOINT, the managed-serviceendpoint_url) except the probeddefault
DEFAULT_CHAT_BASE_URL = "http://127.0.0.1:8000". So when the dashprobed a vLLM already listening on :8000 and fell back to that default, it POSTed
to
/chat/completionsand the server answered404 {"detail":"Not Found"}.Fix: add the
/v1suffix so it matches the other endpoints.Serve modal felt stuck. A managed serve blocks for up to 45s in its HTTP
readiness wait (
wait_for_service_http_ready). During that window the job consoleis
Running, whereEsc/Enterwere swallowed and onlyqclosed — but thefooter advertised only
Ctrl+C cancel, so there was no visible exit and itlooked frozen. Fix: Esc now closes the overlay while a job is running (the
job keeps running in the background, same as
qalready did), and the footersays so. Finished-job
Esc/Enterstill dismiss the console back to the screenbody;
Enterstays inert while running to avoid accidental dismissal.Scope of the Esc-while-running change
The change lives in the shared
job_console::on_console_key, so it applies toevery overlay that hosts a job console — 12 callers in total: serve, examine,
engine, runtime, install, update, config, logs, command, automations, services,
and onboarding.
Behavior change worth flagging:
Escon a running onboarding step(
onboarding.rs:180,*ob = None) now closes the whole onboarding wizard. Thisis consistent with what
qalready did there, and it's a deliberate decision —a running step keeps running in the background. Every caller's
Unhandledarm waschecked; none branch on
Esc, so nothing screen-specific is silently intercepted.Changes
llm.rs:DEFAULT_CHAT_BASE_URL→http://127.0.0.1:8000/v1(+ rationale doc),plus a revert-proof regression test asserting the resolved
base_urlends in/v1.ui/job_console.rs:Escon a running job →Closed; updated footer hint.ui/serve_wizard.rs,ui/examine_manager.rs: updated tests to the new contract.Test plan
cargo test -p rocm-dash-tui --lib -- --test-threads=1→ 461 passedcargo clippy -p rocm-dash-tui --all-targets→ clean/v1suffix is revertedStacked on #67 (retire-legacy-tui).