Limit serving engines to Lemonade and vLLM - #79
Conversation
4147004 to
a84f44f
Compare
|
Note on the CodeQL check: it flagged 3 |
Narrow the supported model backends to Lemonade (with its bundled engines) and vLLM, and remove the standalone atom, llama-cpp, pytorch, and sglang engine integrations. This shrinks the maintenance, testing, and packaging surface. vLLM is kept as a separate backend since Lemonade's bundled vLLM is not yet tuned for Instinct. - Delete the four engine crates and their workspace members, path deps, and serve/registry/inventory/dispatch wiring. - Constrain the user-facing `--engine` / `engines install|shell` / `config set[-default]-engine` arguments to `lemonade`/`vllm` via a clap value_parser, so `--help` and shell completion advertise exactly those two. - Retarget built-in model recipes off the removed engines: GGUF recipes serve via Lemonade; HF safetensors recipes serve via vLLM; existing vLLM preferences are unchanged. - Prune the EngineKind registry, TUI engine lists, the local-assistant system prompt, docs, CI, and acceptance/smoke scripts (Linux and Windows). Framework diagnostics for PyTorch and llama.cpp (as user frameworks that `rocm diagnose`/`examine` help debug) and Lemonade's own llama.cpp backend are intentionally retained. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
a84f44f to
f74180e
Compare
volen-silo
left a comment
There was a problem hiding this comment.
Reviewed at f74180e (fan-out review over the 54 files + synthesis; findings verified against source). Verification gates green locally: cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace --all-targets (all suites, 0 failed), and scripts/smoke_local.py all pass — confirming the PR's claim. Approving — the removal is complete and clean, no blocking issues.
Removal completeness is compiler-enforced: EngineKind is now exhaustively {Vllm, Lemonade} with no wildcard arm, so nothing could be left silently half-wired, and a clean -D warnings clippy run means no orphaned dead helper survived. Cargo.toml members / apps/rocm/Cargo.toml path-deps / Cargo.lock are all pruned; dispatch bails or safe-defaults on unknown engine strings (no panic/unwrap). Every intended survivor was verified as intentional, not stale removal: PyTorch/llama.cpp diagnostics (examine/diagnose/fix), Lemonade's bundled llama.cpp, PyTorch runtime-dep handling (libatomic/libnuma/OpenMPI) that vLLM + the SDK wheel need, and the "One Dark — Atom" editor theme. The --engine value_parser restriction is correctly wired to all five subcommands (serve, engines install|shell, config set-engine|set-default-engine) with help + completion.
Non-blocking
-
Recipe metadata mismatch —
crates/rocm-core/src/lib.rs:5319. TheQwen/Qwen2.5-1.5B-Instructrecipe (aliasesqwen2.5,qwen-small) is retargeted topreferred_engines: ["lemonade"]but keepsloader: "transformers",dtype: "float16", and a safetensorsartifact_hint— the only lemonade recipe not markedloader: llamacpp / dtype: gguf. At serve timeresolve_lemonade_model_refsilently remaps it toQwen3-4B-Instruct-2507-GGUF(a ~4 GiB Q4 GGUF), sorocm modelsshows float16/transformers/1.5B while a 4B GGUF actually loads. Serving works (lemonade remaps internally); the metadata just misreports. Suggest aligning it with itsqwen/qwen-ggufsiblings:loader: "llamacpp",dtype: "gguf", and anartifact_hintnoting the GGUF it resolves to. -
Back-compat UX for a stale saved default_engine.
default_enginedeserializes as a plain string and isn't validated at config load (thevalue_parseronly guards CLI args). An upgrader withdefault_engine = "pytorch"(or sglang/atom/llama-cpp) hits a clean-but-developer-flavored failure at serve time —engine \pytorch` is not built into this rocm binary/ \"build the workspace or install the engine package\". Not a blocking regression (no panic/corruption;rocm config clear-default-enginerecovers it, and the platform default is valid), but the message offers no migration guidance for an ordinary upgrader. Suggest: afterselect_serve_engine, if the configured default isn't in the inventory, bail with something like \"the saved default_engine '{engine}' is no longer supported; run \rocm config set-default-engine lemonade` to update your config." -
Stale engine-list strings.
crates/rocm-core/src/lib.rs:5473— the liveqwen3_5"not a verified PyTorch smoke path" warning (shown viarocm models) now describes a vLLM recipe; reword or drop.- Pre-existing, outside this diff, but this PR makes them wrong:
apps/rocm/src/serve_summary.rs:15module doc(lemonade, vLLM, SGLang, PyTorch, llama.cpp, atom), andskills/rocm-cli-assistant/SKILL.mdstill lists SGLang/PyTorch/llama.cpp as serving engines (would steer the assistant to manage engines that no longer exist). Worth a follow-up trim tolemonade/vllm.
Tradeoffs (deliberate, no change requested)
- Lemonade's internal safetensors-alias → GGUF remap is what lets the mismatched
qwen2.5recipe "just work"; it hides the metadata drift in #1 until someone inspectsrocm models. therock_sdk_install_test.pynow asserts on theexternal_vllmruntime_kindJSON field instead of the old llama-cpp log fragment — correct, slightly more format-coupled.
Positives
Compiler-enforced completeness via the exhaustive EngineKind match; clean single commit whose message explicitly documents the intentional survivors (no AI footer, DCO signed-off); no broken doc links (deleted docs/{atom,sglang}.md have zero inbound refs); README/MANIFEST/AGENTS/engine-plugins engine lists all updated; deleted *_therock_gpu_test.py scripts fully de-referenced from CI; no internal URLs / Jira refs introduced.
Deployment note
Breaking change: removed --engine values and stale saved default_engine values are rejected. Worth a release-note line. No DB/migration steps.
Align the Qwen2.5-1.5B recipe's loader/dtype/artifact_hint with the GGUF model Lemonade actually resolves it to, and drop stale mentions of removed serving engines (SGLang, PyTorch, atom, llama.cpp) from a recipe warning string, the serve-summary module doc, and the CLI assistant skill doc. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
|
Addressed the non-blocking findings in #79 (review) (a6da53d):
No CHANGELOG file exists in this repo, and the PR description already documents the breaking |
What
Narrows the supported model-serving backends to Lemonade and vLLM, and removes the standalone
atom,llama-cpp,pytorch, andsglangengine integrations (crates, workspace members, path deps, and all serve/registry/inventory/dispatch wiring).Why
Supporting Lemonade (with the engines it bundles) plus vLLM is a reasonable backend set for now, and it shrinks the maintenance, testing, and packaging surface considerably. vLLM stays a separate first-party backend rather than relying on Lemonade's bundled vLLM.
User-visible changes
rocm serve --engine,rocm engines install|shell, androcm config set-engine|set-default-enginenow accept onlylemonadeandvllm. These values are advertised in--helpand shell completion (via a clapvalue_parser), and an unknown engine is rejected with a clear error:Intentionally retained
rocm diagnose/examinestill help debug a user's own PyTorch/llama.cpp installs; that is a separate feature from serving backends.llama.cppbackend, which Lemonade uses internally.Scope of the change
Beyond the engine crates, this prunes the
EngineKindregistry, both engine inventories, the TUI engine lists, the local-assistant system prompt, docs (removes the dedicated ATOM/SGLang adapter docs), CI, and the acceptance/smoke scripts on both Linux and Windows.Verification
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(all suites), andscripts/smoke_local.pypass.