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

Use pep517/518 build system for modern build isolation

Open jules-ch opened this issue 2 years ago • 9 comments

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.

jules-ch avatar Jul 20 '22 20:07 jules-ch

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.

jules-ch avatar Jul 22 '22 13:07 jules-ch

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

kandersolar avatar Jul 22 '22 14:07 kandersolar

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.

jules-ch avatar Jul 25 '22 17:07 jules-ch

@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.

jules-ch avatar Jul 25 '22 17:07 jules-ch

@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.

kandersolar avatar Jul 26 '22 01:07 kandersolar

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.

wholmgren avatar Jul 26 '22 15:07 wholmgren

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

kandersolar avatar Jul 26 '22 16:07 kandersolar

Seems to work. I did the following

  1. check out branch
  2. activate conda env with existing pvlib dev installation
  3. import pvlib. pvlib.__version__ returns 0.7.2 (that's what was checked out when I last ran pip install -e .)
  4. 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)
  5. exit ipython
  6. pip install -e . Successfully uninstalled pvlib-0.7.2 Successfully installed pvlib-0.9.2.dev16+g3bcd63e
  7. import pvlib; pvlib.__version__ '0.9.2.dev16+g3bcd63e'
  8. 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.

wholmgren avatar Jul 29 '22 22:07 wholmgren

@jules-ch can you resolve the merge conflicts?

wholmgren avatar Aug 12 '22 18:08 wholmgren

I've rebased, should be good now @wholmgren

jules-ch avatar Aug 16 '22 08:08 jules-ch

FYI CI is failing because I'm ignoring coverage on version.py & it is reducing code coverage which is making CI fails.

jules-ch avatar Aug 16 '22 08:08 jules-ch

thanks @jules-ch! let's see how it goes...

wholmgren avatar Aug 16 '22 15:08 wholmgren

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?

wholmgren avatar Aug 16 '22 17:08 wholmgren

Shoud not be too difficult

jules-ch avatar Aug 17 '22 15:08 jules-ch

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!

kandersolar avatar Aug 18 '22 01:08 kandersolar