feat(vllm): add dynamic LoRA support to unified backend - #10347
Conversation
e5808e4 to
aeddbd1
Compare
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. WalkthroughThis PR implements a unified engine-update surface across Python/Rust backends and uses it to add dynamic-LoRA loading/unloading to vLLM. The changes establish engine discovery/dispatch hooks ( ChangesEngine update surface and vLLM dynamic-LoRA
🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/src/dynamo/common/backend/engine.py (1)
33-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
modelexplicitly required inGenerateRequest.
GenerateRequestisTypedDict(total=False), so the plainmodel: strkey is non-required by default. Mark it asRequired[str]to keep the cross-layer request contract intact.Proposed fix
class GenerateRequest(TypedDict, total=False): @@ - model: str + model: Required[str]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/common/backend/engine.py` around lines 33 - 50, GenerateRequest currently uses TypedDict(total=False) and declares model: str which is treated as optional; change the model field to be explicitly required by using Required[str] (i.e. model: Required[str]) in the GenerateRequest TypedDict and add the appropriate import for Required (from typing or typing_extensions consistent with project usage) so the request contract is enforced; update the GenerateRequest definition and imports only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/src/dynamo/vllm/llm_engine.py`:
- Around line 635-646: The current idempotency path in load_lora treats any
entry in self.loaded_loras as fully healthy and returns early, which can mask
partial failures when rollback/discovery reconciliation fails; update the logic
in load_lora (and the similar blocks around the current checks at the sections
handling lines ~635, ~724-733, and ~830-839) to distinguish between "loaded" vs
"published/reconciled" state—either by adding a separate registry (e.g.,
self.published_loras or a status flag on the loaded entry) or by checking a
published marker on the LoRA entry before short-circuiting; change the
early-return to only succeed when the LoRA is both loaded and
published/reconciled, and ensure that on rollback/discovery failure you either
remove the partial entry from self.loaded_loras or mark it as failed so
subsequent load_lora calls will retry discovery reconciliation.
In `@lib/backend-common/src/worker.rs`:
- Around line 1009-1020: The current engine_update_callback forwards any JSON
body to engine.engine_update(...) which can cause handler exceptions for
non-object bodies; update engine_update_callback to first validate the incoming
serde_json::Value (use body.is_object()) and if it's not an object return a
structured client error (e.g., an Err with a clear message like "invalid engine
update body: expected JSON object for <update_name>") instead of calling
engine.engine_update; only call engine.engine_update(update_name, body).await
when the body is an object to avoid HTTP 500s from scalar/array payloads.
In `@lib/runtime/src/system_status_server.rs`:
- Around line 588-593: Update the error message produced by the anyhow::bail
call so it is neutral about operator actions: locate the bail that references
endpoint_name in system_status_server.rs (the anyhow::bail(...) that currently
says "LoRA management not available: no '{}' handler is registered ... Ensure
the worker was started with LoRA support enabled."), and replace the message
with a neutral statement such as "LoRA management not available: no '{}' handler
is registered (neither a local LoRA endpoint nor an engine update); this backend
does not expose LoRA management." Keep the endpoint_name interpolation and
overall error type the same.
---
Outside diff comments:
In `@components/src/dynamo/common/backend/engine.py`:
- Around line 33-50: GenerateRequest currently uses TypedDict(total=False) and
declares model: str which is treated as optional; change the model field to be
explicitly required by using Required[str] (i.e. model: Required[str]) in the
GenerateRequest TypedDict and add the appropriate import for Required (from
typing or typing_extensions consistent with project usage) so the request
contract is enforced; update the GenerateRequest definition and imports only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61015477-d71d-4543-bf41-344adfe6b57d
📒 Files selected for processing (10)
components/src/dynamo/common/backend/README.mdcomponents/src/dynamo/common/backend/engine.pycomponents/src/dynamo/vllm/llm_engine.pycomponents/src/dynamo/vllm/tests/test_vllm_lora.pylib/backend-common/CLAUDE.mdlib/backend-common/Cargo.tomllib/backend-common/src/engine.rslib/backend-common/src/worker.rslib/bindings/python/rust/backend.rslib/runtime/src/system_status_server.rs
Closes the unified-backend vLLM LoRA parity gap (DIS-2032). Adds dynamic adapter load/unload/list, discovery (MDC) publish/unpublish, and per-request LoRA routing to the unified vLLM engine. LoRA ops ride a new `engine_update`/`supported_updates` surface (sibling to `engine_control`), exposed at `/engine/update/load_lora|unload_lora|list_loras` via the shared engine_routes registry. A one-time `on_endpoint_ready` handoff gives the Python engine its runtime Endpoint (before route registration, fatal on failure) so it can publish its own discovery records. A `/v1/loras` compat shim in the per-worker system status server falls back to the engine_routes registry so the legacy HTTP surface keeps working for unified workers, with a clean error when LoRA is unavailable. Scope is unified vLLM only; legacy (worker_factory) vLLM, SGLang, and TRT-LLM paths are untouched. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
- test_vllm_lora.py: importorskip vllm.usage.usage_lib so the pytest-marker-report hook skips collection when vllm is absent (llm_engine.py imports it; the lint env has no vllm). - llm_engine.py: guard engine_client is None in load_lora/unload_lora so mypy narrows the type before add_lora/remove_lora. - worker.rs: collapse engine_update_callback signature to one line to satisfy cargo fmt. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Migrate every engine-control route from bare /engine/<name> to the symmetric /engine/control/<name>, paired with /engine/update/<name> for engine-managed assets (LoRA), giving controls and updates clear separation and semantic readability. Updates the unified Worker registration plus the legacy vLLM/SGLang/TRT-LLM register_engine_route paths, docs, examples, elastic-EP scale scripts, and tests. No bare-route backward-compat alias. Also addresses review feedback on unified vLLM LoRA: separate loaded vs published adapter state, validate engine-update request bodies, neutralize the LoRA-unavailable message, and use the configured KV-event block size when publishing LoRA discovery cards. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
418d3bb to
1e7f23e
Compare
Resolve CI failures on the unified LoRA path: - Annotate self._endpoint as Optional[Endpoint] and narrow it in _publish_lora_card so register_model type-checks (mypy). - Derive (model_type, worker_type, needs) from disaggregation_mode when publishing the LoRA card so prefill workers no longer advertise adapters as decode-capable chat/completions models, and skip tool/reasoning parsers for prefill (mirrors base-model registration). - Set _kv_event_block_size in the test helper that bypasses __init__. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
…-lora Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Move on_endpoint_ready ahead of local_model.attach in serve_with_orchestrator. The handoff is fatal, so running it first means a failure leaves nothing published to discovery and no stale entry to reclaim. It still runs before register_engine_controls, preserving the /engine/* race fix. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
- _resolve_lora_request rejects an unknown or just-unloaded adapter name when LoRA is enabled instead of silently serving the base model; base-model names and the LoRA-disabled case keep the existing passthrough behavior. - Replace the unbounded per-name lock dict with fixed lock striping, bounding lock memory while preserving same-name serialization and removing the eviction race the old design warned about. - cleanup() now clears the serving endpoint and LoRA bookkeeping so a shut-down engine holds no dangling references. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
…-lora Signed-off-by: Connor Carpenter <connorc@nvidia.com> # Conflicts: # lib/backend-common/src/worker.rs
The main merge dropped the trait-method delegations for supported_updates, engine_update, and on_endpoint_ready from `impl LLMEngine for PyLLMEngine`, leaving the PyEngineCore helpers orphaned. clippy denies dead_code, so the python bindings failed to compile. Re-add the three delegations. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Overview:
Closes the unified-backend vLLM LoRA parity gap (DIS-2032). The legacy
(non-unified) vLLM path already supported dynamic LoRA; the unified vLLM engine
did not. This adds dynamic adapter load / unload / list, discovery (MDC)
publish / unpublish, and per-request LoRA routing to the unified vLLM
engine.
Scope: unified vLLM only. Legacy (
worker_factory.py) vLLM, SGLang, andTRT-LLM paths are untouched and inherit no LoRA controls.
Details:
engine_update/supported_updatessurface (sibling to the existingengine_control/supported_controls), for engine-managed assets like LoRA.Both share the single
engine_routes()registry and the single/engine/{*path}route; namespacing is by the registered key(
control/<name>vsupdate/<name>). The engine receives the bare op name.POST /engine/update/load_lora,POST /engine/update/unload_lora,POST /engine/update/list_loras(gated on
--enable-loraandDYN_LORA_ENABLED).on_endpoint_readyhandoff: a new one-time optional trait/ABC methodhands the Python engine its runtime
Endpointso it can publish its owndiscovery records (
register_model/unregister_model). The Worker calls itbefore registering engine updates and treats failure as fatal to
startup (closes the race where a route could fire before the engine stashed
the endpoint).
/v1/lorascompatibility shim:/v1/lorasroutes are registered wheneverDYN_LORA_ENABLED=true, regardless of backend.call_lora_endpointnow fallsback to the
engine_routes()registry (update/<name>) so the legacy HTTPsurface keeps working for unified workers, returning a clean "LoRA
management not available" error (not the opaque "local endpoint not found")
when neither registry holds the name. Legacy
/v1/lorasHTTP semantics(
status:"error"→ HTTP 500) are preserved.Testing:
components/src/dynamo/vllm/tests/test_vllm_lora.py,17 tests) — passed on a real-vLLM GPU session: update gating / dispatch,
LoRA↔control disjointness, load/unload happy-path + rollback,
on_endpoint_readystash,generate()lora_requestrouting.cargo test -p dynamo-backend-common(handoff ordering /fatal-on-failure) and
cargo test -p dynamo-runtime(call_lora_endpointfallback + clean-error + error-status semantics). These are behind the
integrationfeature and need live etcd+NATS → run in CI.DYN_LORA_ENABLED=true --enable-lora;exercise
/engine/update/{load_lora,unload_lora,list_loras}and the/v1/lorascompat shim;model=<lora_name>completion; adapter appears in/v1/models; base-model requests unaffected.Out of scope: SGLang / TRT-LLM LoRA; Worker-side (Rust) LoRA MDC publishing;
new LoRA storage/download/cache; multimodal LoRA.
Where should the reviewer start?
lib/backend-common/src/worker.rs— theon_endpoint_readyhandoff ordering(before update registration, fatal on failure) and
engine_updateregistration.lib/runtime/src/system_status_server.rs— the/v1/lorascompat shim incall_lora_endpoint(engine_routes()fallback + clean unsupported error).components/src/dynamo/vllm/llm_engine.py— LoRA load/unload/list,per-request routing in
generate(), and the--enable-lora+DYN_LORA_ENABLEDgating.
lib/backend-common/src/engine.rs/components/src/dynamo/common/backend/engine.py— the new
on_endpoint_readyandengine_update/supported_updatessurface.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Documentation