-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
More async tool fixes #1204
More async tool fixes #1204
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1204 +/- ##
===========================================
+ Coverage 30.38% 50.03% +19.65%
===========================================
Files 32 32
Lines 4302 4355 +53
Branches 994 1066 +72
===========================================
+ Hits 1307 2179 +872
+ Misses 2901 1963 -938
- Partials 94 213 +119
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Another option would be allowing both async and sync functions to be called using something like asyncer (https://github.com/tiangolo/asyncer). However, I would not add it as an additional dependency but rather rewrite it for our needs (that ~300 lines of code altogether and can be done in a day). |
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.
LGTM.
I think we need to add tests for all of the new functionality, but that should be another PR because these fixes cannot wait.
* tool_responses fixes * [] is false
* add readme * migration headsup * remove move date * Update README.md Co-authored-by: Chi Wang <[email protected]> --------- Co-authored-by: Chi Wang <[email protected]>
* tool_responses fixes * [] is false
Why are these changes needed?
Fixes for #1174, also includes the bug fix in #1201
Started thinking about test coverage, a little awkward to exercise correctly.
Also there is odd interplay between sync/async tool funcs and if that conversations is calling through the sync or async send/receive methods.
Current setup
Previously
I see a similar thing happen with check_termination_and_human_reply when running in async, it gets called twice if a_human_reply returns None for no human input.
I think https://github.com/miguelgrinberg/promisio can resolve the issue by providing the same interface for both sync and async functions. But that is a much bigger change and likely not backward compatible.
Related issue number
closes #1174
Checks