ROCMAI-52: Modularize engines/lemonade/src/lib.rs - #457
Open
jussielo-amd wants to merge 3 commits into
Open
jussielo-amd wants to merge 3 commits into
jussielo-amd wants to merge 3 commits into
Conversation
Split the 7,886-line lib.rs (actual size, not the ticket's stale 4,915-line estimate) into six modules following the full-domain-extraction convention established by vllm's Phase 1 split: install.rs (archive download/verify/extract, cache locking), backend_alignment.rs (ROCm-SDK to llama.cpp backend Tier1/2/3 alignment), process.rs (spawn/env, backend install/pull/model-load, direct-llama-server readiness), runtime_dir.rs (XDG_RUNTIME_DIR hardening), direct_llama.rs (HF checkpoint/GGUF resolution), and state.rs (service state I/O, device policy). lib.rs keeps only CLI/envelope dispatch and the service-lifecycle orchestration that calls across those modules. No behavior change: verified via cargo build/clippy/test across the workspace, all 121 pre-split tests partitioned and passing, and the mock-lane e2e scenario (serve-18). Updates the architecture doc in the same PR per EAI-7768's rules. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
jussielo-amd
requested review from
fredespi
and
a balanced review from Copilot
September 29, 2026 07:59
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The private backend-alignment module unintentionally hides a previously public constant, introducing an API compatibility regression.
Review effort: Balanced
Findings: 1
What changed in this PR
Refactors the Lemonade engine adapter into focused domain modules while preserving dispatch and lifecycle orchestration in lib.rs.
Changes:
- Extracts installation, backend alignment, process, runtime-directory, direct-llama, and state logic.
- Moves associated unit tests alongside each domain.
- Updates the architecture documentation.
| File | Description |
|---|---|
engines/lemonade/src/lib.rs |
Retains dispatch and lifecycle orchestration. |
engines/lemonade/src/install.rs |
Handles runtime installation and archive caching. |
engines/lemonade/src/backend_alignment.rs |
Handles ROCm backend alignment and selection. |
engines/lemonade/src/process.rs |
Manages processes, environment, and readiness. |
engines/lemonade/src/runtime_dir.rs |
Secures runtime-directory preparation. |
engines/lemonade/src/direct_llama.rs |
Handles GGUF resolution and direct serving. |
engines/lemonade/src/state.rs |
Manages service state and device selection. |
docs/architecture.md |
Documents the new module structure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… root backend_alignment is a private module, so moving this constant into it silently dropped a path that was pub at the crate root before the split. rocm-engine-lemonade is consumed as a lib by apps/rocm, so this is a real (if currently unused) API surface change, not the "every pub item in a binary crate is unreachable" case docs/architecture.md's disabled unreachable_pub lint covers. Re-export restores the pre-split path. Flagged by Copilot review on PR ROCm#457. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
windows-build-and-test failed: rocm-engine-lemonade no longer built on
Windows once split, because several imports are only used by unix-only
(runtime_dir.rs) or non-Windows (state.rs's spawn_serve_http_background)
code paths. In the monolithic file these were folded in among hundreds of
other always-used imports and never went unused on any one platform;
isolated per-module, the platform-specific half becomes visible per file.
- runtime_dir.rs: `anyhow!`/`bail!`/`OsStr` are only reachable from
`#[cfg(unix)]` functions; gate the imports the same way. Also gate the
test module's `use super::*`/`use crate::process::{...}` behind
`#[cfg(target_os = "linux")]`, since their only consumer
(`mod child_runtime_dir`) is itself Linux-only.
- state.rs: `Command as ProcessCommand`/`Stdio` are only used by
`spawn_serve_http_background`'s `#[cfg(not(windows))]` variant.
Verified via `cargo build --workspace --all-targets --target
x86_64-pc-windows-gnu` with `RUSTFLAGS=-D warnings` (matching
actions-rust-lang/setup-rust-toolchain's default, which is what turned
these warnings into the CI failure) — clean, 0 warnings.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
engines/lemonade/src/lib.rs(7,886 lines — the ticket's stated 4,915-line estimate was stale, per the epic's own re-verification rule) into six modules following the full-domain-extraction convention established by Phase 1 (engines/vllm, PR EAI-8033: Modularize engines/vllm/src/lib.rs #436):install.rs— embeddable-runtime archive download/verify/extract, cache locking, manifest persistencebackend_alignment.rs(new, not in the ticket's original 4-module list) — the ROCm-SDK-to-llama.cpp-backend Tier1/2/3 alignment state machine, a ~800-line self-contained subsystem the stale estimate didn't account forprocess.rs—lemond/llama-server spawn, env/path setup, backend install/pull/model-load, direct-serve readiness pollingruntime_dir.rs(new) —XDG_RUNTIME_DIRtier1/2/3 hardening, a ~500-line security-critical subsystem with its own 25 dedicated testsdirect_llama.rs— Hugging Face checkpoint/GGUF resolution and the direct-llama-server serve pathstate.rs— service state file I/O, device-policy/GPU-selection parsing, port/endpoint helperslib.rskeeps only CLI/envelope dispatch and the service-lifecycle orchestration that calls across the six modulesdocs/architecture.mdin the same PR (per ROCMAI-72/EAI-7768's rule — not deferred).main(per the epic's own rule to never trust a child ticket's stored line ranges) surfaced two large, genuinely self-contained subsystems — backend alignment and runtime-dir hardening — that the epic's own "full domain extraction" convention says deserve their own files rather than being folded intoprocess.rsas a second god-file.Test plan
cargo build,cargo test -p rocm-engine-lemonade— 120/121 tests pass (1 is#[cfg(windows)]-gated and doesn't compile on this Linux host, same as pre-split behavior)cargo clippy -p rocm-engine-lemonade --all-targets -- -D warnings— cleancargo build --workspace,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace --all-targets --exclude e2e-cucumber— all cleancargo xtask manifest --check— no-op (no dependency changes)cargo fmt --all --check/prek run --all-files --no-group local-tools— cleancargo xtask e2e -- -n "^serve-18"(mock-lane lemonade canary) — passes. GPU-gated scenarios (serve-07,serve-08,runtime-16,runtime-17) could not run in this sandbox (no GPU device) — deferred to CI's GPU lanerun_cli/handle_envelope) is structurally unchanged: sameCommandKind/EngineMethodarms in the same order, calling the same handler functions