BLD Check build dependencies in meson.build #28721

lesteve · 2024-03-28T16:13:49Z

I recently realised that we don't check build dependencies version when building with Meson. I was getting some weird Cython errors until I realised that I had 3.0.8 in my env.

I added the necessary logic in sklearn/meson.build to do something similar as what we do in setup.py.

This is a bit unfortunate that the logic is a bit duplicated between meson.build and setup.py but I think it is acceptable. I reused the sklearn/_min_dependencies.py script to avoid hard-coding minimum versions in yet another place.

I originally thought we could run the same Python script file from meson.build and setup.py, except that there are some complications:

if we want to reuse packaging version comparison we need to to import sklearn.external._packaging.parse and for this you need the builtins.__SETUP__SKLEARN__ hack to allow partial import of sklearn before sklearn is actually built
the script needs to be in the root folder in order to be able to import sklearn, or you need to do sys.path manipulations

Full disclosure: version comparison in meson.build only takes into account X.Y.Z versions i.e. not full PEP440. I think this is OK, since for minimum dependencies it is very rare that you want a dev version that is close to a min dependency.

github-actions · 2024-03-28T16:15:20Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: e1a208f. Link to the linter CI: here}

jeremiedbb · 2024-03-29T10:22:12Z

sklearn/meson.build

+if not py.version().version_compare('>=3.9')
+  error('scikit-learn requires Python>=3.9, got ' + py.version() + ' instead')
+endif


I don't think this one is necessary since we have the requirement in pyproject.toml. meson is already aware of the lower bound

I tested and meson raises an error in that case

I could remove it, but I like the fact that you have an early error if you have a too old Python.

I tested it locally and for now you need to install numpy and scipy to be told "actually you know what your Python is too old, sorry".

It seems it is meson-python that raises the error according to the error:

('\x1b[31m',)meson-python: error: Package requires Python version >=3.9, running on 3.8.18

As noted in #28721 (comment) it would be nice if you can get the error without going through pip, e.g. spin build (if we use it one day) will call Meson directly.

jeremiedbb · 2024-03-29T11:01:32Z

sklearn/meson.build

+cython_min_version = run_command(py, ['_min_dependencies.py', 'cython'], check: true).stdout().strip()
+if not cython.version().version_compare('>=' + cython_min_version)
+  error('scikit-learn requires Cython>=' + cython_min_version + ', got ' + cython.version() + ' instead')
+endif


I was indeed able to build with cython 3.0.8 without any complain. Since the min dependencies are listed in pyproject.toml, is there no automated way to make meson aware of them ?

I haven't found anything, pyproject.toml is Python-specific and Meson is a generic build tool. You can run a command from meson.build and check its exit code, see https://mesonbuild.com/External-commands.html for more details.

I think there are a few options:

do what is currently done in this PR, _min_dependencies.py for getting minimum versions + Meson-native version comparison. I am not saying this is great, but to me this seems a reasonable trade-off. I spent a bit of time trying to find something that I found significantly better and I was not able to, but better suggestions welcome!

write an external Python script but we need packaging to compare versions. packaging is vendored inside pip, maybe we could assume that in most cases you have pip when you are trying to build scikit-learn ...

use our vendored packaging but we need __SKLEARN_SETUP__ hack + sys.path manipulations to import from sklearn before sklearn is built

maybe better suggestions?

Note: we could give a more complicated pip install command with --check-build-dependencies but that would only work when building with Meson through pip. If one day we use spin, spin build will call meson directly (i.e. not going through pip) and as such will not error in there are some build dependencies missing.

One thing I had some hope for would be to use run_command + right pip incantation to check build-dependencies in a light-weight manner (assuming pip is installed which is the most common case I think). I thought maybe

