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

Replace tenacity.retry with our own decorator #12705

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ichard26
Copy link
Member

@ichard26 ichard26 commented May 15, 2024

Tenacity is a 1600+ LOC dependency which is only used for a convenient @retry decorator... which is used a grand total of two times. This is a lot of code for what can be trivially reimplemented by us.

This also incidentally improves startup performance as tenacity imports asyncio which is expensive and unnecessary for us.

Resolves #10822.

Tenacity is a 1600+ LOC dependency which is only used for a convenient
`@retry` decorator... which is used a grand total of two times. This is a
lot of code for what can be trivially reimplemented by us.

This also incidentally improves startup performance as tenacity imports
asyncio which is expensive and unnecessary for us.
@ichard26
Copy link
Member Author

CI is failing as typing-extensions doesn't support Python 3.13 (at least not for ParamSpec). Jelle informed me that there should be a new release fixing these issues soon, though. I'm not very happy with the jankiness of my unit tests, so I think I'll rewrite them. I won't have time until tomorrow, if not the weekend though.

@notatallshaw
Copy link
Contributor

notatallshaw commented May 16, 2024

As you have no runtime need for these type hints, a workaround is to put the type annotation constructions behind an if TYPE_CHECKING and then use string annotations or from __future__ import annotations,

@ichard26
Copy link
Member Author

I literally contribute to mypyc and yet I somehow completely forgot about that trick. Gosh, that's what I get for writing code when I'm too tired. Thanks for the reminder!

(*) I'm not sure if the use of from __future__ import annotations is approved for this codebase. IIRC the maintainers wanted to wait until the situation between annotations and co_annotations was resolved before making a decision.

@uranusjr
Copy link
Member

This seems simple enough, but do we have analysis on how the decorator compares to tenacity’s implementation? Are there edge cases?

@ichard26 ichard26 force-pushed the replace-tenacity branch 3 times, most recently from 2d97e4d to 37f233d Compare May 28, 2024 20:26
@ichard26
Copy link
Member Author

This seems simple enough, but do we have analysis on how the decorator compares to tenacity’s implementation? Are there edge cases?

I did briefly read through tenacity's source code to get a feel for how it works (especially in regards to the time limit), but I haven't examined it carefully. However, I have written a decent set of unit tests for my decorator. Notably, they still all pass when testing tenacity's @retry implementation. I tested this by commenting out the pip._internal import and adding this near the top of the test module:

from tenacity import wait_fixed, stop_after_delay as _stop_after_delay, retry as _retry

def retry(wait, stop_after_delay):
    return _retry(wait=wait_fixed(wait), stop=_stop_after_delay(stop_after_delay), reraise=True)

I can take a closer look at tenacity's implementation if you'd like, but I think these tests demonstrate that the two decorators are functionally equivalent in the areas that matter.

@ichard26
Copy link
Member Author

Windows' such a joy, sigh ✨

@ichard26
Copy link
Member Author

ichard26 commented Jun 1, 2024

This is reasonably close to be ready for review. I do want to make some more minor changes to the tests, but CI is going to keep failing until #12732 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove tenacity dependency
3 participants