chore(sdk): refactor sdk service lifecycle - #937
Open
ironcommit wants to merge 1 commit 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:
📝 WalkthroughWalkthroughSDK and HTTP client ownership were refactored: shared module-level HTTP clients were removed in favor of SDK-owned immutable clients, a ChangesSDK Lifecycle and Service Integration Refactor
Sequence Diagram(s)sequenceDiagram
participant App as create_app
participant Provider as DependencyProvider
participant Factory as ServiceSDKFactory
participant Endpoint as PlatformEndpoint
App->>Provider: configure_platform_endpoint / configure_http_client
Provider->>Factory: create ServiceSDKFactory(endpoint, http_client)
Factory->>Endpoint: sync_sdk_http_client / async_sdk_http_client
Endpoint-->>Factory: immutable routing client
Factory-->>Provider: platform SDK instance
Provider-->>App: SDK client
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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: 7
🧹 Nitpick comments (8)
packages/nmp_common/src/nmp/common/platform_endpoint.py (1)
231-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
UDS_BASE_URLinstead of re-hardcoding the host.- return url.copy_with(scheme="http", host="nemo-platform.local", port=None) + uds_url = httpx.URL(UDS_BASE_URL) + return url.copy_with(scheme=uds_url.scheme, host=uds_url.host, port=None)🤖 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/src/nmp/common/platform_endpoint.py` around lines 231 - 235, Update _url_for_endpoint to use the existing UDS_BASE_URL value for the Unix-domain-socket host instead of hardcoding "nemo-platform.local", while preserving the current scheme and port handling.packages/nemo_platform_ext/tests/local/test_health_child.py (1)
110-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest no longer asserts anything new.
With
services=[]and no controllers, this only checks that lifespan sets a flag — already implied bytest_create_app_starts_and_joins_controller_threadsabove. Since the removed test covered shutdown cleanup, consider asserting the replacement behavior instead (e.g.DependencyProvider.close()/ SDK factoryaclose()runs on shutdown).🤖 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_health_child.py` around lines 110 - 130, The test_lifespan_shutdown_marks_controller_stop_signal test duplicates existing lifespan coverage because it only asserts the stop signal with no controllers. Replace this assertion with verification that shutdown performs the intended cleanup, specifically that DependencyProvider.close() or the SDK factory aclose() is invoked when the TestClient context exits.packages/nmp_common/src/nmp/common/immutable_http_client.py (1)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilently dropping response cookies diverges from httpx contract.
extract_cookiesbecoming a no-op means auth/session flows that rely onSet-Cookie(e.g. redirect chains) break with no signal. Intentional per the comment, but consider a debug log so it's diagnosable.🤖 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/src/nmp/common/immutable_http_client.py` around lines 74 - 78, Update _ImmutableCookies.extract_cookies to emit a debug-level diagnostic when response cookies are intentionally discarded, while preserving its non-persistent behavior. Include enough context to identify the dropped Set-Cookie handling without changing cookie storage or raising an error.packages/nemo_platform_plugin/tests/test_sdk_provider.py (1)
132-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNegative-only assertion.
Nothing verifies
_warn_missing_task_principalactually fires when the principal is absent and no token file is set — the assertion would pass even if the warning were deleted. Add the positive case totest_get_task_sdk_without_principal.🤖 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_plugin/tests/test_sdk_provider.py` around lines 132 - 148, The test coverage only asserts that no warning appears in the workload-identity path and does not verify the missing-principal warning. Update test_get_task_sdk_without_principal to configure no principal or token file, invoke DefaultSDKProvider.get_task_sdk, and assert caplog contains the warning emitted by _warn_missing_task_principal while preserving the existing SDK assertions.packages/nmp_common/tests/test_platform_endpoint.py (1)
100-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the routing transports themselves.
route_request_urlis well covered, but_SyncPlatformEndpointRoutingTransport.handle_request/ async counterpart — URL rewrite,Hostheader rewrite, and per-UDS transport caching — are untested. AMockTransport-backed test would catch regressions in_set_request_url.🤖 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/test_platform_endpoint.py` around lines 100 - 204, Add MockTransport-backed tests covering _SyncPlatformEndpointRoutingTransport.handle_request and its async counterpart: verify routed URL rewriting, matching Host header rewriting, and per-UDS transport caching while exercising _set_request_url. Reuse the existing endpoint fixtures/configuration and assert both sync and async transport behavior without expanding unrelated routing coverage.services/core/entities/src/nmp/core/entities/controllers/main.py (2)
1-1: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winController
DependencyProviders are never closed on shutdown. Both controllers create/receive aDependencyProviderfor their SDK client but thefinallyblock never callsprovider.close(), leaking the SDK's HTTP client on every restart.
services/core/entities/src/nmp/core/entities/controllers/main.py#L84-94: addloop.run_until_complete(provider.close())before returning (reuses the existing event loop).services/core/models/src/nmp/core/models/controllers/main.py#L111-122: addasyncio.run(provider.close())in thefinallyblock.🤖 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/entities/src/nmp/core/entities/controllers/main.py` at line 1, Close each controller’s DependencyProvider during shutdown: in the core entities controller’s finally block, call provider.close() through the existing event loop before returning; in the core models controller’s finally block, call asyncio.run(provider.close()).
46-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winController's
DependencyProvideris never closed.
provider(injected or freshly created) is used for the SDK client but never closed in thefinallyblock — its HTTP client/SDK leak on every shutdown/restart. An event loop already exists (loop) that can run the async close.♻️ Proposed fix
finally: cleanup_loop.stop() cleanup_loop.join(timeout=10) if cleanup_loop.is_alive(): logger.warning("Workspace cleanup loop did not stop in time") + loop.run_until_complete(provider.close()) logger.info("Entities controller stopped")Also applies to: 84-94
🤖 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/entities/src/nmp/core/entities/controllers/main.py` around lines 46 - 47, Update the controller cleanup around the provider initialization and existing finally block so the DependencyProvider used by get_sdk_client is always closed, including injected providers, by running its asynchronous close operation through the existing loop. Preserve the current SDK shutdown behavior and ensure provider cleanup executes during every shutdown or restart path.services/core/models/src/nmp/core/models/controllers/main.py (1)
61-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winController's
DependencyProvideris never closed.Same gap as
entitiescontroller:provider's SDK/HTTP client are never closed on shutdown.♻️ Proposed fix
if models_controller_loop.is_alive(): logger.warning("Models controller loop did not stop, forcing cleanup") models_controller.shutdown() + asyncio.run(provider.close()) logger.info("Models controller stopped")Also applies to: 111-122
🤖 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/src/nmp/core/models/controllers/main.py` around lines 61 - 62, Update the controller initialization and shutdown flow around DependencyProvider and provider.get_sdk_client(as_service="models") so the provider-owned SDK/HTTP client is closed when the controller shuts down. Match the cleanup pattern used by the entities controller, while preserving externally supplied providers and normal request handling.
🤖 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/nmp_common/src/nmp/common/platform_endpoint.py`:
- Around line 262-271: Make the UDS transport lookup and creation in
_transport_for_endpoint thread-safe so concurrent requests for the same socket
path cannot create duplicate transports. Guard the _uds_transports
check-and-insert with an appropriate lock, ensuring close() can still manage
every cached transport.
- Around line 149-165: Update resolve_platform_endpoint so malformed URLs from
individual service routes do not abort default endpoint resolution. Wrap each
resolve_service_endpoint call for discovered routes in targeted exception
handling, log the offending service name and URL, and omit only that route from
service_endpoints while still requiring the base_url to parse successfully.
In `@packages/nmp_common/src/nmp/common/sdk_factory.py`:
- Around line 43-44: Update _should_bootstrap_workload_identity to depend only
on is_workload_identity_token_file_set(), removing the http_client presence
check. Preserve the injected client by threading it separately through
create_platform_sdk() and create_async_platform_sdk() so the token exchange
still uses that client when workload identity bootstrap is enabled.
In `@packages/nmp_common/src/nmp/common/service/base.py`:
- Around line 73-79: Update DependencyProvider initialization in
Service.__init__ and the entity-client creation flow around
_get_entity_sdk_on_behalf_of() to pass the actual Service.name value instead of
retaining the hard-coded "platform" _service_name. Ensure the real service name
is set before creating the entity SDK so requests use the correct service
identifier.
In `@packages/nmp_common/src/nmp/common/service/sdk_factory.py`:
- Line 1: Update ServiceSDKFactory.aclose() and HTTP-client initialization to
track whether the factory created the client or received it from
DependencyProvider.configure_http_client(). Only close the client when owned by
the factory; leave injected shared clients open.
In `@services/core/models/tests/unit/api/test_models_api.py`:
- Around line 416-423: Rename the test containing the direct
start_update_model_spec_job invocation to describe that it verifies swallowing a
NemoTransportError, or instead route the test through create_model and assert
the persisted entity. Ensure the test name accurately matches the behavior
actually exercised.
In `@services/studio/src/nmp/studio/service.py`:
- Around line 147-153: Update the telemetry request flow around the service
method containing DefaultAsyncHttpxClient so it reuses a service-owned or
injected HTTP client instead of creating one per request. Initialize or accept
the client at service scope, use it for upstream_response requests, and close it
during the service’s shutdown lifecycle.
---
Nitpick comments:
In `@packages/nemo_platform_ext/tests/local/test_health_child.py`:
- Around line 110-130: The test_lifespan_shutdown_marks_controller_stop_signal
test duplicates existing lifespan coverage because it only asserts the stop
signal with no controllers. Replace this assertion with verification that
shutdown performs the intended cleanup, specifically that
DependencyProvider.close() or the SDK factory aclose() is invoked when the
TestClient context exits.
In `@packages/nemo_platform_plugin/tests/test_sdk_provider.py`:
- Around line 132-148: The test coverage only asserts that no warning appears in
the workload-identity path and does not verify the missing-principal warning.
Update test_get_task_sdk_without_principal to configure no principal or token
file, invoke DefaultSDKProvider.get_task_sdk, and assert caplog contains the
warning emitted by _warn_missing_task_principal while preserving the existing
SDK assertions.
In `@packages/nmp_common/src/nmp/common/immutable_http_client.py`:
- Around line 74-78: Update _ImmutableCookies.extract_cookies to emit a
debug-level diagnostic when response cookies are intentionally discarded, while
preserving its non-persistent behavior. Include enough context to identify the
dropped Set-Cookie handling without changing cookie storage or raising an error.
In `@packages/nmp_common/src/nmp/common/platform_endpoint.py`:
- Around line 231-235: Update _url_for_endpoint to use the existing UDS_BASE_URL
value for the Unix-domain-socket host instead of hardcoding
"nemo-platform.local", while preserving the current scheme and port handling.
In `@packages/nmp_common/tests/test_platform_endpoint.py`:
- Around line 100-204: Add MockTransport-backed tests covering
_SyncPlatformEndpointRoutingTransport.handle_request and its async counterpart:
verify routed URL rewriting, matching Host header rewriting, and per-UDS
transport caching while exercising _set_request_url. Reuse the existing endpoint
fixtures/configuration and assert both sync and async transport behavior without
expanding unrelated routing coverage.
In `@services/core/entities/src/nmp/core/entities/controllers/main.py`:
- Line 1: Close each controller’s DependencyProvider during shutdown: in the
core entities controller’s finally block, call provider.close() through the
existing event loop before returning; in the core models controller’s finally
block, call asyncio.run(provider.close()).
- Around line 46-47: Update the controller cleanup around the provider
initialization and existing finally block so the DependencyProvider used by
get_sdk_client is always closed, including injected providers, by running its
asynchronous close operation through the existing loop. Preserve the current SDK
shutdown behavior and ensure provider cleanup executes during every shutdown or
restart path.
In `@services/core/models/src/nmp/core/models/controllers/main.py`:
- Around line 61-62: Update the controller initialization and shutdown flow
around DependencyProvider and provider.get_sdk_client(as_service="models") so
the provider-owned SDK/HTTP client is closed when the controller shuts down.
Match the cleanup pattern used by the entities controller, while preserving
externally supplied providers and normal request handling.
🪄 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: 373a22c4-c7ff-4d6e-adff-f5ebb6505bdd
📒 Files selected for processing (40)
packages/nemo_platform_ext/tests/local/test_health_child.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.pypackages/nemo_platform_plugin/tests/client/test_adapter.pypackages/nemo_platform_plugin/tests/test_sdk_provider.pypackages/nmp_common/src/nmp/common/entities/client.pypackages/nmp_common/src/nmp/common/http_clients.pypackages/nmp_common/src/nmp/common/immutable_http_client.pypackages/nmp_common/src/nmp/common/platform_endpoint.pypackages/nmp_common/src/nmp/common/sdk_factory.pypackages/nmp_common/src/nmp/common/service/__init__.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/src/nmp/common/service/dependencies.pypackages/nmp_common/src/nmp/common/service/sdk_factory.pypackages/nmp_common/tests/nmp_common/test_dependency_provider.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_common/tests/service/test_sdk_factory.pypackages/nmp_common/tests/test_immutable_http_client.pypackages/nmp_common/tests/test_platform_endpoint.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_server.pypackages/nmp_testing/src/nmp/testing/client.pyservices/core/entities/src/nmp/core/entities/controllers/main.pyservices/core/files/src/nmp/core/files/api/endpoint_helpers.pyservices/core/inference-gateway/src/nmp/core/inference_gateway/service.pyservices/core/inference-gateway/src/nmp/core/inference_gateway/testing/fixtures.pyservices/core/inference-gateway/tests/unit/conftest.pyservices/core/inference-gateway/tests/unit/test_service.pyservices/core/jobs/src/nmp/core/jobs/api/dependencies.pyservices/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.pyservices/core/models/src/nmp/core/models/api/dependencies.pyservices/core/models/src/nmp/core/models/api/service/adapter_entity_service.pyservices/core/models/src/nmp/core/models/api/service/model_entity_service.pyservices/core/models/src/nmp/core/models/api/v2/models.pyservices/core/models/src/nmp/core/models/controllers/main.pyservices/core/models/tests/unit/api/test_models_api.pyservices/guardrails/src/nmp/guardrails/api/dependencies.pyservices/hello-world/tests/integration/tasks/test_workload_workspace_get_task.pyservices/studio/src/nmp/studio/service.pyservices/studio/tests/unit/test_service.py
💤 Files with no reviewable changes (3)
- packages/nmp_common/src/nmp/common/http_clients.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.py
- packages/nmp_testing/src/nmp/testing/client.py
Contributor
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nmp_common/src/nmp/common/service/base.py (1)
250-253: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClear configured clients even when no SDK factory was created.
configure_http_client()can set_configured_http_clientbefore first SDK access, but this branch leaves it populated when_service_sdk_factoryisNone. Clear it unconditionally duringclose()to prevent stale or later-closed clients from being reused.🤖 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/src/nmp/common/service/base.py` around lines 250 - 253, Update the close() cleanup around _service_sdk_factory so _configured_http_client is cleared unconditionally, including when no SDK factory exists. Keep SDK factory shutdown conditional, but move the configured-client reset outside that condition to prevent reuse of stale clients.
🤖 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/nmp_common/src/nmp/common/service/base.py`:
- Around line 510-511: Update the OpenAPI metadata setup in
_ServiceFastAPI.openapi() so nmp_openapi_description preserves
Service.description by removing the empty-string assignment or assigning
self.description. Keep the existing nmp_openapi_summary behavior unchanged.
In `@plugins/nemo-customizer/openapi/openapi.yaml`:
- Around line 395-400: The OpenAPI specification removes the still-supported RL
job routes. In openapi.yaml, retain the existing
/apis/customization/v2/workspaces/{workspace}/rl/jobs paths alongside the
Unsloth routes, mark them as deprecated, and preserve their documented behavior
until RL consumers migrate.
---
Outside diff comments:
In `@packages/nmp_common/src/nmp/common/service/base.py`:
- Around line 250-253: Update the close() cleanup around _service_sdk_factory so
_configured_http_client is cleared unconditionally, including when no SDK
factory exists. Keep SDK factory shutdown conditional, but move the
configured-client reset outside that condition to prevent reuse of stale
clients.
🪄 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: 9728c8f3-a7dd-4be6-a4c2-2a7bd02aaa88
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_health_child.pyis excluded by!sdk/**
📒 Files selected for processing (8)
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.pypackages/nmp_common/src/nmp/common/service/__init__.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/src/nmp/common/service/dependencies.pypackages/nmp_common/tests/api/test_query_param_schemas.pypackages/nmp_common/tests/nmp_common/test_dependency_provider.pyplugins/nemo-customizer/openapi/openapi.yamlplugins/nemo-deployments/openapi/openapi.yaml
💤 Files with no reviewable changes (3)
- packages/nmp_common/src/nmp/common/service/init.py
- packages/nmp_common/src/nmp/common/service/dependencies.py
- packages/nmp_common/tests/nmp_common/test_dependency_provider.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.py
ironcommit
force-pushed
the
AIRCORE-950-dependency-provider-sdk-test-client-migration/rsadler
branch
from
July 28, 2026 01:06
e2ccece to
7707646
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
plugins/nemo-customizer/openapi/openapi.yaml (1)
395-400: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRL job paths removed without a deprecation window.
All eight
/rl/jobsroutes are gone whileplugins/nemo-rlstill implements them. Keep them asdeprecated: truealiases until consumers migrate.🤖 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 `@plugins/nemo-customizer/openapi/openapi.yaml` around lines 395 - 400, Restore all eight /rl/jobs routes in the OpenAPI specification as aliases to the existing nemo-rl endpoints, and mark each route deprecated: true. Preserve their existing request/response definitions and operation behavior while retaining the current /unsloth/jobs routes.packages/nmp_common/src/nmp/common/platform_endpoint.py (2)
262-271: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUDS transport cache is racy.
Concurrent first requests to the same socket path can each build a transport; the loser is orphaned and never closed. Guard the check-and-insert with a lock, or pre-populate from
endpoint.service_endpointsin__init__since the routing table is fixed.Also applies to: 290-299
🤖 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/src/nmp/common/platform_endpoint.py` around lines 262 - 271, The UDS transport cache in _transport_for_endpoint is racy during concurrent initialization. Guard the _uds_transports lookup and HTTPTransport creation/insertion with a shared lock, ensuring only one transport is created per socket path and reused by all callers; apply the same synchronization to the related cache access around the other endpoint transport path.
149-165: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOne malformed service URL breaks all SDK construction.
Every discovered route (including env-derived
NMP_<SVC>_URL) is parsed eagerly, so a single bad value makesresolve_platform_endpointraise for callers that never touch that service. Skip and log the offending route instead.🤖 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/src/nmp/common/platform_endpoint.py` around lines 149 - 165, Update resolve_platform_endpoint and its service-endpoint construction so malformed URLs from discovered routes, including NMP_<SVC>_URL values, are caught per service rather than aborting SDK construction. Log the offending service route, omit it from service_endpoints, and preserve successful routes and default endpoint resolution.
🤖 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/nmp_common/src/nmp/common/platform_endpoint.py`:
- Around line 231-235: The _url_for_endpoint function drops the configured base
URL path for non-UDS endpoints. Preserve endpoint_url.path when constructing the
returned URL so prefixes such as /entities-prefix remain part of the request
route, or enforce rejection of non-root paths during endpoint parsing if that is
the established contract.
---
Duplicate comments:
In `@packages/nmp_common/src/nmp/common/platform_endpoint.py`:
- Around line 262-271: The UDS transport cache in _transport_for_endpoint is
racy during concurrent initialization. Guard the _uds_transports lookup and
HTTPTransport creation/insertion with a shared lock, ensuring only one transport
is created per socket path and reused by all callers; apply the same
synchronization to the related cache access around the other endpoint transport
path.
- Around line 149-165: Update resolve_platform_endpoint and its service-endpoint
construction so malformed URLs from discovered routes, including NMP_<SVC>_URL
values, are caught per service rather than aborting SDK construction. Log the
offending service route, omit it from service_endpoints, and preserve successful
routes and default endpoint resolution.
In `@plugins/nemo-customizer/openapi/openapi.yaml`:
- Around line 395-400: Restore all eight /rl/jobs routes in the OpenAPI
specification as aliases to the existing nemo-rl endpoints, and mark each route
deprecated: true. Preserve their existing request/response definitions and
operation behavior while retaining the current /unsloth/jobs routes.
🪄 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: f2271d61-d238-494d-baf2-8e9f37766d29
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_health_child.pyis excluded by!sdk/**
📒 Files selected for processing (44)
packages/nemo_platform_ext/tests/local/test_health_child.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.pypackages/nemo_platform_plugin/tests/client/test_adapter.pypackages/nemo_platform_plugin/tests/test_sdk_provider.pypackages/nmp_common/src/nmp/common/entities/client.pypackages/nmp_common/src/nmp/common/http_clients.pypackages/nmp_common/src/nmp/common/immutable_http_client.pypackages/nmp_common/src/nmp/common/platform_endpoint.pypackages/nmp_common/src/nmp/common/sdk_factory.pypackages/nmp_common/src/nmp/common/service/__init__.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/src/nmp/common/service/dependencies.pypackages/nmp_common/src/nmp/common/service/sdk_factory.pypackages/nmp_common/tests/api/test_query_param_schemas.pypackages/nmp_common/tests/nmp_common/test_common_service.pypackages/nmp_common/tests/nmp_common/test_dependency_provider.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_common/tests/service/test_sdk_factory.pypackages/nmp_common/tests/test_immutable_http_client.pypackages/nmp_common/tests/test_platform_endpoint.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_server.pypackages/nmp_testing/src/nmp/testing/client.pyplugins/nemo-customizer/openapi/openapi.yamlplugins/nemo-deployments/openapi/openapi.yamlservices/core/entities/src/nmp/core/entities/controllers/main.pyservices/core/files/src/nmp/core/files/api/endpoint_helpers.pyservices/core/inference-gateway/src/nmp/core/inference_gateway/service.pyservices/core/inference-gateway/src/nmp/core/inference_gateway/testing/fixtures.pyservices/core/inference-gateway/tests/unit/conftest.pyservices/core/inference-gateway/tests/unit/test_service.pyservices/core/jobs/src/nmp/core/jobs/api/dependencies.pyservices/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.pyservices/core/models/src/nmp/core/models/api/dependencies.pyservices/core/models/src/nmp/core/models/api/service/adapter_entity_service.pyservices/core/models/src/nmp/core/models/api/service/model_entity_service.pyservices/core/models/src/nmp/core/models/api/v2/models.pyservices/core/models/src/nmp/core/models/controllers/main.pyservices/core/models/tests/unit/api/test_models_api.pyservices/guardrails/src/nmp/guardrails/api/dependencies.pyservices/hello-world/tests/integration/tasks/test_workload_workspace_get_task.pyservices/studio/src/nmp/studio/service.pyservices/studio/tests/unit/test_service.py
💤 Files with no reviewable changes (2)
- packages/nmp_common/src/nmp/common/http_clients.py
- packages/nmp_testing/src/nmp/testing/client.py
🚧 Files skipped from review as they are similar to previous changes (32)
- packages/nmp_common/src/nmp/common/service/init.py
- services/core/files/src/nmp/core/files/api/endpoint_helpers.py
- packages/nmp_common/src/nmp/common/service/dependencies.py
- plugins/nemo-deployments/openapi/openapi.yaml
- packages/nmp_common/src/nmp/common/entities/client.py
- packages/nmp_platform_runner/tests/test_server.py
- services/core/models/src/nmp/core/models/api/service/model_entity_service.py
- services/core/inference-gateway/tests/unit/test_service.py
- services/hello-world/tests/integration/tasks/test_workload_workspace_get_task.py
- services/core/models/tests/unit/api/test_models_api.py
- services/core/entities/src/nmp/core/entities/controllers/main.py
- services/core/jobs/src/nmp/core/jobs/api/dependencies.py
- services/core/inference-gateway/src/nmp/core/inference_gateway/service.py
- services/core/inference-gateway/tests/unit/conftest.py
- services/core/models/src/nmp/core/models/controllers/main.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.py
- services/guardrails/src/nmp/guardrails/api/dependencies.py
- services/core/models/src/nmp/core/models/api/service/adapter_entity_service.py
- services/studio/tests/unit/test_service.py
- services/core/models/src/nmp/core/models/api/dependencies.py
- services/studio/src/nmp/studio/service.py
- services/core/inference-gateway/src/nmp/core/inference_gateway/testing/fixtures.py
- packages/nmp_common/src/nmp/common/service/sdk_factory.py
- packages/nemo_platform_ext/tests/local/test_health_child.py
- services/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.py
- packages/nmp_common/src/nmp/common/immutable_http_client.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.py
- packages/nmp_common/tests/api/test_query_param_schemas.py
- services/core/models/src/nmp/core/models/api/v2/models.py
- packages/nmp_common/src/nmp/common/sdk_factory.py
- packages/nmp_platform_runner/src/nmp/platform_runner/server.py
- packages/nmp_common/src/nmp/common/service/base.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nmp_common/src/nmp/common/sdk_factory.py (1)
112-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the private
_resolve_bootstrapimport. Expose a public bootstrap helper for injected-client use, or fail fast here with a clear import error.🤖 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/src/nmp/common/sdk_factory.py` around lines 112 - 126, Update _workload_identity_bootstrap_for_injected_client to avoid importing the private _resolve_bootstrap symbol; preferably expose and call a public bootstrap helper that accepts the same configuration, URL, token, and extra-header inputs, or otherwise fail immediately with a clear import error when the public API is unavailable.
🤖 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 `@packages/nmp_common/src/nmp/common/sdk_factory.py`:
- Around line 112-126: Update _workload_identity_bootstrap_for_injected_client
to avoid importing the private _resolve_bootstrap symbol; preferably expose and
call a public bootstrap helper that accepts the same configuration, URL, token,
and extra-header inputs, or otherwise fail immediately with a clear import error
when the public API is unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c5b4fcea-e175-4214-bb90-4e73a34bc8ad
📒 Files selected for processing (9)
packages/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/nmp_common/test_common_service.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_common/tests/test_platform_endpoint.pyservices/core/models/tests/unit/api/test_models_api.pyservices/studio/src/nmp/studio/service.pyservices/studio/tests/unit/test_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nmp_common/src/nmp/common/service/base.py
- packages/nmp_common/src/nmp/common/platform_endpoint.py
ironcommit
force-pushed
the
AIRCORE-950-dependency-provider-sdk-test-client-migration/rsadler
branch
from
July 28, 2026 02:42
e0c8403 to
2186927
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nmp_common/src/nmp/common/sdk_factory.py (1)
311-341: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWorkload-identity path silently drops
as_serviceandon_behalf_of.Callers passing delegation get it discarded when the token file is set, with no log. Document it in the docstring (and ideally debug-log the drop).
📝 Docstring note
base_url: Optional platform base URL. When omitted with no explicit http_client, the resolved platform endpoint supplies both the base URL and HTTP client. + + Note: + When ``NMP_WORKLOAD_IDENTITY_TOKEN_FILE`` is set, authentication comes from + the exchanged workload token; ``as_service`` and ``on_behalf_of`` are ignored.🤖 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/src/nmp/common/sdk_factory.py` around lines 311 - 341, Update the workload-identity branch in the SDK factory around _sync_workload_identity_sdk so the docstring explicitly documents that as_service and on_behalf_of are ignored when workload identity is bootstrapped; add a debug log at that branch when either delegation argument is supplied, indicating they were dropped. Preserve existing behavior for base_url, internal, and http_client.
🤖 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/nmp_common/src/nmp/common/platform_endpoint.py`:
- Around line 205-229: Restrict _service_url_env_names and its use from
_service_route_names to URL environment variables whose normalized service names
are already present in the platform configuration’s service_discovery or
get_services results. Exclude unknown NMP_*_URL variables while preserving BASE
exclusion and existing normalization behavior, so non-service environment values
cannot affect resolve_platform_endpoint().
In `@packages/nmp_common/tests/test_platform_endpoint.py`:
- Around line 92-133: Add an autouse pytest fixture in test_platform_endpoint.py
that removes every environment variable matching NMP_*_URL before each test,
while preserving NMP_BASE_URL. Use monkeypatch.delenv with a snapshot of
os.environ keys so resolve_platform_endpoint tests run without ambient service
routes.
---
Outside diff comments:
In `@packages/nmp_common/src/nmp/common/sdk_factory.py`:
- Around line 311-341: Update the workload-identity branch in the SDK factory
around _sync_workload_identity_sdk so the docstring explicitly documents that
as_service and on_behalf_of are ignored when workload identity is bootstrapped;
add a debug log at that branch when either delegation argument is supplied,
indicating they were dropped. Preserve existing behavior for base_url, internal,
and http_client.
🪄 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: 6fb33f6e-eae9-4ed2-b428-d1dac6ca248e
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_health_child.pyis excluded by!sdk/**
📒 Files selected for processing (54)
packages/nemo_platform_ext/tests/local/test_health_child.pypackages/nemo_platform_ext/tests/local/test_services_contract.pypackages/nemo_platform_ext/tests/local/test_sidecar_integration.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.pypackages/nemo_platform_plugin/tests/client/test_adapter.pypackages/nemo_platform_plugin/tests/test_sdk_provider.pypackages/nmp_common/src/nmp/common/entities/client.pypackages/nmp_common/src/nmp/common/http_clients.pypackages/nmp_common/src/nmp/common/immutable_http_client.pypackages/nmp_common/src/nmp/common/platform_endpoint.pypackages/nmp_common/src/nmp/common/sdk_factory.pypackages/nmp_common/src/nmp/common/service/__init__.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/src/nmp/common/service/dependencies.pypackages/nmp_common/src/nmp/common/service/sdk_factory.pypackages/nmp_common/tests/api/test_query_param_schemas.pypackages/nmp_common/tests/nmp_common/test_common_service.pypackages/nmp_common/tests/nmp_common/test_dependency_provider.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_common/tests/service/test_sdk_factory.pypackages/nmp_common/tests/test_immutable_http_client.pypackages/nmp_common/tests/test_platform_endpoint.pypackages/nmp_platform_runner/src/nmp/platform_runner/loader.pypackages/nmp_platform_runner/src/nmp/platform_runner/plugin_adapter.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_run.pypackages/nmp_platform_runner/tests/test_server.pypackages/nmp_platform_runner/tests/test_sidecars.pypackages/nmp_testing/src/nmp/testing/client.pyservices/core/entities/src/nmp/core/entities/controllers/main.pyservices/core/files/src/nmp/core/files/api/endpoint_helpers.pyservices/core/inference-gateway/src/nmp/core/inference_gateway/service.pyservices/core/inference-gateway/src/nmp/core/inference_gateway/testing/fixtures.pyservices/core/inference-gateway/tests/unit/conftest.pyservices/core/inference-gateway/tests/unit/test_service.pyservices/core/jobs/src/nmp/core/jobs/api/dependencies.pyservices/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.pyservices/core/jobs/src/nmp/core/jobs/controllers/main.pyservices/core/models/src/nmp/core/models/api/dependencies.pyservices/core/models/src/nmp/core/models/api/service/adapter_entity_service.pyservices/core/models/src/nmp/core/models/api/service/model_entity_service.pyservices/core/models/src/nmp/core/models/api/v2/models.pyservices/core/models/src/nmp/core/models/config.pyservices/core/models/src/nmp/core/models/controllers/backends/registry.pyservices/core/models/src/nmp/core/models/controllers/main.pyservices/core/models/src/nmp/core/models/sidecars/adapters/main.pyservices/core/models/tests/unit/api/test_models_api.pyservices/core/models/tests/unit/sidecars/test_adapters_controller.pyservices/guardrails/src/nmp/guardrails/api/dependencies.pyservices/hello-world/tests/integration/tasks/test_workload_workspace_get_task.pyservices/studio/src/nmp/studio/service.pyservices/studio/tests/unit/test_service.py
💤 Files with no reviewable changes (2)
- packages/nmp_common/src/nmp/common/http_clients.py
- packages/nmp_testing/src/nmp/testing/client.py
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/nmp_common/src/nmp/common/service/init.py
- services/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.py
- packages/nmp_common/src/nmp/common/entities/client.py
- services/core/files/src/nmp/core/files/api/endpoint_helpers.py
- services/core/inference-gateway/tests/unit/conftest.py
- services/core/inference-gateway/src/nmp/core/inference_gateway/testing/fixtures.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.py
- services/core/models/src/nmp/core/models/api/service/model_entity_service.py
- services/core/models/src/nmp/core/models/api/service/adapter_entity_service.py
- packages/nmp_common/src/nmp/common/service/sdk_factory.py
- services/core/inference-gateway/src/nmp/core/inference_gateway/service.py
- services/hello-world/tests/integration/tasks/test_workload_workspace_get_task.py
- services/core/inference-gateway/tests/unit/test_service.py
- packages/nmp_common/src/nmp/common/immutable_http_client.py
- services/guardrails/src/nmp/guardrails/api/dependencies.py
- services/core/models/src/nmp/core/models/api/dependencies.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.py
- services/studio/src/nmp/studio/service.py
- packages/nmp_common/tests/api/test_query_param_schemas.py
- packages/nmp_common/src/nmp/common/service/base.py
- services/core/models/src/nmp/core/models/api/v2/models.py
ironcommit
force-pushed
the
AIRCORE-950-dependency-provider-sdk-test-client-migration/rsadler
branch
from
July 28, 2026 16:11
4eef4f6 to
f5b4501
Compare
Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
ironcommit
force-pushed
the
AIRCORE-950-dependency-provider-sdk-test-client-migration/rsadler
branch
from
July 28, 2026 16:29
f5b4501 to
bd8750f
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
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 by CodeRabbit