feat(models): generic engine support - #476
Merged
Merged
Conversation
Signed-off-by: Ben McCown <bmccown@nvidia.com>
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)
📝 WalkthroughWalkthroughAdds ChangesGeneric engine support
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 7
🧹 Nitpick comments (1)
services/core/models/tests/unit/test_model_deployment_config_service_unit.py (1)
167-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden the generic-validation regression coverage.
The negative case only matches
image_namewhile both required fields are missing, so it would still pass ifhealth_check_pathvalidation regressed. Add a case that isolates missinghealth_check_path, and mirror the same checks onupdate_deployment_config()since that path was changed too.🤖 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/test_model_deployment_config_service_unit.py` around lines 167 - 213, The generic validation tests only cover a missing image_name case, so a regression in health_check_path could still slip through. Add a focused test in test_model_deployment_config_service_unit.py that creates a generic request with image_name set but health_check_path omitted and asserts create_deployment_config() raises on health_check_path, then add matching update_deployment_config() coverage for the same missing-field scenario to exercise the updated validation path.
🤖 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/api/service/model_deployment_config_service.py`:
- Around line 41-49: The generic executor validation only uses strip() for
presence checks, so whitespace-padded values can still be persisted and later
fail downstream. Update model_deployment_config_service’s generic-field handling
around the executor_config.image_name and executor_config.health_check_path
checks to either normalize these values by trimming before saving or reject them
when the trimmed value differs from the original. Make the fix in the same
validation path that raises the ValueError for missing executor_config fields so
these inputs are handled consistently.
In
`@services/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.py`:
- Around line 901-912: The generic engine path in create_args still applies
/model-store and /scratch mounts even though the ENGINE_GENERIC branch is meant
to run the image raw with only the user’s env and args. Update the container
spec assembly in creation_reconciler so create_args["volumes"] is only populated
for engines that actually need platform storage, and leave it unset or empty for
ENGINE_GENERIC while preserving the existing volume behavior for the other
engine branches.
- Around line 496-499: The generic backend branch in creation_reconciler should
not allow the model-puller path when the deployment engine is generic. Update
the puller decision in _needs_puller() (and any related caller logic) so it
explicitly checks config_engine(state.config) != ENGINE_GENERIC before
scheduling downloads, even when model_weights_type is present. Use the existing
ENGINE_GENERIC and _needs_puller symbols to keep the fix localized and ensure
generic deployments never trigger weight pulling.
In
`@services/core/models/src/nmp/core/models/controllers/backends/generic_compiler.py`:
- Around line 34-37: The `generic_compiler` image reference normalization still
allows blank-space tags to pass through as invalid values. Update the logic in
the compiler helper that returns `view.image_name` and `image_tag` so both
fields are trimmed before use: keep the existing `image_name` presence check,
return the stripped `image_name`, and compute `image_tag` from the stripped
value with a fallback to `"latest"` when the trimmed tag is empty. Refer to the
return path in the `generic_compiler` flow that currently assigns `image_tag`
and returns the pair.
In
`@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py`:
- Around line 244-246: The engine selection logic in the reconciler lookup
currently falls through to _nim_reconciler for every non-vLLM/non-generic value,
which incorrectly treats unknown engine strings as supported. Update the
backend’s engine dispatch method to explicitly reject invalid engines by
returning _unsupported_engine() unless the engine matches ENGINE_VLLM,
ENGINE_GENERIC, or the supported NIM case, and keep the None/unsupported
behavior aligned with the docstring.
In
`@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/k8s.py`:
- Around line 527-540: The generic update path in _update_generic currently
relies on _create_generic_serving_objects, which treats a 409 as “already
exists” and skips reapplying changed spec fields. Update the logic so generic
resources are actually patched on update: in the _update_generic flow, or via a
dedicated helper called from _create_generic_serving_objects, use
patch_namespaced_deployment and patch_namespaced_service when the
Deployment/Service already exists, ensuring image, args, env, health path, GPU,
and labels are refreshed instead of left stale.
- Line 353: Keep _vllm_objects_exist() limited to vLLM objects only: it is used
by the vLLM update path, so remove the ENGINE_GENERIC match from the labels
check and have the helper return true only for objects labeled with ENGINE_VLLM.
Update the isinstance(labels, dict) / labels.get("nmp.nvidia.com/engine") logic
in k8s.py so generic Deployments with the same name do not satisfy the vLLM
existence check and bypass the PVC/puller flow.
---
Nitpick comments:
In
`@services/core/models/tests/unit/test_model_deployment_config_service_unit.py`:
- Around line 167-213: The generic validation tests only cover a missing
image_name case, so a regression in health_check_path could still slip through.
Add a focused test in test_model_deployment_config_service_unit.py that creates
a generic request with image_name set but health_check_path omitted and asserts
create_deployment_config() raises on health_check_path, then add matching
update_deployment_config() coverage for the same missing-field scenario to
exercise the updated validation path.
🪄 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: 43116890-b366-45c6-a719-4c037a2ca7a4
📒 Files selected for processing (11)
services/core/models/src/nmp/core/models/api/service/model_deployment_config_service.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.pyservices/core/models/src/nmp/core/models/controllers/backends/generic_compiler.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/k8s.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/vllm_k8s_compiler.pyservices/core/models/tests/unit/controllers/backends/test_generic_compiler.pyservices/core/models/tests/unit/controllers/backends/test_vllm_k8s_compiler.pyservices/core/models/tests/unit/controllers/test_docker_backend.pyservices/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.pyservices/core/models/tests/unit/test_model_deployment_config_service_unit.py
Contributor
|
Address CodeRabbit review feedback on generic engine support: - Make generic deployments weight-aware on both backends: when the config resolves to a fileset-backed model, pull weights (docker puller / k8s PVC+puller) and mount /model-store; otherwise run the image raw with no platform volumes. - k8s: refactor K8sReconciler to choose staged vs. immediate rollout by weight presence (not engine), patch serving Deployment/Service in place on update, and reject unknown engines explicitly instead of defaulting to NIM. - Validate + reject whitespace-padded generic image_name/health_check_path; trim defensively in the generic compiler. - Expand unit tests (validation, compiler, docker + k8s weight-aware paths, update patching). Signed-off-by: Ben McCown <bmccown@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/k8s_nim_operator/reconcilers/k8s.py`:
- Around line 217-224: The re-pull check in k8s.py is using
_existing_model_source() before the puller Job is gone, so it can miss the model
source stored on the PVC for already-serving weighted deployments. Update the
reconciliation flow around the create/resolved path and _existing_model_source()
so it reads the source from the PVC when the Job no longer exists, then compare
that value against source_tag before deciding whether to delete resources and
call self.create(resolved).
🪄 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: 23c83761-a91d-4403-befc-76c9149e62ff
📒 Files selected for processing (9)
services/core/models/src/nmp/core/models/api/service/model_deployment_config_service.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.pyservices/core/models/src/nmp/core/models/controllers/backends/generic_compiler.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/k8s.pyservices/core/models/tests/unit/controllers/backends/test_generic_compiler.pyservices/core/models/tests/unit/controllers/test_docker_backend.pyservices/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.pyservices/core/models/tests/unit/test_model_deployment_config_service_unit.py
🚧 Files skipped from review as they are similar to previous changes (5)
- services/core/models/tests/unit/controllers/backends/test_generic_compiler.py
- services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py
- services/core/models/src/nmp/core/models/controllers/backends/generic_compiler.py
- services/core/models/src/nmp/core/models/api/service/model_deployment_config_service.py
- services/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.py
mckornfield
approved these changes
Jun 26, 2026
mckornfield
left a comment
mckornfield
left a comment
Contributor
There was a problem hiding this comment.
out of curiosity, do we have a deployment example with the generic compiler, like I'd guess sglang?
tylersbray
approved these changes
Jun 26, 2026
tylersbray
left a comment
tylersbray
left a comment
Contributor
There was a problem hiding this comment.
Giving a +1 approval. Added comments for the medium-level things that got flagged by machine.
- Reject engine=generic with model_spec.lora_enabled in the deployment
config validation (no engine compiler to wire the adapter sidecar, so it
would otherwise be silently ignored).
- k8s: restore the mid-pull no-op on update — when the serving Deployment
is absent but the puller Job is still present, accept the update without
re-running create(), only falling back to create() on genuine drift.
- Reword the create() docstring ("start at phase P0").
- Add unit tests for the generic+LoRA rejection and the mid-pull no-op update.
Signed-off-by: Ben McCown <bmccown@nvidia.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
3 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.
Here's a concise PR description:
feat(models): add
genericengine support for ModelDeploymentsWhat
Adds a new
genericinference engine alongsidenimandvllm, on both the docker and k8s service backends. A generic deployment is "declare a container image + tag + health path and run it" — no inference-engine compiler, no model-weight pull. The user'sadditional_argsbecome the container command verbatim andadditional_envsbecome its env.This covers cases like classification NIMs (e.g. NemoGuard JailbreakDetect) and any self-contained server image, without needing engine-specific compile logic.
Key changes
generic_compiler.py(new): trivial backend-agnostic compiler — resolves the image (required, no platform default), and passes args/env through verbatim.engine=genericnow requiresexecutor_config.image_nameandhealth_check_pathat create/update time (no platform defaults exist for an arbitrary container). Returns a 400 with a clear message.genericin image resolution and env/args compilation; runs the image as a local container with the engine + health-path labels; no model puller; no LoRA sidecar.generic(andvllm) through the direct-emissionK8sReconciler, which now branches on engine. The generic path emits a Deployment + Service immediately with no PVC / no puller Job / nomodel-storemount (it has no weights).vllm_k8s_compiler.compile_deploymentgained amount_model_storeflag to support this. Delete cascades via ownerRef as before.Testing
vllm/vllm-openaias a generic container servingfacebook/opt-125m:/v1/completions.model-storemount,/healthprobe, reaches READY, serves inference, deletes cleanly.Summary by CodeRabbit
model-storemounting.health-check-path, including weight-fileset pull behavior.