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

setup.py: use setuptools.find_namespace_packages() #1483

Merged
merged 8 commits into from Aug 17, 2022

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Jun 26, 2022

According to people who keep better track than I do of the evolution of python packaging over time, any folder is now considered a python package, even if it doesn't have any python-related files (see pypa/setuptools#3340 (comment)). This prompted some deprecations in setuptools, leading to the warnings in #1474. According to pypa/setuptools#3340 (comment) and similar posts elsewhere, using setuptools.find_namespace_packages() is a recommended fix.

In our case find_namespace_packages() includes a bunch of other stuff we don't actually want, but find_packages() seems closer to the mark. Making that change seems to get rid of the warnings without changing which files get included in the sdist:

# use `cut` to remove the first component of filepaths, which we don't care about here
$ tar tf pvlib-0.9.1+12.gc8b04a56.tar.gz | cut -d / -f 2- > old.txt  # current master
$ tar tf pvlib-0.9.1+18.g071cc215.tar.gz  | cut -d / -f 2- > new.txt  # current HEAD on this branch
$ diff old.txt new.txt  # no output (except unrelated changes, e.g. new pvfactors gallery example file), so list of filenames did not change

I don't really understand what's happening under the hood here, but I want to see what the CI thinks of this change. Keeping as a draft until I have a better understanding of what setuptools is doing.

@kandersolar kandersolar added this to the 0.9.2 milestone Jul 5, 2022
@kandersolar
Copy link
Member Author

Ready for review. I guess the long and the short of it is that we should have been listing all packages (including sub-packages and even pvlib.data) all along, and until now setuptools has been silently fixing this for us by including all those other directories as "data" rather than normal code. It worked, but was not the right way to do it, and now setuptools has deprecated that behavior.

The old distutils documentation agrees that all directories should be listed; see the last example in section 6.2 here: https://docs.python.org/3/distutils/examples.html#pure-python-distribution-by-package

@kandersolar kandersolar marked this pull request as ready for review July 5, 2022 16:31
@mikofski
Copy link
Member

mikofski commented Jul 5, 2022

TBH that’s the way (listing all packages) I have been doing it, and I was reluctant to use start using find_packages() until recently. I also use MANIFEST.in for sdist archives. Check out sunpower/PVmismatch.

@wholmgren
Copy link
Member

@kanderso-nrel I just tagged an alpha release after merging #1495. We should double check that installs as expected and then merge this one and perhaps make another alpha release

@kandersolar kandersolar mentioned this pull request Aug 17, 2022
8 tasks
should hopefully address and silence this warning:
Warning: You are using "pypa/gh-action-pypi-publish@master". The "master" branch of this project has been sunset and will not receive any updates, not even security bug fixes. Please, make sure to use a supported version. If you want to pin to v1 major version, use "pypa/gh-action-pypi-publish@release/v1". If you feel adventurous, you may opt to use use "pypa/gh-action-pypi-publish@unstable/v1" instead. A more general recommendation is to pin to exact tags or commit shas.
@kandersolar kandersolar changed the title setup.py: use setuptools.find_packages() setup.py: use setuptools.find_namespace_packages() Aug 17, 2022
@kandersolar
Copy link
Member Author

After merging master and updating the MANIFEST.in prune and exclude entries, the difference in tar.gz contents between v0.9.1 and 70c9ab3 looks as expected:

13a14
> docs/examples/bifacial/plot_pvfactors_fixed_tilt.py
133a135
> docs/sphinx/source/whatsnew/v0.9.2.rst
149d150
< pvlib/_version.py
162a164
> pvlib/data/Burlington, United States SolarAnywhere Time Series 2021 Lat_44_465 Lon_-73_205 TMY3 format.csv
190c192
< pvlib/data/pvgis_hourly_Timeseries_45.000_8.000_CM_10kWp_CIS_5_2a_2013_2014.json
---
> pvlib/data/pvgis_hourly_Timeseries_45.000_8.000_SA2_10kWp_CIS_5_2a_2013_2014.json
316a319
> pyproject.toml
319d321
< versioneer.py

Also, the new 0.9.2-alpha.2 wheel seems to work fine for me in a clean python 3.7 environment. +1 from me to make another alpha release after merging this.

@wholmgren wholmgren merged commit 3692427 into pvlib:master Aug 17, 2022
@kandersolar kandersolar deleted the installation_warnings branch August 18, 2022 00:34
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.

"Package would be ignored" warnings when building the pvlib distribution
3 participants