pip install . --dry-run --no-build-isolation --check-build-dependencies`

may work, but no it does not, causing an error because scikit-learn is already being built

LookupError: file:///home/lesteve/dev/scikit-learn is already being built: file:///home/lesteve/dev/scikit-learn

Side-comment: actually even pip does not check build-dependencies are satisfied unless you use --check-build-dependencies. See pypa/pip#11116 if you are curious why.

do what is currently done in this PR, _min_dependencies.py for getting minimum versions + Meson-native version comparison. I am not saying this is great, but to me this seems a reasonable trade-off. I spent a bit of time trying to find something that I found significantly better and I was not able to, but better suggestions welcome!

I think that this approach is reasonable in that the benefit outweighs the complexity, however I'm unfamiliar with the meson build system so, similarly, I can't say for sure that there isn't a better way of doing it. Thanks for looking into this!

lesteve · 2024-04-02T09:32:54Z

Another thing I am not too sure about is the cross-compiling case which is why I only check numpy and scipy when building natively (i.e. not cross-compiling). It seems like when cross-compiling the build machine can not necessarily call the host machine Python interpreter, maybe more details can be found in scipy/scipy#16783, scipy/scipy#14812 and https://mesonbuild.com/Cross-compilation.html.

There is a comment that mentions this in sklearn/meson.build (taken from Scipy):

# For cross-compilation it is often not possible to run the Python interpreter
# in order to retrieve numpy's include directory. It can be specified in the
# cross file instead:
#   [properties]
#   numpy-include-dir = /abspath/to/host-pythons/site-packages/numpy/core/include
#
# This uses the path as is, and avoids running the interpreter.

lesteve · 2024-05-16T13:35:39Z

Hmmm I forgot about this PR, maybe it should be considered for 1.5? Not checking versions when going via Meson may cause late confusing build errors rather than early easily understandable errors (e.g. your Cython version is too old).

Micky774 · 2024-06-11T18:50:25Z

Found this PR right as I myself ran into mysterious build errors from an outdated Cython version. Indeed an explicit check to compliment the meson build system would be wonderful.

lesteve · 2024-06-12T07:03:04Z

@Micky774 after we merged #29132, for now it is up to the user to add --check-build-dependencies to its pip command. make dev-meson does this for example and maybe one day with #29012 something like spin install will as well.

I may have a another look at this PR at one point, but I have to admit this is not at the top of my priorities right now 😅.

…nto meson-check-versions

jeremiedbb · 2024-07-01T10:08:56Z

I just pushed a new commit with the correct tag to trigger the wheel builder :)

lesteve · 2024-07-01T13:05:54Z

sklearn/meson.build

+
+# Only check numpy and scipy versions when not cross-compiling, as numpy and
+# scipy are not installed in the build machine
+if not meson.is_cross_build()


Note to myself: I think this one should be removed, there are other places in the code where we execute Python script without protection.

I have to say I don't remember all the details about this and need to have a closer look.

So I looked a little bit more at this and I moved all the build dependency version checks inside the if not meson.is_cross_build() clause, because I think this is safer. If I am wrong, I guess we may hear from some of this people relying on cross-compilation or they will do patches without telling us ...

It looks like conda-forge does not care about whether there is the protection if not meson.is_cross_build(), I tested this in conda-forge/scikit-learn-feedstock#268.

On the other hand, other cross-compilation settings may care more, so I would still leave this.

For example the scipy Nix package is using numpy-include-dir which is also meant to be used for tricky cross-compilation settings (basically you can not run the Python interpreter or the wrong numpy include dir gets picked): https://github.com/NixOS/nixpkgs/blob/81c768e223876ce8ff5120934c5d8f485731c6ed/pkgs/development/python-modules/scipy/default.nix#L61

The scikit-learn Nix package has a patch to get the version directly rather than running a Python script:
https://github.com/NixOS/nixpkgs/blob/fcf65eb2954fc78232b038ceef5d6a5390a734ec/pkgs/development/python-modules/scikit-learn/default.nix#L42-L44

lesteve · 2024-07-01T14:11:42Z

We were wondering about this with @jeremiedbb, from build log, the wheels are built with pip, the command is:

python -m pip wheel /project --wheel-dir=/tmp/cibuildwheel/built_wheel --no-deps -v

That means that build dependencies are installed in a new separate environment which will use numpy>=2.

The Wheels CI was green on this commit: e32e4c2

jeremiedbb

LGTM.

check-build-dependencies was causing issues in the 1.5.1 release branch because we have numpy>=2 there in the pyproject.toml, so I'm +1 for this alternative.

Side question: do you know what's the role of this section of pyproject.toml ?

[project.optional-dependencies]
build = ["numpy>=1.19.5", "scipy>=1.6.0", "cython>=3.0.10", "meson-python>=0.16.0"]

Should we set numpy>=2 here as well ?

lesteve · 2024-07-03T09:15:53Z

[project.optional-dependencies]
build = ["numpy>=1.19.5", "scipy>=1.6.0", "cython>=3.0.10", "meson-python>=0.16.0"]

This section are the equivalent of extra_requires, in other words it allows you to do pip install scikit-learn[build].

I don't think anyone is using this, but this was in setup.py so I added it at one point in the Meson work I think. I think this was more intended as a convenient way to install things when developing scikit-learn so we should probably not require "numpy>=2" here. Having said that, I don't think this is actually that useful. You could argue other optional dependencies like doc are actually more useful in theory although not used either.

…as this will go away with scikit-learn#28721

lesteve · 2024-07-15T12:33:56Z

I am going to merge this even if there is only one approval (thanks @jeremiedbb!).

I feel nobody has had the time or courage to look at this which is perfectly understandable. I feel this is something internal and we can always adapt it when needed.

Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

BLD Check build dependencies in meson.build

f0f1685

jeremiedbb reviewed Mar 29, 2024

View reviewed changes

lesteve mentioned this pull request May 29, 2024

MAINT Add user-friendly error when build dependencies are not satifisfied #29132

Merged

This was referenced Jun 28, 2024

MAINT Bump Numpy min version for the build #29362

Closed

Bumping to NumPy 2 in build requirements #29360

Closed

lesteve and others added 5 commits July 1, 2024 11:45

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

6ff3276

…nto meson-check-versions

Remove --check-build-dependencies

d70de20

Use numpy>=2 in pyproject.toml build dependencies

5f759ec

[gh cd build] [azure parallel]

84b9b0f

[azure parallel] [cd build]

e32e4c2

lesteve commented Jul 1, 2024

View reviewed changes

jeremiedbb approved these changes Jul 3, 2024

View reviewed changes

lesteve added 2 commits July 3, 2024 13:33

[cd build gh] [azure parallel]

3dc8999

Add comment

b82336d

lesteve mentioned this pull request Jul 8, 2024

Test meson-check-versions branch conda-forge/scikit-learn-feedstock#268

Closed

5 tasks

lesteve added 2 commits July 8, 2024 13:57

Tweak comment

f8930b3

Run the build dependencies version checks only when not cross-compiling

e1a208f

lesteve mentioned this pull request Jul 9, 2024

BLD Remove support for setuptools build #29400

Merged

lesteve added a commit to lesteve/scikit-learn that referenced this pull request Jul 9, 2024

[azure parallel] [cd build gh] Remove the --check-build-dependencies …

c147b33

…as this will go away with scikit-learn#28721

lesteve merged commit d79cb58 into scikit-learn:main Jul 15, 2024
30 checks passed

lesteve deleted the meson-check-versions branch July 15, 2024 12:34

lesteve mentioned this pull request Sep 2, 2024

BLD Add Meson OpenMP checks #29762

Merged

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Sep 9, 2024

BLD Check build dependencies in meson.build (scikit-learn#28721)

8c0a0ee

Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

glemaitre added a commit that referenced this pull request Sep 11, 2024

BLD Check build dependencies in meson.build (#28721)

2190f04

Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLD Check build dependencies in meson.build #28721

BLD Check build dependencies in meson.build #28721

lesteve commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading

jeremiedbb Mar 29, 2024

jeremiedbb Mar 29, 2024

lesteve Apr 2, 2024

jeremiedbb Mar 29, 2024

lesteve Apr 2, 2024 •

edited

Loading

Micky774 Jun 11, 2024

lesteve commented Apr 2, 2024 •

edited

Loading

lesteve commented May 16, 2024 •

edited

Loading

Micky774 commented Jun 11, 2024

lesteve commented Jun 12, 2024 •

edited

Loading

jeremiedbb commented Jul 1, 2024

lesteve Jul 1, 2024

lesteve Jul 8, 2024 •

edited

Loading

lesteve commented Jul 1, 2024 •

edited

Loading

jeremiedbb left a comment

lesteve commented Jul 3, 2024

lesteve commented Jul 15, 2024 •

edited

Loading

BLD Check build dependencies in meson.build #28721

BLD Check build dependencies in meson.build #28721

Conversation

lesteve commented Mar 28, 2024 • edited Loading

github-actions bot commented Mar 28, 2024 • edited Loading

✔️ Linting Passed

jeremiedbb Mar 29, 2024

Choose a reason for hiding this comment

jeremiedbb Mar 29, 2024

Choose a reason for hiding this comment

lesteve Apr 2, 2024

Choose a reason for hiding this comment

jeremiedbb Mar 29, 2024

Choose a reason for hiding this comment

lesteve Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Micky774 Jun 11, 2024

Choose a reason for hiding this comment

lesteve commented Apr 2, 2024 • edited Loading

lesteve commented May 16, 2024 • edited Loading

Micky774 commented Jun 11, 2024

lesteve commented Jun 12, 2024 • edited Loading

jeremiedbb commented Jul 1, 2024

lesteve Jul 1, 2024

Choose a reason for hiding this comment

lesteve Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

lesteve commented Jul 1, 2024 • edited Loading

jeremiedbb left a comment

Choose a reason for hiding this comment

lesteve commented Jul 3, 2024

lesteve commented Jul 15, 2024 • edited Loading

lesteve commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading

lesteve Apr 2, 2024 •

edited

Loading

lesteve commented Apr 2, 2024 •

edited

Loading

lesteve commented May 16, 2024 •

edited

Loading

lesteve commented Jun 12, 2024 •

edited

Loading

lesteve Jul 8, 2024 •

edited

Loading

lesteve commented Jul 1, 2024 •

edited

Loading

lesteve commented Jul 15, 2024 •

edited

Loading