Fix unreachable error handling code in streaming #2849
Closed
+79
−2
Conversation
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
Fixed unreachable error check in Stream and AsyncStream classes where
`sse.event == "error"` could never be true inside a block that requires
`sse.event.startswith("thread.")`. Changed condition to check for
`sse.event == "thread.error"` which is the correct event name.
Added test cases to verify thread.error events are properly caught and
raise APIError, and that non-error thread events continue to process
correctly.
Fixes openai#2796
Author
|
Ready for review and merge. All tests passing. |
Author
|
Hi maintainers, gentle ping on this PR. It's been open for a couple of days now with all tests passing. Would appreciate a review when you have a moment. Happy to address any feedback. Thank you! |
Collaborator
|
duplicate of #2804 |
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
StreamandAsyncStreamclassessse.event == "error"could never be true inside a block that requiressse.event.startswith("thread.")sse.event == "thread.error"which is the correct event name for thread error eventsChanges
Stream.__stream__()(line 67)AsyncStream.__stream__()(line 170)thread.errorevents to verify they raiseAPIErrorTest Results
All tests pass:
$ pytest tests/test_streaming.py -v ======================== 24 passed in 0.04s ========================Specifically:
test_thread_error_event[sync]- Verifies thread.error raises APIErrortest_thread_error_event[async]- Verifies thread.error raises APIError (async)test_thread_non_error_event[sync]- Verifies other thread.* events worktest_thread_non_error_event[async]- Verifies other thread.* events work (async)Fixes #2796