pvlib-python
pvlib-python copied to clipboard
Use pep517/518 build system for modern build isolation
Following #1486
- Remove pvlib/_version.py (versioneer)
- Use importlib.metadata to retrieve version from PKG-INFO
- Add importlib-metadata backport for python < 3.8
- Create pyproject.toml with setuptools build backend
- Update workflow to use pypa/build module to build sdist & wheel
editable installation still works with the new setuptools 63
~- [ ] Closes #xxxx~
- [x] I am familiar with the contributing guidelines
~- [ ] Tests added~
~- [ ] Updates entries in
docs/sphinx/source/reference
for API changes.~ - [x] 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.
- [ ] Maintainer: Appropriate GitHub Labels (including
remote-data
) and Milestone are assigned to the Pull Request and linked Issue.
About top files included in dist, I think setuptools_scm by default include files checked in VCS. I'll have to check.
About the version name, by default it generates a dev version with number as number of commit. This explains why it displays next version with dev.
Then local part, after +
you get commit hash + date if dirty. This is customizable if we want to have the exact same as versioneer.
I think setuptools_scm by default include files checked in VCS.
I think you're right, and we should adapt our MANIFEST.in accordingly: https://github.com/pypa/setuptools_scm#file-finders-hook-makes-most-of-manifestin-unnecessary
It's fine by me to change the version string format, especially if the new format is what the PyPA suggests/prefers. I only brought it up in case the other maintainers care more about non-tagged version strings than I do :)
I've removed include in MANIFEST.in
since files tracked by VCS are by default included.
Also I've removed reference to versioneer files from .lgtm & .stickler files.
@kanderso-nrel can you check building SPA C extension is still working ? Should not be a problem, setuptools build backend is still using setup.py under the hood.
@kanderso-nrel can you check building SPA C extension is still working ? Should not be a problem, setuptools build backend is still using setup.py under the hood.
After building the .so
file:
-
$ pip install .
seems to do the right thing: it produces and installs a platform-specific wheel (pvlib-0.9.2.dev16+g3bcd63e-cp39-cp39-linux_x86_64.whl
) that includes the.so
file, and the C function executes as expected from python. -
$ python -m build --wheel
also produces a platform-specific wheel containing the.so
. Seems like it would work, although I did not actually install it to be sure. -
$ python -m build
produces a platform agnostic wheel (pvlib-0.9.2.dev16+g3bcd63e-py3-none-any.whl
) which doesn't contain the.so
.
I'm not sure why python -m build
behaves differently from python -m build --wheel
in that regard, but I also think it doesn't really matter since our distribution files don't include the SPA C extension. Building it locally and installing with pip install .
still works, and I think that's all we care about (I hope another maintainer will confirm this).
Unrelated to the SPA C extension, I think we should add a bunch of excludes to the MANIFEST so that docs
, benchmarks
, ci
etc don't get bundled. But I wouldn't object to skipping that for now and adding it to #1483.
I thought editable installs do not work with this pattern. That would destroy my workflow.
No objection to the change in version string format. I'm guessing we'll want to make alpha tags when we anticipate that the next release will be a 0.x.0. (I'd like to see more pre-releases anyways.)
Are the changes to MANIFEST consistent with the discussion in https://github.com/pvlib/pvlib-python/pull/580? I am a -1 on removing anything from the distribution at this time, so let's take up that discussion somewhere else.
I thought https://github.com/pvlib/pvlib-python/discussions/1486#discussioncomment-3085581 with this pattern. That would destroy my workflow.
I think editable installs are only a problem if you ditch setup.py
altogether (https://github.com/pypa/setuptools/issues/2816#issuecomment-1195451458), which is not the case here. pip install -e .
seemed to do the right thing when I tried it out just now, but it would be good if you could try it on your end too since I don't know how to thoroughly vet that it actually works.
Are the changes to MANIFEST consistent with the discussion in https://github.com/pvlib/pvlib-python/pull/580? I am a -1 on removing anything from the distribution at this time, so let's take up that discussion somewhere else.
Sorry, maybe my phrasing was confusing. As I understand it, this PR doesn't remove anything from the distribution. It does add things to the distribution because of the switch to setuptools-scm
, which includes everything tracked by git (ref), and this PR doesn't prune
them back out again.
I thought it would be easier to prune them back out in another PR (e.g. #1483, which is already dealing with what gets included in the dist) so that we don't ask too much of @jules-ch :)
Seems to work. I did the following
- check out branch
- activate conda env with existing pvlib dev installation
- import pvlib.
pvlib.__version__
returns 0.7.2 (that's what was checked out when I last ranpip install -e .
) - confirm that despite version report above, I actually have all the features on this branch (like Array, which was created long after 0.7.2)
- exit ipython
-
pip install -e .
Successfully uninstalled pvlib-0.7.2
Successfully installed pvlib-0.9.2.dev16+g3bcd63e
-
import pvlib; pvlib.__version__
'0.9.2.dev16+g3bcd63e'
- define a new function in irradiance.py. use
importlib.reload
to reload module. new function works
I'm good with merging. I'd like to give @mikofski an opportunity to review too (or decline to review) since he's been fighting python packaging longer than any of us.
@jules-ch can you resolve the merge conflicts?
I've rebased, should be good now @wholmgren
FYI CI is failing because I'm ignoring coverage on version.py & it is reducing code coverage which is making CI fails.
thanks @jules-ch! let's see how it goes...
One thing that I failed to test above... make a change, then check the version without reinstalling. It used to be the case that versioneer would immediately report .dirty
or, if a commit was made, +X.abcdefg
. Now it seems that I must rerun pip install -e .
to get the new version. Is that correct? That seems to track the discussion in https://github.com/matplotlib/matplotlib/pull/18971. And mpl now uses a function in __init__.py
as a workaround. Thoughts on a follow up PR for that?
Shoud not be too difficult
Ok, makes sense that switching from versioneer (run-time version determination) to setuptools_scm (install-time determination) would cause this issue. After some searching, it seems like people either just live with outdated version strings for editable installs or use some variant of the matplotlib approach of dynamically computing the version if the code detects that it's being imported from a git repo. I don't love it but it seems there's not a better option, or at least I haven't found one!