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

Fix tests package being incorrectly included in builds #3440

Merged
merged 2 commits into from
May 3, 2024

Conversation

asumagic
Copy link
Contributor

@asumagic asumagic commented Apr 4, 2024

When installing the latest version of flair through PyPI, I noticed that a tests/ package could be found in my site-packages directory, and traced it back to flair.

In setup.py, the exclusion list expects a list of exclusions, not a single excluded path str. The end result is that before this PR, the exclusion list seems to be interpreted as an exclusion of "t", "e", "s" instead of "tests"...

Now, for a reason that is beyond my understanding of Python packaging, with that fixed it would still list tests under the site package's top_level.txt, and from tests import embedding_test_utils would locally fail with an "unknown location" error rather than a "module not found" error.
Not sure if this was some caching issue, but it was annoying nonetheless, thus:
I figured just listing the flair package under packages= would be just as good and might avoid some issues. It seems to work from my editable install testing, but I have not tested further.

(The above was not valid: Submodules need to be specified too in regular (non-editable) installs, so find_packages is the way to go.)

"tests.*" has to be excluded too or the subpackages (apparently determined by the presence of a __init__.py) still appear in the returned list. If only excluding ["tests"], then from tests import embedding_test_utils would fail with an "unknown location" error rather than a proper "no module named 'tests'" error.


To reproduce before this PR, try from tests import embedding_test_utils inside an environment with flair installed, with the working directory being outside of your install. It will find something to import, but that shouldn't have been the case.

Note -- flair is honestly probably not the only package that encountered this issue, considering I accidentally found this diagnosing an issue with the same package name in SpeechBrain... So while testing, make sure yet another package in your environment is not also pulling a tests package. :)

@asumagic asumagic marked this pull request as ready for review April 4, 2024 10:51
@asumagic asumagic marked this pull request as draft April 4, 2024 11:06
@asumagic asumagic marked this pull request as ready for review April 4, 2024 11:07
@asumagic
Copy link
Contributor Author

asumagic commented Apr 5, 2024

Ugh, nevermind -- specifying only the root module does not work with non-editable installs and docs are rather adamant about it: https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#packages

Will have to diagnose why the exclude list was causing issues for me.

@asumagic asumagic marked this pull request as draft April 5, 2024 08:52
@asumagic asumagic force-pushed the packaging/remove-tests-package branch from 608f954 to 5a267c9 Compare April 5, 2024 08:57
@asumagic
Copy link
Contributor Author

asumagic commented Apr 5, 2024

Ok, solved the above and edited the main PR message for accuracy. I'm now a bit more confident this is valid, and this was tested with both regular and editable installs.

Sorry if someone was subscribed to too many notifications :)

@asumagic asumagic marked this pull request as ready for review April 5, 2024 09:03
Copy link
Collaborator

@helpmefindaname helpmefindaname left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @asumagic
thank you for fixing this,
I verified this manually and can comfirm.
@alanakbik as the test failures are obviously not related to the PR, I think you can merge this without successful tests.

@alanakbik alanakbik merged commit 617c2d3 into flairNLP:master May 3, 2024
1 check failed
@alanakbik
Copy link
Collaborator

Thanks a lot @asumagic!

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.

3 participants