feat: unify Docker capability detection (AIRCORE-971) - #1082
Open
tylersbray wants to merge 13 commits into
Open
Conversation
Introduce nemo_platform_plugin.capabilities with ProbeResult, per-host caching, and reset_capability_cache for CLI retry. Delegate validate_docker_available and document Runtime.NONE soft-downgrade as deprecated toward AIRCORE-972. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Route merge/registry/init and setup/quickstart through probe_docker, subclass MissingBackendDependencyError from CapabilityUnavailableError, declare the jobs package dependency on nemo-platform-plugin, and re-vendor the SDK. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Stop treating Runtime.DOCKER vs NONE as the Docker availability signal for Automodel/Unsloth compile. Probe Docker directly so soft-downgraded NONE still works when the daemon is reachable (AIRCORE-971). Signed-off-by: Tyler Bray <tbray@nvidia.com>
Bind probe_docker at module level in call sites and update tests to patch the local binding. Re-vendor SDK after quickstart changes. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Use use_cache=False in customization compile so retries see a newly started daemon, and probe docker_host via from_env(base_url=...) so TLS env vars match DockerDeploymentBackend client creation. Signed-off-by: Tyler Bray <tbray@nvidia.com>
validate_docker_available uses an uncached probe so Runtime.NONE soft-downgrade does not memoize a miss before jobs/deployments registry construction. Server paths still call probe_docker() with caching. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Executor registration is fixed for the controller process lifetime; compile/CLI paths use uncached probes for retry UX (AIRCORE-971). Signed-off-by: Tyler Bray <tbray@nvidia.com>
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:
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 (2)
📝 WalkthroughWalkthroughChangesDocker capability migration
Sequence Diagram(s)sequenceDiagram
participant JobsAPI
participant BackendRegistry
participant probe_docker
participant DockerDaemon
JobsAPI->>BackendRegistry: request execution profiles
BackendRegistry->>probe_docker: probe Docker availability
probe_docker->>DockerDaemon: create client and ping
DockerDaemon-->>probe_docker: availability and detail
probe_docker-->>BackendRegistry: ProbeResult
BackendRegistry->>BackendRegistry: register backends and mark profiles ready
BackendRegistry-->>JobsAPI: readiness state
JobsAPI-->>JobsAPI: return profiles or HTTP 503
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/capabilities.py`:
- Around line 119-123: The Docker host is passed using unsupported base_url
kwargs. In capabilities.py, update both Docker client initialization sites to
copy the environment, set DOCKER_HOST when docker_host is configured, and pass
that environment to docker.from_env instead of base_url; update
test_capabilities.py to assert kwargs["environment"]["DOCKER_HOST"] for the
configured host case.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/registry.py`:
- Around line 84-86: Update the error guidance in the backend
capability/configuration message to be backend-neutral instead of directing
operators to configure a non-Docker executor or reach Docker. Derive recovery
advice from the matching ExecutorSpec when available, or use generic guidance
that applies to any executor backend, while preserving the existing error
context.
🪄 Autofix
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: 9a46a7b4-6919-44b6-9201-65c032296af2
⛔ Files ignored due to path filters (5)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/setup.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/quickstart/preflight.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/quickstart/validators.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_setup.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.pypackages/nemo_platform_ext/src/nemo_platform_ext/quickstart/preflight.pypackages/nemo_platform_ext/src/nemo_platform_ext/quickstart/validators.pypackages/nemo_platform_ext/tests/cli/commands/test_setup.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/capabilities.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/config.pypackages/nemo_platform_plugin/tests/test_capabilities.pypackages/nemo_platform_plugin/tests/test_config.pypackages/nmp_customization_common/src/nmp/customization_common/contributor/jobs.pypackages/nmp_customization_common/tests/contributor/test_jobs.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.pyservices/core/jobs/pyproject.tomlservices/core/jobs/src/nmp/core/jobs/controllers/backends/config.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/registry.pyservices/core/jobs/tests/test_config.py
Contributor
|
docker-py 7.x rejects base_url on from_env; override DOCKER_HOST in the environment instead (probe + deployments client). Stabilize unavailable error wording and reset the capability cache in deployments/jobs tests so a miss cannot poison later fixtures. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Drop arch-wheel churn from a full relock; surgically add only the nmp-jobs → nemo-platform-plugin workspace dependency. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Import-time merge uses probe_docker(use_cache=False) so it does not pin the process cache; registry boot remains the authoritative cached probe and compile shares that verdict via asyncio.to_thread. GET /v2/execution-profiles returns 503 until registry construction. GPU checks also apply under soft-downgraded NONE when Docker is reachable. Drop redundant quickstart cache resets; re-vendor SDK. Signed-off-by: Tyler Bray <tbray@nvidia.com>
7 tasks
Avoid OpenAPI/SDK drift from expanding the GET /v2/execution-profiles docstring. Sync the wrapper jobs-service extra and lock metadata with the nmp-jobs → nemo-platform-plugin edge after make vendor. Signed-off-by: Tyler Bray <tbray@nvidia.com>
nmp.common.jobs.docker now delegates validation to the plugin module, so the Jobs API test must patch the plugin path. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Avoid Docker-specific advice when any backend can fail registration. Mention the configured executors backend when known. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
Author
|
@coderabbitai resume |
Contributor
|
✅ Action performedReviews resumed. Review finished.
|
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
1 participant
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
nemo_platform_plugin.capabilities) withProbeResult, per-docker_hostcaching, andreset_capability_cachefor tests.MissingBackendDependencyErrornow subclassesCapabilityUnavailableError.probe_dockerinstead of treatingRuntime.DOCKER/NONEas the Docker signal (async compile runs the sync probe viaasyncio.to_thread).DOCKER→NONEsoft-downgrade as a deprecated convenience toward AIRCORE-972.Depends on the tactical soft-skip in #997. Capability-driven default selection (shrinking/removing Runtime) remains AIRCORE-972.
Linear: AIRCORE-971
Design contract
default_executorcould not register — owned by deploymentsExecutorRegistry(behavior from fix: properly handle NONE case for runtime executor #997; this PR clarifies the user-facing message). Models import-time fail-closed is out of scope: models cannot map executor name→Docker backend, and raising inmerge_backendsat import would kill the whole process. Soft-downgrade already clears auto modelsdocker_executordefaults underNONE.probe_docker(use_cache=False)so it does not pin the process cache.BackendRegistry.from_configis the authoritative cached boot probe; customization compile shares that cached verdict (restart required after starting Docker mid-process). CLI/quickstart useuse_cache=Falsefor retry UX.GET /v2/execution-profilesreturns 503 until the jobs controller registry has constructed and pruned advertised profiles.Runtime inventory (AIRCORE-972 handoff)
Topology (keep for shrink decision): Runtime enum + soft-downgrade; jobs/models default profile/backend selection; models
executor_for_runtime/deployments_enabled; compiler paths; customization multi-node +require_distributed_runtime.Capability (migrated here): jobs merge/registry Docker filter; deployments Docker backend/registry; setup/quickstart; customization
require_container_runtime.Deferred: agents
improvement/preflight.py(docker info); deploymentsdocker/gpu.pypool recovery; API/controller capability snapshot; GPU/k8s probe extension points (stubs intentionally not shipped).Test plan
uv run --frozen pytest packages/nemo_platform_plugin/tests/test_capabilities.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py(includes init-unavailable cases; suite lacksunitmarker — run explicitly)uv run --frozen pytest services/core/jobs/tests/test_config.py plugins/nemo-deployments/tests/unit/test_registry.pyuv run --frozen pytest packages/nmp_customization_common/tests/contributor/test_jobs.py packages/nmp_common/tests/jobs/test_docker.pyGET /v2/execution-profiles503-until-ready unit coveragemake vendor-nemo-platform-extafter ext changesuv.lockvs main is +2 lines only (nmp-jobs→nemo-platform-plugin)default_executor: local-dockerSummary by CodeRabbit