Skip to content

fix: preserve HTTP errors across process boundaries - #2726

Merged
ananthsub merged 1 commit into
NVIDIA-NeMo:mainfrom
ananthsub:ansubramania/picklable-client-response-error
Aug 26, 2026
Merged

fix: preserve HTTP errors across process boundaries#2726
ananthsub merged 1 commit into
NVIDIA-NeMo:mainfrom
ananthsub:ansubramania/picklable-client-response-error

Conversation

@ananthsub

@ananthsub ananthsub commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace unpicklable aiohttp.ClientResponseError proxy metadata with pickle-safe values before propagation
  • preserve HTTP status, message, URL, case-insensitive multi-value request and response headers, and response body
  • verify the exception and header semantics survive a spawned-process round trip

This extracts and hardens the exception-handling fix proposed by @yuchenwang3 in #1788. The original PR also changes rollout retries; this PR intentionally contains only the serialization fix.

Co-authored-by: Yuchen Wang yuchenwang3@users.noreply.github.com

Test plan

  • uv run --extra dev pre-commit run --files nemo_gym/server_utils.py tests/unit_tests/test_server_utils.py
  • uv run --extra dev pytest tests/unit_tests/test_server_utils.py -q

@copy-pr-bot

copy-pr-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ananthsub

Copy link
Copy Markdown
Contributor Author

/ok to test 48ea269

@ananthsub
ananthsub force-pushed the ansubramania/picklable-client-response-error branch 2 times, most recently from 4f861a2 to 15a9d6a Compare August 25, 2026 19:17
@ananthsub

Copy link
Copy Markdown
Contributor Author

/ok to test 15a9d6a

@yuchenwang3

Copy link
Copy Markdown
Contributor

The current head is green across unit tests, all server shards, the container build, and the Qwen GPU E2E. The only red check is fireworks-e2e: it collected 1/1 rollout with reward 1.0, but the external model returned no function call and the verifier saw []. That matches the ambiguous-prompt flake already identified in #2753, rather than this exception-serialization path.

@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Aug 25, 2026
init-nikhil
init-nikhil previously approved these changes Aug 26, 2026

@init-nikhil init-nikhil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other than the header information loss issue, looks good 👍

Comment thread nemo_gym/server_utils.py Outdated
@ananthsub
ananthsub force-pushed the ansubramania/picklable-client-response-error branch from 15a9d6a to 8884c85 Compare August 26, 2026 10:45
@ananthsub

Copy link
Copy Markdown
Contributor Author

/ok to test 8884c85

@ananthsub

Copy link
Copy Markdown
Contributor Author

/ok to test 179d8e0

Replace aiohttp's unpicklable response metadata with pickle-safe values while preserving the status, message, URL, case-insensitive multi-value headers, and response body. Based on the exception-handling fix proposed by Yuchen Wang in NVIDIA-NeMo#1788.

Co-authored-by: Yuchen Wang <yuchenwang3@users.noreply.github.com>
Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
@ananthsub
ananthsub force-pushed the ansubramania/picklable-client-response-error branch from 179d8e0 to 1e67fc4 Compare August 26, 2026 17:48
@ananthsub

Copy link
Copy Markdown
Contributor Author

/claude review

@ananthsub

Copy link
Copy Markdown
Contributor Author

/ok to test 1e67fc4

@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

SHIP — targeted fix, no reliability concerns.

raise_for_status was attaching an aiohttp ClientResponseError that couldn't cross a process boundary: request_info.headers / .headers are CIMultiDictProxy and history holds ClientResponse objects, all unpicklable. When such an error propagated out of a Ray remote task (or any ProcessPoolExecutor worker), the pickle would fail and the original error was masked by an opaque PicklingError — swallowing the real 5xx/verifier failure detail.

The fix replaces the unpicklable members with pickle-safe equivalents (_PickleSafeRequestInfo NamedTuple, CIMultiDict, empty history) and, critically, rewrites e.args so BaseException.__reduce__ reconstructs the exception from the safe values rather than the originals. Verified:

  • _PickleSafeRequestInfo mirrors RequestInfo's field names (url/method/headers/real_url), so ClientResponseError.__str__ (formats real_url) still works post-restore — asserted in the test.
  • All downstream consumers I could find read only preserved fields: response_content (base_responses_api_model.py:829, server_utils.py:600, mcp_auto_exposure.py:349), status (browsecomp_agent/app.py:199, stirrup_agent). None read .history or depend on request_info.url being a yarl.URL.
  • New test exercises a real spawn ProcessPoolExecutor round-trip and checks multi-value header preservation (getall on repeated Retry-After/Set-Cookie) — asserts real behavior, not a pass-through.

One minor note (author's call, non-blocking): the raise_for_status body carries # pragma: no cover, so the mutation lines aren't counted toward coverage even though the new test drives them via the mocked response. That's consistent with the existing pragma on the function and fine as-is.

@ananthsub
ananthsub merged commit 32c52f0 into NVIDIA-NeMo:main Aug 26, 2026
33 checks passed
@ananthsub
ananthsub deleted the ansubramania/picklable-client-response-error branch August 26, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sla:review-overdue Review response is over the one-business-day SLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants