jd / tenacity

Retrying library for Python
http://tenacity.readthedocs.io
Apache License 2.0
6.6k stars 281 forks source link

8.4.0 release is broken due to missing files #475

Open voegtlel opened 3 months ago

voegtlel commented 3 months ago

Apparently, asyncio is missing from the released wheel. All imports fail immediately with

  File "site-packages/tenacity/__init__.py", line 653, in <module>
    from tenacity.asyncio import AsyncRetrying  # noqa:E402,I100
ModuleNotFoundError: No module named 'tenacity.asyncio'

checked the released wheel, and it's missing there.

voegtlel commented 3 months ago

A quick fix for everybody facing the same issue, manually downgrade to 8.3.0:

pip install --force-reinstall tenacity==8.3.0
leondz commented 3 months ago

thanks for the quick fix @voegtlel

please update pypi with a working version of tenacity

uinfante commented 3 months ago

Seems it's fixed in the new patch 8.4.1

claudiocmp commented 3 months ago

I appreciate your quick response. However, I would expect to see a test for the API to pass, e.g.:

from tenacity.asyncio import AsyncRetrying

or an alternative using importlib - so that this issue doesn't go unchecked in the future.

ewjoachim commented 3 months ago

A test like you mention would do nothing: the project is not installed the same way in tests as it is for regular users, so even if there was a test, it wouldn't really test anything.

Logic errors, code errors, we can test, packaging errors, I don't really think we easily can, and I'd argue I don't think we should.

claudiocmp commented 3 months ago

I'd argue I don't think we should.

Perhaps OT - but could you expand on the reason for this?

ryancausey commented 3 months ago

A test like you mention would do nothing: the project is not installed the same way in tests as it is for regular users, so even if there was a test, it wouldn't really test anything.

Logic errors, code errors, we can test, packaging errors, I don't really think we easily can, and I'd argue I don't think we should.

The only way I know of achieving tests of the package as it is installed by someone using pip or a similar tool is to use tox to run the tests. It specifically builds a package and installs it in a clean venv to help catch packaging errors like this. I am unsure how hard it is to migrate an existing test suite to be run by tox, however.

ewjoachim commented 3 months ago

could you expand on the reason for this?

I'm not someone who takes any kind of decisions on this project, so this is just my very own opinion.

Adding tests that are similar to existing tests is (normally) cheap, and mostly always free, especially if they're unit tests and run in milliseconds. In that sense, making a rule of always adding a quick non-reg test is almost always worth it.

When you start talking about adding tests that have a different way of being run, or changing the way all of your tests run, suddenly, the impacts are different. Does this mean you want another job in your CI ? that is going to potentially slow down every build by a non-negligible factor, potentially make it more complex to run locally, which means raising the bar for contributions etc. At this point, it's necessary to weight the pros and cons. The pros are: that bug will not happen again unnoticed (do I have a reason of believing that this bug, more than all other possible bugs in the universe, will happen again ?)

At this point, it's not a "we have a bug, we need a non-reg tes, period". It's negociations and taking decisions with many aspects in mind.

Now, I can't take the decision for the owners here, but I would say that from seeing what went wrong, and what would be an effective step to avoid this from happening, the risk-benefit analysis seems to me in favor of just fixing.

tanhaipeng commented 3 months ago

This error is too low-level and can be found by simple testing.

claudiocmp commented 3 months ago

Thanks much for sharing @ewjoachim. I am probably spoiled by my workplace - our internal runner (soft) installs the repo in a venv as @ryancausey said.

tan-wei commented 3 months ago

Seems 8.4.0 should be yanked.