tenacity icon indicating copy to clipboard operation
tenacity copied to clipboard

8.4.0 release is broken due to missing files

Open voegtlel opened this issue 1 year ago • 11 comments

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 avatar Jun 17 '24 12:06 voegtlel

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

pip install --force-reinstall tenacity==8.3.0

voegtlel avatar Jun 17 '24 12:06 voegtlel

thanks for the quick fix @voegtlel

please update pypi with a working version of tenacity

leondz avatar Jun 17 '24 12:06 leondz

Seems it's fixed in the new patch 8.4.1

uinfante avatar Jun 17 '24 12:06 uinfante

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.

claudiocmp avatar Jun 17 '24 12:06 claudiocmp

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.

ewjoachim avatar Jun 17 '24 13:06 ewjoachim

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

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

claudiocmp avatar Jun 17 '24 13:06 claudiocmp

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.

ryancausey avatar Jun 18 '24 00:06 ryancausey

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.

ewjoachim avatar Jun 18 '24 09:06 ewjoachim

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

tanhaipeng avatar Jun 18 '24 11:06 tanhaipeng

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.

claudiocmp avatar Jun 18 '24 11:06 claudiocmp

Seems 8.4.0 should be yanked.

tan-wei avatar Jun 30 '24 15:06 tan-wei