feat(models): add typed NemoClient models foundation - #993
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds typed Models service DTOs and endpoint contracts, synchronous and asynchronous clients, OpenAI route builders, deployment/provider polling, conflict handling, deployment deletion history, and focused tests. 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
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
marcusds
approved these changes
Aug 12, 2026
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>
…provider id Address review findings on the typed Models client foundation: - method() class-level access returns a per-owning-class callable stub (async def for async clients, def for sync) instead of the raw descriptor, so unittest.mock classifies async endpoints as AsyncMock. Previously Mock(spec=AsyncModelsClient).create_model was a sync MagicMock and could not be awaited, and create_autospec yielded non-callable stubs -- defeating the typed async client's purpose. - delete_deployment appends a DELETING entry to status_history so the client (which reads status_history[-1] as current) no longer sees a stale status after a delete request. - get_provider_route_openai_url_for_deployment guards a model_provider_id that lacks the workspace/ prefix with a clear ValueError instead of an opaque unpack crash. Adds regression tests for all three. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
maxdubrinsky
force-pushed
the
aircore-876-typed-models-client/md
branch
from
August 12, 2026 17:54
773663e to
8103d6f
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Replace two call-time __import__("...types", fromlist=[...]) lookups in
the endpoint tests with normal names added to the existing top-level
import block (CreateModelAdapterRequest, UpdateModelDeploymentConfigRequest).
Addresses a CodeRabbit maintainability nit on PR #993.
Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/nemo_platform_plugin/tests/client/test_method.py (1)
87-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest autospec signature enforcement.
This test checks mock classification only. Add invalid positional-call assertions for both clients. This verifies that
create_autospecpreserves the wrapped keyword-only endpoint signature.Proposed test
auto_sync = create_autospec(ModelsClient) assert callable(auto_sync.create_model) assert not isinstance(auto_sync.create_model, AsyncMock) + with pytest.raises(TypeError): + auto_sync.create_model("workspace", object()) auto_async = create_autospec(AsyncModelsClient) assert callable(auto_async.create_model) assert isinstance(auto_async.create_model, AsyncMock) + with pytest.raises(TypeError): + auto_async.create_model("workspace", object())🤖 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/client/test_method.py` around lines 87 - 99, Extend test_create_autospec_yields_awaitable_endpoint_stubs to invoke create_model on both auto_sync and auto_async with an invalid positional argument and assert each call raises TypeError. Keep the existing callable and AsyncMock classification checks, and ensure the invalid calls exercise the wrapped keyword-only signature enforced by create_autospec.packages/nemo_platform_plugin/src/nemo_platform_plugin/models/types.py (2)
1417-1466: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
total=FalseplusNotRequiredis redundant.Every key already defaults to not-required under
total=False. RemoveNotRequiredor removetotal=Falsefor one consistent style.🤖 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/types.py` around lines 1417 - 1466, The query parameter TypedDicts redundantly combine total=False with NotRequired annotations. Update ListModelsQueryParams, GetModelQueryParams, ListAdaptersQueryParams, ListProvidersQueryParams, ListPromptsQueryParams, ListDeploymentsQueryParams, ListDeploymentConfigsQueryParams, and UpdateDeploymentStatusQueryParams to use one consistent optional-key style by removing either total=False or the NotRequired wrappers throughout.
451-458: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
list[...] | Nonewithdefault_factory=listis contradictory.The field can never be
Noneby default, but the type permitsNone. Choose one: drop| None, or usedefault=None. This affects generated schema nullability for consumers.Also applies to: 960-963
🤖 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/types.py` around lines 451 - 458, Resolve the nullability mismatch in the model fields around served_models and the corresponding field near the later occurrence: either remove | None to make the list non-nullable with default_factory=list, or change the default to None to preserve nullable typing. Apply the same consistent choice to both fields and ensure the generated schema reflects it.packages/nemo_platform_plugin/tests/models/test_client.py (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse concrete collection type hints.
Replace bare
dictandlistannotations with parameterized types such asdict[str, object]andlist[dict[str, object]].As per coding guidelines, “Prefer concrete type hints over string-based annotations.”
Also applies to: 38-38, 53-53
🤖 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` at line 26, Update the type annotations in _model_json and the additionally referenced declarations to use concrete parameterized collection types, replacing bare dict and list annotations with forms such as dict[str, object] and list[dict[str, object]].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_plugin/src/nemo_platform_plugin/models/types.py`:
- Around line 112-120: Update the temperature Field description in the model
definition to match its actual constraints: allow values from 0 through 2,
explicitly permit 0.0, and remove the contradictory “between 0 and 1” and “can't
be set to 0.0” wording.
In
`@services/core/models/src/nmp/core/models/api/service/model_deployment_service.py`:
- Around line 497-501: Add a shared helper for deletion status-history updates
and use it in both DELETING branches near the direct status_history.append
calls. Have the helper compact adjacent entries with the same status and
message, cap history at 100 entries, and perform this normalization before
entity.update(); preserve the DELETING entry as the latest status.
---
Nitpick comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/models/types.py`:
- Around line 1417-1466: The query parameter TypedDicts redundantly combine
total=False with NotRequired annotations. Update ListModelsQueryParams,
GetModelQueryParams, ListAdaptersQueryParams, ListProvidersQueryParams,
ListPromptsQueryParams, ListDeploymentsQueryParams,
ListDeploymentConfigsQueryParams, and UpdateDeploymentStatusQueryParams to use
one consistent optional-key style by removing either total=False or the
NotRequired wrappers throughout.
- Around line 451-458: Resolve the nullability mismatch in the model fields
around served_models and the corresponding field near the later occurrence:
either remove | None to make the list non-nullable with default_factory=list, or
change the default to None to preserve nullable typing. Apply the same
consistent choice to both fields and ensure the generated schema reflects it.
In `@packages/nemo_platform_plugin/tests/client/test_method.py`:
- Around line 87-99: Extend test_create_autospec_yields_awaitable_endpoint_stubs
to invoke create_model on both auto_sync and auto_async with an invalid
positional argument and assert each call raises TypeError. Keep the existing
callable and AsyncMock classification checks, and ensure the invalid calls
exercise the wrapped keyword-only signature enforced by create_autospec.
In `@packages/nemo_platform_plugin/tests/models/test_client.py`:
- Line 26: Update the type annotations in _model_json and the additionally
referenced declarations to use concrete parameterized collection types,
replacing bare dict and list annotations with forms such as dict[str, object]
and list[dict[str, object]].
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33eaa1d4-354c-4be1-a02e-407be522e0ce
📒 Files selected for processing (10)
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.pyservices/core/models/src/nmp/core/models/api/service/model_deployment_service.pyservices/core/models/tests/unit/test_model_deployment_service_unit.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/models/endpoints.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/models/client.py
maxdubrinsky
enabled auto-merge
August 12, 2026 17:58
CI Lint all caught two issues from the earlier commits: - ruff format collapses the malformed-model_provider_id message onto one line in client.py. - ty reports call-non-callable / invalid-argument-type on the new class-level-access tests: EndpointMethod.__get__(obj=None) is typed as the descriptor because the overload cannot distinguish the sync vs async owning class, so ty cannot see the callable stub returned at runtime. The three tests deliberately exercise that runtime stub, so they carry targeted ty: ignore suppressions with an explanatory note. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 12, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 12, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 12, 2026
This was referenced Aug 13, 2026
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
2 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.
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
DELETINGstatus-history entry.