-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix async callable object tools #568
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
Fix async callable object tools #568
Conversation
436673f
to
00c730a
Compare
00c730a
to
92d9426
Compare
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.
Thank you for your contribution!
Suggestion: APPROVE
Comment: This PR correctly addresses issue #567 by enhancing the detection of asynchronous functions in Tool.from_function
to properly handle asynchronous callable objects. The implementation follows Python best practices by adding a helper function _is_async_callable
that handles both regular async functions and async callable objects with __call__
methods. The changes are minimal, targeted to fix only the specific issue, and include comprehensive tests to verify the functionality. This fix properly supports a valid use case without introducing any breaking changes or performance overhead.
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.
Thank you for working on this!
Please can you address limit of loop iterations and it's good to merge.
|
||
|
||
def _is_async_callable(obj: Any) -> bool: | ||
while isinstance(obj, functools.partial): |
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 limit loop iterations here
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.
Thank you for the review! What would you suggest? I don't think there is any limit to how many partials you can have applied.
I do want to note that the reference implementation from Starlette is used by every consumer of FastAPI and does not specify a limit.
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.
The CPython standard library has a very similar implementation, also without a limit.
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.
scary, but consistent :)
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.
Thank you!!
|
||
|
||
def _is_async_callable(obj: Any) -> bool: | ||
while isinstance(obj, functools.partial): |
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.
scary, but consistent :)
@stephanlensky please can you merge main and I'll merge! |
Should be all set now! Thanks again 🙂 |
Makes detection of asynchronous functions more robust in
Tool.from_function
, allowing usage of asynchronous callable objects, e.g.Motivation and Context
Fixes #567.
How Has This Been Tested?
Unit tests were added to verify the change.
Breaking Changes
This is a non-breaking change.
Types of changes
Checklist
Additional context