feat: unify Docker capability detection (AIRCORE-971) - #1082
Conversation
|
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:
📝 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 |
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
|
|
@coderabbitai resume |
|
✅ Action performedReviews resumed. Review finished.
|
ironcommit
left a comment
There was a problem hiding this comment.
apart from the global state, a step in the right direction. This start up code still needs more work.
7730d3c to
45fe6ac
Compare
Update after plan-audit + product syncRebased onto current
Accepted limitation this release: Harbor/evals on already-booted reduced configs can still fail at use-time. Linear: AIRCORE-971 (direction comment posted). |
|
/nvskills-ci |
9dcd940 to
98656a9
Compare
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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@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> Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
GET /v2/execution-profiles no longer 503s until a controller-only process flag is set, which broke k8s and services-only runs. Advertise the capability-filtered merge from the API process instead. Also address review feedback: share docker_from_env_kwargs, remove automodel PlatformJobSpec casts via typed compile return, and add a docker_probe_unavailable test fixture. Signed-off-by: Tyler Bray <tbray@nvidia.com> Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Add a config-aware preflight used by setup and services run/start/restart. It triggers only when the resolved selection starts deployments with a docker-backed default_executor (pre-soft-downgrade YAML intent), not as a global Docker mandate. Restart checks before stop so a healthy instance is not torn down when the daemon is missing. Signed-off-by: Tyler Bray <tbray@nvidia.com> Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Reset the capability cache before the jobs BackendRegistry boot probe so a transient earlier miss cannot skip Docker executors for the process lifetime. Probe the default deployments executor docker_host in CLI preflight so remote daemons are not false-failed against the local socket. Signed-off-by: Tyler Bray <tbray@nvidia.com> Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
6dbecd8 to
56c1f12
Compare
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
56c1f12 to
6a7a5de
Compare
`lint-cli` runs `make update-cli` and fails if it dirties the tree. Running it regenerates the jobs execution-profiles docstring in the generated CLI command (and its vendored SDK copy + CLI reference docs). This is drift from #1082 (AIRCORE-971, merged to main today), which changed the jobs API docstring without regenerating the spec/SDK/CLI — swept up here by the clean regen, since the generators produce it deterministically and the branch cannot go green otherwise. Generated output, no hand edits. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com>
) * fix(intake): accept ATIF observation on all step types Relay emits an `observation` on any ATIF step, but Intake only permitted it on agent steps. In-process harnesses like DeepAgents put an observation on a nested system step, so Intake rejected the entire ATIF POST with HTTP 422 and stored nothing. Move `observation` from AtifStepAgent to the shared AtifStepBase; run the tool-call-reference and v1.7 subagent-ref validators on all step types; let the mapper read observations on any step. Adds a regression test reproducing a DeepAgents nested-system-step observation. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com> * test(intake): assert observation reachable on nested system step Address review: the prior `assert spans` passed even if `_step_observation` regressed to agent-only, since trajectory/step spans emit independently. Locate the nested system-step span and assert `_step_observation` returns the observation value on the non-agent step, so the test fails if the mapper stops reading observations there. Signed-off-by: Nathan Walston <nwalston@nvidia.com> * chore(openapi): regenerate spec for ATIF observation on all step types The intake fix moved `observation` from `AtifStepAgent` to `AtifStepBase`, which changes the ATIF ingest contract: `observation` now appears on the `system` and `user` step schemas as well. Regenerated via `script/generate-openapi-spec.sh`; no hand edits. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com> * chore(sdk): regenerate SDK for ATIF observation on all step types Stainless sync (`sdk/stainless.sh sync`) for the ATIF ingest contract change: `observation` moved from `AtifStepAgent` to `AtifStepBase`, so the generated `AtifStepSystemParam` / `AtifStepUserParam` models now carry it, and the `.nmpcontext` snapshot matches the regenerated spec (clears `lint-python-sdk`). Also sweeps one unrelated docstring-only update in `resources/jobs/jobs.py` (execution-profiles description), which was pre-existing drift on main picked up by the clean regen — generated output, not hand-edited. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com> * chore(cli): regenerate CLI commands and reference docs `lint-cli` runs `make update-cli` and fails if it dirties the tree. Running it regenerates the jobs execution-profiles docstring in the generated CLI command (and its vendored SDK copy + CLI reference docs). This is drift from NVIDIA-NeMo#1082 (AIRCORE-971, merged to main today), which changed the jobs API docstring without regenerating the spec/SDK/CLI — swept up here by the clean regen, since the generators produce it deterministically and the branch cannot go green otherwise. Generated output, no hand edits. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com> --------- Signed-off-by: Nathan Walston <nwalston@nvidia.com> Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com>
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