Assorted cleanups around recording the version number

[dnicolodi]

See comments to #690

[rgommers]

Thanks @dnicolodi! LGTM locally and GHA changes look right, so let's give them a go.

[tacaswell]

Can __version__ be restored? This breaks building at least contourpy and I suspect there are others.

[tacaswell]

It will likely also break sklearn's build.

[dnicolodi]

How does it break building contourpy? meson-python does not have a public API, importing mesonpy is not supported, let alone using symbols from the module. Why does contourpy need to check the version of meson-python? And why does it need to do it looking at mesonpy.__version__?

[tacaswell]

https://github.com/contourpy/contourpy/blob/1429fbfd1ba5edcb9d32dd0efd1ec3d8aa2e122b/lib/contourpy/util/meson.build#L28-L34

https://github.com/scikit-learn/scikit-learn/blob/f106177572c031d0b5c574f02a139a6a050b9343/sklearn/meson.build#L48-L58

are the usages. Both look reasonable things to want to do to me (one looks like it is logging the version and one looks like it is doing an extra version check), but I'm not actively involved in either project (other than trying to build them) so can not really defend the code or agree it should be changed.

[dnicolodi]

meson-python has never documented the mesonpy.__version__ symbol. Using it is the same as using any other undocumented internal: it is going to break soon or later and the user is going to keep both pieces without any right of complaining. There is a proper way for checking the installed version of Python packages: https://packaging.python.org/en/latest/discussions/versioning/#accessing-version-information-at-runtime

[rgommers]

That code in contourpy and scikit-learn should be changed, but at the same time if it's a real problem because those projects need more time to fix things up, we should put __version__ back temporarily.

Cc @ianthomas23 for ContourPy and @lesteve for scikit-learn. Could you please have a look at this? I'd recommend dropping the version check completely (it's a bit ugly/slow, and I've never seen a real need for doing this check in NumPy, SciPy or elsewhere), but if you do want a version check then importlib.metadata.version('meson-python') is the idiomatic way to do it.

[dnicolodi]

That code in contourpy and scikit-learn should be changed, but at the same time if it's a real problem because those projects need more time to fix things up, we should put __version__ back temporarily.

__version__ has been removed only on the main branch. If someone is testing with a git checkout of meson-python they can as well test with a version of these projects that fixes the version check.

[eli-schwartz]

The version check in scikit-learn looks strange as it is "only needed when going through pip" but pip already checks that for you based on build-system.requires?

The contourpy stuff isn't a version check. It's adding a contourpy/util/_build_config.py module which allows you to print at runtime, the versions of meson, mesonpy, pybind11, etc that were used as well as various configure options and even the source and build directories.

It's true this could probably be done via importlib.metadata instead.

[ianthomas23]

Yes I can change it in ContourPy, no problem. As some have mentioned here it is not checking the version, it is recording the version used to build so I can check it at runtime. Such information is very useful, e.g. just last week to see what wheels have been released with the problematic pybind11 2.13.1 (pybind/pybind11#5420).

[lesteve]

I'd recommend dropping the version check completely (it's a bit ugly/slow, and I've never seen a real need for doing this check in NumPy, SciPy or elsewhere), but if you do want a version check then importlib.metadata.version('meson-python') is the idiomatic way to do it.

Using importlib.metadata.version('meson-python') seems OK enough.

One of the reason we wanted to check the meson-python version was that there were bug fixes in meson-python 0.16, without them some of the scikit-learn tests fail with a failure that is not easy to diagnose. This is probably related to improvements in meson-python editable install #569.

The version check in scikit-learn looks strange as it is "only needed when going through pip" but pip already checks that for you based on build-system.requires?

pip install --no-build-isolation does not check build requirements by default, you have to use --check-build-dependencies. Our install instructions follow this pattern:

1. install dependencies (numpy, scipy, etc ...)
2. build scikit-learn with pip install --no-build-isolation <more_arguments> so that you use dependencies installed in step 1.

The thing is that there are cases where you don't want to use --check-build-dependencies. Right now we have numpy>=2 in build.system.requires to make sure wheels are built with numpy>=2 but we support numpy<2 and you should be allowed to build fine with numpy<2.

[eli-schwartz]

The thing is that there are cases where you don't want to use --check-build-dependencies. Right now we have numpy>=2 in build.system.requires to make sure wheels are built with numpy>=2 but we support numpy<2 and you should be allowed to build fine with numpy<2.

Maybe you should simply not have numpy>=2 in build-system.requires and instead just guarantee that such a version is used in official PyPI wheel release processes? I recall discussing this¹ with @rgommers "somewhere on github" as one of the hot topics for packaging native code (though I cannot find any explicit mention of it on https://pypackaging-native.github.io/ as it happens) -- but IIRC you can e.g. use constraint files for this as long as you have a dedicated process for releasing those wheels.

Footnotes

1. the dual role of build-system.requires as both "this is what you need to build a wheel" and "I need to build wheels in a specific way for the sake of distributing them publicly on PyPI" ↩

[rgommers]

Maybe you should simply not have numpy>=2 in build-system.requires and instead just guarantee that such a version is used in official PyPI wheel release processes?

That will be fragile with from-source builds. E.g., if a package depending on scikit-learn is built with pip install <pkg-name> --no-binary and that package wants numpy<2, then a scikit-learn that isn't ABI-compatible with numpy 2.0 will end up in the pip cache for that user (since build isolation doesn't "cascade" to dependencies), and it'll lead to hard to debug failures at runtime in future venvs. Scikit-learn is following the advice at https://numpy.org/devdocs/dev/depending_on_numpy.html#numpy-2-0-specific-advice here. You're not wrong in that adding >=2.0 in metadata is also not 100% correct, but it's the lesser of two evils given the limitations of Python packaging.

I think wanting to check a version number is reasonable if there were problems with editable installs. I'd just switch it to importlib.metadata.version.

[tacaswell]

Using __version__ to get the run-time version does have a long conventional usage and as shown by these cases is what people expect to reach for.

[dnicolodi]

There is a very very long thread on Python discourse about the topic. It resulted in these guidelines https://packaging.python.org/en/latest/discussions/versioning/#accessing-version-information-at-runtime where it is clearly stated that relying on __version__ being available is not good practice. I don't want to argue on the principle, but I will not bringing __version__ for meson-python either.

[eli-schwartz]

That is explicitly not a guideline, only a discussion.

And even so, the page doesn't suggest you shouldn't use __version__, only that not all packages agree to provide it and you should be aware that depending on the package you cannot use it.

But for a package that absolutely did provide it, the discussion you linked to trivially reads as "okay, good job -- you verified that meson-python does provide it, therefore you may rely on it" which is in fact what at least two projects went ahead and did.

I see nowhere in your appeal to authority, that the authority says what you say it does. We are left, instead, with a library-level API change following a common convention that people were relying on, which has now switched to a different and one-way-incompatible (subset) convention and the need for people to adapt.

[ianthomas23]

Please can we have __version__ restored temporarily? It only needs to be until contourpy and scikit-learn have made releases with the necessary changes. It seems like a small price to pay to avoid potentially annoying downstream users and packagers.

If it helps, I am happy to take on the responsibility of a future PR to remove it here, liaising with @lesteve to ensure we get the timing correct for all parties.