Skip to content
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: replace asyncio default event loop with uvloop #1790

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

asvishnyakov
Copy link
Contributor

@asvishnyakov asvishnyakov commented Jan 22, 2025

This is my proposed solution to #1789. Feel free to edit or close if I'm wrong

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. backend Pertains to the Python backend. labels Jan 22, 2025
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jan 22, 2025
@willydouhard
Copy link
Collaborator

Unfortunately I don't think this is possible, we need re-entrancy for stuff like AskUser

@dokterbob
Copy link
Collaborator

Unfortunately I don't think this is possible, we need re-entrancy for stuff like AskUser

But then the tests are passing.

@asvishnyakov Does this mean we can do away with the asyncio dependency?

As I'm not (yet) convinced there's a strong use case for this and @willydouhard suggests it's not possible, I propose we leave this open for a bit until either:

  1. A benchmark clearly shows there's performance issues in the current version.
  2. Other contributors weigh in on this and/or are able to do additional testing.

The rationale being: if it's not (demonstrably) broken...

@asvishnyakov
Copy link
Contributor Author

@dokterbob I’m new to Python, so I’m not entirely sure myself 😄. I’m trying to investigate performance issues in my app, and while this is definitely not a bottleneck, I was surprised to see it working better after the change. I think we need proper performance tests. Also, it looks like, as a workaround, I can run chainlint as a sub-app and configure uvloop for the main app, if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Pertains to the Python backend. size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants