Improve streaming error handling - #2875
mrutunjay-kinagi wants to merge 3 commits into
Conversation
|
@marcuswood-oai Is this still relevant ? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df0a203dbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Remove legacy duplicate error-checking code that was causing each non-thread SSE event to be yielded twice. The refactored _raise_streaming_error() helper now handles all error checking in a single pass, and we preserve the synthesize_event_and_data wrapping behavior for the else branch. Fixes issue where consumers received duplicate chunks, which could duplicate streamed text, tool-call deltas, and state-machine events. Applies the same fix to both sync (Stream) and async (AsyncStream) implementations. Addresses Codex review feedback on PR openai#2875.
|
Yes—thanks for checking back, and for fixing the duplicate-yield issue from the review! I checked current main and tested 3ac0c06. The I'd focus this PR on the alias (#2853) and removing the unreachable checks (#2796). Main already raises Please add focused regression tests for The other items are worth tracking separately: the missing ChatModel literal and generated compaction examples need fixes in their source schema/configuration, and a smaller Wikimedia image URL isn't yet a verified fix for the download error. Please leave those changes and their closing references out of this focused streaming patch. Thanks again for working on this—there is still a useful contribution here. |
Thank you so much for such quick response. Can you please assign me the relevant issues ? I'll put sometime in getting those resolved. |
|
Thanks for the contribution, and sorry for the delay in getting to this! We’re going with #2854 to address this, so I’m closing this PR. Posted with Codex. |
@marcuswood-oai Okay, are there any other issues i can contribute to ? |
Summary
response.errorevents and exposeAsyncStream.aclose()so clients can always close the underlying response (AsyncStream missing aclose() causes AttributeError in streaming structured output #2853, Bug: Dead code -sse.event == "error"check is unreachable in _streaming.py #2796)context_managementexample to usetype="compaction"so the tests and API expectations align (context_management=[{"type": "type"}] in tests, not 'compaction' #2868)gpt-audio-minito the public chat model literal list and mention a smaller Vision example URL with guidance about invalid URL errors (gpt-audio-mini is missing from the ChatModel list — should it be included? #2761, Vision example throws an error #2776)Testing
Closes #2853, #2796, #2868, #2761, #2776