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

8.4.0 release is broken due to missing files #475

Open
voegtlel opened this issue Jun 17, 2024 · 11 comments
Open

8.4.0 release is broken due to missing files #475

voegtlel opened this issue Jun 17, 2024 · 11 comments

Comments

@voegtlel
Copy link

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
Copy link
Author

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

pip install --force-reinstall tenacity==8.3.0

@leondz
Copy link

leondz commented Jun 17, 2024

thanks for the quick fix @voegtlel

please update pypi with a working version of tenacity

@uinfante
Copy link

Seems it's fixed in the new patch 8.4.1

@claudiocmp
Copy link

claudiocmp commented Jun 17, 2024

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
Copy link

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
Copy link

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

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

@ryancausey
Copy link

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
Copy link

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
Copy link

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

@claudiocmp
Copy link

claudiocmp commented Jun 18, 2024

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
Copy link

tan-wei commented Jun 30, 2024

Seems 8.4.0 should be yanked.

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

No branches or pull requests

8 participants