fix(customizer): change entrypoint of side car to avoid API spin up for LoRA and unwritable dirs - #1185
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe LoRA adapters sidecar now launches through direct Python module execution. Deployment configuration, Helm values, Docker helpers, compiler tests, compiler environment setup, and setup documentation use the updated command and writable scratch-backed XDG directories. ChangesLoRA sidecar command
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.py (1)
362-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the Docker command too.
The test compiles both runtimes, but Line 362 checks only the Kubernetes sidecar. Add an assertion for
docker.server_config.containers[1].command.Proposed assertion
assert sidecar.command == ["python", "-m", "nmp.core.models.sidecars.adapters.main"] + assert docker.server_config.containers[1].command == [ + "python", + "-m", + "nmp.core.models.sidecars.adapters.main", + ]🤖 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_compiler.py` at line 362, Add an assertion alongside the existing Kubernetes sidecar check in the test covering both compiled runtimes, validating that docker.server_config.containers[1].command matches the expected Python module command.
🤖 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_compiler.py`:
- Line 362: Add an assertion alongside the existing Kubernetes sidecar check in
the test covering both compiled runtimes, validating that
docker.server_config.containers[1].command matches the expected Python module
command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aca0f37a-751b-4f8f-895e-43110ea55e99
📒 Files selected for processing (5)
docs/set-up/config-reference.mdxk8s/helm/values.yamlplugins/nemo-deployments/tests/unit/backends/docker/docker_helpers.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/config.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.py
albcui
left a comment
There was a problem hiding this comment.
I think for good measure, let's just add "XDG_CONFIG_HOME": _LORA_SIDECAR_XDG_HOME to services/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.py as well?
|
…or LoRA and unwritable directories Signed-off-by: Sam Oluwalana <soluwalana@nvidia.com>
52985a3 to
502a651
Compare
|
/nvskills-ci |
Summary
Related Issue
Changes
Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
Summary by CodeRabbit
New Features
Documentation