Skip to content

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Jul 8, 2022

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

@Bibo-Joshi Bibo-Joshi added enhancement 🛠 refactor change type: refactor labels Jul 8, 2022
@Bibo-Joshi Bibo-Joshi added this to the v20.0a3 milestone Jul 8, 2022
Copy link

@github-actions github-actions bot left a 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
Copy link

@github-actions github-actions bot left a 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 :)

@Bibo-Joshi
Copy link
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

@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review July 14, 2022 05:53
@Bibo-Joshi

This comment was marked as off-topic.

@Bibo-Joshi
Copy link
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 😬

@harshil21 harshil21 self-requested a review August 2, 2022 20:04
@bharel
Copy link

bharel commented Aug 3, 2022

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

@harshil21 harshil21 left a 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

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

much awaited addition!

@Bibo-Joshi Bibo-Joshi merged commit 741a50a into master Aug 26, 2022
@Bibo-Joshi Bibo-Joshi deleted the rate-limiter branch August 26, 2022 04:50
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2022
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔌 enhancement pr description: enhancement 🛠 refactor change type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Mechanism for Avoiding Flood Limits
3 participants