-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New rate limiting mechanism #3148
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
Conversation
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.
Hey! Looks like you edited the requirements or the pre-commit hooks. I'm just a friendly reminder to keep the additional dependencies for the hooks in sync with the requirements :)
# Conflicts: # .github/workflows/pre-commit_dependencies_notifier.yml
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.
Hey! Looks like you edited README.rst or README_RAW.rst. I'm just a friendly reminder to apply relevant changes to both of those files :)
I'll have to try & check how I can get the tests to work on unix. otherwise, this ready for a first round of review imo |
This comment was marked as off-topic.
This comment was marked as off-topic.
got tests to work on ubuntu. Skipped them on macOS, b/c I don't think that it's really worth the effort 😬 |
@Bibo-Joshi @harshil21 I created an asyncio limiter that doesn't have the httpx issue. In fact, the issue doesn't have anything to do with httpx at all. The interface is similar although a tad different, but it is considerably more efficient. You're welcome to test it and tell me if it's better for you :-) |
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.
Finally we have a working rate limiter! Didn't test out the changes locally yet. Also have a gut feeling that there's probably a bug here and there which I didn't catch
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
# Conflicts: # telegram/_bot.py
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.
much awaited addition!
closes #3018
Some comments:
aiolimiter
library despite New Mechanism for Avoiding Flood Limits #3018 (comment). AFAIK the performance problems are only relevant for very high load and the built-in implementation is only supposed to be a minimal effort reference implementation …aiolimiter
as optional requirement, since using theAIORateLimiter
is purely opt-in. This design goes in the direction that I'd like to discuss in the context of https://github.com/python-telegram-bot/python-telegram-bot/projects/7#card-77767364rate_imit_args
to shortcut methods. IMO that would add too much of a connection between tg and tg.ext and if users want to passrate_limit_args
they can always use the standard methods instead.Checklist for PRs
.. versionadded:: version
,.. versionchanged:: version
or.. deprecated:: version
to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)AUTHORS.rst
(optional)__all__
s