fix(deployments): restore NIM NGC authentication - #852
Merged
Conversation
Signed-off-by: Tyler Bray <tbray@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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:
📝 WalkthroughWalkthroughAdds controller-managed NGC secret references, resolves them during deployment execution, emits secret-backed NIM configuration, authenticates NGC Docker pulls, and updates Docker one-shot recreation and GPU cleanup behavior. ChangesNGC deployment flow
Sequence Diagram(s)sequenceDiagram
participant ModelCompiler
participant DeploymentBackend
participant PlatformSecrets
participant DockerEngine
ModelCompiler->>DeploymentBackend: provide secretRef-backed NGC_API_KEY
DeploymentBackend->>PlatformSecrets: resolve configured secret
PlatformSecrets-->>DeploymentBackend: return credential
DeploymentBackend->>DockerEngine: pull NGC image with auth_config
DockerEngine-->>DeploymentBackend: return pull result
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: 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
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py`:
- Around line 132-142: Update the exited one-shot container cleanup in the
backend recreate flow to catch Docker’s APIError from existing.remove, return a
failed BackendStatusUpdate instead of aborting, and preserve normal recreation
when removal succeeds. Add a regression test covering the removal failure and
verifying the failed status is returned.
🪄 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: 5e2379da-be3c-40f1-ac12-d48653c735ca
📒 Files selected for processing (11)
packages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/ngc.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/tests/unit/backends/docker/test_idempotency.pyplugins/nemo-deployments/tests/unit/backends/docker/test_image_pull_auth.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/ngc.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/nim_compiler.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_ngc.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_nim_compiler.py
Contributor
|
benmccown
requested changes
Jul 22, 2026
benmccown
left a comment
benmccown
left a comment
Contributor
There was a problem hiding this comment.
Flagging one concern with secrets being written to entity store unencrypted. Otherwise LGTM.
Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@plugins/nemo-deployments/openapi/openapi.yaml`:
- Around line 1089-1090: Update the OpenAPI schema path for
CreateDeploymentConfigRequest so secretRef is not accepted as a writable request
property. Split the environment schemas into request and response variants, or
mark secretRef as response-only, while preserving its availability in deployment
responses.
In
`@services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.py`:
- Around line 124-128: Update the environment construction around
platform_ngc_secret_ref() to detect when the input values contain NGC_API_KEY
but the secret reference resolves to None, and raise a clear configuration error
instead of omitting the credential. Preserve the existing behavior for valid
secret references and inputs without NGC_API_KEY.
🪄 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: 66a09b26-aed1-4cd9-bf8b-dbea58dad7c3
📒 Files selected for processing (14)
plugins/nemo-deployments/openapi/openapi.yamlplugins/nemo-deployments/src/nemo_deployments_plugin/api/v2/deployment_configs.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/entities.pyplugins/nemo-deployments/src/nemo_deployments_plugin/secrets.pyplugins/nemo-deployments/tests/unit/backends/docker/test_idempotency.pyplugins/nemo-deployments/tests/unit/backends/docker/test_image_pull_auth.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_backend.pyplugins/nemo-deployments/tests/unit/test_deployment_config_secret_refs.pyplugins/nemo-deployments/tests/unit/test_secrets.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_nim_compiler.py
💤 Files with no reviewable changes (1)
- services/core/models/tests/unit/controllers/backends/deployments_plugin/test_nim_compiler.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py
Use AsyncNeMoPlatform for secret resolution, keep secretRef off the public create request schema, and fail when an explicit NGC_API_KEY would be dropped. Signed-off-by: Tyler Bray <tbray@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 `@plugins/nemo-deployments/src/nemo_deployments_plugin/schema.py`:
- Around line 45-51: Update RequestEnvVar’s generated JSON Schema to declare
value and valueFrom as mutually exclusive, using the model’s schema
customization mechanism while preserving validate_single_source as runtime
validation. Ensure schemas reject payloads containing both properties.
🪄 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: ae06ad73-94d0-4b7c-ab22-c8b696ddd310
📒 Files selected for processing (7)
plugins/nemo-deployments/openapi/openapi.yamlplugins/nemo-deployments/src/nemo_deployments_plugin/api/v2/deployment_configs.pyplugins/nemo-deployments/src/nemo_deployments_plugin/schema.pyplugins/nemo-deployments/src/nemo_deployments_plugin/secrets.pyplugins/nemo-deployments/tests/unit/test_deployment_config_secret_refs.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.py
🚧 Files skipped from review as they are similar to previous changes (3)
- services/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.py
- services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/secrets.py
Expose the value/valueFrom mutual-exclusion constraint in the generated OpenAPI schema so clients cannot send payloads the API rejects. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Omit unresolved NGC_API_KEY at execution time so mock/local NIM images can reach READY when the platform secret and env fallback are absent. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-deployments/tests/unit/test_secrets.py (1)
114-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the NotFound path is exercised.
The current assertions also pass if the implementation skips Platform secret access and simply drops the variable. Verify the mocked client was awaited with the expected secret coordinates.
Proposed assertion
with ( patch("nemo_deployments_plugin.secrets.client_from_platform", return_value=secrets), patch("nemo_deployments_plugin.secrets.get_platform_config", return_value=platform), ): resolved = await resolve_deployment_config_secrets(MagicMock(), config) + secrets.access_secret.assert_awaited_once_with( + name="ngc-api-key", + workspace="system", + )🤖 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/tests/unit/test_secrets.py` around lines 114 - 126, The test for resolve_deployment_config_secrets must verify that the Platform secret lookup occurs before handling the NotFound result. Add an assertion on the mocked secrets.access_secret call confirming it was awaited with the expected platform and secret coordinates, while preserving the existing resolved configuration assertions.
🤖 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/tests/unit/test_secrets.py`:
- Around line 114-126: The test for resolve_deployment_config_secrets must
verify that the Platform secret lookup occurs before handling the NotFound
result. Add an assertion on the mocked secrets.access_secret call confirming it
was awaited with the expected platform and secret coordinates, while preserving
the existing resolved configuration assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 30dbf86d-b1f9-4fe0-b8a3-1bde1e1d8568
📒 Files selected for processing (2)
plugins/nemo-deployments/src/nemo_deployments_plugin/secrets.pyplugins/nemo-deployments/tests/unit/test_secrets.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/nemo-deployments/src/nemo_deployments_plugin/secrets.py
benmccown
approved these changes
Jul 23, 2026
anubhutivyas
approved these changes
Jul 23, 2026
AnuradhaKaruppiah
pushed a commit
to AnuradhaKaruppiah/nemo-platform
that referenced
this pull request
Jul 24, 2026
* fix(deployments): restore NIM NGC authentication Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): defer NGC secret resolution Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): tighten NGC secret request typing Use AsyncNeMoPlatform for secret resolution, keep secretRef off the public create request schema, and fail when an explicit NGC_API_KEY would be dropped. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): document RequestEnvVar value exclusivity Expose the value/valueFrom mutual-exclusion constraint in the generated OpenAPI schema so clients cannot send payloads the API rejects. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): allow NIM start without NGC credentials Omit unresolved NGC_API_KEY at execution time so mock/local NIM images can reach READY when the platform secret and env fallback are absent. Signed-off-by: Tyler Bray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@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
4 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
platform.ngc_api_key_secretresolution and injectNGC_API_KEYinto NIM containersnvcr.iowith the resolved credentialThis restores Docker NIM parity lost in the deployments-plugin cutover (#705) and addresses both failures reported by the embedding customization workflow.
Test plan
uv run --frozen pytestfor the affected models compiler/backend and deployments Docker suites (58 passed)uv run pre-commit run -aSummary by CodeRabbit
secretRefsupport for deployment env vars, with request/response-specific schema rules.