Skip to content

Switch to setuptools_scm. #18971

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

Merged
merged 1 commit into from
Mar 25, 2021
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 18, 2020

Per yesterday's dev call.

A few noteworthy points:

  • The contents in the sdist now exactly match what git archive would
    include; this avoids having to additionally keep track of things in
    MANIFEST.in (as noted in contributing.rst).

  • The __version__ of an editable install is no longer recomputed at
    each import (as was done with versioneer, but only whenever the
    project is (re)installed; this can actually save quite a bit of time
    when working with many editable installs as each version recomputation
    requires shelling out a subprocess. (If really wanted we could
    keep the old behavior by optionally adding a runtime dependency
    on setuptools_scm and calling setuptools_scm.get_version in
    matplotlib/__init__.py.)

Also note that I don't personally use the tarballs auto-created by github
or setuptools_scm_git_archive, so I'm not sure I got the config correct
on that side, I just copied what the docs say...

attn @tacaswell

Feel free to push to this PR; I'm mostly just posting an old patch I had...

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@anntzer anntzer added the Build label Nov 18, 2020
@dopplershift
Copy link
Contributor

So the only downside to this approach is that for development installs, the version only updates when setup.py is re-run. On metpy, rather than have setuptools_scm write to a version file, I opted to try to use setuptools_scm to get the live version for dev installs fallback to use importlib.metadata to get the version for installed versions.

Matplotlib is different since it has compiled code and setup.py is needed to be re-run more regularly--though I bet python setup.py build_ext --inplace doesn't update the version either.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 18, 2020

So the only downside to this approach is that for development installs, the version only updates when setup.py is re-run. On metpy, rather than have setuptools_scm write to a version file, I opted to try to use setuptools_scm to get the live version for dev installs fallback to use importlib.metadata to get the version for installed versions.

This can easily be fixed: in matplotlib/__init__.py, we can check if we are in a git repo ((Path(__file__).parent / "../../.git").exists()) and if so call setuptools_scm.get_version() instead of importing the version from _version.py. However, as described above, I have intentionally chosen against doing so: my normal Python working environment basically consists of a large number of personal projects all editably installed, and I found out that my script startup times were significantly slowed down by the large number of git describe that were run again and again every time I ran a script (also note that things are even worse on Windows, where subprocesses are even more costly to start). So nowadays I have chosen to accept the slightly out-of-syncness for my personal projects. For Matplotlib I'm fine either way.

Matplotlib is different since it has compiled code and setup.py is needed to be re-run more regularly--though I bet python setup.py build_ext --inplace doesn't update the version either.

We already hook the build_ext command to customize it, it's "probably" not too hard to customize it further to ensure it updates _version.py.

@timhoffm
Copy link
Member

Would it be reasonable to update the version in a dev-install at runtime if setuptools_scm is available? - I assume performance-wise that's the same as versioneer currently.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 18, 2020

Yes (that's basically the alternative I propose above).

@timhoffm
Copy link
Member

Then I'd go with that. Feels more correct.

We can always introduce an environment variable to deactivate the lookup should the need arise to do so.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 18, 2020

One point I forgot to ask is: do we want to add a runtime dependency on setuptools_scm (for what is a fairly rare use case)? or we can also just silently ignore the case of editable installs with setuptools_scm not present (unfortunately, I don't think "runtime dependency only for editable installs" is a thing).

@dopplershift
Copy link
Contributor

setuptools_scm will become a build-time dependency anyway--which means it will be available for all useful editable installs.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 21, 2020

No, setup_requires are not available at runtime (they're only temporarily made present in the build environment via .eggs). This can easily be checked by installing something into a temporary environment:

python -mvenv /tmp/testenv
source /tmp/testenv/bin/activate
mkdir /tmp/foo; cd /tmp/foo
echo 'from setuptools import setup; setup(name="foo", use_scm_version=True, setup_requires=["setuptools_scm>=4"])' > setup.py
git init . && git add . && git commit -m "Initial commit." && git tag v0.0
pip install -ve .
pip list

pip list shows that foo has been installed into the current environment, but not setuptools_scm (as can also be checked by trying to import it.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 21, 2021

Discussed on call today. Decision:
Make setuptools_scm an install_requires but only for dev installs (by dynamically manipulating install_requires in setup.py depending e.g. on checking the filesystem). In __init__.py, do generate the up-to-date version from setuptools_scm, but gate that behind a module-level __getattr__ to avoid the import penalty.

Likely I won't do this in the 3.4 time frame. edit: actually that was easy :)

@timhoffm timhoffm added this to the v3.5.0 milestone Jan 22, 2021
@anntzer anntzer force-pushed the setuptools-scm branch 3 times, most recently from b1d250d to 5d9a362 Compare January 23, 2021 20:22
`versioneer <https://github.com/warner/python-versioneer>`__
which uses a format string in
.. [#] The tarball that is provided by GitHub is produced using `git archive`_.
We use setuptools_scm_ which uses a format string in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
We use setuptools_scm_ which uses a format string in
We use `setuptools_scm`_ which uses a format string in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick test shows this isn't needed in rst?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☑️ I've learned something new.

While the Sphinx reStructuredText Primer only mentions the variant with backticks, the specification indeed allows to leave them out. (Could somebody donate a tiny bit of CSS to the docutils folks? - The no-CSS style of the specs is quite unreadable, which is why I try to avoid going there.)

@tacaswell
Copy link
Member

@anntzer I took the liberty of rebasing.

@timhoffm
Copy link
Member

@tacaswell Is there a reason that you did not merge?

@tacaswell
Copy link
Member

@timhoffm I think I approved, did the rebase, and then did not come back to it when CI passed. I'll rebase again and keep an eye on it this time!

A few noteworthy points:

- The contents in the sdist now exactly match what `git archive` would
  include; this avoids having to additionally keep track of things in
  MANIFEST.in (as noted in contributing.rst).

- The `__version__` of an editable install is no longer recomputed at
  each import (as was done with `versioneer`, but only whenever the
  project is (re)installed; this can actually save quite a bit of time
  when working with many editable installs as each version recomputation
  requires shelling out a subprocess.  (If really wanted we could
  keep the old behavior by optionally adding a runtime dependency
  on setuptools_scm and calling `setuptools_scm.get_version` in
  `matplotlib/__init__.py`.)
@tacaswell tacaswell merged commit 222bd9a into matplotlib:master Mar 25, 2021
@anntzer anntzer deleted the setuptools-scm branch March 26, 2021 03:09
@pllim
Copy link

pllim commented May 10, 2021

Hello. I see that this is merged but astropy is still picking up git+https://github.com/matplotlib/matplotlib.git#egg=matplotlib as 3.4.2.postN+gNNNNNN. Is this expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants