feat(client): add pagination, retry, and client-side options to NemoClient#454
Merged
matthewgrossman merged 13 commits intoJun 25, 2026
Conversation
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
…upport-to-nemoclient-replace Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
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:
📝 WalkthroughWalkthroughTyped client support now includes paginated responses, retry policies, and client-side options like ChangesClient pagination, retries, and client options
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 5
🧹 Nitpick comments (3)
packages/nemo_platform_plugin/tests/client/test_pagination.py (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove postponed annotations here.
This new test file does not need stringified annotations. As per coding guidelines, “Always prefer concrete type hints over string-based ones in Python code.”
🤖 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_pagination.py` at line 6, The new test module is unnecessarily using postponed annotations, so remove the __future__ import in the test file and keep the type hints concrete. Update the test module around the top-level imports in test_pagination.py so it follows the guideline of preferring normal type annotations over stringified ones.Source: Coding guidelines
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.py (1)
284-284: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHardcoded
current_page=1breaks the documented cursor (str) pagination.Lines 224-225 advertise
strpage values for cursor-based strategies, but the firstnext_page(body, 1)call passes anint. Any non-offset strategy gets a wrong initial page. Derive the starting page from the parsed body (or the strategy) instead of literal1.Also applies to: 339-339
🤖 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/client/response.py` at line 284, The pagination flow in `Response.next_page` is hardcoding `current_page` as `1`, which breaks cursor-based strategies that expect the documented `str` page value. Update the `next_page` call sites in `Response` to derive the initial/current page from the parsed response body or the strategy itself instead of passing a literal `1`, so non-offset strategies receive the correct type and value.packages/nemo_platform_plugin/src/nemo_platform_plugin/client/types.py (1)
145-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
BLESSED_CLIENT_PARAMStype values are dead and the comment misleads.The comment says each entry maps to its "expected Python type," but
endpoint._identify_client_option_paramsonly reads.keys(). A mis-annotatedexist_ok: strpasses validation silently. Either validate against the type or drop it to afrozenset.🤖 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/client/types.py` around lines 145 - 150, BLESSED_CLIENT_PARAMS is treating the values as if they matter, but endpoint._identify_client_option_params only uses the keys, so the current type mapping is misleading and can silently allow bad annotations. Update the client-option validation to either actually check the annotated type against the expected Python type for each entry, or replace BLESSED_CLIENT_PARAMS with a key-only structure like a frozenset if only membership is needed. Keep the fix centered around BLESSED_CLIENT_PARAMS and endpoint._identify_client_option_params so the declared contract matches the behavior.
🤖 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/client/client.py`:
- Around line 318-332: The retry loop in client handling is replaying every
request, which can duplicate non-idempotent writes or resend exhausted bodies
after failures. Update the retry logic in the request-sending path around the
_send_once call to only retry replay-safe operations, using method/idempotency
checks or an explicit opt-in/idempotency key before entering the retry loop.
Apply the same guard to the related retry block in the other affected section so
only safe requests can back off and retry.
- Around line 283-287: The paginated request path in NemoClient currently
bypasses the retry logic, so first-page and later-page 503s are not handled by
the configured policy. Update the paginated flow in NemoClient’s request
handling and page fetcher creation so that both the initial page and subsequent
page requests go through the same retry-aware path used by _send_with_retry, and
ensure NemoPaginatedResponse receives a fetcher that preserves retry behavior
for each page request. Apply the same fix anywhere the affected paginated
request helpers are used so all pagination paths share consistent retry
handling.
- Around line 205-211: The paginated send overload is too narrow because
`PreparedRequest[Paginated[ModelT]]` only covers the default pagination
strategy, causing custom paginated requests to miss the `NemoPaginatedResponse`
return type. Update the `send` overloads in the client API so they accept
`PreparedRequest[Paginated[ModelT, Any]]` or add a second strategy type
parameter for both the sync and async variants, keeping the overloads aligned
with `NemoPaginatedResponse` and the generic `Paginated` model.
In `@packages/nemo_platform_plugin/tests/client/test_pagination.py`:
- Around line 149-150: The pagination test assertion is too permissive because
the fallback allows non-dict request arguments to pass without checking page 2;
tighten the check in the test around mock_http.request.call_args_list so it
explicitly verifies the second call includes params and that the page value is
2, using second_call_params as the key reference and failing the assertion when
params are missing or not a dict.
In `@plugins/example-plugin/src/nemo_example_plugin/types/endpoints.py`:
- Around line 39-41: The `list_items` endpoint signature in `ExamplePlugin`
dropped the `query_params` support even though the route still accepts `filter`,
`page_size`, and `sort`. Update the abstract `list_items` method to keep a
`query_params` argument (alongside `workspace`) and preserve the ability to pass
these server-side filters through the SDK, matching the existing endpoint
contract in the endpoints types.
---
Nitpick comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.py`:
- Line 284: The pagination flow in `Response.next_page` is hardcoding
`current_page` as `1`, which breaks cursor-based strategies that expect the
documented `str` page value. Update the `next_page` call sites in `Response` to
derive the initial/current page from the parsed response body or the strategy
itself instead of passing a literal `1`, so non-offset strategies receive the
correct type and value.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/types.py`:
- Around line 145-150: BLESSED_CLIENT_PARAMS is treating the values as if they
matter, but endpoint._identify_client_option_params only uses the keys, so the
current type mapping is misleading and can silently allow bad annotations.
Update the client-option validation to either actually check the annotated type
against the expected Python type for each entry, or replace
BLESSED_CLIENT_PARAMS with a key-only structure like a frozenset if only
membership is needed. Keep the fix centered around BLESSED_CLIENT_PARAMS and
endpoint._identify_client_option_params so the declared contract matches the
behavior.
In `@packages/nemo_platform_plugin/tests/client/test_pagination.py`:
- Line 6: The new test module is unnecessarily using postponed annotations, so
remove the __future__ import in the test file and keep the type hints concrete.
Update the test module around the top-level imports in test_pagination.py so it
follows the guideline of preferring normal type annotations over stringified
ones.
🪄 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: 6477ef84-7cc9-4aa5-8b32-48180ad7ad67
📒 Files selected for processing (7)
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/endpoint.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/types.pypackages/nemo_platform_plugin/tests/client/test_client_options.pypackages/nemo_platform_plugin/tests/client/test_pagination.pyplugins/example-plugin/src/nemo_example_plugin/types/endpoints.py
Contributor
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/client/client.py`:
- Around line 287-288: The paginated response path in NemoClient is dropping the
caller’s resolved retry policy, so later page fetches and the first page’s
retryable-status handling do not consistently use the same behavior. Update the
send/_send_once flow so the effective retry policy is passed through to
_send_once and used when constructing the lazy page fetcher via
_make_page_fetcher, and extend the retryable-status check in the send helpers to
treat NemoPaginatedResponse and AsyncNemoPaginatedResponse the same as
NemoResponse; apply the same fix in all matching sync and async send variants.
🪄 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: 638bb59f-97e4-4094-849a-af121bcaa18b
📒 Files selected for processing (1)
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.py
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.py (2)
285-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParameterize the
dictreturn type.
tuple[list[ModelT], dict]→tuple[list[ModelT], dict[str, Any]]. Applies to the async twin at line 343 too.As per coding guidelines: "Always prefer concrete type hints over string-based ones in Python code".
🤖 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/client/response.py` at line 285, The return type for _parse_page is using an unparameterized dict, so update it to a concrete mapping type and mirror the same change in the async twin method later in the class. Adjust the annotation on _parse_page (and the corresponding async parser) from tuple[list[ModelT], dict] to tuple[list[ModelT], dict[str, Any]] so the response shape is fully typed and consistent with the coding guidelines.Source: Coding guidelines
343-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicated
_parse_pageanddataacross sync/async wrappers.These two methods are byte-identical to
NemoPaginatedResponse(lines 285-302). Extract into a shared base mixin to avoid drift.♻️ Sketch
class _BasePaginatedResponse(Generic[ModelT]): _first_response: httpx.Response _model_type: type[ModelT] _strategy: type[PaginationStrategy] `@property` def http_response(self) -> httpx.Response: return self._first_response def _parse_page(self, raw: httpx.Response) -> tuple[list[ModelT], dict[str, Any]]: raw.raise_for_status() body = raw.json() items = [self._model_type.model_validate(i) for i in self._strategy.extract_items(body)] return items, body def data(self) -> PageResult[ModelT]: items, body = self._parse_page(self._first_response) pagination = body.get("pagination") or {} return PageResult( items=items, page=pagination.get("page"), page_size=pagination.get("page_size"), total_pages=pagination.get("total_pages"), total_results=pagination.get("total_results"), )🤖 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/client/response.py` around lines 343 - 360, The _parse_page and data methods on this paginated response wrapper are duplicated with NemoPaginatedResponse, so move the shared parsing/metadata logic into a common base mixin or base class and have both sync/async wrappers inherit it. Keep the shared behavior in a single implementation that uses _first_response, _model_type, and _strategy, and let both response classes reuse the same http_response property and PageResult construction to prevent drift.
🤖 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/nemo_platform_plugin/src/nemo_platform_plugin/client/response.py`:
- Line 285: The return type for _parse_page is using an unparameterized dict, so
update it to a concrete mapping type and mirror the same change in the async
twin method later in the class. Adjust the annotation on _parse_page (and the
corresponding async parser) from tuple[list[ModelT], dict] to
tuple[list[ModelT], dict[str, Any]] so the response shape is fully typed and
consistent with the coding guidelines.
- Around line 343-360: The _parse_page and data methods on this paginated
response wrapper are duplicated with NemoPaginatedResponse, so move the shared
parsing/metadata logic into a common base mixin or base class and have both
sync/async wrappers inherit it. Keep the shared behavior in a single
implementation that uses _first_response, _model_type, and _strategy, and let
both response classes reuse the same http_response property and PageResult
construction to prevent drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e142f4ff-e231-4393-b79e-4534fa35af07
📒 Files selected for processing (4)
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/endpoint.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.pypackages/nemo_platform_plugin/tests/client/test_pagination.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nemo_platform_plugin/src/nemo_platform_plugin/client/endpoint.py
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
…upport-to-nemoclient-replace Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
list_items now returns Paginated[ExampleItem] instead of ExampleItemPage. Update tests to use PageResult.items instead of .data, and remove the query_params test since pagination is now handled internally by the client. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
EndpointMethod.__get__ always returned Callable[P, NemoResponse[ResponseT]], which meant Paginated endpoints returned NemoResponse[Paginated[...]] to the type checker instead of NemoPaginatedResponse[T]. This caused ty to reject .data().items on paginated responses. Add PaginatedEndpointMethod descriptor that returns NemoPaginatedResponse[T], and overload method() to select it automatically for Paginated endpoints. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
matthewgrossman
changed the title
maxdubrinsky
approved these changes
Jun 25, 2026
maxdubrinsky
left a comment
maxdubrinsky
left a comment
Contributor
There was a problem hiding this comment.
Looks good, left a note for future us
matthewgrossman
deleted the
mgrossman/aircore-828-add-first-class-auth-support-to-nemoclient-replace
branch
This was referenced Jul 1, 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.
Summary
Extends the NemoClient typed HTTP client with three features: automatic pagination, configurable retry, and client-side endpoint options.
Linked issue: AIRCORE-843
What changed
Paginated[T]— automatic pagination via return type markerEndpoints declare
-> Paginated[Item]and the client automatically iterates all pages, mirroring the existingStream[T]pattern:OffsetPaginationstrategy matches the standardNemoListResponseenvelope-> Paginated[Item, MyPagination]PaginationStrategyprotocol for extensibilityPageResult[T]dataclass with items + metadata for single-page accessRetryPolicy— configurable retry with exponential backoff_should_retrydecision logic across sync/async pathsBlessed client-side options (
exist_ok)Endpoint signatures can include blessed parameter names that control client behavior without being sent over the wire:
BLESSED_CLIENT_PARAMS/RESERVED_PARAM_NAMESregistries intypes.pyTypeErrorat import timesend()so bothclient.send()andclient.method()paths workEndpointMethodtyping for all response typesmethod()overloads now return correctly-typed descriptors for every response-type marker:method()descriptor returnsBinaryContentNemoBinaryResponse/AsyncNemoBinaryResponseStream[T]NemoStreamResponse[T]/AsyncNemoStreamResponse[T]Paginated[T]NemoPaginatedResponse[T]/AsyncNemoPaginatedResponse[T]NoneNemoResponse[None]BaseModelNemoResponse[T]Architecture
Test plan
exist_okstripping from request, 409 swallowing (sync + async, via send + method)PageResult.data()with metadata (sync + async)method()descriptor returns correct types for paginated endpoints🤖 Generated with Claude Code