feat(models): add typed NemoClient models foundation - #993
Open
maxdubrinsky wants to merge 1 commit into
Open
Conversation
Introduces the typed Models service client that the AIRCORE-876 consumer migration will build on: request/response DTOs (types), PreparedRequest endpoint builders (endpoints), and the sync/async ModelsClient surface (client). Purely additive: no existing code imports it yet, so it changes no runtime behavior and carries zero risk to current consumers. Also carries the method() descriptor fix that this client requires -- class-level attribute access now resolves without invoking the wrapped callable, so Mock(spec=ModelsClient) and other introspection no longer break -- plus a response docstring note on distinguishing 202/204 deletes. The consumer repoint, the packages/models resources rewrite, and the vendored SDK sync land separately as the breaking, coupled steps. Covered by endpoint-builder, client-surface, and descriptor tests. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds typed Models service DTOs and endpoint contracts, synchronous and asynchronous clients, OpenAI route builders, deployment/provider polling, conflict handling, and tests for descriptor introspection, requests, routing, CRUD, and polling. ChangesModels service client
Sequence Diagram(s)sequenceDiagram
participant ModelsClient
participant ModelsAPI
participant StatusHistory
ModelsClient->>ModelsAPI: Fetch deployment or provider status
ModelsAPI-->>ModelsClient: Return typed status response
ModelsClient->>StatusHistory: Inspect new history entries
StatusHistory-->>ModelsClient: Return current status and message
ModelsClient->>ModelsAPI: Poll until desired, error, deletion, or timeout
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/nemo_platform_plugin/src/nemo_platform_plugin/models/client.py (1)
245-329: 🩺 Stability & Availability | 🔵 TrivialSync polling blocks the calling thread for up to
timeoutseconds.
wait_for_deployment_status/wait_for_provider_statusonModelsClientblock synchronously (default up to 1200s / 60s). Consumer wiring is deferred to a separate PR — worth confirming those call-sites don't invoke this from a request-handling thread; preferAsyncModelsClientor aNemoJobfor long-running polls.🤖 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/src/nemo_platform_plugin/models/client.py` around lines 245 - 329, The wait_for_deployment_status and wait_for_provider_status methods synchronously block the calling thread for their full polling timeout. Review their call sites and ensure they are not invoked from request-handling threads; route long-running polling through AsyncModelsClient or a NemoJob instead, while preserving the existing polling behavior for suitable synchronous callers.packages/nemo_platform_plugin/tests/models/test_client.py (1)
179-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProvider fixtures built with
_model_json, not_provider_json.Both
test_create_provider_exist_ok_resolves_conflict(Line 186) andtest_provider_route_for_deployment_fetches_provider(Line 240) build the mockedModelProviderresponse via_model_json(...)(the ModelEntity-shaped helper) instead of_provider_json(...). It passes today, but it's confusing and fragile if the two DTOs diverge further.♻️ Proposed fix
- existing = httpx.Response( - 200, - request=httpx.Request("GET", BASE), - json=_model_json("p", host_url="http://x") | {"host_url": "http://x"}, - ) + existing = httpx.Response( + 200, + request=httpx.Request("GET", BASE), + json=_provider_json(name="p", host_url="http://x"), + )- http.request.return_value = httpx.Response( - 200, - request=httpx.Request("GET", BASE), - json=_model_json("my-provider", host_url="https://api.example.com"), - ) + http.request.return_value = httpx.Response( + 200, + request=httpx.Request("GET", BASE), + json=_provider_json(name="my-provider", host_url="https://api.example.com"), + )Also applies to: 235-247
🤖 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/models/test_client.py` around lines 179 - 198, Update the provider response fixtures in test_create_provider_exist_ok_resolves_conflict and test_provider_route_for_deployment_fetches_provider to use _provider_json(...) instead of _model_json(...), while preserving the existing provider-specific field overrides and assertions.
🤖 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/models/client.py`:
- Around line 237-243: Validate that model_provider_id contains the required
“workspace/name” delimiter before unpacking it in both
get_provider_route_openai_url_for_deployment and its async counterpart. Raise a
clear ValueError identifying the deployment and malformed model_provider_id,
while preserving the existing provider lookup for valid identifiers.
In `@packages/nemo_platform_plugin/tests/models/test_endpoints.py`:
- Around line 119-126: Replace the dynamic __import__ calls in
test_create_model_adapter_nested_path and the corresponding
update-model-deployment test with normal top-level imports for
CreateModelAdapterRequest and UpdateModelDeploymentConfigRequest, then
instantiate those imported classes directly at the call sites.
---
Nitpick comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/models/client.py`:
- Around line 245-329: The wait_for_deployment_status and
wait_for_provider_status methods synchronously block the calling thread for
their full polling timeout. Review their call sites and ensure they are not
invoked from request-handling threads; route long-running polling through
AsyncModelsClient or a NemoJob instead, while preserving the existing polling
behavior for suitable synchronous callers.
In `@packages/nemo_platform_plugin/tests/models/test_client.py`:
- Around line 179-198: Update the provider response fixtures in
test_create_provider_exist_ok_resolves_conflict and
test_provider_route_for_deployment_fetches_provider to use _provider_json(...)
instead of _model_json(...), while preserving the existing provider-specific
field overrides and assertions.
🪄 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: 22aa54f6-4522-4cd6-b964-c9dfc8e2671c
📒 Files selected for processing (8)
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/method.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/models/client.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/models/endpoints.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/models/types.pypackages/nemo_platform_plugin/tests/client/test_method.pypackages/nemo_platform_plugin/tests/models/test_client.pypackages/nemo_platform_plugin/tests/models/test_endpoints.py
Comment on lines
+237
to
+243
| def get_provider_route_openai_url_for_deployment(self, deployment: DeploymentLike) -> str: | ||
| """Fetch a deployment's ModelProvider and return its OpenAI route URL.""" | ||
| if not deployment.model_provider_id: | ||
| raise ValueError(f"Deployment '{deployment.name}' has no associated model_provider_id") | ||
| workspace, name = deployment.model_provider_id.split("/", 1) | ||
| provider = self.get_provider(name=name, workspace=workspace).data() | ||
| return self.get_provider_route_openai_url(provider) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unvalidated model_provider_id split raises unclear error.
model_provider_id.split("/", 1) yields a 1-item list if there's no /, and the unpack (workspace, name = ...) throws an opaque ValueError: not enough values to unpack instead of a clear message. Same pattern in both sync (Line 241) and async (Line 338) versions.
🐛 Proposed fix (apply to both sync and async methods)
- workspace, name = deployment.model_provider_id.split("/", 1)
+ parts = deployment.model_provider_id.split("/", 1)
+ if len(parts) != 2:
+ raise ValueError(
+ f"Deployment '{deployment.name}' has malformed model_provider_id: "
+ f"{deployment.model_provider_id!r}"
+ )
+ workspace, name = partsAlso applies to: 334-340
🤖 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/src/nemo_platform_plugin/models/client.py`
around lines 237 - 243, Validate that model_provider_id contains the required
“workspace/name” delimiter before unpacking it in both
get_provider_route_openai_url_for_deployment and its async counterpart. Raise a
clear ValueError identifying the deployment and malformed model_provider_id,
while preserving the existing provider lookup for valid identifiers.
Comment on lines
+119
to
+126
| def test_create_model_adapter_nested_path() -> None: | ||
| prepared = endpoints.create_model_adapter( | ||
| workspace="w", | ||
| model_name="base", | ||
| body=__import__( | ||
| "nemo_platform_plugin.models.types", fromlist=["CreateModelAdapterRequest"] | ||
| ).CreateModelAdapterRequest(name="a", fileset="w/fs", finetuning_type=FinetuningType.LORA), | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Dynamic __import__ instead of normal import.
CreateModelAdapterRequest and UpdateModelDeploymentConfigRequest are fetched at call time via __import__("nemo_platform_plugin.models.types", fromlist=[...]) instead of just adding them to the existing top-level import block.
♻️ Proposed fix
from nemo_platform_plugin.models.types import (
Adapter,
ContainerExecutorConfig,
+ CreateModelAdapterRequest,
CreateAdapterRequest,
CreateModelDeploymentConfigRequest,
CreateModelDeploymentRequest,
CreateModelEntityRequest,
CreateModelProviderRequest,
CreatePromptRequest,
Engine,
FinetuningType,
ModelDeployment,
ModelDeploymentConfig,
ModelDeploymentConfigModelSpec,
ModelDeploymentStatus,
ModelEntity,
ModelProvider,
ModelProviderStatus,
Prompt,
UpdateAdapterRequest,
UpdateModelDeploymentRequest,
+ UpdateModelDeploymentConfigRequest,
UpdateModelDeploymentStatusRequest,
UpdateModelEntityRequest,
UpdateModelProviderStatusRequest,
UpdatePromptRequest,
UpsertModelProviderRequest,
)- body=__import__(
- "nemo_platform_plugin.models.types", fromlist=["CreateModelAdapterRequest"]
- ).CreateModelAdapterRequest(name="a", fileset="w/fs", finetuning_type=FinetuningType.LORA),
+ body=CreateModelAdapterRequest(name="a", fileset="w/fs", finetuning_type=FinetuningType.LORA),- body=__import__(
- "nemo_platform_plugin.models.types", fromlist=["UpdateModelDeploymentConfigRequest"]
- ).UpdateModelDeploymentConfigRequest(
+ body=UpdateModelDeploymentConfigRequest(
engine=Engine.VLLM,
model_spec=ModelDeploymentConfigModelSpec(model_name="llama"),
executor_config=ContainerExecutorConfig(gpu=1),
),Also applies to: 289-296
🤖 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/models/test_endpoints.py` around lines
119 - 126, Replace the dynamic __import__ calls in
test_create_model_adapter_nested_path and the corresponding
update-model-deployment test with normal top-level imports for
CreateModelAdapterRequest and UpdateModelDeploymentConfigRequest, then
instantiate those imported classes directly at the call sites.
maxdubrinsky
marked this pull request as draft
July 30, 2026 17:50
Contributor
|
maxdubrinsky
marked this pull request as ready for review
July 30, 2026 18:30
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.
What
Introduce the typed
NemoClientModels service client that the AIRCORE-876 consumer migration will build on:models/types.py)PreparedRequestendpoint builders (models/endpoints.py)ModelsClientsurface (models/client.py)Why this is safe to merge now
Purely additive. No existing code imports this module yet, so it changes no runtime behavior and carries zero risk to current consumers. It is split out of the large AIRCORE-876 migration so reviewers can scrutinize the typed contract in isolation, before the mechanical consumer repoint lands.
The breaking, coupled steps land separately: the consumer repoint, the
packages/modelsresources rewrite, and the vendored SDK sync (make vendor, which removessdk.models.retrieve/create/list/adapters.*).Also carries the
method()descriptor fix this client requires: class-level attribute access now resolves without invoking the wrapped callable, soMock(spec=ModelsClient)and other introspection tools no longer break. Plus aresponse.pydocstring note on distinguishing 202/204 deletes.Scope
models/{client,endpoints,types}.pyclient/method.py(descriptor fix),client/response.py(docstring)tests/models/{test_client,test_endpoints}.py,tests/client/test_method.pyDisjoint from the retry-fix PR; the two can merge in any order.
Verification
pytest packages/nemo_platform_plugin/tests-> 1076 passedexclude_unset, conflict-resolver wiring, and response typingruff check/ruff format --checkcleanSummary by CodeRabbit