Retire legacy tui.rs: route rocm bootstrap setup to the dash onboarding, then delete the ~45k-line module - #67
Conversation
9af7363 to
711d3ae
Compare
rominf
left a comment
There was a problem hiding this comment.
The retirement and the dead-code prune look clean — compiler/clippy-driven, no dangling references to the removed helpers, and the bootstrap reroute is wired correctly (the onboarding overlay renders and takes input regardless of the active tab, and nothing else shadows it on the bootstrap path). The split into reroute-then-delete makes a 45k-line removal reviewable. One thing holds me back from approving:
The dropped flags are asserted as intentional but not justified. The UX note says:
The legacy install-folder picker (
--prefix) and Release/Nightly channel toggle are intentionally dropped.
"Intentionally dropped" tells us it wasn't an accident, but not why — and these are user-facing capabilities being removed. Could you record the reasoning, for reviewers now and readers later? Specifically:
- Why drop them rather than carry them into the dash onboarding wizard — a current limitation of the wizard, a deliberate UX simplification, or deferred work?
- Is losing the install-location choice and the nightly channel acceptable for the
bootstrap setupaudience, or is a follow-up planned to restore them?
A sentence or two of rationale (plus a tracking issue if it's deferred) would make this tradeoff reviewable rather than just declared.
Related concrete consequence: the non-interactive fallback in run_setup still tells users to "choose an install folder":
"ROCm setup needs an interactive terminal. Run `rocm` from a terminal to choose an install folder and set up ROCm/TheRock."
With the folder picker gone this is now inaccurate, and bare rocm opens the dash chat rather than the setup wizard — so the guidance points at the wrong command too. Worth updating in this PR to point at rocm bootstrap setup and drop the folder-picker wording.
|
It was intended but only until my next sprint. Restored in PR #73 |
711d3ae to
43ce97b
Compare
…wizard The legacy bootstrap onboarding lived in apps/rocm/src/tui.rs (fused to the monolith App/run_loop). The dash crate already has a modern, App-free onboarding wizard (rocm-dash-tui ui::onboarding over jobs/job_console), so `rocm bootstrap setup` now launches the dashboard straight into that overlay instead of the legacy TUI. - Add ResolvedArgs::start_onboarding; honor it at app startup by opening the onboarding overlay. - Add dash::run_bootstrap (+ testable bootstrap_args helper) mirroring the run/run_chat runtime path. - Rewire bootstrap.rs to call dash::run_bootstrap. - tui::run_bootstrap_setup is now unused; marked #[allow(dead_code)] for this commit only — tui.rs is deleted in the next commit. UX note: per decision, bootstrap adopts the dash onboarding UX (install release wheels to the default location + adopt-existing-folder); the legacy install-folder picker (--prefix) and Release/Nightly toggle are dropped. Signed-off-by: Michael Roy <michael.roy@amd.com>
`rocm bootstrap setup` now routes to the dash onboarding (previous commit), so the ~45k-line monolith TUI module is fully unreferenced. - Delete apps/rocm/src/tui.rs (incl. its test module). - main.rs: drop `mod tui;`, the _RETAINED_TUI_ENTRY anchor + its guard test, and refresh the reroute prose comments. - Prune helpers that only tui.rs used (compiler/clippy -D warnings driven): driver-install TUI-approval cluster (+test), render_sidebar_text + friendly_provider_label, freeform_plan_uses_provider, the *_for_tui log renderers + logs_browser_page_count, tui_help_text; gate build_freeform_plan #[cfg(test)] (now test-only over the live build_freeform_plan_with_recipes). - comfyui.rs: drop render_tui_status/render_tui_logs/is_installed/running_url (+ their tests); keep render_logs/models_folder/render_models_path and the unmanaged-url helpers. - docs/testing.md: remove the obsolete legacy-TUI navigability test list. The reroute tests (launch_default / Command::Chat must not call tui::run) are retained and still pass. Signed-off-by: Michael Roy <michael.roy@amd.com>
Address review on #67: with the reroute to the dash onboarding wizard, the non-interactive fallback message was inaccurate on two counts — bare `rocm` now opens the dash chat (not setup), and the guided onboarding no longer has an install-folder picker. Point users at `rocm bootstrap setup` and drop the folder-picker wording. Signed-off-by: Michael Roy <michael.roy@amd.com>
a3a3691 to
5574565
Compare
|
Thanks @rominf — both points addressed, plus a history cleanup.
Separately, this branch's CI was red only on Commit signatures + sign-off: a GitHub web merge commit (resolving conflicts) landed without a Re-review when you have a moment? |
* 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>
|
Correction to my previous comment: I had it backwards on #68. #68 was merged into this branch ( Final branch: 4 commits, linear on |
Retiring apps/rocm/src/tui.rs removed its test module, which held ~40 of the test names docs/testing.md invokes by exact name (onboarding/folder validation, log pagination, proposal-approval, provider-key confirmation, TUI mode-state, and the proposal_sandbox_args/*_cache_warm builders). Those `cargo test <name>` filters now match zero tests silently, so the doc read as covered when the coverage is gone. Remove the stale references, collapse the now-single-entry blocks, and keep only the tests that still exist. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
rominf
left a comment
There was a problem hiding this comment.
Approving — this addresses my earlier review, and the split-out was the right call.
Verified during review:
- The dead-code prune is complete and correct: every removed symbol (comfyui
render_tui_status/render_tui_logs/is_installed/running_url, the driver-install TUI-approval cluster,render_sidebar_text,friendly_provider_label,freeform_plan_uses_provider, the*_for_tuirenderers,logs_browser_page_count,tui_help_text,_RETAINED_TUI_ENTRY) has zero remaining references tree-wide, and every "kept" helper still has live callers.build_freeform_plan's#[cfg(test)]gate is correct (its only caller is also test-gated).cargo build+clippy -D warningsclean. - Surviving shipping logic kept its own tests, so no silent coverage regression — the coverage gate (78.53% > 70%) holds.
- The chat
/v1fix is correct and the regression test is genuinely revert-proof (asserts the resolved probed defaultends_with("/v1"), matching Rig'sbase_url + "/" + pathjoin to.../v1/chat/completions).
I pushed one follow-up commit (docs cleanup): docs/testing.md still invoked ~40 test names by exact name that lived in the deleted tui.rs test module — cargo test <name> matches those silently with 0 tests, so the doc read as covered when it wasn't. Removed the stale references, keeping only tests that still exist.
Non-blocking, for a follow-up (not gating this PR):
- The production
run_setup -> dash::run_bootstrapdispatch has no test; onlybootstrap_argsis covered. - Esc-closes-console now fans out to the other manager screens with no regression test guarding that behavior.
- Tradeoff worth noting: for an explicitly
--foregroundserve, Esc drops the wizard state with no in-TUI path back to that still-running job (fine for the default managed path via the Services tab). Consistent with the stated "keeps running in the background" intent — flagging so it's a conscious choice.
Summary
Retires the legacy ~45k-line monolith TUI (
apps/rocm/src/tui.rs), which untilnow was kept compiling only via the
_RETAINED_TUI_ENTRYdead-code anchor. Theone live consumer —
rocm bootstrap setup(viarun_bootstrap_setup) — isre-routed to the modern, App-free onboarding wizard that already lives in the
dash crate (
rocm-dash-tuiui::onboardingoverjobs/job_console).Follows up @rominf's review on #62 ("legacy code isn't fully retired"), split
into its own change as requested so #62 didn't carry the giant delete.
Commit 1 — route
rocm bootstrap setupto the dash onboardingResolvedArgs::start_onboarding; honored at app startup by opening theonboarding overlay.
dash::run_bootstrap(+ testablebootstrap_argshelper) reusing therun/run_chatruntime path;bootstrap.rscalls it.Commit 2 — delete tui.rs + prune the dead-code cascade
apps/rocm/src/tui.rs(incl. its test module); dropmod tui;, theretention anchor, and its guard test.
TUI-approval cluster,
render_sidebar_text,friendly_provider_label,freeform_plan_uses_provider, the*_for_tuilog renderers,logs_browser_page_count,tui_help_text);build_freeform_planis now#[cfg(test)]over the livebuild_freeform_plan_with_recipes.comfyui.rs: droprender_tui_status/render_tui_logs/is_installed/running_url(+ their tests); liverender_logs/models_folder/render_models_pathand the unmanaged-URL helpers stay.docs/testing.md: remove the obsolete legacy-TUI navigability test list.Commit 3 — correct the non-interactive setup guidance
bootstrap.rs: the non-interactive fallback previously told users to run barerocm"to choose an install folder". Post-reroute that's wrong on both counts(bare
rocmopens the dash chat; the wizard has no folder picker). Now pointsat
rocm bootstrap setupand drops the folder-picker wording. (Addresses@rominf's review.)
Commit 4 — chat
/v1404 + Esc closes a running serve modal (#68)mainwith this PR:
DEFAULT_CHAT_BASE_URLgains the required/v1suffix (Rigappends
chat/completionstobase_url), andEscnow closes a job consolewhile the job keeps running. Includes the review follow-ups from fix(dash): chat /v1 404 + Esc closes a running serve modal #68 (a
revert-proof
/v1regression test + softened doc wording).UX note (decision + rationale)
Bootstrap adopts the dash onboarding UX (install release wheels to the default
location + adopt-existing-folder).
onboarding wizard by feat(dash): restore Release/Nightly channel toggle + version pin in onboarding #73 (now on
main), sobootstrap setupretains channelchoice.
--prefix): deferred, not removed by design. Thedash onboarding wizard doesn't yet have an install-location step, so the guided
flow installs to the default location only. The
--prefixplumbing still exists(
install_manager.rs, and the chat tool path), so restoring it is awizard-UI addition rather than a re-plumb. Tracked in Restore install-folder (--prefix) choice in the dash onboarding wizard #75.
Test plan
cargo fmt --all --checkcargo clippy --locked --workspace --all-targets -- -D warningscargo xtask manifest --checkhawkeye check --config licenserc.tomlcargo llvm-cov … --fail-under-lines 70(78.53% lines)cargo test --workspace --all-targets -- --test-threads=1cargo test --workspace --all-targets(parallel)cargo xtask verify-commits --base origin/main(linear onto main; all commits signed + signed-off)rocm bootstrap setuplaunches the dash onboarding and installs / adopts (needs an interactive terminal + GPU host)Note: rebased linearly onto
mainto drop a GitHub web merge commit that lacked aSigned-off-bytrailer (the sole CI failure). #68's squash-merge commit was re-applied here as a signed + signed-off cherry-pick, so its fix is retained on the path tomain; the final tree matches the pre-rebase merged state.