8000 MAINT: Remove versioneer by stefanv · Pull Request #24196 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

MAINT: Remove versioneer #24196

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 30 commits into from
Aug 11, 2023
Merged

MAINT: Remove versioneer #24196

merged 30 commits into from
Aug 11, 2023

Conversation

stefanv
Copy link
Contributor
@stefanv stefanv commented Jul 16, 2023

This removes versioneer, as per the discussion on gh-23981

This is the simplest approach I know of. It does not cover all use cases. E.g., it does not bundle the git hash with built wheels (so version number would be 2.0.0dev0 there). But it does work with common developer setups: meson build or editable installs.

If it is deemed necessary to have the git hash also in wheels etc., then we'd need to add one additional step: generate the version number into a file that gets distributed alongside numpy (the approach SciPy takes).

FWIW, it looks like much of this used to exist in NumPy, but was ripped out in favor of versioneer. But versioneer is a LOT of black box code that, as far as I can tell, adds little but obscurity.

This is very much a draft PR, and is aimed as a conversation starter; there are several ways to do this, and I'll adjust in response to the conversation.

@rgommers
Copy link
Member

Thanks for getting the ball rolling here Stefan!

Removing versioneer is good, and the wheel filename is fine as 2.0.0.dev0 (at least it works just fine for SciPy, and there's no real compelling reason for the hash to be in the filename, nor is it easy to do).

I haven't looked in detail at the rest yet. The SciPy approach does keep the hash in np.__version__, and works reliably. I haven't looked at the pros and cons compared to what you did here. I assume that that is largely coming from scikit-image?

@charris
Copy link
Member
charris commented Jul 16, 2023

Note that versioneer uses git describe, most of the complication comes from supporting multiple version formats. We only use something like 25 lines of the code :) We do want the numpy module stored in the wheels to have complete version names for the weekly builds, the extra information is only missing from the wheel names because meson doesn't support it. I'd just copy scipy here.

@charris
Copy link
Member
charris commented Jul 16, 2023

I do want to keep making development tags, whether the version machinery uses them or not. They are handy for marking branch points, finding those points is a tricky thing to do without them. They are also useful for generating the changelogs.

@stefanv
Copy link
Contributor Author
stefanv commented Jul 16, 2023

I haven't looked in detail at the rest yet. The SciPy approach does keep the hash in np.__version__, and works reliably. I haven't looked at the pros and cons compared to what you did here. I assume that that is largely coming from scikit-image?

That's right---I just used that one because I was familiar with it. But after hashing it out here, I may go back and update that approach to match.

@charris
Copy link
Member
charris commented Aug 3, 2023

@stefanv Soft ping. Looks like we will need this for 1.26.

Copy link
Member
@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

I had a look at the state of this PR. A couple of thoughts:

  • the semi-public numpy.version submodule is removed; that's fine for 2.0, but probably not for 1.26.x
  • moving run_command into the project() call in meson.build causes some changes in behavior:
    • when installing from sdist, np.__version__ loses the hash behind .dev0,
    • the run_command is no longer retriggered when running (e.g.) spin build on new commits, which means that __version__ goes out of date on dev builds. I'll note that this is not only a negative, there's an upside too - not re-running that allows rebuilds to be true no-ops if no files changed (not interesting from a build time perspective, but it has the effect of not re-invoking ninja and hence not clobbering .ninja_log which is good for profiling)
    • the git hash ends up in the wheel filename (also has pros and cons; it's more explicit but also very long and we'll have to manually delete nightlies rather than overwriting them)
  • make the version dynamic in pyproject.toml is the biggest downside of this approach. fully static metadata is valuable in multiple ways, so it's a big price to pay and I'd like to avoid it. E.g., the prepare_metadata_for_build_wheel build hook is now definitely going to invoke a full build, even if meson-python would implement the optimization of directly returning the metadata if available.

@stefanv
Copy link
Contributor Author
stefanv commented Aug 4, 2023

Thanks, Ralf! I'll try to address these concerns soon.

