-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
MAINT: Remove versioneer #24196
Conversation
Thanks for getting the ball rolling here Stefan! Removing I haven't looked in detail at the rest yet. The SciPy approach does keep the hash in |
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. |
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. |
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. |
@stefanv Soft ping. Looks like we will need this for 1.26. |
There was a problem hiding this 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 theproject()
call inmeson.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)
- when installing from sdist,
- 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.
Thanks, Ralf! I'll try to address these concerns soon. |
We are going to need this in a couple of days. |
I restored I've moved the version string into I'm not sure why The hash in the wheel is intentional, but can easily be changed as desired; let me know. EDIT: Since version is static in pyproject.toml, the git hash is not included in the wheel filename. |
It's the same;
I think it's because it's inside 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 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
This is a problem for Let me see if I can fix this by running the command in both
It really has not been a problem for |
If you run 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. |
Ah, here is where things break down further: @eli-schwartz is using |
Yes, it is future proof as long as the file is created before 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. |
Thanks @eli-schwartz! @stefanv as far as I can tell we have two choices here:
Either way works for me, and it seems both ways are future-proof. |
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. |
I wonder, would it hurt to do both options 1 and 2? |
Looks like that Travis-CI job is failing on main as well. |
f3acaca breaks my doc build locally, but seems to work on CI 🤔 |
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?
|
I think yes, that is possible.
|
There was a problem hiding this 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.
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. |
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. |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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]
Everything is happy, in it goes. Thanks @stefanv! |
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
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.