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

No async because Piero is paniking #957

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

Pingdred
Copy link
Member

@pieroit
Copy link
Member

pieroit commented Oct 27, 2024

Best PR ever 💊

@matteocacciola

This comment was marked as abuse.

@pieroit
Copy link
Member

pieroit commented Oct 28, 2024

Some more context, please?

I see async/await creeping in the core code, so much so as to put in question plugin primitives (should tools/hooks/forms/handlers be sync or async? should we support both?).
This will complicate core development and documentation, and even more external dev experience.

As a maintainer the question is: given this ulterior complexity, are there benchmarks and confirmations that is indeed worth it to introduce coroutines? How deep in the framework?

What I discovered is:

  • FastAPI invites the usage of async when you need to use async libraries; fastAPI distributes autonomously sync workloads in a threadpool and eventually over workers, allowing tens of concurrent requests; we don't need to over-optimize.
  • we defined the StrayCat and the Agent in async, but the whole conversation is still run in a threadpool, making things unnecessarily convoluted (i.e. an event loop per stray) and factually not an advantage if not for rare use cases.
  • benchmarks (like this and many others) on async vs sync endpoints show async is indeed faster, but not even an order of magnitude, so the performance advantage is not worth the complexity of letting coroutines go deeply into the StrayCat / Agent.
  • the async/await proliferation in our codebase is more related to an experimentation / fun phase than to an actual intent to speed up.
  • as a general open source strategy, if somebody (i.e. external company) wants to help us make the cat scale, they will be available to sponsor the effort economically or with workforce. If not, we stay happy and have fun and ignore biz people asking - without helping - for features most of the community does not need.

Hope this is clear. I'm going nuclear on this.
No more async / await unless it is strictly necessary.

@pieroit pieroit merged commit a5e7d96 into cheshire-cat-ai:develop Oct 28, 2024
1 of 2 checks passed
@sambarza
Copy link
Member

FastAPI invites the usage of async when you need to use async libraries; fastAPI distributes autonomously sync workloads in a threadpool and eventually over workers, allowing tens of concurrent requests; we don't need to over-optimize.

For my side at the moment that's the only point to use coroutine in the Cat, no performance, no power consumption, nothing else.

@Pingdred Pingdred deleted the no-async branch November 5, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants