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

Add AnyIO CapacityLimiter docs #2288

Closed

Conversation

aminalaee
Copy link
Member

@aminalaee aminalaee commented Sep 19, 2023

Related to #1724

Summary

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@@ -47,6 +47,7 @@ nav:
- Exceptions: 'exceptions.md'
- Configuration: 'config.md'
- Test Client: 'testclient.md'
- Sync Threadpools: 'threadpools.md'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not that easy to find this in the docs, if you don't know that you should look for the threadpool docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed but can't think of a better name right now. @Kludex any ideas?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concurrency?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concurrency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Sync" threadpools, as opposed to what?

@Kludex Kludex added hold Don't merge it documentation Project documentation labels Sep 19, 2023
docs/threadpools.md Outdated Show resolved Hide resolved
Comment on lines +17 to +18
# Set number of threads in threadpool to 100
anyio.to_thread.current_default_thread_limiter().total_tokens = 100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Set number of threads in threadpool to 100
anyio.to_thread.current_default_thread_limiter().total_tokens = 100
# Set number of threads in threadpool to 100
# Note that this increases the limit for anything using AnyIO, not just Starlette
anyio.to_thread.current_default_thread_limiter().total_tokens = 100

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devil's advocate: if we document this would it then be breaking change if we start using our own capacity limiter(s), meaning this stops working? I think not, it's okay to break docs to some extent.

Copy link
Member Author

@aminalaee aminalaee Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can update the docs when we add the CapacityLimiter to explain this.

@@ -47,6 +47,7 @@ nav:
- Exceptions: 'exceptions.md'
- Configuration: 'config.md'
- Test Client: 'testclient.md'
- Sync Threadpools: 'threadpools.md'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed but can't think of a better name right now. @Kludex any ideas?

Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
from starlette.routing import Route

# Set number of threads in threadpool to 100
anyio.to_thread.current_default_thread_limiter().total_tokens = 100
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this needs to be in the lifespan if using multiple workers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think each worker will do this, so there’s no difference.

but I can do a quick test to confirm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each worker runs in a separate process, right?

@Kludex Kludex requested a review from agronholm December 1, 2023 15:50
@Kludex
Copy link
Sponsor Member

Kludex commented Dec 24, 2023

@aminalaee Do you still want to work on this?

@Kludex Kludex mentioned this pull request Jan 8, 2024
1 task
@Kludex
Copy link
Sponsor Member

Kludex commented Jan 11, 2024

I'm closing this as stale. Please reopen if you still want to work on it, or if someone else wants to take it over, go ahead.

@Kludex Kludex closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Project documentation hold Don't merge it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants