Skip to content

BLD Add Meson support #28040

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 115 commits into from
Jan 23, 2024
Merged

BLD Add Meson support #28040

merged 115 commits into from
Jan 23, 2024

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Jan 2, 2024

This adds support for building with Meson. Locally this compiles and the test pass.

As far as I can see, the main motivations are:

  • use a well-established building tool, with some wide adoption in in the Scientific Python ecosystem (NumPy, SciPy, pandas, scikit-image, etc ...) and plenty of expertise around this tricky topic if we need to ask.
  • my experience with Meson while working on this PR
    • as a scikit-learn developer/contributor, the experience is very nice:
      • ccache support works without having to tweak PATH or alternative
      • parallel compilation just works no need to remember to use SKLEARN_BUILD_PARALLEL
      • the compilation log is a lot less noisy, I spotted some Cython warnings that may be relevant, and I never noticed them with setuptools because they were lost in the noise
    • as a meson.build writer:
      • there is definitely a learning curve. Now that this is done, I don't expect it to be much more additional work to maintain it than setuptools, we don't tend to create cython files or change significantly the cython code structure very often. In some cases, e.g. sklearn/svm I found the meson.build files a lot more explicit and readable than our setuptools setup. It is true that they are more verbose, but I think this is a good thing.
      • OpenMP support is also built-in rather than having to write custom scripts for OpenMP detection and rely on custom environment variable in special cases (Pyodide, crossenv, etc ...)
      • debuggability is great, e.g. ninja -C build -t missing-deps allows to spot issues in your meson.build file for example, that was super useful for sklearn.metrics._pairwise_distances_reduction, as the dependency between cython code files is far from trivial. Being able to look at ninja files to figure out the exact flags used was super useful (O3 in the trees). commands like meson introspect build/ -i --targets could be handy too.
  • our setuptools is not really sustainable long-term, for example incremental builds are broken when using pip (i.e. you recompile from scratch each time) so we tell contributors to use --no-use-pep517 i.e. python setup.py develop which has been discouraged for a while, see Installation guidelines now display default pip install commands and only expands in the notes #27960. We also bump into weird edge cases with Pytest on Windows see BUG: pytest error when loading conftest (seemingly platform-specific) #27806.
  • you can have a look at https://labs.quansight.org/blog/2021/07/moving-scipy-to-meson for more details about why SciPy switched to Meson that give motivations with more details

Other things Meson could allow:

I guess the plan for now would be to have Meson supported, maybe tested in one CI build in this PR. Longer-term, the idea would be to give people time to get used to Meson and eventually for Meson to be our main way of building scikit-learn.

Tip

If you want to give this a try locally, the simplest is probably:

Make sure you have meson-python installed:

mamba install meson-python -y

To make it more easy to try, I added dev-meson to the Makefile in this PR:

make dev-meson

And also clean-meson in case you want to go back using setuptools:

make clean-meson

@adrinjalali
Copy link
Member

I wonder at which point @lesteve thinks this is ready to be merged 😁

@adrinjalali adrinjalali self-requested a review January 22, 2024 18:11
Comment on lines 221 to 223
Use conda for this, if you install meson-python with pip, this does not not
include ninja, and you need another way to install ninja (e.g. package
manager)
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 gonna be a challenge. We have a LOT of contributors who don't use mamba/conda. But I think it'd be okay, yet another system dependency like the compiler. I'm just not sure about ninja version compatibility with meson-python.

Copy link
Contributor

Choose a reason for hiding this comment

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

