Skip to content

[WIP] Move from retrying to tenacity #7592

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

Closed
wants to merge 8 commits into from

Conversation

yeraydiazdiaz
Copy link

@yeraydiazdiaz yeraydiazdiaz commented Jan 13, 2020

Closes #7530

I've replaced retrying with tenacity on _vendor. Bot some modifications had to be made to the source to adapt it to pip, specifically absolute imports of tenacity and six. This is the bulk of the contents of this PR, you can safely ignore all files in _vendor/tenacity.

However, these changes cause the vendoring tox environment to fail, I'm not really sure what the solution would be.

Adapted API calls to retry to tenacity:

  • Timeouts are in seconds.
  • stop_max_delay and wait_fixed are no longer kwargs to retry but are imported and passed as values to the stop and wait kwargs.

Yeray Diaz Diaz added 5 commits January 13, 2020 12:38
@yeraydiazdiaz
Copy link
Author

Question: I noticed retrying and all other vendored dependencies have pyi files, a quick search shows these can be created using stubgen but the output's contents differ quite a bit from the existing pyi.

Any pointers on how to deal with these?

@pradyunsg
Copy link
Member

Huh, looks like I never made my comment about the vendoring process. :/

Any pointers on how to deal with these?

pip uses vendoring to vendor dependencies. The main way to use it is to run tox -e vendoring. It loads configuration from pyproject.toml and rewrites imports and various other minor things. Updating src/pip/_vendor/vendor.txt to include the relevant dependencies should automatically generate the required mypy stubs.

If we need to make changes, the patches are held in the tools/automation/vendoring/patches directory, and you can generate them by manually making changes and running git diff.

There's a Travis CI job to ensure that the vendoring is done correctly, which essentially re-runs the vendoring process, to make sure that the result of re-vendoring dependencies is the same as what's been committed. :)

@yeraydiazdiaz
Copy link
Author

yeraydiazdiaz commented Jan 14, 2020

Right, so I basically was doing things manually 😅 Thanks!

I do face a problem with futures though, vendoring is ran in Python 3.8 and futures cannot be installed in it, so the command fails.

I tried adding it manually but vendoring removes it. To work around it I tried including futures in the protected_files configuration, but it seems it only works for files, not directories, so I'm stuck 😞

Seems like a lot of work for tenacity 😕

@pradyunsg
Copy link
Member

Hmm... It's a code smell and I don't like it. :/

We could add a "pass --ignore-requires-python to pip" configuration to vendoring but that feels a bit hacky to me. I'm not opposed to doing that -- I just want to avoid it if we can.

Are there alternatives that, like, aren't tricky to vendor?

@pradyunsg
Copy link
Member

I tried including futures in the protected_files configuration, but it seems it only works for files, not directories

Yea, this was an intentional design choice, to prevent users from adding dependencies and not specifying them as such (this is all working as planned basically).

@yeraydiazdiaz
Copy link
Author

Hmm... It's a code smell and I don't like it. :/
We could add a "pass --ignore-requires-python to pip" configuration to vendoring but that feels a bit hacky to me. I'm not opposed to doing that -- I just want to avoid it if we can.

Agreed.

Are there alternatives that, like, aren't tricky to vendor?

Not that I know of.

Given the use of the retry function is pretty basic I think it's not worth the hacks to vendor tenacity. On the flip side, we would be unofficially maintaining retrying though if anything went wrong.

I'm fine with closing this PR if you feel it's not worth it. Or maybe you have an alternative?

@yeraydiazdiaz
Copy link
Author

I'm going to close this now since, as I mentioned, I don't feel it's worth the hackery required to vendor tenacity. Happy to pick it up if we find another way.

@pfmoore
Copy link
Member

pfmoore commented Jan 16, 2020

Thanks for working on this @yeraydiazdiaz!

One thought - would it be possible to vendor tenacity without its Python 2 dependencies, and use retrying on Python 2 but tenacity on Python 3? That would (IMO) fit with our policy of making improvements Python 3 only if implementing them on Python 2 is too costly. It would mean we vendor both retrying and tenacity, but maybe that's still worth it.

@yeraydiazdiaz
Copy link
Author

That's a good idea.

Assuming vendoring is happy with it, which I think it might, we'd have to accomodate for the API differences between the two (see bulletpoints in #7592 (comment)), maybe write a simple Python 2.7-only adapter for retrying.

I'll give it a go, thanks for the suggestion 👍

@yeraydiazdiaz yeraydiazdiaz reopened this Jan 16, 2020
@yeraydiazdiaz
Copy link
Author

So I've spent some time on this and I feel that it might be more trouble than it's worth.

There's enough differences between the two APIs to make the wrapper quite complex and bug-prone. It would also give the false impression to the user that they can use whatever feature of tenacity when, in fact, they can only use features that are also in retrying.

All in all, IMHO keeping the vendored retrying is the best option atm. 🙂

@pfmoore
Copy link
Member

pfmoore commented Jan 16, 2020

Sounds reasonable. Thanks for investigating the suggestion anyway :-)

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 15, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch from retrying to tenacity
4 participants