fix: properly handle NONE case for runtime executor - #997
Open
mckornfield wants to merge 9 commits into
Open
Conversation
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:
📝 WalkthroughWalkthroughChangesExecutor availability and runtime compatibility
Sequence Diagram(s)sequenceDiagram
participant SetupCommand
participant ServiceProcess
participant Platform
SetupCommand->>ServiceProcess: start services
SetupCommand->>Platform: poll readiness with process
Platform-->>SetupCommand: readiness result
SetupCommand->>ServiceProcess: check exit status on failure
ServiceProcess-->>SetupCommand: return exit code
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
|
mckornfield
force-pushed
the
no-docker-local-fixes/mck
branch
from
July 30, 2026 19:07
2759ee3 to
6a95658
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/core/jobs/src/nmp/core/jobs/controllers/backends/config.py (1)
140-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument Volcano filtering too.
_RUNTIME_NONE_UNSUPPORTED_BACKENDSalso containsvolcano_job, but this docstring names only Kubernetes. Explicitly mention both backends so the documentedRuntime.NONEcontract matches the implementation. (github.com)Proposed wording
- When runtime is NONE, Kubernetes-backed custom profiles are skipped. Docker custom profiles are skipped only when + When runtime is NONE, Kubernetes- and Volcano-backed custom profiles are skipped. Docker custom profiles are skipped only when🤖 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/jobs/src/nmp/core/jobs/controllers/backends/config.py` around lines 140 - 142, Update the docstring describing Runtime.NONE behavior near _RUNTIME_NONE_UNSUPPORTED_BACKENDS to explicitly state that both Kubernetes and Volcano-backed custom profiles are skipped, while preserving the existing Docker availability 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.
Nitpick comments:
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/config.py`:
- Around line 140-142: Update the docstring describing Runtime.NONE behavior
near _RUNTIME_NONE_UNSUPPORTED_BACKENDS to explicitly state that both Kubernetes
and Volcano-backed custom profiles are skipped, while preserving the existing
Docker availability behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48d59dc6-10fe-4585-8aa8-43344e898208
📒 Files selected for processing (2)
services/core/jobs/src/nmp/core/jobs/controllers/backends/config.pyservices/core/jobs/tests/test_config.py
Contributor
Takeover notes (capability soft-skip reshape)Took over this PR while @mckornfield is OOO and reshaped it away from blanketing all deployments executors under What changed vs the original approach
Still out of scope (follow-up)Runtime removal / unified capability framework (Ryan’s longer-term point). This PR is a tactical fix for NVBug 6537617 that does capability detection at backend registration rather than treating Please re-review when convenient. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nemo_platform_ext/tests/cli/commands/test_setup.py (1)
768-775: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the launched process reaches the polling helper.
This test replaces
_wait_for_platformwith a stub. It does not catch a regression that removesproc=procfrom the call site. Assert that the mock receivesproc=dead.Suggested assertion
- patch(f"{SETUP_MOD}._wait_for_platform", return_value=False), + patch(f"{SETUP_MOD}._wait_for_platform", return_value=False) as mock_wait, ... + mock_wait.assert_called_once() + assert mock_wait.call_args.kwargs["proc"] is dead🤖 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 `@packages/nemo_platform_ext/tests/cli/commands/test_setup.py` around lines 768 - 775, Update the test around _maybe_start_services to retain a mock for _wait_for_platform and assert it is called with proc=dead. Keep the existing ClickExit expectation and setup unchanged while verifying the launched process is passed to the polling helper.
🤖 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 `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.py`:
- Around line 956-960: Update the Docker availability check around
validate_docker_available() so it cannot block for the Docker SDK’s default
60-second timeout when DOCKER_HOST is unreachable. Pass a short explicit timeout
through docker.from_env(), or bypass this probe after _wait_for_platform()
fails, while preserving the existing unavailable-Docker message.
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/registry.py`:
- Around line 141-152: Update the exception handling around backend construction
in the registry initialization block to catch only the project’s
Docker-unavailability exception, matching the distinction used by the Nemo
deployments registry. Preserve the existing warning-and-skip behavior for that
exception, while allowing configuration, validation, and other unexpected errors
from backend(...) to propagate normally.
---
Nitpick comments:
In `@packages/nemo_platform_ext/tests/cli/commands/test_setup.py`:
- Around line 768-775: Update the test around _maybe_start_services to retain a
mock for _wait_for_platform and assert it is called with proc=dead. Keep the
existing ClickExit expectation and setup unchanged while verifying the launched
process is passed to the polling helper.
🪄 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: b0ecb4b3-a08a-4808-a079-81344cfedf8f
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/setup.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_setup.pyis excluded by!sdk/**
📒 Files selected for processing (11)
packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.pypackages/nemo_platform_ext/tests/cli/commands/test_setup.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/base.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/registry.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/test_registry.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/config.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/registry.pyservices/core/jobs/src/nmp/core/jobs/controllers/main.pyservices/core/jobs/tests/test_config.py
Contributor
Follow-on coherence patches (post gap review)
Longer-term work (shared capability probe / Runtime shrink) tracked in AIRE Core Linear tickets — links coming shortly. |
Contributor
Follow-up Linear tickets (AIRE Core)
Please leave design feedback on those tickets before implementation. |
tylersbray
force-pushed
the
no-docker-local-fixes/mck
branch
from
July 31, 2026 21:12
545c0de to
e8ff6c4
Compare
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Skip Docker backends when the daemon is unreachable instead of blanketing all executors under Runtime.NONE, and clear a skipped default_executor so the deployments service can still boot. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Filter Docker executor profiles whenever Docker is unavailable, not only under Runtime.NONE, and skip Docker backend construction in the jobs registry so local startup survives a missing daemon. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Poll the service process during readiness wait so missing Docker no longer burns the full timeout, and surface an explicit Docker hint when the daemon is unavailable. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Align DockerDeploymentBackend soft-skip with validate_docker_available by also mapping request timeouts to MissingBackendDependencyError. Signed-off-by: Tyler Bray <tbray@nvidia.com>
After BackendRegistry skips unavailable Docker backends, prune the shared profiles list so /v2/execution-profiles matches what can run. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Narrow jobs Docker construction soft-skip to daemon/connection errors so ValidationError still fails, align validate_docker_available catch set, and only print the setup Docker hint when early exit or log evidence points at Docker. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
force-pushed
the
no-docker-local-fixes/mck
branch
from
July 31, 2026 22:24
e8ff6c4 to
f4579fc
Compare
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 by CodeRabbit