search

flairNLP / flair

A very simple framework for state-of-the-art Natural Language Processing (NLP)
https://flairnlp.github.io/flair/
Other
13.85k stars 2.09k forks source link

Fix `tests` package being incorrectly included in builds #3440

Closed asumagic closed 5 months ago

asumagic commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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 :)

alanakbik commented 5 months ago

Thanks a lot @asumagic!