@charris
Copy link
Member
charris commented Aug 8, 2023

We are going to need this in a couple of days.

@stefanv
Copy link
Contributor Author
stefanv commented Aug 8, 2023

I restored numpy.version, but it's not clear to me what the difference between version.__version__ and version.version is supposed to be.

I've moved the version string into pyproject.toml (i.e. version is no longer dynamic); everything else derives from that version.

I'm not sure why run_command does not trigger on new commits; the way Ralf describes it, it is clearly meant to work this way. But I don't know if that behavior is ideal for our use-case. I left it as-is for now.

The hash in the wheel is intentional, but can easily be changed as desired; let me know.
My gut feeling is that you want the git version in the wheel filename, otherwise it is hard to know which nightly wheel you are dealing with.

EDIT: Since version is static in pyproject.toml, the git hash is not included in the wheel filename.

@rgommers
Copy link
Member
rgommers commented Aug 9, 2023

I restored numpy.version, but it's not clear to me what the difference between version.__version__ and version.version is supposed to be.

It's the same; numpy.version is one of those things that may never have been intentionally public. That's why we can simply delete it in 2.0. The way you restored it now seems fine to me.

I'm not sure why run_command does not trigger on new commits; the way Ralf describes it, it is clearly meant to work this way.

I think it's because it's inside project() now, and hence it's part of the configure stage of the build only and doesn't end up being triggered on regular rebuilds. gitversion.py does end up in build.ninja, but all the way at the end in a REGENERATE_BUILD rule:

build build.ninja: REGENERATE_BUILD ../meson.build /home/rgommers/mambaforge/envs/numpy-dev/bin/python3.9 ../numpy/_build_utils/gitversion.py ../meson_options.txt ../numpy/meson.build ../numpy/__config__.py.in ../numpy/core/meson.build ../numpy/core/code_generators/verify_c_api_version.py ../numpy/core/config.h.in ../numpy/core/include/numpy/_numpyconfig.h.in ../numpy/core/npymath.ini.in ../numpy/core/mlib.ini.in ../numpy/core/include/meson.build ../numpy/fft/meson.build ../numpy/linalg/meson.build ../numpy/random/meson.build meson-private/coredata.dat
 pool = console

build reconfigure: REGENERATE_BUILD PHONY
 pool = console

build ../meson.build /home/rgommers/mambaforge/envs/numpy-dev/bin/python3.9 ../numpy/_build_utils/gitversion.py ../meson_options.txt ../numpy/meson.build ../numpy/__config__.py.in ../numpy/core/meson.build ../numpy/core/code_generators/verify_c_api_version.py ../numpy/core/config.h.in ../numpy/core/include/numpy/_numpyconfig.h.in ../numpy/core/npymath.ini.in ../numpy/core/mlib.ini.in ../numpy/core/include/meson.build ../numpy/fft/meson.build ../numpy/linalg/meson.build ../numpy/random/meson.build meson-private/coredata.dat: phony 

For comparison, in SciPy's it's a regular "custom command" that's run on every ninja invocation:

build scipy/version.py: CUSTOM_COMMAND ../scipy/../tools/version_utils.py | /home/rgommers/mambaforge/envs/scipy-dev/bin/python3.10 PHONY
 COMMAND = /home/rgommers/mambaforge/envs/scipy-dev/bin/python3.10 ../scipy/../tools/version_utils.py --source-root ..
 description = Generating$ scipy/generate-version$ with$ a$ custom$ command

I pushed one commit with a fix. To illustrate the problem with not retriggering the git commit hash inclusion, I then ran spin docs which ends with this issue:

make: Entering directory '/home/rgommers/code/numpy/doc'
installed numpy 927d2a807d != current repo git version 'a36f96934'
use "make dist" or "GITVER=927d2a807d make html ..."

This is a problem for setup.py builds too, and I was quite happy that it went away in scipy after the switch to Meson, since it trips up a lot of new contributors (and even maintainers).

Let me see if I can fix this by running the command in both project() and outside of it.

