Skip to content

Add AnyIO CapacityLimiter docs#2288

Closed
aminalaee wants to merge 2 commits intoKludex:masterfrom
aminalaee:add-anyio-capacity-limiter-docs
Closed

Add AnyIO CapacityLimiter docs#2288
aminalaee wants to merge 2 commits intoKludex:masterfrom
aminalaee:add-anyio-capacity-limiter-docs

Conversation

@aminalaee
Copy link
Contributor

@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.

- Exceptions: 'exceptions.md'
- Configuration: 'config.md'
- Test Client: 'testclient.md'
- Sync Threadpools: 'threadpools.md'
Copy link
Contributor 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
Contributor

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
Owner

Choose a reason for hiding this comment

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

Concurrency?

Copy link
Owner

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
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
Contributor

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
Contributor

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
Contributor 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.

- Exceptions: 'exceptions.md'
- Configuration: 'config.md'
- Test Client: 'testclient.md'
- Sync Threadpools: 'threadpools.md'
Copy link
Contributor

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
Owner

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
Contributor 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
Owner

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
Owner

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.

4 participants