feat(client): Add nemoclient error hierarchy and fix streaming - #539
Merged
matthewgrossman merged 4 commits intoJul 1, 2026
Conversation
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds shared HTTP error handling, default headers, query serialization, and streaming response wrappers. Client and SDK tests now assert raised errors and the new sync/async streaming APIs. ChangesClient error handling, request wiring, and streaming refactor
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🧹 Nitpick comments (3)
plugins/example-plugin/tests/test_sdk.py (2)
247-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing async
download_blobstream test.Sync stream is tested (
test_sync_download_blob_stream), async isn't. Add the async counterpart for parity.✅ Suggested test
`@pytest.mark.asyncio` async def test_async_download_blob_stream() -> None: client, mock_http = _async_client() raw = httpx.Response(200, content=b"chunk1chunk2", request=httpx.Request("GET", BASE)) mock_http.stream = _async_stream_ctx(raw) resp = await client.download_blob(name="pic.png") async with resp.stream() as chunks: result = b"".join([c async for c in chunks]) assert result == b"chunk1chunk2"🤖 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/example-plugin/tests/test_sdk.py` around lines 247 - 301, Add the missing async parity test for blob streaming in the existing test module by mirroring test_sync_download_blob_stream with a new test_async_download_blob_stream. Use _async_client, set mock_http.stream with _async_stream_ctx, call client.download_blob(name="pic.png"), and verify the streamed chunks join to the expected bytes inside an async with resp.stream() block.
247-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo error-path test for streaming/binary responses.
read()/stream()callraise_for_statusinternally (response.py), but no test here exercises a non-2xx status fordownload_bloborcount. Worth adding to lock in the new error hierarchy on these paths.🤖 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/example-plugin/tests/test_sdk.py` around lines 247 - 336, The streaming/binary response tests for download_blob and count only cover success cases, so add error-path coverage to lock in the new raise_for_status behavior from response.py. Extend test_sync_download_blob_read, test_sync_download_blob_stream, and the count stream tests (or add focused tests beside them) to simulate a non-2xx httpx.Response for client.download_blob, resp.read(), resp.stream(), and client.count, then assert the expected error hierarchy is raised for both sync and async paths.packages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.py (1)
182-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden
query_paramsto match runtime values._resolve_query_params()serializes nesteddict/listvalues, butpackages/nemo_platform_plugin/src/nemo_platform_plugin/client/types.pyandpackages/nemo_platform_plugin/src/nemo_platform_plugin/client/endpoint.pystill typequery_paramsas scalar-only. Update the shared alias to include the serialized shapes.🤖 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 182 - 194, The shared query_params typing is too narrow for what _resolve_query_params() actually accepts and serializes. Update the QueryParams alias in types.py and the corresponding Endpoint.query_params annotation in endpoint.py to allow nested dict/list values as well as the existing scalar types, so the runtime serialization behavior in client.py is reflected consistently across the client API.
🤖 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 201-205: The ConflictError handling in client.send still falls
through into model validation when request.client_options includes exist_ok, so
suppressed 409 responses are not being skipped. Update the send() flow around
raise_for_status(raw) in client.py to return immediately for the suppressed
conflict case, or otherwise bypass model_validate(raw.json()) whenever a 409 is
intentionally accepted. Make sure both send() paths use the same early-exit
behavior tied to ConflictError and exist_ok.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.py`:
- Around line 112-116: The streaming parser in the `_iter()` helpers of
`response.py` treats every non-empty line as raw JSON, but SSE frames like
`data: {...}` will fail in `model_validate_json()`. Update the response
streaming logic to either parse SSE framing by stripping and decoding `data:`
payloads before validation, or narrow the `Response`/streaming classes so they
are explicitly NDJSON-only and do not claim SSE support. Apply the same fix to
both affected `_iter()` implementations.
- Around line 74-76: In the streaming response path of response.py,
`raise_for_status()` is called before the body is consumed, so `NemoHTTPError`
cannot read JSON `detail` from the response. Update the streaming methods in the
response handling code to read the body first (`raw.read()` for sync and `await
raw.aread()` for async) and only then call `raise_for_status()`, using the
existing stream context/response helpers to locate the affected logic.
---
Nitpick comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/client.py`:
- Around line 182-194: The shared query_params typing is too narrow for what
_resolve_query_params() actually accepts and serializes. Update the QueryParams
alias in types.py and the corresponding Endpoint.query_params annotation in
endpoint.py to allow nested dict/list values as well as the existing scalar
types, so the runtime serialization behavior in client.py is reflected
consistently across the client API.
In `@plugins/example-plugin/tests/test_sdk.py`:
- Around line 247-301: Add the missing async parity test for blob streaming in
the existing test module by mirroring test_sync_download_blob_stream with a new
test_async_download_blob_stream. Use _async_client, set mock_http.stream with
_async_stream_ctx, call client.download_blob(name="pic.png"), and verify the
streamed chunks join to the expected bytes inside an async with resp.stream()
block.
- Around line 247-336: The streaming/binary response tests for download_blob and
count only cover success cases, so add error-path coverage to lock in the new
raise_for_status behavior from response.py. Extend test_sync_download_blob_read,
test_sync_download_blob_stream, and the count stream tests (or add focused tests
beside them) to simulate a non-2xx httpx.Response for client.download_blob,
resp.read(), resp.stream(), and client.count, then assert the expected error
hierarchy is raised for both sync and async paths.
🪄 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: 638bac52-2f0a-48f9-bfd4-19306172cd86
📒 Files selected for processing (8)
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/endpoint.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/errors.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/response.pypackages/nemo_platform_plugin/tests/client/test_client.pypackages/nemo_platform_plugin/tests/client/test_client_options.pyplugins/example-plugin/tests/test_sdk.py
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
|
mckornfield
approved these changes
Jul 1, 2026
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
matthewgrossman
enabled auto-merge
July 1, 2026 21:24
matthewgrossman
deleted the
mgrossman/GENERIC1-aircore-827-migrate-first-plugin-to-nemoclient-typed-http-client-files-
branch
July 1, 2026 21:53
This was referenced Jul 6, 2026
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
* feat(client): Add nemoclient error hierarchy and fix streaming Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * remove exist_ok Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix sse and code review Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * code review Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> --------- Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
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
Foundational improvements to the
NemoClienttyped HTTP client, extracted from the Files migration PR (#429) to keep that PR focused on Files-specific changes.Error hierarchy (
errors.py— new)NemoHTTPErrorout ofresponse.pyinto its own moduleBadRequestError(400),AuthenticationError(401),PermissionDeniedError(403),NotFoundError(404),ConflictError(409),UnprocessableEntityError(422),RateLimitError(429),InternalServerError(500+)raise_for_status()function that maps status codes to the correct subclassNemoHTTPErrorsubclasses consistently — previously binary/stream paths raisedhttpx.HTTPStatusErrorsend()raises on non-2xx (client.py)send()now callsraise_for_status(raw)before parsing the body, so callers don't need.data()to trigger errors.data()simplified to a plain body accessor_apply_client_options()and_raise_for_status()—exist_okinfrastructure remains inPreparedRequest.client_optionsbut is not acted on until the server returns entities on 409 (AIRCORE-866)Binary/stream response API (
response.py)__enter__/__exit__context manager protocol with simplerread()+stream()APIread()— opens the stream, reads all bytes, closes (one-shot); reads body beforeraise_for_status()so error detail is availablestream()—@contextmanageryieldingIterator[bytes](binary) orIterator[ModelT](stream)_responsestate orhttp_responsepropertyContent-Type: text/event-streamand stripsdata:prefixes before JSON parsing; skips non-data SSE fields (event:,id:, comments). Shared_parse_stream_line()helper used by both sync and async paths.Other improvements
default_headersonBaseNemoClient— merged into every request;client_from_platform()propagates_custom_headersjson.dumps()-ed instead of Pythonrepr()exclude_unset=Trueon body serialization — partial update bodies only send explicitly set fieldsTest plan
packages/nemo_platform_plugin/tests/client/)plugins/example-plugin/tests/test_sdk.py), including new coverage for:read()andstream()stream()🤖 Generated with Claude Code