feat(models): support LoRA deployments on the docker backend - #870
Merged
Conversation
The deployments-plugin docker backend was single-container: it rejected init_containers and multi-container DeploymentConfigs, so LoRA deployments (server + adapters sidecar + lora-cache-init) could not run and the models controller fast-failed docker + LoRA with a 'single-container' ERROR. Add targeted multi-container orchestration to the docker backend so it can run the LoRA shape the models compiler already emits, then lift the guardrail: - containers.py: replace the single-container validator with build_docker_plan, which splits a DeploymentConfig into init containers, a primary (server) container, and sidecars. Rejects sidecars that declare host ports. - backend.py create_deployment: run init containers to completion in order, then the primary (publishes host ports, owns GPUs), then sidecars that join the primary's network namespace (network=container:<server>) and share volumes, with no ports/GPUs of their own. Image pulls tolerate local-only images (e.g. the nmp-api sidecar) by falling back to a locally present copy. - backend.py read_status: gate READY on sidecar health (a stopped sidecar keeps the deployment in STARTING; its Always restart policy recreates it). - backend.py delete_deployment: tear down the whole labeled container group (primary + sidecars), not just the primary. - labels.py: add companion_container_name and a container-role label so group members are discoverable. - models deployments_plugin/backend.py: remove the docker + LoRA fast-fail. Verified in a docker-backend dev deployment: vLLM + LoRA reaches READY, the adapters sidecar loads the adapter into vLLM over the shared network namespace, and gateway chat-completion works for both the base model and the adapter; delete tears down the full group. Signed-off-by: Ben McCown <bmccown@nvidia.com>
benmccown
force-pushed
the
docker-lora-support
branch
from
July 23, 2026 19:17
4d13fcb to
034c3d8
Compare
benmccown
marked this pull request as ready for review
July 23, 2026 19:27
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughDocker deployments now support grouped init, server, and sidecar containers. Startup, image handling, readiness, rollback, and deletion operate across the group. Docker LoRA deployments proceed through model deployment creation instead of failing validation. ChangesDocker multi-container deployment
LoRA deployment integration
Sequence Diagram(s)sequenceDiagram
participant ModelDeploymentController
participant DockerDeploymentBackend
participant DockerDeploymentPlan
participant DockerEngine
ModelDeploymentController->>DockerDeploymentBackend: create_deployment(config)
DockerDeploymentBackend->>DockerDeploymentPlan: build_docker_plan(config)
DockerDeploymentBackend->>DockerEngine: pull all group images
DockerDeploymentBackend->>DockerEngine: run init containers sequentially
DockerDeploymentBackend->>DockerEngine: start server container
DockerDeploymentBackend->>DockerEngine: start sidecars in server network
DockerDeploymentBackend->>DockerEngine: check sidecar readiness
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/core/models/tests/unit/controllers/backends/deployments_plugin/test_backend.py (1)
179-182: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the LoRA container shape, not just the server deployment.
This only proves that
my-dep-serverwas created. A regression that drops the adapters sidecar—or its shared-network, role, port, or GPU settings—would still pass. Inspect the created server configuration and assert the multi-container LoRA contract.🤖 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 `@services/core/models/tests/unit/controllers/backends/deployments_plugin/test_backend.py` around lines 179 - 182, Strengthen the test around the created “my-dep-server” Deployment by inspecting its server configuration and asserting the complete multi-container LoRA contract, including the adapters sidecar, shared network, container role, port, and GPU settings. Keep the existing creation assertion, but verify the relevant configuration fields rather than only the deployment name.plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py (1)
544-578: 🚀 Performance & Scalability | 🔵 TrivialExtra
containers.liston every readiness poll._sidecars_healthyruns a labeledcontainers.listfor every readyAlwaysdeployment, including single-container ones with no sidecars — added daemon load on the reconciler's hot path. Consider gating the enumeration behind a cheap signal (e.g. a per-deployment "has sidecars" marker/label set at create time) so single-container deployments skip the query.🤖 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 `@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py` around lines 544 - 578, The _sidecars_healthy method performs a Docker containers.list query on every readiness poll, even for deployments without sidecars. Add and maintain a cheap per-deployment sidecar-presence signal, such as a label set during creation, and have _sidecars_healthy return the existing healthy result without enumerating containers when that signal indicates no sidecars; preserve the current labeled lookup and health checks for deployments marked as having sidecars.
🤖 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.
Nitpick comments:
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py`:
- Around line 544-578: The _sidecars_healthy method performs a Docker
containers.list query on every readiness poll, even for deployments without
sidecars. Add and maintain a cheap per-deployment sidecar-presence signal, such
as a label set during creation, and have _sidecars_healthy return the existing
healthy result without enumerating containers when that signal indicates no
sidecars; preserve the current labeled lookup and health checks for deployments
marked as having sidecars.
In
`@services/core/models/tests/unit/controllers/backends/deployments_plugin/test_backend.py`:
- Around line 179-182: Strengthen the test around the created “my-dep-server”
Deployment by inspecting its server configuration and asserting the complete
multi-container LoRA contract, including the adapters sidecar, shared network,
container role, port, and GPU settings. Keep the existing creation assertion,
but verify the relevant configuration fields rather than only the deployment
name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ec38fb6-ef60-4987-a76b-bd1a1639108b
📒 Files selected for processing (9)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/containers.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.pyplugins/nemo-deployments/tests/unit/backends/docker/docker_helpers.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_containers.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/backend.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_backend.pyservices/core/models/tests/unit/controllers/test_deployment_reconciler.py
💤 Files with no reviewable changes (1)
- services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/backend.py
Contributor
|
tylersbray
reviewed
Jul 23, 2026
tylersbray
reviewed
Jul 23, 2026
tylersbray
reviewed
Jul 23, 2026
tylersbray
reviewed
Jul 23, 2026
tylersbray
reviewed
Jul 23, 2026
tylersbray
reviewed
Jul 23, 2026
tylersbray
approved these changes
Jul 23, 2026
tylersbray
left a comment
tylersbray
left a comment
Contributor
There was a problem hiding this comment.
Non-blocking approval, thanks for tackling this. The comments in the main body of the code are really helpful. GPU accounting looks good. New test coverage looks good despite the nit for one more and I appreciate the lora fixture.
Review follow-ups for the docker multi-container LoRA support: - _sidecars_healthy now derives the expected sidecar set from the DeploymentConfig, so a *removed* (not just exited) sidecar is detected as not-ready. Deployments with no sidecars short-circuit and skip the Docker containers.list on the readiness hot path. - Narrow the image-pull local-fallback except to docker ImageNotFound/NotFound so unexpected client errors are not masked as 'image present locally'. - Document the sidecar network=container:<server> netns coupling as a known limitation across a server-container recreate. - Note that init containers are intentionally CPU-only (no device_requests). - Strengthen the docker LoRA backend test to assert the full multi-container contract (server + adapters sidecar; sidecar has no ports/GPU; server owns the port and GPU; lora-cache-init init present). - Add read_status tests for sidecar readiness gating: running sidecar -> READY, stopped sidecar -> STARTING, removed sidecar -> STARTING. Signed-off-by: Ben McCown <bmccown@nvidia.com>
AnuradhaKaruppiah
pushed a commit
to AnuradhaKaruppiah/nemo-platform
that referenced
this pull request
Jul 24, 2026
…NeMo#870) * feat(models): support LoRA deployments on the docker backend The deployments-plugin docker backend was single-container: it rejected init_containers and multi-container DeploymentConfigs, so LoRA deployments (server + adapters sidecar + lora-cache-init) could not run and the models controller fast-failed docker + LoRA with a 'single-container' ERROR. Add targeted multi-container orchestration to the docker backend so it can run the LoRA shape the models compiler already emits, then lift the guardrail: - containers.py: replace the single-container validator with build_docker_plan, which splits a DeploymentConfig into init containers, a primary (server) container, and sidecars. Rejects sidecars that declare host ports. - backend.py create_deployment: run init containers to completion in order, then the primary (publishes host ports, owns GPUs), then sidecars that join the primary's network namespace (network=container:<server>) and share volumes, with no ports/GPUs of their own. Image pulls tolerate local-only images (e.g. the nmp-api sidecar) by falling back to a locally present copy. - backend.py read_status: gate READY on sidecar health (a stopped sidecar keeps the deployment in STARTING; its Always restart policy recreates it). - backend.py delete_deployment: tear down the whole labeled container group (primary + sidecars), not just the primary. - labels.py: add companion_container_name and a container-role label so group members are discoverable. - models deployments_plugin/backend.py: remove the docker + LoRA fast-fail. Verified in a docker-backend dev deployment: vLLM + LoRA reaches READY, the adapters sidecar loads the adapter into vLLM over the shared network namespace, and gateway chat-completion works for both the base model and the adapter; delete tears down the full group. Signed-off-by: Ben McCown <bmccown@nvidia.com> * fix(models): address docker LoRA review feedback Review follow-ups for the docker multi-container LoRA support: - _sidecars_healthy now derives the expected sidecar set from the DeploymentConfig, so a *removed* (not just exited) sidecar is detected as not-ready. Deployments with no sidecars short-circuit and skip the Docker containers.list on the readiness hot path. - Narrow the image-pull local-fallback except to docker ImageNotFound/NotFound so unexpected client errors are not masked as 'image present locally'. - Document the sidecar network=container:<server> netns coupling as a known limitation across a server-container recreate. - Note that init containers are intentionally CPU-only (no device_requests). - Strengthen the docker LoRA backend test to assert the full multi-container contract (server + adapters sidecar; sidecar has no ports/GPU; server owns the port and GPU; lora-cache-init init present). - Add read_status tests for sidecar readiness gating: running sidecar -> READY, stopped sidecar -> STARTING, removed sidecar -> STARTING. Signed-off-by: Ben McCown <bmccown@nvidia.com> --------- Signed-off-by: Ben McCown <bmccown@nvidia.com>
This was referenced Jul 27, 2026
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
2 participants
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
The deployments-plugin docker backend was single-container: it rejected
init_containersand multi-containerDeploymentConfigs, so LoRA deployments (server + adapters sidecar +lora-cache-init) could not run there. The models controller fast-failed docker + LoRA with a "single-container" ERROR and told users to deploy on kubernetes.This adds targeted multi-container orchestration to the docker backend so it can run the LoRA shape the models compiler already emits, then lifts the guardrail.
Changes
backends/docker/containers.py— replace the single-container validator withbuild_docker_plan(), which splits aDeploymentConfiginto init containers, a primary (server) container, and sidecars. Rejects sidecars that declare host ports (they share the primary's netns).backends/docker/backend.pycreate_deployment: run init containers to completion in order → primary (publishes host ports, owns GPUs) → sidecars that join the primary's network namespace (network=container:<server>) and share volumes, with no ports/GPUs of their own. Group image pulls reuse the existing_pull_image(NGC auth) and fall back to a locally present image (e.g. the local-onlynmp-apiadapters sidecar) instead of failing.read_status: gate READY on sidecar health (a stopped sidecar keeps the deployment STARTING; its Always restart policy recreates it).delete_deployment: tear down the whole labeled container group (primary + sidecars), not just the primary.backends/labels.py— addcompanion_container_nameand a container-role label so group members are discoverable.deployments_plugin/backend.py— remove the docker + LoRA fast-fail guardrail.Tests
New/updated unit tests (deployments-plugin: 302 passed; models + reconciler suites green; lint clean):
test_containers.py— plan builder splits init/primary/sidecars; rejects sidecar ports / empty config.network=container:<server>and no sidecar ports; init non-zero fails before the server starts; delete removes the whole group; pull-fallback (local image present vs absent).Manual verification (dev pod, docker backend)
docker vLLM + LoRA (Qwen3-1.7B +
linear-algebraadapter):lora-cache-initruns to completion → server +lora-adapterssidecar come up as a group (sidecar shares the server netns) → READY.default--linear-algebrainto vLLM over shared localhost; provider/v1/modelslists base + adapter.deletetears down the full group and frees the GPU.Notes
main.nmp-apiimage. On the docker backend this image must already be present on the host (built/loaded locally, e.g.nmp-api:local) — it is a local-only tag and not pullable from a registry. The pull step tolerates this via a local-copy fallback, but a fresh host without the image will fail at sidecar start. This is inherent to the local docker dev/test flow (k8s pulls the platform image normally).Review feedback addressed
Follow-up to the review on PR #870 (tylersbray non-blocking approval + CodeRabbit nits):
_sidecars_healthynow derives the expected sidecar set from theDeploymentConfig, so a removed (not just exited) sidecar is detected as not-ready; single-container deployments short-circuit thecontainers.listentirely.exceptto dockerImageNotFound/NotFoundso unexpected client errors aren't masked.network=container:<server>netns coupling across a server recreate.lora-cache-initpresent).read_statustests for sidecar readiness gating: running sidecar → READY; stopped sidecar → STARTING; removed sidecar → STARTING.Summary by CodeRabbit