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

DOC Mention that Meson is the main supported way to build scikit-learn #29008

Merged
merged 4 commits into from May 13, 2024

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented May 13, 2024

This fills a TODO in the changelog about Meson.

The wording + strategy is up for debate. I know scipy moved setup.py to _setup.py (without testing it not 100% sure) so that at least people relying on it had to make an explicit step and realise this was deprecated.

Copy link

github-actions bot commented May 13, 2024

✔️ Linting Passed

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

Generated for commit: 584d4ab. Link to the linter CI: here

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 103 to 104
support until v1.6 and officially remove setuptools support when scikit-learn
1.6 is released.
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can target 1.7 to be more inline with our usual 2 releases deprecation duration

Copy link
Member

Choose a reason for hiding this comment

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

Does the type of build system that scikit-learn use effect users? I thought it was basically only people building from source or release managers who would notice the change? Which is why I think dropping in 1.6 would be Ok. Of course a bit more time for transitioning is also Ok.

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 guess that some people doing packaging themselves may be affected e.g. Linux distributions maybe?

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 not a library API thingy so I think we can state that we will drop support for setuptools in 1.6.

If people complain, we can always extend this to 1.7 later.

Copy link
Member Author

@lesteve lesteve May 13, 2024

Choose a reason for hiding this comment

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

maybe we can target 1.7 to be more inline with our usual 2 releases deprecation duration

I thought about that indeed, my current plan:

  1. rename setup.py to _setup.py in 1.6.dev0 (seemed to be the consensus during the meeting today)
  2. in 1.6 you can rename _setup.py to setup.py to have one more release to actually move to Meson. Probably we still want to test setuptools during this period?
  3. remove setuptools files completely in 1.7

I need to find again how Scipy did it but I think it was very reasonable, e.g. if you try to do python setup.py something it would tell you "setup.py is deprecated, please move to Meson and at last resort you still have the _setup.py option". What I am not sure about is whether this was guaranteed to work or more "this may work if you are lucky"

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 found the scipy way scipy/scipy#19027. They were still testing setuptools in 1.11.2 after setup.py -> _setup.py renaming and dropped it in 1.12.

Inspiration for the wording may also be used from the Scipy 1.9 release notes (a bit more detailed than what I currently have):
https://docs.scipy.org/doc/scipy/release/1.9.0-notes.html#scipy-switched-to-meson-as-its-build-system

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not a library API thingy so I think we can state that we will drop support for setuptools in 1.6.

If people complain, we can always extend this to 1.7 later.

Yeah thinking a bit more about this, this would be my preferred option too ...

doc/whats_new/v1.5.rst Outdated Show resolved Hide resolved
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

LGTM, one comment to make it easier to read, otherwise good to merge

Co-authored-by: Tim Head <betatim@gmail.com>
@ogrisel ogrisel enabled auto-merge (squash) May 13, 2024 14:30
@lesteve
Copy link
Member Author

lesteve commented May 13, 2024

It seems like a job got stuck for some reason, I restarted the build.

@ogrisel ogrisel merged commit 68b7598 into scikit-learn:main May 13, 2024
30 checks passed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request May 20, 2024
@jeremiedbb jeremiedbb mentioned this pull request May 20, 2024
14 tasks
jeremiedbb pushed a commit that referenced this pull request May 21, 2024
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

4 participants