My gut feeling is that you want the git version in the wheel filename, otherwise it is hard to know which nightly wheel you are dealing with.

It really has not been a problem for scipy, so I don't care too much either way right now. I see it as a pretty minor thing that somehow people start to care about a lot and do a ton of effort and ugly hacks (looking at you versioneer) for. It should be supported natively in both meson and meson-python at some point, but isn't right now. Here's an open feature request: mesonbuild/meson-python#159 (comment).

@rgommers
Copy link
Member
rgommers commented Aug 9, 2023

If you run python -m build, which builds a wheel via the sdist, the git hash is getting lost right now. The deletion of the meson.add_dist_script part in the top-level meson.build file is the cause. If we restore that, I think this PR will be in good shape.

EDIT: there's more to it than that. The version also has to then be loaded like so: https://github.com/scipy/scipy/blob/fe05d2e6c36c46d0afddba9f10f715979a5b7fd8/tools/version_utils.py#L21-L26. We can also decide to simply not care about having the git hash in builds that go via sdist.

@rgommers
Copy link
Member
rgommers commented Aug 9, 2023

Ah, here is where things break down further: version.py is generated in-tree rather than in the build directory. This is kinda breaking the rules, Meson is fully out of place by design. The only reason this works is because the run_command call is tucked away inside project() I think, so the file is created before Meson first checks if it exists. I'm not sure if this is guaranteed to continue working.

@eli-schwartz is using run_command inside project() and then generating a file inside the source tree for inclusion into py.install_sources() future-proof?

@eli-schwartz
Copy link

Yes, it is future proof as long as the file is created before py.install_sources() is called (and naturally, things running inside project() are the very height of "XXX before anything else".

Note that anyone who requires out of source builds because e.g. they mount the source directory read-only, is going to have problems. i.e. out of source builds aren't just a matter of theory and good behavior, but have a practical side.

I guess they already had the issue with setuptools and have existing workarounds though. ;) They would probably just make a copy of the entire source tree into the build worktree, which is meh but does functionally work.

@rgommers
Copy link
Member
rgommers commented Aug 9, 2023

Thanks @eli-schwartz!

@stefanv as far as I can tell we have two choices here:

  1. run the gitversion.py script only inside project(), once. This means __version__ will be wrong on dev builds and the git hash will be empty when going via sdist. And we should probably just delete the check in docs/Makefile then to fix how spin docs works.
  2. do it the way SciPy does it, with gitversion.py not run inside project() but as a regular custom_command. As a result, the build log will show Project version: 2.0.0.dev0 rather than including +git20230809.783e3c723.

Either way works for me, and it seems both ways are future-proof.

@charris charris changed the title Remove versioneer MAINT: Remove versioneer Aug 9, 2023
@stefanv
Copy link
Contributor Author
stefanv commented Aug 9, 2023

I don't have a strong feeling about it; we could also make spin rebuild when the git hash changes, but of course it's better to have Meson handle that.

@stefanv
Copy link
Contributor Author
stefanv commented Aug 9, 2023

I wonder, would it hurt to do both options 1 and 2?

@stefanv
Copy link
Contributor Author
stefanv commented Aug 10, 2023

Looks like that Travis-CI job is failing on main as well.

@stefanv
Copy link
Contributor Author
stefanv commented Aug 10, 2023

f3acaca breaks my doc build locally, but seems to work on CI 🤔

@mattip
Copy link
Member
mattip commented Aug 10, 2023

The workflow on CircleCI first installs NumPy in a virtualenv, then uses that virtualenv to build the docs in a different directory. Is that similar to your workflow?

. venv/bin/activate
cd doc
SPHINXOPTS="-j2 -n" make -e html || echo "ignoring errors for now, see gh-13114"

@rgommers
Copy link
< 8000 /details>
Member

I wonder, would it hurt to do both options 1 and 2?

I think yes, that is possible.

f3acaca breaks my doc build locally, but seems to work on CI thinking

git rev-parse --short HEAD has given length 9 output for numpy for a long time I believe. --short isn't guaranteed to give exactly 9 chars though it seems; it's 7 by default and then it grows as needed to make the short hash unique. Do you have a ton of branches lying around perhaps?

Copy link
Member
@mattip mattip left a comment

Choose a reason for hiding this comment

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

I think we should be explicit in how many characters to use in the git hash formatting.

@stefanv
Copy link
Contributor Author
stefanv commented Aug 11, 2023

I think this is close. I fixed the problem with the docs Makefile (it didn't correctly detect the case where the git repo was missing).

Now, version.py is installed when it pre-exists (only in the sdist), otherwise it is generated during build. This means that the git hash is always available, whether dev or release, whether building from sdist or git checkout.

@rgommers
Copy link
Member

Great! I'd like to merge the large SIMD support PR first, and then I'll try to have a final look and merge this one after that.

Copy link
Member
@rgommers rgommers left a comment

Choose a reason for F438 hiding this comment

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

This is looking pretty good. I found two more issues that I pointed out and pushed fixes for. If CI is happy, we can squash-merge this.

@@ -518,7 +519,7 @@ def setup_package():
classifiers=[_f for _f in CLASSIFIERS.split('\n') if _f],
platforms=["Windows", "Linux", "Solaris", "Mac OS-X", "Unix"],
test_suite='pytest',
version=versioneer.get_version(),
version=VERSION,
Copy link
Member

Choose a reason for hiding this comment

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

This is a change, because VERSION is defined as

VERSION = '{}.{}.{}'.format(MAJOR, MINOR, MICRO)

and versioneer.get_version() returns the version including git hash. And it results in:

$ python setup.py sdist
numpy-2.0.2.0.0.dev0+git20230811.8287648.tar.gz

easy enough to fix it looks like, by using FULLVERSION instead.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, that didn't do it:

ERROR: Could not find a version that satisfies the requirement numpy (from versions: 2.0.0.dev0-git20230811.6c254b42.0.0.dev0-git20230811.6c254b4)

I'll do some more testing locally. I really want to merge this PR, but this seems like a blocker. Shouldn't be too difficult to fix.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, locally I get sdist and wheel that are named sensibly:

numpy-2.0.0.dev0+807.gf015fe226-cp39-cp39-linux_x86_64.whl

Copy link
Member

Choose a reason for hiding this comment

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

Okay, whatever, CI was green so I'm reverting to what it was. I really do get numpy-2.0.2.0.0.dev0+git20230811.6aff0bc locally, no idea what is going on there. But we're switching all jobs away from setup.py asap, so maybe it doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been building wheels using python -m build --sdist. Is it necessary to support another pathway?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think that's a known issue indeed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another line in setup.py that doesn't seem quite right:

def get_docs_url():
    if 'dev' in VERSION:
        return "https://numpy.org/devdocs"

Another reason I think that your FULLVERSION patch is correct.

Copy link
Member

Choose a reason for hiding this comment

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

Ah well, let's leave that for a follow-up. I'm kinda running out of steam and have some non open source plans for the weekend.

Copy link
Member

Choose a reason for hiding this comment

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

The use_wheel job is happy to install this wheel, I guess it's a valid version number:

+ pip install --pre --no-index --upgrade --find-links=. numpy
Looking in links: .
Processing ./numpy-2.0.2.0.0.dev0+git20230811.f30bdab-cp39-cp39-linux_x86_64.whl
Installing collected packages: numpy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2.0.2.0.0 🤪

[skip azp] [skip cirrus] [skip circle] [skip travis]
@rgommers rgommers merged commit be896c9 into numpy:main Aug 11, 2023
@rgommers
Copy link
Member

Everything is happy, in it goes. Thanks @stefanv!

charris pushed a commit to charris/numpy that referenced this pull request Aug 12, 2023
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 12, 2023
charris pushed a commit to charris/numpy that referenced this pull request Nov 11, 2023
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
@TimotheusBachinger TimotheusBachinger mentioned this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0