feat(models): deployments_plugin backend (AIRCORE-759 PR1)#663
Merged
Conversation
…ject Introduce a selectable ServiceBackend that compiles model deployments into Volume + OnFailure puller + Always server plugin entities and projects plugin status onto ModelDeployment. Old docker/nim_operator backends remain for dual-path rollout (AIRCORE-759 PR1). Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (11)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds a configurable ChangesDeployments Plugin Integration
Sequence Diagram(s)sequenceDiagram
participant ModelsService
participant DeploymentsPluginServiceBackend
participant NemoEntitiesClient
ModelsService->>DeploymentsPluginServiceBackend: resolve and create deployment
DeploymentsPluginServiceBackend->>NemoEntitiesClient: create volume, puller, and server
NemoEntitiesClient-->>DeploymentsPluginServiceBackend: return entity status and endpoints
DeploymentsPluginServiceBackend-->>ModelsService: return DeploymentStatusUpdate
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.
Actionable comments posted: 3
🧹 Nitpick comments (3)
services/core/models/tests/unit/controllers/backends/deployments_plugin/test_status.py (2)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd missing
map_statuscases.
_STATUS_MAPalso definesUNKNOWN,DELETING, andSUCCEEDED, none of which are parametrized here.♻️ Extend parametrize cases
[("PENDING", "PENDING"), ("STARTING", "PENDING"), ("READY", "READY"), ("FAILED", "ERROR"), ("LOST", "LOST"), + ("UNKNOWN", "UNKNOWN"), ("DELETING", "DELETING"), ("SUCCEEDED", "PENDING")],🤖 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_status.py` around lines 14 - 19, Extend the parameter list in test_map_status to cover the UNKNOWN, DELETING, and SUCCEEDED statuses defined by _STATUS_MAP, using each status’s mapped model value. Keep the existing map_status assertions and cases unchanged.
34-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test for volume-FAILED or default-PENDING branches.
aggregate_statushas a dedicated volume-FAILED → ERROR branch and a default PENDING fallback (status.py lines 76-86), neither of which is exercised here.🤖 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_status.py` around lines 34 - 37, Extend test_missing_ready_server_is_lost_and_failed_puller_is_error to cover aggregate_status branches for a FAILED volume returning ERROR and for the default PENDING fallback. Add focused assertions using suitable Deployment inputs, while preserving the existing LOST and failed-puller ERROR assertions.services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.py (1)
161-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail fast instead of compiling a known-broken Docker+LoRA spec.
The comment states create "will ERROR" for LoRA on Docker until the docker backend supports multi-container configs. Right now that failure surfaces later as an opaque plugin-level error. Raising a clear
ValueErrorhere (or increate_model_deployment) forruntime == Runtime.DOCKER and lora_enabledwould give operators an actionable message instead.🤖 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/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.py` around lines 161 - 176, Update the LoRA container handling in the compiler flow to raise a clear ValueError when resolved.runtime is Runtime.DOCKER and resolved.view.lora_enabled, before constructing or returning the known-invalid multi-container deployment spec. Include an actionable message identifying that Docker does not yet support LoRA or multi-container DeploymentConfigs, and preserve the existing non-Docker container behavior.
🤖 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
`@services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/backend.py`:
- Around line 107-122: Update _rollback_create so each failed entities.delete
operation logs the exception and the entity type/name being cleaned up instead
of silently passing. Preserve the best-effort rollback behavior and continue
attempting subsequent deletions, using the module’s existing logging mechanism.
In
`@services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/config.py`:
- Around line 31-32: Update the delete_wait_seconds and delete_poll_seconds
fields in the deployment configuration model to enforce strictly positive values
using the model’s existing validation mechanism with gt=0, preventing zero or
negative deletion timing settings.
In
`@services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/status.py`:
- Around line 70-81: Update the deployment status handling around the puller and
volume failure checks to treat both LOST and UNKNOWN statuses as terminal ERROR
states, alongside FAILED. Preserve the existing status messages, fallback text,
and substrate error_details for each corresponding component, and ensure
aggregate_status no longer lets these statuses fall through to PENDING.
---
Nitpick comments:
In
`@services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.py`:
- Around line 161-176: Update the LoRA container handling in the compiler flow
to raise a clear ValueError when resolved.runtime is Runtime.DOCKER and
resolved.view.lora_enabled, before constructing or returning the known-invalid
multi-container deployment spec. Include an actionable message identifying that
Docker does not yet support LoRA or multi-container DeploymentConfigs, and
preserve the existing non-Docker container behavior.
In
`@services/core/models/tests/unit/controllers/backends/deployments_plugin/test_status.py`:
- Around line 14-19: Extend the parameter list in test_map_status to cover the
UNKNOWN, DELETING, and SUCCEEDED statuses defined by _STATUS_MAP, using each
status’s mapped model value. Keep the existing map_status assertions and cases
unchanged.
- Around line 34-37: Extend
test_missing_ready_server_is_lost_and_failed_puller_is_error to cover
aggregate_status branches for a FAILED volume returning ERROR and for the
default PENDING fallback. Add focused assertions using suitable Deployment
inputs, while preserving the existing LOST and failed-puller ERROR assertions.
🪄 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: Enterprise
Run ID: bb6fc8d7-2fe2-465a-9109-69d8973c5a26
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
docs/set-up/config-reference.mdxservices/core/models/pyproject.tomlservices/core/models/src/nmp/core/models/config.pyservices/core/models/src/nmp/core/models/controllers/backends/__init__.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/__init__.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/config.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/executor.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/naming.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/resolve.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/status.pyservices/core/models/src/nmp/core/models/controllers/backends/registry.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_backend.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_executor.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_naming.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_status.py
Contributor
|
Teardown leftover substrate before recreate, use controlled delete on rollback, and wire LoRA sidecar env/mounts/scratch volume so drift recovery and adapters match existing backends. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Fix CI and CodeRabbit feedback on the deployments_plugin models backend: - Lazy-import the backend so the core models service (and its standalone wheel) boots without the optional nemo_deployments_plugin package. The vendored models-service extra excludes plugins, so eager import broke the wheel build+test and Kind e2e jobs. - Regenerate the vendored SDK metadata (adds nemo-platform-plugin to the models-service extra) to fix lint-sdk-vendored. - Constrain delete_wait_seconds/delete_poll_seconds with gt=0. - aggregate_status now surfaces LOST/UNKNOWN puller/volume substrate instead of hiding a dead prerequisite behind PENDING. - Log rollback failures instead of swallowing them. - Fail fast with an actionable error for LoRA on the docker runtime. - Extend status/backend unit tests for the above. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/core/models/tests/unit/controllers/backends/deployments_plugin/test_backend.py (1)
151-169: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that pending teardown prevents substrate creation.
This test only checks the returned status and message. Also assert
_entities.createwas not called; otherwise a regression could reportPENDINGwhile creating over substrate that is still being deleted.🤖 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 151 - 169, Update test_create_waits_when_prior_teardown_incomplete to patch or mock _entities.create and assert it was not called after create_model_deployment returns PENDING. Keep the existing status and teardown-message assertions unchanged.
🤖 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
`@services/core/models/tests/unit/controllers/backends/deployments_plugin/test_backend.py`:
- Around line 151-169: Update test_create_waits_when_prior_teardown_incomplete
to patch or mock _entities.create and assert it was not called after
create_model_deployment returns PENDING. Keep the existing status and
teardown-message assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d20e2c3c-b9e8-42e2-90bb-312bb824b71b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/nemo_platform/pyproject.tomlservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/__init__.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/config.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/status.pyservices/core/models/src/nmp/core/models/controllers/backends/registry.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_backend.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_status.py
🚧 Files skipped from review as they are similar to previous changes (5)
- services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/init.py
- services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/config.py
- services/core/models/tests/unit/controllers/backends/deployments_plugin/test_status.py
- services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.py
- services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/backend.py
Escalate stuck PENDING deployments to ERROR via pending_timeout_seconds, fail fast when no executor is configured for the runtime, and extend unit tests for teardown-wait, timeout, and executor validation. Signed-off-by: Tyler Bray <tbray@nvidia.com>
mckornfield
approved these changes
Jul 14, 2026
- Drop __getattr__ lazy export; registry imports backend directly with ImportError guidance when nemo-deployments-plugin is missing - Document none backend as intentional no-op for platform.runtime none - Use ModelEntity | None, extract files URL vars, structured rollback logs - Make LoRA busybox init image configurable (docker.io/library/busybox) - Clarify executor_for_runtime misconfiguration scenario in docstring Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
enabled auto-merge
July 14, 2026 23:01
7 tasks
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
deployments_pluginServiceBackendthat compiles NIM/vLLM/generic deployments into deployments-plugin entities:Volume+ OnFailure puller + Always server withprerequisite: succeeded.Deploymentstatus ontoModelDeployment(trust READY; substrate details inerror_details); missing server after READY →LOST.docker/nim_operator/nonebackends for dual-path rollout. Enable viamodels.controller.backends.deployments_plugin.enabled: true(and disable the other backend).Linear: AIRCORE-759
This is PR1 of 2. PR2 (delete backends + drop
k8s-nim-operator) waits for this to merge.Resolved open questions
ModelDeployment(spec/status/status_message/error_details/model_provider_id/host_url). Plugin ownsDeployment/Volume/DeploymentConfig. Models projects plugin status with Layer-3 enrichment; raw substrate inerror_details.READY→LOSTfor manual recreate. No auto-orphan cleanup of old docker containers / NIMService CRs / PVCs.docker_executor/k8s_executor/default_executoron the new backend config; mapplatform.runtime→ named executor. No per-ModelDeploymentexecutor field.init_containersnative-sidecar (restart_policy: Always). docker: second container emitted (plugin docker v1 is still single-container — LoRA-on-docker via plugin needs a follow-up).Test plan
pytest services/core/models/tests/unit/controllers/backends/deployments_plugin/ -v(17 tests)ty checkon new package; ruff cleandeployments_pluginagainst a local platform with deployments executors and deploy a vLLM (and NIM) model end-to-endEnablement sketch
Summary by CodeRabbit