-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(tools): run sync tools in a thread to avoid event loop blocking #820
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: main
Are you sure you want to change the base?
Conversation
Here's a sample test that fails on the |
Requesting for reviews 🙇 @rm-openai |
This PR is stale because it has been open for 10 days with no activity. |
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.
These changes look good to me; @rm-openai thoughts?
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.
hmm i'd definitely not want to move things to a separate thread by default. That would cause issues for anyone who ignores thread safety today based on the existing implementation.
Thoughts on making this configurable?
@rm-openai sure, it makes sense to make it configurable. I've added a I'm curious about your thoughts on how to add a unit test for it - I had added a test here using pyleak, but I'm not sure if we want to add a new test dependency. |
Synchronous tools execution blocks the event loop, preventing other requests and
asyncio.Task
s. This PR runs the sync tools in a separate thread usingasyncio.to_thread()
Consider adding
pyleak
in CI to catch asyncio task leaks and event loop blocking (disclaimer: I'm the author).