fix(models): lora model revision re-pull logic - #508
Merged
Conversation
The k8s direct-emission reconciler stamps the model source on both the weight-puller Job and the PVC, but _existing_model_source() only read the Job. At P3 the Job is deleted to release the RWO volume, so for an already-serving deployment the Job is gone and the lookup returned None -- causing a later model-revision change to skip the re-pull branch and serve stale weights. Read the source from the Job first, then fall back to the (long-lived) PVC annotation, and propagate non-404 API errors instead of swallowing them. Signed-off-by: Ben McCown <bmccown@nvidia.com>
Add executor_config.run_as_user / run_as_group to override the serving pod's securityContext uid/gid on the k8s backend. An explicit value wins over the engine default (vLLM pins its image's 2000/0; generic runs as the image's own user). Applied to both the serving container and the weight-puller Job so pulled weights are owned by the same user the server reads them as. Ignored by the docker backend, which does not set a container user. Regenerates the OpenAPI spec for the new fields. (SDK regeneration via the Stainless flow is a separate maintainer/CI step.) Signed-off-by: Ben McCown <bmccown@nvidia.com>
Stainless regeneration picks up the new ContainerExecutorConfig.run_as_user / run_as_group fields added in the prior commit. Note: the TypeScript web SDK (web/packages/sdk) was not regenerated here because node_modules are not installed in this worktree; `make update-web-sdk` (pnpm) should be run before merge to keep the web SDK in sync. 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds optional ChangesPod securityContext UID/GID overrides and model-source PVC fallback
Sequence Diagram(s)sequenceDiagram
participant Schema as ContainerExecutorConfig
participant View as DeploymentConfigView
participant Reconciler as K8sReconciler
participant Job as puller Job
participant PVC as PVC
Schema->>View: run_as_user/run_as_group
View->>Reconciler: flattened overrides
Reconciler->>Reconciler: _pod_user(view)
Reconciler->>Job: read model-source annotation
Job-->>Reconciler: annotation or 404
Reconciler->>PVC: read annotation on 404
PVC-->>Reconciler: annotation or none
Suggested labels
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.
🧹 Nitpick comments (2)
services/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.py (2)
2514-2595: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-404
ApiExceptionregression test.These additions only exercise the 404 fallback path. The source change also promises that non-404 errors from
read_namespaced_jobandread_namespaced_persistent_volume_claimare re-raised, and that behavior is still untested here.Possible test shape
+def test_existing_model_source_reraises_job_api_errors(k8s_backend): + backend = _vllm_backend(k8s_backend) + backend._batch_v1.read_namespaced_job.side_effect = _api_exception(500) + + _sync_reconcilers(backend) + with pytest.raises(k8s_client.exceptions.ApiException): + backend._k8s_reconciler._existing_model_source("md-default-qwen") + + +def test_existing_model_source_reraises_pvc_api_errors(k8s_backend): + backend = _vllm_backend(k8s_backend) + backend._batch_v1.read_namespaced_job.side_effect = _api_exception(404) + backend._core_v1.read_namespaced_persistent_volume_claim.side_effect = _api_exception(500) + + _sync_reconcilers(backend) + with pytest.raises(k8s_client.exceptions.ApiException): + backend._k8s_reconciler._existing_model_source("md-default-qwen")🤖 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/test_k8s_nim_operator_backend.py` around lines 2514 - 2595, The new tests cover only the 404 fallback path, but they do not verify that non-404 ApiException failures are propagated. Add a regression test around _existing_model_source in test_k8s_nim_operator_backend.py that makes read_namespaced_job or read_namespaced_persistent_volume_claim raise an ApiException with a non-404 status and asserts the exception is re-raised instead of falling back to PVC or returning None. Use the existing _vllm_backend, _sync_reconcilers, and _existing_model_source setup patterns to keep the test aligned with the current behavior.
1937-1937: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd concrete types for the new helper params.
run_as_userandrun_as_groupare nullable ints in the production contract, but both helpers leave them untyped. Annotate them asint | Noneso these fixtures stay aligned with the config shape.As per coding guidelines,
**/*.py: Always prefer concrete type hints over string-based ones in Python code; do not import types under TYPE_CHECKING, instead import types as regular imports when possible.Also applies to: 2005-2012
🤖 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/test_k8s_nim_operator_backend.py` at line 1937, The helper _vllm_config in the k8s nim operator backend tests leaves run_as_user and run_as_group untyped even though they model nullable integers in the production config. Update the helper signature (and the related fixture helpers around the same area) to use concrete type hints of int | None for these parameters so the test helpers stay aligned with the config contract and follow the Python typing guideline.Source: Coding guidelines
🤖 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/test_k8s_nim_operator_backend.py`:
- Around line 2514-2595: The new tests cover only the 404 fallback path, but
they do not verify that non-404 ApiException failures are propagated. Add a
regression test around _existing_model_source in
test_k8s_nim_operator_backend.py that makes read_namespaced_job or
read_namespaced_persistent_volume_claim raise an ApiException with a non-404
status and asserts the exception is re-raised instead of falling back to PVC or
returning None. Use the existing _vllm_backend, _sync_reconcilers, and
_existing_model_source setup patterns to keep the test aligned with the current
behavior.
- Line 1937: The helper _vllm_config in the k8s nim operator backend tests
leaves run_as_user and run_as_group untyped even though they model nullable
integers in the production config. Update the helper signature (and the related
fixture helpers around the same area) to use concrete type hints of int | None
for these parameters so the test helpers stay aligned with the config contract
and follow the Python typing guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3b791391-20b3-488c-ab4b-498b7bce8065
⛔ Files ignored due to path filters (4)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/inference/container_executor_config.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/inference/container_executor_config_param.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/inference/test_deployment_configs.pyis excluded by!sdk/**
📒 Files selected for processing (7)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/core/models/src/nmp/core/models/controllers/backends/common.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/k8s.pyservices/core/models/src/nmp/core/models/schemas.pyservices/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.py
Contributor
|
mckornfield
approved these changes
Jun 29, 2026
Address PR review nits: - Add regression tests asserting _existing_model_source re-raises non-404 ApiException from both the Job and PVC reads (only the 404 fallback was covered before). - Add concrete int | None type hints to the run_as_user/run_as_group params on the _vllm_config / _generic_config test helpers. Signed-off-by: Ben McCown <bmccown@nvidia.com>
Contributor
Author
|
Response from claude session: Addressed both CodeRabbit nitpicks in c348d0a:
|
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.
Follow-ups to generic engine support (#476)
Addresses two items deferred from the generic engine PR review.
1. Fix stale-weights bug in the k8s re-pull check (
fix)_existing_model_source()only read the model-source annotation off the weight-puller Job, but that Job is deleted at P3 once weights are pulled. For an already-serving deployment the lookup returnedNone, so a latermodel_revisionchange skipped the re-pull and served stale weights.Now reads the Job first, then falls back to the (long-lived) PVC annotation, and propagates non-404 API errors instead of swallowing them. Pre-existing bug in the vLLM weighted path; surfaced more broadly now that weight-aware generic deployments ride the same path.
2. Configurable
run_as_user/run_as_group(feat)Adds
executor_config.run_as_user/run_as_groupto override the serving pod's securityContext uid/gid on the k8s backend. An explicit value wins over the engine default (vLLM pins its image's 2000/0; generic runs as the image's own user). Applied to both the serving container and the puller Job so pulled weights are owned by the same user the server reads them as. Ignored by the docker backend (it sets no container user).Includes OpenAPI + Python SDK regeneration.
Summary by CodeRabbit
New Features
run_as_userandrun_as_groupsettings to container executor configuration.securityContext(k8s only; docker ignores them).Bug Fixes