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
BLD Check build dependencies in meson.build #28721
base: main
Are you sure you want to change the base?
Conversation
if not py.version().version_compare('>=3.9') | ||
error('scikit-learn requires Python>=3.9, got ' + py.version() + ' instead') | ||
endif |
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 don't think this one is necessary since we have the requirement in pyproject.toml
. meson is already aware of the lower bound
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 tested and meson raises an error in that case
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 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.
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 |
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 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 ?
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 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 fromsklearn
beforesklearn
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.
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 # 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. |
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 insetup.py
.This is a bit unfortunate that the logic is a bit duplicated between
meson.build
andsetup.py
but I think it is acceptable. I reused thesklearn/_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
andsetup.py
, except that there are some complications:sklearn.external._packaging.parse
and for this you need thebuiltins.__SETUP__SKLEARN__
hack to allow partial import ofsklearn
beforesklearn
is actually builtsklearn
, or you need to dosys.path
manipulationsFull disclosure: version comparison in
meson.build
only takes into accountX.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.