This note should be removed I think. You can pip install ninja just fine (even though it's better done through you distro package manager perhaps).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I was misled by the fact that pip install meson-python did not install ninja. Maybe there is a good reason for this?

Choose a reason for hiding this comment

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

Because ninja is a C++ application, not a python application.

It is more common for ninja to be installed by a linux distro or as part of Visual Studio, than it is for ninja to be installed from (the unofficial third-party package on) PyPI. In such cases, it won't be detected as "installed" by pip at all, no more than GNU make is detected as installed, or GNU bash.

What meson-python does is check at the time you invoke the build-backend, whether ninja is available somewhere on $PATH. If it is NOT available, then meson-python adds ninja to get_requires_for_build_wheel, so pip installs ninja after installing and running meson-python, but before launching meson setup.

So, meson-python guarantees that ninja is installed. But it doesn't voice an opinion on whether you install it via your package manager or via pip install.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK good to know, thanks a lot for the details!

Because we use --no-build-isolation, the mechanism for installing ninja automatically is not going to cover our use case unfortunately.

This is really not a big deal at all, this is completely fine to tell contributors to do pip install meson-python ninja.

Choose a reason for hiding this comment

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

Indeed, absolutely. And if the contributors are positive they already have ninja, they can modify the command line they run, which is quite trivial as long as meson-python doesn't unconditionally force it.


.. code-block:: bash

make dev-meson
Copy link
Member

Choose a reason for hiding this comment

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

so this does an editable install. How does one installs in non-editable mode?

Copy link
Member Author

@lesteve lesteve Jan 23, 2024

Choose a reason for hiding this comment

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

I would argue that for now editable command is the way to go. As I mentioned a few times, the idea is to have maintainers include Meson in their day-to-day work and improve/tweak things as we go.

Do you have a specific reason in mind, why you would want to use non-editable installs? One way is to edit pyproject.toml set build-backend and use the pip command without --editable.

Copy link
Member

Choose a reason for hiding this comment

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

The idea is to see how the workflow works for us, that includes:

  • building and installing with meson
  • building for contributors, a bunch of those having windows machines where they don't have make

I'm quite uncomfortable with having a python file calling pip. Also, I would like to separate build from install. We should have the equivalent of pip install -e . --no-build-isolation as well as make in, which do very different things. The former including the latter as a step. Also, a make clean shouldn't uninstall the package.

Exactly because the idea is to check our day-to-day work, I'm not comfortable with the current solution.

Copy link
Member Author

@lesteve lesteve Jan 23, 2024

Choose a reason for hiding this comment

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

Maybe I was not explicit enough about this and/or it may have been lost in the many comments above. This is the way I see things unfolding (happy to tweak things according to feed-back):

  • until we decide otherwise, setuptools remain the main way to build scikit-learn. This is the case even if this PR is merged.
  • if this PR is merged, maintainers would be more than welcome to try out Meson through make dev-meson and report issues so that we can improve the situation if needed. During this transition period, non-maintainers are not expected to try out Meson. Maintainers that want to keep using setuptools are completely free to do so (i.e. use make in, pip install -e . --no-use-pep517 --no-build-isolation or python setup.py develop)
  • hopefully after some time (maybe 1 or 2 months?), most maintainers are convinced that Meson works well and we have fixed teething issues if any arises. We can then switch to Meson as the main build backend, i.e. set build-backend = "mesonpy" in pyproject.toml. This removes the need from build_tools/build-meson-editable-install.py (I guess this is what you mean by by "a python file calling pip"). This file main goal is to conveniently build with Meson during the transition period.
  • once Meson is our main building tool, the doc is updated and all contributors use Meson. You can still use setuptools if you want (via make in or python setup.py develop).
  • Hopefully for our next release (1.5), Meson is our main building tool
  • One day (maybe for 1.6, maybe later, maybe sooner) we remove setup.py and Meson is our only way to build scikit-learn

Hopefully, this alleviates some of your concerns 🤞!

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Let's see what we're gonna break 😉 I'm very excited for this.

@adrinjalali adrinjalali merged commit 56625c9 into scikit-learn:main Jan 23, 2024
@lesteve lesteve deleted the meson branch January 23, 2024 14:09
@lesteve
Copy link
Member Author

lesteve commented Jan 23, 2024

@adrinjalali Wow nice 😍, given your previous message, I was expecting that more convincing would be needed 😅.

Anyone should feel completely free to ping me directly on any Meson-related issue.

Thanks a lot to everyone that contributed questions, improvements, clarifications on this PR!

@adrinjalali
Copy link
Member

@lesteve I'm not completely happy with the setup as is, but it's much easier to iterate from here than trying to fix things in this huge PR. Since merging this was quite inconsequential, I thought having discussions in smaller issues/prs is gonna be more productive :)

@eli-schwartz
Copy link

Anyone should feel completely free to ping me directly on any Meson-related issue.

Or me. ;)

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Feb 10, 2024
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Feb 13, 2024
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
glemaitre added a commit that referenced this pull request Feb 13, 2024
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@lesteve lesteve restored the meson branch February 22, 2024 09:38
@lesteve lesteve deleted the meson branch March 28, 2024 13:54
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.