-
Notifications
You must be signed in to change notification settings - Fork 611
fix(a2a): use a2a artifact update event #1401
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
Conversation
4fca63b to
4b5ba77
Compare
4b5ba77 to
39aab40
Compare
Unshure
left a comment
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.
Lets make sure to put this new behavior behind the enable_a2a_compliant_streaming flag so that existing integrations are not impacted, but notified of this change.
7574c91 to
350c9da
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Unshure
left a comment
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.
Code looks good, but a few of our tests failed. Left some comments throughout about ways to address it.
fix: update tests fix: simplify code by storing in class fix: remove uneeded code change fix: hide a2a artifact streaming under feature flag fix: use walrus operator fix: use star to signify end of unnamed fix: add check for walrus legacy fix: clarify enable_a2a_compliant_streaming parameter in StrandsA2AExecutor initialization fix: update tests refactor: streamline artifact addition logic in StrandsA2AExecutor
350c9da to
1766f35
Compare
Unshure
left a comment
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.
Codecov is 2% off, which I think is fine. And the api diff check sees the additional enable_a2a_compliant_streaming parameter as potentially breaking, but its been implemented such that it wont break existing users
Description
This PR updates the a2a executor to use the a2a libraries artifact append function. Intermediate chunks are best represented by artifact updates with append set to true, not task status updates. Please see this discussion on the a2a repo for more justification.
This feature is now hidden behind the
enable_a2a_compliant_streamingflag inA2AServer.enable_a2a_compliant_streaming False
enable_a2a_compliant_streaming True
Related Issues
#1377
Documentation PR
N/A
Type of Change
Bug fix
Testing
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.