feat: add local services lifecycle SDK with Unix domain socket support - #734
Merged
Conversation
ironcommit
changed the title
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:
📝 WalkthroughWalkthroughThis change adds local NeMo service lifecycle APIs with TCP/UDS support, typed endpoint resolution, Unix socket binding, and launcher-specific OTLP logging and secret access. ChangesLocal service lifecycle
Typed platform endpoint integration
Job launcher endpoint and telemetry configuration
Sequence Diagram(s)sequenceDiagram
participant SDK
participant EndpointResolver
participant ServiceHandle
participant LocalService
SDK->>EndpointResolver: Resolve TCP or UDS endpoint
SDK->>ServiceHandle: Start or connect to services
ServiceHandle->>LocalService: Probe /status
LocalService-->>ServiceHandle: Readiness response
ServiceHandle-->>SDK: Transport-aware client
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 16
🤖 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/local/process.py`:
- Around line 678-694: The SIGKILL path in the process-stop flow reports success
without confirming that the process exited. After sending SIGKILL in the visible
stop logic, wait for confirmed termination before calling remove_descriptor or
returning the PID in StopResult; if the process remains alive, preserve the
descriptor and return without reporting it as stopped, while retaining the
existing child-sweeping behavior.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/local/services.py`:
- Around line 91-92: Update the configuration class’s __post_init__ to validate
readiness_timeout and readiness_poll_interval before any child process is
spawned, rejecting values less than or equal to zero with an appropriate
validation error.
- Around line 298-312: Update _write_run_request in
packages/nemo_platform_ext/src/nemo_platform_ext/local/services.py at lines
298-312 to retain and pass the unique tempfile path returned by mkstemp
directly, removing the shared run-request.json replacement. Update the
request-reading flow in
packages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.py at
lines 20-22 to unlink that request file in a finally block after reading it.
- Around line 402-412: Bound each readiness probe and polling sleep by the
remaining deadline in services.py around the startup readiness loop: recalculate
remaining time before probe_status, pass it as the probe timeout, and sleep for
no longer than the remaining duration. Apply the same remaining-time budgeting
to both synchronous and asynchronous loops in transport.py around the affected
transport methods; preserve process-exit checks and successful completion
behavior.
- Around line 327-352: The startup sequence after process.acquire_lock must be
wrapped in a try/finally so every exception releases the lock. Within the same
cleanup scope, preserve the original NMP_DATA_DIR state before mutation and
restore its prior value—or remove it if it was previously absent—in the finally
block, covering log-path resolution, descriptor construction, and
process.write_descriptor.
- Around line 389-397: Update the startup flow surrounding subprocess.Popen so
the parent retains ownership of the child through all readiness operations,
including handle creation, probing, sleeping, and interruption. Add a guard that
terminates and, if necessary, kills the spawned process whenever startup fails
or raises; mark ownership transferred only after readiness completes
successfully, preserving normal detached-daemon behavior after success.
- Around line 271-284: Update _prepare_socket to validate the resolved UDS
socket path’s encoded byte length against the platform AF_UNIX limit before
creating directories or probing/removing the socket. Apply this check to both
generated and explicitly configured paths, and raise the project’s established
configuration or socket-path error with a clear message when the path is too
long.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/local/transport.py`:
- Around line 27-40: Update build_sync_http_client and build_async_http_client
to use a concrete timeout union type and a finite non-None default value instead
of object = None. Preserve explicit caller-provided timeout values while
ensuring calls from local services without timeout retain HTTPX timeouts.
In `@packages/nmp_common/src/nmp/common/auth/client.py`:
- Around line 423-424: Update wait_role’s client initialization before the try
block to validate that a PDP URL is configured before calling
_new_pdp_http_client or otherwise ensure the missing URL raises the expected
configuration error. Preserve the existing client reuse behavior for valid
configurations and align the guard with the handling used around the paths at
lines 141 and 229.
In `@packages/nmp_common/src/nmp/common/sdk_factory.py`:
- Line 47: Update the SDK endpoint initialization to call
resolve_service_endpoint(api_name, platform_config) instead of directly parsing
platform_config.get_service_url(api_name), so NMP_<SERVICE>_URL overrides are
honored.
- Around line 78-99: The endpoint client helpers _sync_http_client_for_endpoint
and _async_http_client_for_endpoint must not reuse a single transport-bound
client when routed endpoints mix TCP and UDS. Update the client selection and
_create_url_router integration to reject mixed transports explicitly or
create/select a client per endpoint, ensuring TCP requests use TCP clients and
UDS requests use endpoint UDS clients.
In `@packages/nmp_platform_runner/src/nmp/platform_runner/run.py`:
- Line 78: Update run_server_with_reload() and its caller to handle socket_path
when reload is enabled: pass the Unix socket path through so the reload server
binds consistently with apply_run_environment(), or explicitly reject the
socket_path-plus-reload combination before starting. Preserve existing host/port
behavior when no socket path is provided.
In `@services/core/jobs/jobs-launcher/cmd/otel.go`:
- Around line 153-159: Update the OTLP UDS setup around
ResolvePlatformEndpointFromEnv and parseLauncherOTLPTimeout so the parsed
timeout is assigned to endpoint.HTTPClient().Timeout before passing that client
to otlploghttp.WithHTTPClient. Ensure the timeout is applied when configured
while preserving the existing exporter options behavior.
In `@services/core/jobs/jobs-launcher/cmd/run.go`:
- Around line 221-230: Add a bounded timeout to the client or request used by
fetchSecretsWithEndpoint when resolving secrets through secretEndpoint, covering
both TCP and UDS transports instead of relying on the unbounded
Endpoint.HTTPClient(). Preserve the existing secret-fetch behavior while
ensuring a stalled endpoint cannot block launcher startup indefinitely.
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/base.py`:
- Line 310: The endpoint handling in base.py at lines 310-310 and
subprocess_runtime.py at lines 161-177 discards Unix-domain-socket metadata and
passes a placeholder URL to OTLPLogExporter. Preserve the PlatformEndpoint
transport and socket_path from parse_platform_endpoint through the backend
configuration, then update the subprocess runtime exporter construction to use a
UDS-aware client/exporter for Unix endpoints while retaining normal HTTP
behavior for TCP endpoints.
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py`:
- Around line 622-629: Protect the private launcher telemetry environment
variables from user overrides across all backends. In
services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py:622-629
reserve the endpoint, exporter, and protocol names; at :643-644 reserve the
authenticated headers name. In
services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.py:905-913
filter these names during pod environment construction and at :926-929 protect
the authenticated headers entry. In
services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py:450-454
apply the trusted endpoint, exporter, and protocol values after processing the
step environment.
🪄 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: 54d726a6-c175-497d-be5a-2c9c38f4e929
⛔ Files ignored due to path filters (8)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/services/_process.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/_service_child.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/process.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/services.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/transport.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_services.pyis excluded by!sdk/**services/core/jobs/jobs-launcher/go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/process.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/services.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/transport.pypackages/nemo_platform_ext/tests/local/test_services.pypackages/nmp_common/src/nmp/common/auth/client.pypackages/nmp_common/src/nmp/common/auth/middleware.pypackages/nmp_common/src/nmp/common/platform_endpoint.pypackages/nmp_common/src/nmp/common/sdk_factory.pypackages/nmp_common/src/nmp/common/service/api/health.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_common/tests/test_platform_endpoint.pypackages/nmp_platform_runner/src/nmp/platform_runner/config.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_config.pypackages/nmp_platform_runner/tests/test_server.pyservices/core/jobs/jobs-launcher/cmd/otel.goservices/core/jobs/jobs-launcher/cmd/run.goservices/core/jobs/jobs-launcher/cmd/run_test.goservices/core/jobs/jobs-launcher/go.modservices/core/jobs/jobs-launcher/nmpclient/client.goservices/core/jobs/jobs-launcher/nmpclient/client_test.goservices/core/jobs/jobs-launcher/nmpclient/endpoint.goservices/core/jobs/jobs-launcher/nmpclient/endpoint_test.goservices/core/jobs/src/nmp/core/jobs/controllers/backends/base.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.pyservices/core/jobs/tests/controllers/test_base.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.py
💤 Files with no reviewable changes (1)
- services/core/jobs/jobs-launcher/go.mod
Contributor
|
ironcommit
force-pushed
the
services-sdk/rsadler
branch
from
July 16, 2026 23:51
a66dc1b to
5f80e5b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/nmp_common/tests/sdk_factory/test_sdk.py (1)
49-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the UDS client transport, not only its placeholder URL.
Both tests pass if the factory accidentally creates a TCP client for
nemo-platform.local. Inspect the endpoint-client builder or perform a request over a temporary UDS listener.Also applies to: 98-104
🤖 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/nmp_common/tests/sdk_factory/test_sdk.py` around lines 49 - 55, Strengthen the UDS coverage in test_get_platform_sdk_uses_uds_endpoint_from_base_url and its corresponding test so they verify the SDK uses a Unix-domain-socket transport, not merely the placeholder base_url. Inspect the constructed endpoint client’s transport configuration or issue a request through a temporary UDS listener, while preserving the existing configuration setup.
🤖 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/local/services.py`:
- Around line 322-327: Update the startup sequence around _prepare_socket and
process.acquire_lock so the instance lock is acquired before preparing or
removing the socket. Keep the liveness and TCP availability checks as
appropriate, but ensure socket cleanup occurs only after the launching process
owns the lock, preventing a concurrent starter from unlinking a live socket.
In `@packages/nmp_common/src/nmp/common/platform_endpoint.py`:
- Around line 73-74: Update the HTTP(S) handling in the platform endpoint parser
to validate that the parsed authority contains a valid host before constructing
PlatformEndpoint. Reject hostless values such as http:// or https://, while
preserving the existing normalized connect_base_url behavior for valid
endpoints.
- Around line 25-45: Update sync_http_client and async_http_client to type
timeout as the supported httpx.TimeoutTypes and omit the timeout argument when
callers leave it unset, allowing HTTPX defaults to apply; pass timeout
explicitly only when provided, while preserving the existing UDS transport
behavior.
In `@services/core/jobs/jobs-launcher/nmpclient/client_test.go`:
- Around line 174-177: Update the test using net.Listen in the Unix-socket setup
to remove socketPath during cleanup after server.Close(), ensuring the temporary
socket pathname does not remain after the test.
- Around line 179-186: Replace t.Fatalf in the HTTP handlers in
services/core/jobs/jobs-launcher/nmpclient/client_test.go lines 179-186 and
services/core/jobs/jobs-launcher/nmpclient/endpoint_test.go lines 90-97 with
t.Errorf, then return an HTTP error response from each handler when the request
path is unexpected.
---
Nitpick comments:
In `@packages/nmp_common/tests/sdk_factory/test_sdk.py`:
- Around line 49-55: Strengthen the UDS coverage in
test_get_platform_sdk_uses_uds_endpoint_from_base_url and its corresponding test
so they verify the SDK uses a Unix-domain-socket transport, not merely the
placeholder base_url. Inspect the constructed endpoint client’s transport
configuration or issue a request through a temporary UDS listener, while
preserving the existing configuration setup.
🪄 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: d2bd83f3-4740-4006-b383-d853d4a22bc5
⛔ Files ignored due to path filters (8)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/services/_process.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/_service_child.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/process.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/services.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/transport.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_services.pyis excluded by!sdk/**services/core/jobs/jobs-launcher/go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/process.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/services.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/transport.pypackages/nemo_platform_ext/tests/local/test_services.pypackages/nmp_common/src/nmp/common/auth/client.pypackages/nmp_common/src/nmp/common/auth/middleware.pypackages/nmp_common/src/nmp/common/platform_endpoint.pypackages/nmp_common/src/nmp/common/sdk_factory.pypackages/nmp_common/src/nmp/common/service/api/health.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_common/tests/test_platform_endpoint.pypackages/nmp_platform_runner/src/nmp/platform_runner/config.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_config.pypackages/nmp_platform_runner/tests/test_server.pyservices/core/jobs/jobs-launcher/cmd/otel.goservices/core/jobs/jobs-launcher/cmd/run.goservices/core/jobs/jobs-launcher/cmd/run_test.goservices/core/jobs/jobs-launcher/go.modservices/core/jobs/jobs-launcher/nmpclient/client.goservices/core/jobs/jobs-launcher/nmpclient/client_test.goservices/core/jobs/jobs-launcher/nmpclient/endpoint.goservices/core/jobs/jobs-launcher/nmpclient/endpoint_test.goservices/core/jobs/src/nmp/core/jobs/controllers/backends/base.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.pyservices/core/jobs/tests/controllers/test_base.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.py
💤 Files with no reviewable changes (1)
- services/core/jobs/jobs-launcher/go.mod
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.py
- services/core/jobs/jobs-launcher/nmpclient/client.go
- packages/nmp_common/src/nmp/common/auth/middleware.py
- packages/nmp_platform_runner/src/nmp/platform_runner/server.py
- packages/nemo_platform_ext/src/nemo_platform_ext/local/transport.py
- services/core/jobs/jobs-launcher/cmd/run.go
- services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py
- services/core/jobs/tests/controllers/test_docker_backend.py
- packages/nmp_platform_runner/src/nmp/platform_runner/run.py
- services/core/jobs/tests/controllers/test_kubernetes_backend.py
- packages/nemo_platform_ext/tests/local/test_services.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.py
- packages/nmp_common/src/nmp/common/service/api/health.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/base.py
- services/core/jobs/jobs-launcher/cmd/otel.go
- services/core/jobs/jobs-launcher/nmpclient/endpoint.go
- packages/nmp_common/src/nmp/common/auth/client.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py
- packages/nmp_common/src/nmp/common/service/base.py
ironcommit
force-pushed
the
services-sdk/rsadler
branch
from
July 17, 2026 00:05
5f80e5b to
52d43fc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/nmp_common/tests/sdk_factory/test_sdk.py (1)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert UDS client wiring, not only the placeholder URL.
These tests still pass if SDK construction uses a normal TCP client. Mock the endpoint client factory or issue a request over a temporary UDS listener.
Also applies to: 98-104
🤖 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/nmp_common/tests/sdk_factory/test_sdk.py` around lines 49 - 55, Update test_get_platform_sdk_uses_uds_endpoint_from_base_url and the corresponding test at lines 98-104 to verify Unix-domain-socket wiring rather than only asserting the placeholder base_url. Mock the endpoint client factory and assert it receives the UDS path, or perform a request through a temporary UDS listener, while preserving the existing configuration setup.
🤖 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/local/services.py`:
- Around line 124-129: Require absolute Unix-domain socket paths at both
configuration entry points: in
packages/nemo_platform_ext/src/nemo_platform_ext/local/services.py lines
124-129, validate the path returned by resolved_socket_path, whether explicit or
runtime-derived, before launch; in
packages/nmp_platform_runner/src/nmp/platform_runner/config.py lines 68-69,
normalize or reject relative socket_path arguments before storing them. Preserve
the existing non-UDS behavior.
In `@packages/nmp_common/src/nmp/common/service/base.py`:
- Around line 560-564: Update the status polling logic around response.json() to
catch ValueError and continue retrying when a 200 response contains malformed
JSON. Validate that the parsed payload is a mapping before accessing
data.get("services"), treating non-object payloads as malformed and preserving
the existing retry behavior used for request failures.
In `@services/core/jobs/jobs-launcher/nmpclient/endpoint_test.go`:
- Around line 91-94: Update the http.HandlerFunc in the test server setup to
record the received request path instead of calling t.Fatalf from the server
goroutine. After the request completes, assert the recorded path from the main
test goroutine, preserving the existing unexpected-path validation.
---
Nitpick comments:
In `@packages/nmp_common/tests/sdk_factory/test_sdk.py`:
- Around line 49-55: Update
test_get_platform_sdk_uses_uds_endpoint_from_base_url and the corresponding test
at lines 98-104 to verify Unix-domain-socket wiring rather than only asserting
the placeholder base_url. Mock the endpoint client factory and assert it
receives the UDS path, or perform a request through a temporary UDS listener,
while preserving the existing configuration setup.
🪄 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: fa9406aa-9c1c-4431-8d65-e3a185fd4eb3
⛔ Files ignored due to path filters (8)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/services/_process.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/_service_child.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/process.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/services.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/transport.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_services.pyis excluded by!sdk/**services/core/jobs/jobs-launcher/go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/process.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/services.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/transport.pypackages/nemo_platform_ext/tests/local/test_services.pypackages/nmp_common/src/nmp/common/auth/client.pypackages/nmp_common/src/nmp/common/auth/middleware.pypackages/nmp_common/src/nmp/common/platform_endpoint.pypackages/nmp_common/src/nmp/common/sdk_factory.pypackages/nmp_common/src/nmp/common/service/api/health.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_common/tests/test_platform_endpoint.pypackages/nmp_platform_runner/src/nmp/platform_runner/config.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_config.pypackages/nmp_platform_runner/tests/test_server.pyservices/core/jobs/jobs-launcher/cmd/otel.goservices/core/jobs/jobs-launcher/cmd/run.goservices/core/jobs/jobs-launcher/cmd/run_test.goservices/core/jobs/jobs-launcher/go.modservices/core/jobs/jobs-launcher/nmpclient/client.goservices/core/jobs/jobs-launcher/nmpclient/client_test.goservices/core/jobs/jobs-launcher/nmpclient/endpoint.goservices/core/jobs/jobs-launcher/nmpclient/endpoint_test.goservices/core/jobs/src/nmp/core/jobs/controllers/backends/base.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.pyservices/core/jobs/tests/controllers/test_base.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.py
💤 Files with no reviewable changes (1)
- services/core/jobs/jobs-launcher/go.mod
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py
- packages/nmp_platform_runner/src/nmp/platform_runner/run.py
- services/core/jobs/jobs-launcher/nmpclient/endpoint.go
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.py
- packages/nemo_platform_ext/src/nemo_platform_ext/local/transport.py
- services/core/jobs/jobs-launcher/nmpclient/client.go
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/base.py
- services/core/jobs/tests/controllers/test_docker_backend.py
- packages/nmp_common/src/nmp/common/auth/client.py
- packages/nmp_platform_runner/src/nmp/platform_runner/server.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/_process.py
- packages/nmp_common/src/nmp/common/auth/middleware.py
- packages/nemo_platform_ext/tests/local/test_services.py
- services/core/jobs/jobs-launcher/cmd/run.go
- packages/nmp_common/src/nmp/common/service/api/health.py
- services/core/jobs/jobs-launcher/cmd/run_test.go
- services/core/jobs/jobs-launcher/cmd/otel.go
- services/core/jobs/tests/controllers/test_kubernetes_backend.py
- packages/nmp_common/src/nmp/common/sdk_factory.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nemo_platform_ext/tests/local/test_transport.py (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove postponed annotations.
This import converts otherwise concrete annotations into strings and is unnecessary here.
Proposed fix
-from __future__ import annotationsAs per coding guidelines, prefer concrete type hints over string-based type hints.
🤖 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/local/test_transport.py` at line 4, Remove the `from __future__ import annotations` import from `test_transport.py` so annotations remain concrete at runtime; leave the rest of the test code unchanged.Source: Coding guidelines
🤖 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/local/services.py`:
- Around line 314-331: Update _write_run_request to avoid a potentially short
os.write call by wrapping the temporary file descriptor with os.fdopen and
writing the payload through json.dump (including the trailing newline), or by
reliably looping until all encoded bytes are written. Preserve the existing
cleanup and descriptor-closing behavior on success and failure.
---
Nitpick comments:
In `@packages/nemo_platform_ext/tests/local/test_transport.py`:
- Line 4: Remove the `from __future__ import annotations` import from
`test_transport.py` so annotations remain concrete at runtime; leave the rest of
the test code unchanged.
🪄 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: f8726cfe-739c-4e36-ab1d-487e99f09efc
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/src/nemo_platform/local/_service_child.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/process.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/services.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/transport.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_services_process.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_services.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_transport.pyis excluded by!sdk/**
📒 Files selected for processing (9)
packages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/process.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/services.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/transport.pypackages/nemo_platform_ext/tests/cli/commands/test_services_process.pypackages/nemo_platform_ext/tests/local/test_services.pypackages/nemo_platform_ext/tests/local/test_transport.pyservices/core/jobs/jobs-launcher/cmd/otel.goservices/core/jobs/jobs-launcher/cmd/otel_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.py
- packages/nemo_platform_ext/src/nemo_platform_ext/local/transport.py
- services/core/jobs/jobs-launcher/cmd/otel.go
- packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py
ironcommit
force-pushed
the
services-sdk/rsadler
branch
from
July 17, 2026 00:36
993a57c to
a71a598
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
services/core/jobs/src/nmp/core/jobs/controllers/backends/base.py (1)
44-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSingle source of truth needed for new OTLP transport env-var names.
NMP_JOB_LAUNCHER_OTLP_LOGS_SOCKET_PATH_ENVVARand_TRANSPORT_ENVVARare declared with identical string literals independently in both files (same package); nothing enforces they stay in sync.
services/core/jobs/src/nmp/core/jobs/controllers/backends/base.py#L44-L67: keep as the canonical definition (producer viaOtlpLogsEndpointConfig.to_env()).services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.py#L32-L36: drop the local re-declaration and import both constants frombase.pyinstead.🤖 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/base.py` around lines 44 - 67, Keep NMP_JOB_LAUNCHER_OTLP_LOGS_SOCKET_PATH_ENVVAR and NMP_JOB_LAUNCHER_OTLP_LOGS_TRANSPORT_ENVVAR canonical in base.py for OtlpLogsEndpointConfig.to_env(); in subprocess_runtime.py, remove the duplicate declarations and import both constants from base.py, preserving all existing usages.services/core/jobs/tests/controllers/test_subprocess_runtime.py (1)
156-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for unsupported transport value.
Covers
udswithout a socket path, but not theraise ValueError(f"unsupported OTLP logs transport: {transport!r}")branch for an unrecognized transport (e.g.,"https"). Worth a quick test to lock in that error path.🤖 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/tests/controllers/test_subprocess_runtime.py` around lines 156 - 162, Add a test alongside test_build_otlp_log_exporter_rejects_uds_without_socket_path that calls _build_otlp_log_exporter with an unrecognized transport such as "https" and asserts ValueError, matching the unsupported transport error message and value.
🤖 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/local/services.py`:
- Around line 408-414: Update the cleanup logic in the surrounding finally block
so NMP_DATA_DIR restoration and os.close(lock_fd) always execute even when
process.remove_descriptor fails. Preserve the existing cleanup order where
possible, but isolate descriptor removal from the remaining cleanup using nested
cleanup handling or an equivalent guaranteed-finalization structure.
- Line 33: Adjust _AF_UNIX_PATH_MAX_BYTES to 103 on BSD-like platforms and 107
elsewhere to reserve the UDS trailing NUL byte, and ensure
_validate_socket_path_length uses this corrected limit.
---
Nitpick comments:
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/base.py`:
- Around line 44-67: Keep NMP_JOB_LAUNCHER_OTLP_LOGS_SOCKET_PATH_ENVVAR and
NMP_JOB_LAUNCHER_OTLP_LOGS_TRANSPORT_ENVVAR canonical in base.py for
OtlpLogsEndpointConfig.to_env(); in subprocess_runtime.py, remove the duplicate
declarations and import both constants from base.py, preserving all existing
usages.
In `@services/core/jobs/tests/controllers/test_subprocess_runtime.py`:
- Around line 156-162: Add a test alongside
test_build_otlp_log_exporter_rejects_uds_without_socket_path that calls
_build_otlp_log_exporter with an unrecognized transport such as "https" and
asserts ValueError, matching the unsupported transport error message and value.
🪄 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: 8824b50d-1bc1-461f-b298-3601c1b0918b
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/src/nemo_platform/local/_service_child.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/process.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/services.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/local/transport.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_services_process.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_services.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_transport.pyis excluded by!sdk/**
📒 Files selected for processing (29)
packages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/process.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/services.pypackages/nemo_platform_ext/src/nemo_platform_ext/local/transport.pypackages/nemo_platform_ext/tests/cli/commands/test_services_process.pypackages/nemo_platform_ext/tests/local/test_services.pypackages/nemo_platform_ext/tests/local/test_transport.pypackages/nmp_common/src/nmp/common/auth/client.pypackages/nmp_common/src/nmp/common/platform_endpoint.pypackages/nmp_common/src/nmp/common/sdk_factory.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/tests/auth/test_client.pypackages/nmp_common/tests/nmp_common/test_common_service.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_common/tests/test_platform_endpoint.pypackages/nmp_platform_runner/src/nmp/platform_runner/config.pypackages/nmp_platform_runner/tests/test_config.pyservices/core/jobs/jobs-launcher/cmd/otel.goservices/core/jobs/jobs-launcher/cmd/otel_test.goservices/core/jobs/jobs-launcher/cmd/run.goservices/core/jobs/jobs-launcher/cmd/run_test.goservices/core/jobs/jobs-launcher/nmpclient/client_test.goservices/core/jobs/jobs-launcher/nmpclient/endpoint_test.goservices/core/jobs/src/nmp/core/jobs/controllers/backends/base.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.pyservices/core/jobs/tests/controllers/test_base.pyservices/core/jobs/tests/controllers/test_subprocess_backend.pyservices/core/jobs/tests/controllers/test_subprocess_runtime.py
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/nemo_platform_ext/src/nemo_platform_ext/local/_service_child.py
- packages/nemo_platform_ext/tests/cli/commands/test_services_process.py
- packages/nmp_platform_runner/src/nmp/platform_runner/config.py
- services/core/jobs/jobs-launcher/cmd/otel_test.go
- services/core/jobs/jobs-launcher/nmpclient/client_test.go
- packages/nemo_platform_ext/src/nemo_platform_ext/local/transport.py
- services/core/jobs/jobs-launcher/cmd/run.go
- packages/nmp_common/src/nmp/common/auth/client.py
- services/core/jobs/jobs-launcher/cmd/otel.go
- packages/nmp_common/src/nmp/common/platform_endpoint.py
- packages/nmp_common/src/nmp/common/service/base.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py
- packages/nmp_common/src/nmp/common/sdk_factory.py
- services/core/jobs/jobs-launcher/cmd/run_test.go
- packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py
ironcommit
force-pushed
the
services-sdk/rsadler
branch
9 times, most recently
from
July 20, 2026 17:07
8256223 to
3e1d3f6
Compare
mckornfield
approved these changes
Jul 20, 2026
ironcommit
force-pushed
the
services-sdk/rsadler
branch
from
July 20, 2026 21:46
3e1d3f6 to
f876427
Compare
ironcommit
force-pushed
the
services-sdk/rsadler
branch
from
July 21, 2026 02:07
f876427 to
68c25ad
Compare
mikeknep
reviewed
Jul 21, 2026
ironcommit
force-pushed
the
services-sdk/rsadler
branch
2 times, most recently
from
July 21, 2026 16:27
a9149ea to
96fa4da
Compare
ironcommit
force-pushed
the
services-sdk/rsadler
branch
from
July 21, 2026 17:53
96fa4da to
b04c524
Compare
mikeknep
approved these changes
Jul 21, 2026
Extracts the local services process lifecycle out of the CLI into a standalone SDK module (local.services, local.process, local.transport) so that Python callers can programmatically start, stop, connect to, and manage NeMo Platform service instances without importing CLI internals. The key addition is first-class Unix domain socket (UDS) transport support across the entire stack — from the platform runner and uvicorn server binding, through inter-service HTTP clients (auth, health checks, SDK factory), to the jobs-launcher Go sidecar. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
ironcommit
force-pushed
the
services-sdk/rsadler
branch
from
July 21, 2026 18:20
b04c524 to
3c5082e
Compare
ironcommit
enabled auto-merge
July 21, 2026 18:54
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 21, 2026
soluwalana
pushed a commit
that referenced
this pull request
Jul 22, 2026
Extracts the local services process lifecycle out of the CLI into a standalone SDK module (local.services, local.process, local.transport) so that Python callers can programmatically start, stop, connect to, and manage NeMo Platform service instances without importing CLI internals. The key addition is first-class Unix domain socket (UDS) transport support across the entire stack — from the platform runner and uvicorn server binding, through inter-service HTTP clients (auth, health checks, SDK factory), to the jobs-launcher Go sidecar. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com> Signed-off-by: Sam Oluwalana <soluwalana@nvidia.com>
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
3 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
Extracts the local services process lifecycle out of the CLI into a standalone SDK module (
local.services,local.process,local.transport) so that Python callers can programmatically start, stop, connect to, and manage NeMo Platform service instances without importing CLI internals.The key addition is first-class Unix domain socket (UDS) transport support across the entire stack — from the platform runner and uvicorn server binding, through inter-service HTTP clients (auth, health checks, SDK factory), to the jobs-launcher Go sidecar.
Changes
New local services SDK (
nemo_platform_ext.local/nemo_platform.local)ServiceRunConfig— declarative config for transport (UDS or TCP), socket paths, instance naming, and readiness settingsServiceHandle— returned after start; providesclient(),async_client(),stop(),wait_until_ready(), andis_running()run_services()— foreground blocking start with flock-based liveness trackingdaemonize_services()/daemonize_services_async()— background daemon start with readiness pollingconnect_services()/connect_services_async()— get-or-start convenience that returns a wiredNeMoPlatformSDK clientensure_services(),get_service_handle(),list_service_handles(),stop_services()— full lifecycle managementcli/commands/services/_process.pyreduced to a compatibility re-export aliasPlatform endpoint resolution (
nmp.common.platform_endpoint)PlatformEndpointdataclass with typedtransportfield ("tcp"|"uds") andsync_http_client()/async_http_client()factory methodsparse_platform_endpoint()— parseshttp://,https://, andunix://URLs into the typed modelresolve_platform_endpoint()/resolve_service_endpoint()— resolve fromNMP_BASE_URLor per-serviceNMP_<SERVICE>_URLenv varssdk_factory.py,auth/client.py,auth/middleware.py,service/base.py, andservice/api/health.pyso all inter-service HTTP clients correctly use UDS transport when configuredPlatform runner UDS support
run_platform(),resolve_run_configuration(), andrun_server()acceptsocket_pathparameterapply_run_environment()setsNMP_BASE_URL=unix://<path>when a socket path is presentuds=parameter instead ofhost/portJobs launcher Go changes
nmpclient/endpoint.go— parsesunix://endpoints and configures Go HTTP clients with Unix socket dialerNMP_BASE_URLusesunix://docker,kubernetes,subprocess) propagateNMP_BASE_URLfrom the environmentTests
ServiceRunConfigvalidation,ServiceHandleclient wiring,PlatformEndpointparsing, SDK factory UDS integration, runner config, and server socket bindingSummary by CodeRabbit
New Features
Bug Fixes
Tests