fix(client): preserve SDK retry behavior in NemoClient adapters - #992
Open
maxdubrinsky wants to merge 1 commit into
Open
fix(client): preserve SDK retry behavior in NemoClient adapters#992maxdubrinsky wants to merge 1 commit into
maxdubrinsky wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThe retry policy now supports configurable server-error retries, retry-decision headers, and ChangesRetry policy behavior
Sequence Diagram(s)sequenceDiagram
participant Client
participant _should_retry
participant _retry_after
participant Sleep
Client->>_should_retry: evaluate HTTP response and RetryPolicy
_should_retry->>_retry_after: parse retry-after headers when enabled
_retry_after-->>_should_retry: retry delay or None
_should_retry->>Sleep: wait using header delay or exponential backoff
Sleep-->>Client: retry request
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 |
maxdubrinsky
marked this pull request as draft
July 30, 2026 17:50
Contributor
|
Ahead of the AIRCORE-876 Models typed-client migration, split out as an independent client-layer change touching nothing else. The generic adapter now preserves the SDK's retry policy: Stainless retry-decision and Retry-After headers are honored, with a fallback for unreasonable Retry-After values, while the standalone policy keeps its narrower retryable status set. No consumer behavior changes; covered by expanded adapter and client-option tests. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
maxdubrinsky
force-pushed
the
aircore-876-nemoclient-retry-fix/md
branch
from
July 30, 2026 18:05
6f82df7 to
8119bb4
Compare
maxdubrinsky
marked this pull request as ready for review
July 30, 2026 18:11
![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: 1
🧹 Nitpick comments (2)
packages/nemo_platform_plugin/tests/client/test_client_options.py (1)
743-747: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAsync path never asserts the computed delay.
Sync tests verify
retry-afterparsing end-to-end; async only mocksasyncio.sleep. Add one async case assertingsleep.assert_awaited_once_with(...)so an async-specific regression in delay handling is caught.🤖 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_client_options.py` around lines 743 - 747, Extend the async retry test around client.send(GET_ITEM(name="alice")) to retain the patched asyncio.sleep AsyncMock and assert it was awaited exactly once with the computed retry delay. Preserve the existing response and request-count assertions, and use the async mock’s await-specific assertion.packages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.py (1)
159-182: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBackoff is uncapped and jitter-free.
backoff_base * 2**attemptgrows without bound;adapter.pypassesplatform.max_retries, so a large value yields multi-minute blocking sleeps on the calling thread and synchronized retry storms across clients. The Stainless SDK caps delay (~8s) and adds jitter. Consider amax_backofffield plus jitter.🤖 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/client.py` around lines 159 - 182, Update the retry delay calculation in the retry backoff logic to cap exponential growth with a configurable max_backoff value and add randomized jitter before returning the delay. Apply the bounded, jittered delay consistently for exception, retryable-status, and retry-after paths while preserving retry decisions and honoring server-provided retry timing.
🤖 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 134-141: Update _retry_after to catch OverflowError and OSError
alongside TypeError and ValueError when parsing retry_after and converting the
resulting datetime to a timestamp. Return None for these out-of-range HTTP-date
cases so the retry loop continues without propagating the exception.
---
Nitpick comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.py`:
- Around line 159-182: Update the retry delay calculation in the retry backoff
logic to cap exponential growth with a configurable max_backoff value and add
randomized jitter before returning the delay. Apply the bounded, jittered delay
consistently for exception, retryable-status, and retry-after paths while
preserving retry decisions and honoring server-provided retry timing.
In `@packages/nemo_platform_plugin/tests/client/test_client_options.py`:
- Around line 743-747: Extend the async retry test around
client.send(GET_ITEM(name="alice")) to retain the patched asyncio.sleep
AsyncMock and assert it was awaited exactly once with the computed retry delay.
Preserve the existing response and request-count assertions, and use the async
mock’s await-specific assertion.
🪄 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: 8c56af85-3e5a-4082-bc85-c30cecf04165
📒 Files selected for processing (5)
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/adapter.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/types.pypackages/nemo_platform_plugin/tests/client/test_adapter.pypackages/nemo_platform_plugin/tests/client/test_client_options.py
Comment on lines
+134
to
+141
| try: | ||
| retry_date = email.utils.parsedate_to_datetime(retry_after) | ||
| except (TypeError, ValueError): | ||
| return None | ||
| if retry_date.tzinfo is None: | ||
| retry_date = retry_date.replace(tzinfo=timezone.utc) | ||
| delay = retry_date.timestamp() - time.time() | ||
| return delay if 0 < delay <= 60 else None |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against OverflowError/OSError from far-future HTTP-dates.
parsedate_to_datetime and datetime.timestamp() can raise OverflowError (or OSError on some platforms) for out-of-range dates, escaping _retry_after and aborting the retry loop with an unrelated exception.
🛡️ Proposed fix
try:
retry_date = email.utils.parsedate_to_datetime(retry_after)
- except (TypeError, ValueError):
+ except (TypeError, ValueError, OverflowError):
return None
if retry_date.tzinfo is None:
retry_date = retry_date.replace(tzinfo=timezone.utc)
- delay = retry_date.timestamp() - time.time()
+ try:
+ delay = retry_date.timestamp() - time.time()
+ except (OverflowError, OSError, ValueError):
+ return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| retry_date = email.utils.parsedate_to_datetime(retry_after) | |
| except (TypeError, ValueError): | |
| return None | |
| if retry_date.tzinfo is None: | |
| retry_date = retry_date.replace(tzinfo=timezone.utc) | |
| delay = retry_date.timestamp() - time.time() | |
| return delay if 0 < delay <= 60 else None | |
| try: | |
| retry_date = email.utils.parsedate_to_datetime(retry_after) | |
| except (TypeError, ValueError, OverflowError): | |
| return None | |
| if retry_date.tzinfo is None: | |
| retry_date = retry_date.replace(tzinfo=timezone.utc) | |
| try: | |
| delay = retry_date.timestamp() - time.time() | |
| except (OverflowError, OSError, ValueError): | |
| return None | |
| return delay if 0 < delay <= 60 else 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/nemo_platform_plugin/src/nemo_platform_plugin/client/client.py`
around lines 134 - 141, Update _retry_after to catch OverflowError and OSError
alongside TypeError and ValueError when parsing retry_after and converting the
resulting datetime to a timestamp. Return None for these out-of-range HTTP-date
cases so the retry loop continues without propagating the exception.
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
Preserve the SDK's retry behavior in the generic
NemoClientadapter layer. Split out of the AIRCORE-876 Models typed-client migration as an independent, low-risk client-layer change.The generic adapter now preserves the SDK retry policy:
Retry-Afterheaders are honored, with a fallback when the value is unreasonable.502, 503, 504, 429).Why split
The AIRCORE-876 branch (
aircore-876-migrate-models-to-nemoclient/md) is a large, vendoring-gated migration. This retry hardening is conceptually independent of the models work and benefits everyNemoClientconsumer, so it is carved off to shrink the review surface of the big PR. It can merge in any order relative to the typed-models-client PR (disjoint files).Scope
Client layer only, no consumer behavior changes:
client/adapter.py,client/client.py,client/types.pytests/client/test_adapter.py,tests/client/test_client_options.pyVerification
pytest packages/nemo_platform_plugin/tests-> 1042 passedruff check/ruff format --checkcleanSummary by CodeRabbit
New Features
Retry-Afterheaders (including multiple delay formats with safe limits).Bug Fixes
Retry-Afterparsing and application.Tests