Copy link Copy Markdown

Member

closes #3018

Some comments:

I went with the 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 …
I introduced aiolimiter as optional requirement, since using the AIORateLimiter 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-77767364
I did not add rate_imit_args to shortcut methods. IMO that would add too much of a connection between tg and tg.ext and if users want to pass rate_limit_args they can always use the standard methods instead.

Checklist for PRs

Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
Created new or adapted existing unit tests
Documented code changes according to the CSI standard
Added myself alphabetically to AUTHORS.rst (optional)
Added new classes & modules to the docs and all suitable __all__ s

Copy link Copy Markdown

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

Copy link Copy Markdown

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 :)

Copy link Copy Markdown

Member Author

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

Copy link Copy Markdown

Member Author

got tests to work on ubuntu. Skipped them on macOS, b/c I don't think that it's really worth the effort 😬

Copy link Copy Markdown

@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 :-)

Copy link Copy Markdown

Member

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

Closed

Copy link Copy Markdown

Member

Choose a reason for hiding this comment

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

much awaited addition!