-
Notifications
You must be signed in to change notification settings - Fork 367
refactor: process all streaming events for forward compatibility #993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
96899e2 to
8f31d23
Compare
|
@anthropics/sdk can we prioritize? - need this performance improvement - |
|
@RobertCraigie what's the next step to merge this? |
|
It's been already more than two months this PR was opened. The intention of the PR is to increase the speed of events in your SDK. Difficult to understand why it is taking so long for a clear improvement in the code quality of your SDK. cc: @RobertCraigie @anthropics/sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delayed answer, I'll try to review it. Thanks so much for your time and efforts, I'll try to get this merged quickly
src/anthropic/_streaming.py
Outdated
| # Explicitly closes decoder resources if available | ||
| # Immediately releases connection instead of consuming remaining stream | ||
| if hasattr(self._decoder, "close"): | ||
| self._decoder.close() # Properly closes decoder resources without unnecessary iteration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this close the underlying connection? Running a simple streaming script, I see that at that point the decoder is SSEDecoder, which doesn't have a close method. Though if we iterate through it, we will consume the underlying httpx stream and the connection will be closed. Maybe we should call self.response.close() here?
src/anthropic/_streaming.py
Outdated
| or sse.event == "content_block_delta" | ||
| or sse.event == "content_block_stop" | ||
| ): | ||
| # Single, fast membership test instead of multiple string comparisons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, it really speeds things up. Plus, even though in this case we have O(6) = O(1) complexity for a single lookup, our codegen might have N such events, so it's worth it.
src/anthropic/_streaming.py
Outdated
| or sse.event == "content_block_delta" | ||
| or sse.event == "content_block_stop" | ||
| ): | ||
| # same O(1) lookup for consistency between sync/async versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's have the same comment for sync/async case
8f31d23 to
cf51769
Compare
- Replace decoder.close() with response.close() in sync stream - Replace decoder.close() with response.aclose() in async stream - Standardize comments between sync and async implementations Addresses feedback from PR anthropics#993: - SSEDecoder doesn't have a close method, properly close httpx response instead - Ensure consistent comments between sync/async versions
Review Feedback AddressedThank you @karpetrosyan for the review! I've addressed all your feedback: ✅ Fixed: Connection Cleanup (Line 126 & 236)Your feedback:
Changes made:
This ensures proper connection cleanup via the httpx response object instead of attempting to close the SSEDecoder which doesn't have a close method. ✅ Fixed: Comment Consistency (Line 209)Your feedback:
Changes made:
✅ Confirmed: O(1) Performance Improvement (Line 96)Your feedback:
Thank you! The optimization delivers 1.64-1.69x speedup as validated by our performance tests. Test ResultsAll tests pass with live API validation: Commits
Ready for re-review! 🚀 |
|
Reviewing this again, I think we’d better just remove that event name check and process any event we receive instead. The loop could look like this. for sse in iterator:
if sse.event == "completion":
yield process_data(data=sse.json(), cast_to=cast_to, response=response)
if sse.event == "ping":
continue
if sse.event == "error":
body = sse.data
try:
body = sse.json()
err_msg = f"{body}"
except Exception:
err_msg = sse.data or f"Error code: {response.status_code}"
raise self._client._make_status_error(
err_msg,
body=body,
response=self.response,
)
data = sse.json()
if is_dict(data) and "type" not in data:
data["type"] = sse.event
yield process_data(data=data, cast_to=cast_to, response=response)So... I guess our next steps are:
Processing any events would align better with our SDK design in general, where we try to be forward compatible wherever we can. |
Changes based on review feedback from @karpetrosyan: 1. Removed MESSAGE_EVENTS filtering - now processes any event received 2. Removed early response closing (response.close/aclose) - will be ported to codegen 3. Removed performance test file - no longer applicable This change improves forward compatibility by processing any event type the API sends, rather than filtering to a predefined set. The new structure handles completion, ping, and error events explicitly, while processing all other events generically. Closes review feedback in anthropics#993
|
@karpetrosyan Thank you for the review and guidance. I've implemented all the changes you requested: ✅ Changes Implemented
🧪 Testing PerformedI ran comprehensive integration tests to verify the changes work correctly:
📊 Value PropositionWhat this PR provides: Forward compatibility for future streaming events. If Anthropic introduces new event types (e.g., Before: Unknown events would be filtered out by MESSAGE_EVENTS check Trade-off context: The original PR included measurable performance optimizations (1.69x speedup via O(1) frozenset lookups) alongside this forward compatibility improvement. Per your feedback, I've focused solely on the forward compatibility aspect, which provides architectural value but is speculative (depends on future API evolution). I understand the priority for forward compatibility aligns with the SDK's design philosophy. The PR is ready for re-review as implemented. |
src/anthropic/_streaming.py
Outdated
| ) | ||
|
|
||
| # Ensure the entire stream is consumed | ||
| for _sse in iterator: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, we want to keep this part until I port early exit improvement to codegen
|
Fixed! I've added back the stream consumption loop. The code now keeps: # Ensure the entire stream is consumed
for _sse in iterator:
passAnd the async version: # Ensure the entire stream is consumed
async for _sse in iterator:
passThis will remain until you port the early exit improvement to codegen. Changes pushed in commit 2a3e30b. |
|
This looks great, thanks! One note: we need to merge commits to the next branch instead of main |
|
@karpetrosyan done - whats the next step? |
- Replace decoder.close() with response.close() in sync stream - Replace decoder.close() with response.aclose() in async stream - Standardize comments between sync and async implementations Addresses feedback from PR anthropics#993: - SSEDecoder doesn't have a close method, properly close httpx response instead - Ensure consistent comments between sync/async versions
Changes based on review feedback from @karpetrosyan: 1. Removed MESSAGE_EVENTS filtering - now processes any event received 2. Removed early response closing (response.close/aclose) - will be ported to codegen 3. Removed performance test file - no longer applicable This change improves forward compatibility by processing any event type the API sends, rather than filtering to a predefined set. The new structure handles completion, ping, and error events explicitly, while processing all other events generically. Closes review feedback in anthropics#993
2a3e30b to
3216326
Compare
…resource cleanup fix(streaming-performance) - Optimize streaming event processing and resource cleanup fix(streaming-performance) - Optimize streaming event processing and resource cleanup
Updated test assertions to verify response.close() is called instead of decoder.close(), aligning with review feedback. Changes: - NewImplementationSimulator now calls response.close() - Stream cleanup test verifies response.close() - Regression test verifies response.close() All 11 tests now pass with live API validation.
Changes based on review feedback from @karpetrosyan: 1. Removed MESSAGE_EVENTS filtering - now processes any event received 2. Removed early response closing (response.close/aclose) - will be ported to codegen 3. Removed performance test file - no longer applicable This change improves forward compatibility by processing any event type the API sends, rather than filtering to a predefined set. The new structure handles completion, ping, and error events explicitly, while processing all other events generically. Closes review feedback in anthropics#993
3216326 to
cbc3e13
Compare
Rebased on latest
|
|
@karpetrosyan when this can get pushed? Thank you! |
|
@karpetrosyan time to push this? whats the process? |
|
I can't merge this myself, but I've asked the team to check it out. |
|
Sorry for the delay, we're figuring out internally if this is a safe change to make. We did merge one of the original changes in your PR through a codegen change and marked you as a coauthor :) |
refactor: process all streaming events for forward compatibility
Summary
This PR refactors the streaming implementation to process all event types instead of filtering to a predefined set. This improves forward compatibility by ensuring any new event types introduced by the API will be automatically processed without requiring SDK updates.
Changes Made
Rebased on latest
nextbranch which includes theresponse.close()optimization from codegen (commit 109b771).Key Change: Forward Compatibility
Replaced hardcoded event filtering with generic event processing:
Before (filtering to predefined events):
After (processes any event):
Benefits
Forward Compatibility - New event types automatically supported without SDK changes
content_block_thinking,tool_use_start, etc., they'll automatically workCleaner Code - Simpler logic without predefined event filtering
MESSAGE_EVENTSfrozenset constantAPI Alignment - Matches the SDK design philosophy of being forward compatible
Zero Breaking Changes - Existing functionality preserved
response.close()optimization from codegenBackward Compatibility
This change maintains 100% backward compatibility. All existing streaming code continues to work identically, with improved forward compatibility for future API evolution.
Technical Details
Stream) and async (AsyncStream) implementationsresponse.close()/await response.aclose()optimization from codegen