pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

setup.py: use setuptools.find_packages()

Open kandersolar opened this issue 3 years ago • 2 comments

  • [x] Closes #1474
  • [x] I am familiar with the contributing guidelines
  • ~[ ] Tests added~
  • ~[ ] Updates entries in docs/sphinx/source/reference for API changes.~
  • ~[ ] Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).~
  • ~[ ] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.~
  • [x] Pull request is nearly complete and ready for detailed review.
  • [x] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

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 https://github.com/pypa/setuptools/issues/3340#issuecomment-1146676266). This prompted some deprecations in setuptools, leading to the warnings in #1474. According to https://github.com/pypa/setuptools/issues/3340#issuecomment-1146678086 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 avatar Jun 26 '22 18:06 kandersolar

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 avatar Jul 05 '22 16:07 kandersolar

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.

mikofski avatar Jul 05 '22 18:07 mikofski

@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

wholmgren avatar Aug 16 '22 15:08 wholmgren

After merging master and updating the MANIFEST.in prune and exclude entries, the difference in tar.gz contents between v0.9.1 and 70c9ab3e 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.

kandersolar avatar Aug 17 '22 21:08 kandersolar