Skip to content
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

BUILD/CI Switch to Meson as main build backend #28506

Merged
merged 46 commits into from Mar 25, 2024

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Feb 22, 2024

Opening this PR to get some feed-back about people feelings on making Meson our main build backend. Personnally I have used this daily for my scikit-learn and and I have not noticed any real blocker issues.

I kept a single build with setuptools: pymin_conda_defaults_openblas. No particular reason why I chose it.

It could potentially be an option to do a separate doc PR. Two reasons for this:

  • the doc needs a bit of work to mention the meson editable command. In particular we can remove --no-use-pep517 everywhere
  • the old pip command will still work and will ignore pyproject.toml (thanks to the --no-use-pep517)

Some questions I have about the doc:

  • there are plenty of places where we can remove --no-use-pep517 🎉
  • As to the recommended way to build, I am slightly leaning towards giving the editable command in verbose mode (pip install --verbose --no-build-isolation --editable . --config-settings editable-verbose=true). It is quite long but you have to run it once and then import sklearn will recompile as needed. I find having some feed-back when things recompile very reassuring (meson did recompile things and something is happening this is why my script has not started running yet). This new behaviour probably needs to be explained in the doc ...
  • I think using spin may be an option in the medium-term. With spin, the workflow is more similar to the current one: spin build when you modify a Cython file, spin test to test. So changing all the doc one way to explain the new way with meson editable and then in a month or so revert and explain spin may not be a good use of my time. The big advantage of spin in my mind is simplicity of the command and the fact that it is discoverable on the command-line. You don't have to look at the doc or your shell history each time you need to find the very long-winded command you need to type.

Full-disclosure there are two nice-to-have features from meson-python but I don't consider them a real blocker for daily work:

@lesteve lesteve marked this pull request as draft February 22, 2024 16:14
Copy link

github-actions bot commented Feb 22, 2024

✔️ Linting Passed

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

Generated for commit: f8ea3de. Link to the linter CI: here

@lesteve lesteve marked this pull request as ready for review February 24, 2024 05:23
@lesteve
Copy link
Member Author

lesteve commented Feb 24, 2024

The CI failures are in main too, see #27700 (comment)

@adrinjalali
Copy link
Member

I like the settings we have with the editable install now (on meson), so I agree with your points.

As for spin, @glemaitre was also talking about potentially using pixi I think. I think using a tool (whichever we choose) at this point makes sense.

@ogrisel
Copy link
Member

ogrisel commented Feb 26, 2024

I also have the feeling that the concerns I had with meson-python editable installs are all being addressed.

Note that we will also need to check that the CD pipeline works now that meson is the default build in the pyproject.toml. But since we don't plan to make a release in the coming weeks, we have the time to merge and check that the nightly builds work as expected.

Question: any impact on the conda-forge build?

@ogrisel
Copy link
Member

ogrisel commented Feb 26, 2024

Also there are now conflicts on the lock file so this PR needs an update.

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.

Happy to move forward with this and test for the next release maybe?

@lesteve
Copy link
Member Author

lesteve commented Mar 8, 2024

The only remaining TODO would be to check that our conda-forge feedstock can build this branch.

I opened a PR: conda-forge/scikit-learn-feedstock#250, let's see what conda-forge CI has to say about it!

@lesteve
Copy link
Member Author

lesteve commented Mar 9, 2024

I opened a PR: conda-forge/scikit-learn-feedstock#250, let's see what conda-forge CI has to say about it!

After a few iterations, the conda-forge feedstock PR is green.

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

I do not have time to review the changes extensively as a maintainer, but this LGTM from a build system's user perspective.

Thank you for performing this migration, @lesteve.

pyproject.toml Show resolved Hide resolved
@lesteve
Copy link
Member Author

lesteve commented Mar 18, 2024

The consensus during our call today is that this is OK to be merged in main (when it is ready) but that this PR should not be back-ported to 1.4.2 (which will be the first Numpy 2 compatible release).

As I mentioned during our call today, it would be great if someone had a closer look at the pyproject.toml changes.

I commented a bit on the motivation behind the changes in #28506 (comment)

@@ -24,9 +24,7 @@ inplace:
$(PYTHON) setup.py build_ext -i
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment to explain that this is only useful when building with setuptools, e.g. after having run python setup.py develop or by setting PYTHONPATH manually.

Maybe we could have a dev-setuptools entry to make that more explicit?
Not sure about the latter.

build_tools/azure/install.sh Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Could you add a new --update-pyproject option to the sklearn/_min_dependencies.py __main__ script to update the various dependency groups in pyproject.toml from the tag_to_packages mapping?

Otherwise we will have to maintain this mapping by hand and I am pretty sure we will forget things and botch package metadata in releases as a result.

Other than that, LGTM!

pyproject.toml Outdated
"numpydoc>=1.2.0",
"pooch>=1.6.0",
]
maintenance = ["conda-lock>=2.4.2"]
Copy link
Member

Choose a reason for hiding this comment

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

Could you expand test_min_dependencies_pyproject_toml to also check minimum version numbers for all of the optional dependency groups?

Copy link
Member Author

Choose a reason for hiding this comment

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

I propose to add a test for optional dependencies and think about how to automatically the pyproject.toml in a more convenient way in a separate PR.

Medium-term, if I manage to take some time to look at spin (and convince people it is worth using it), this kind of maintenance tasks could be implemented as spin custom command.

Full disclosure: I did generate the optional dependencies from _min_dependencies.py tag_to_packages

Copy link
Member Author

@lesteve lesteve Mar 22, 2024

Choose a reason for hiding this comment

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

My last commit d2402e2 added this additional test (and found a few issues in the process e.g. cython 3.0.8 instead of 3.0.9 in pyproject.toml)

This refactored and cleaned up the test to check a pyproject section e.g. build-system.requires against a _min_depencies.py tag e.g. build.

pyproject.toml Outdated Show resolved Hide resolved
@lesteve
Copy link
Member Author

lesteve commented Mar 25, 2024

codecov is red but I think this is good enough. Currently the test only covers {package}>={version} or {package}=={version} in pyproject.toml. We raise an exception in other cases, and the line raising this exception is not covered by our test.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM!

@ogrisel ogrisel enabled auto-merge (squash) March 25, 2024 17:09
@ogrisel ogrisel merged commit 04c5bdd into scikit-learn:main Mar 25, 2024
28 checks passed
@lesteve lesteve deleted the meson-main-build-backend branch March 25, 2024 17:33
@glemaitre
Copy link
Member

🎉

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.

None yet

7 participants