Use pep517/518 build system for modern build isolation #1495

jules-ch · 2022-07-20T20:26:21Z

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~~

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

kandersolar

Thanks for these excellent PRs @jules-ch! Can you please also remove the versioneer.py and _version.py references in .lgtm.yml, .stickler.yml, and MANIFEST.in?

Also, any idea why a bunch of top-level files are being included in the dist bundles? Compare the build logs for master versus this PR.

pvlib/version.py

kandersolar · 2022-07-21T13:34:22Z

setup.cfg

-versionfile_build = pvlib/_version.py
-tag_prefix = v
-parentdir_prefix = pvlib-python-
-
 [aliases]
 test=pytest

 [flake8]
 max-line-length = 79
 ignore = E201, E241, E226, W503, W504


In case anyone else is like me and wonders if we can move this flake8 config to pyproject.toml and nix setup.cfg altogether, flake8 doesn't support pyproject.toml and it doesn't seem like it will anytime soon: https://github.com/PyCQA/flake8/issues?q=is%3Aissue+pyproject.toml

docs/sphinx/source/whatsnew/v0.9.2.rst

kandersolar · 2022-07-21T15:01:07Z

pyproject.toml

+requires = ["setuptools>=45", "wheel", "setuptools_scm>=6.2"]
+build-backend = "setuptools.build_meta"
+
+[tool.setuptools_scm]


The version string for non-tagged commits using this method references the next pvlib version instead of the previous. For example this PR branch currently references 0.9.2:

master: pvlib-0.9.1+18.g3f397ed4

this PR: pvlib-0.9.2.dev16+g2c4fbe3f

The number of commits since the last tag is .devN instead of +N. "dirty" builds are also a bit different: pvlib-0.9.2.dev16+g2c4fbe3f.d20220721 vs pvlib-0.9.1+18.g3f397ed4.dirty.

Not sure I see either of these as a problem, just pointing it out.

jules-ch · 2022-07-22T13:28:25Z

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.

kandersolar · 2022-07-22T14:16:54Z

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

jules-ch · 2022-07-25T17:28:43Z

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 · 2022-07-25T17:32:11Z

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

kandersolar · 2022-07-26T01:38:57Z

@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

I think I'm okay with these changes, assuming we follow up elsewhere on the MANIFEST excludes. Would definitely be good for another maintainer to take a look though.

wholmgren · 2022-07-26T15:46:03Z

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 #580? I am a -1 on removing anything from the distribution at this time, so let's take up that discussion somewhere else.

kandersolar · 2022-07-26T16:02:36Z

I thought #1486 (comment) with this pattern. That would destroy my workflow.

I think editable installs are only a problem if you ditch setup.py altogether (pypa/setuptools#2816 (comment)), 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 #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 :)

wholmgren · 2022-07-29T22:02:23Z

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 ran pip 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.

wholmgren · 2022-08-12T18:10:55Z

@jules-ch can you resolve the merge conflicts?

- 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 - Update remove versioneer files from .lgtm.yml & .stickler.yml - Remove include from MANIFEST.IN (already included with setuptools-scm)

jules-ch · 2022-08-16T08:29:59Z

I've rebased, should be good now @wholmgren

jules-ch · 2022-08-16T08:41:57Z

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

wholmgren · 2022-08-16T15:19:23Z

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

wholmgren · 2022-08-16T17:35:26Z

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 matplotlib/matplotlib#18971. And mpl now uses a function in __init__.py as a workaround. Thoughts on a follow up PR for that?

jules-ch · 2022-08-17T15:20:49Z

Shoud not be too difficult

kandersolar · 2022-08-18T01:45:10Z

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!

jules-ch force-pushed the pep517-build branch from e252a5a to 2c4fbe3 Compare July 20, 2022 20:42

kandersolar reviewed Jul 21, 2022

View reviewed changes

jules-ch force-pushed the pep517-build branch 2 times, most recently from 2c4fbe3 to 5897132 Compare July 25, 2022 17:26

jules-ch force-pushed the pep517-build branch from 5897132 to 3bcd63e Compare July 25, 2022 17:30

kandersolar added this to the 0.9.2 milestone Jul 26, 2022

kandersolar mentioned this pull request Jul 26, 2022

v0.9.2 release plan #1506

Closed

9 tasks

kandersolar approved these changes Jul 26, 2022

View reviewed changes

wholmgren approved these changes Jul 29, 2022

View reviewed changes

jules-ch force-pushed the pep517-build branch from 3bcd63e to 750a9bc Compare August 16, 2022 08:28

wholmgren merged commit b2eca71 into pvlib:master Aug 16, 2022

wholmgren added installation development_workflow labels Aug 16, 2022

wholmgren mentioned this pull request Aug 16, 2022

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

Merged

4 tasks

kandersolar mentioned this pull request Aug 31, 2022

Fix ASV benchmark configuration issues; don't run benchmarks on non-master fork branches #1542

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pep517/518 build system for modern build isolation #1495

Use pep517/518 build system for modern build isolation #1495

jules-ch commented Jul 20, 2022 •

edited

Loading

kandersolar left a comment

kandersolar Jul 21, 2022

kandersolar Jul 21, 2022

jules-ch commented Jul 22, 2022

kandersolar commented Jul 22, 2022

jules-ch commented Jul 25, 2022

jules-ch commented Jul 25, 2022 •

edited

Loading

kandersolar commented Jul 26, 2022

kandersolar left a comment

wholmgren commented Jul 26, 2022

kandersolar commented Jul 26, 2022

wholmgren commented Jul 29, 2022

wholmgren commented Aug 12, 2022

jules-ch commented Aug 16, 2022

jules-ch commented Aug 16, 2022

wholmgren commented Aug 16, 2022

wholmgren commented Aug 16, 2022

jules-ch commented Aug 17, 2022

kandersolar commented Aug 18, 2022

Use pep517/518 build system for modern build isolation #1495

Use pep517/518 build system for modern build isolation #1495

Conversation

jules-ch commented Jul 20, 2022 • edited Loading

kandersolar left a comment

Choose a reason for hiding this comment

kandersolar Jul 21, 2022

Choose a reason for hiding this comment

kandersolar Jul 21, 2022

Choose a reason for hiding this comment

jules-ch commented Jul 22, 2022

kandersolar commented Jul 22, 2022

jules-ch commented Jul 25, 2022

jules-ch commented Jul 25, 2022 • edited Loading

kandersolar commented Jul 26, 2022

kandersolar left a comment

Choose a reason for hiding this comment

wholmgren commented Jul 26, 2022

kandersolar commented Jul 26, 2022

wholmgren commented Jul 29, 2022

wholmgren commented Aug 12, 2022

jules-ch commented Aug 16, 2022

jules-ch commented Aug 16, 2022

wholmgren commented Aug 16, 2022

wholmgren commented Aug 16, 2022

jules-ch commented Aug 17, 2022

kandersolar commented Aug 18, 2022

jules-ch commented Jul 20, 2022 •

edited

Loading

jules-ch commented Jul 25, 2022 •

edited

Loading