8000 RFC Remove setuptools-based build in scikit-learn 1.6 · Issue #29346 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

RFC Remove setuptools-based build in scikit-learn 1.6 #29346

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.

Sign up for GitHub

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

Jump to bottom
Closed
lesteve opened this issue Jun 24, 2024 · 5 comments · Fixed by #29400
Closed

RFC Remove setuptools-based build in scikit-learn 1.6 #29346

lesteve opened this issue Jun 24, 2024 · 5 comments · Fixed by #29400
Labels
Build / CI RFC

Comments

@lesteve
Copy link
Member
lesteve commented Jun 24, 2024

Proposal

I would be in favour of removing setuptools i.e. all setup.py files in 1.6. Meson is our main building tool for some time, teething issues have been found and fixed, and no major blocker issues have been found. We discussed this at the monthly developers meeting today and there did not seem to be any strong objections.

I asked on the Scipy Slack (see message if you are on the Scipy Slack) and got this response from @rgommers:

Now with also Matplotlib and others having switched over, I think you'd be safe with removing setup.py quickly. I am not aware of any blockers in any package that switched so far.

Some projects have removed setup.py already:

For local development, I am reasonably confident things should be fine:

Potential issues / things to think about

  • OpenMP-specific quirk, From the discussion with @rgommers:

    The one thing I can think of that's different in scikit-learn is OpenMP usage. Not sure if that required any special handling beyond linking to llvm-openmp and then running auditwheel & co?

    I can not think about OpenMP-specific issues but happy to hear other opinions. We have released wheels with Meson for 1.5 roughly one month ago, and it doesn't seem like there has been any issues.

  • Linux distribution packaging may still rely on setup.py. I am certainly not a Linux packaging expert but it seems like they may not rely on setup.py much:

  • any other use case that I didn't think of and that may be using setup.py?

Alternative more conservative option

If we are more comfortable with the more conservative, we could follow what Scipy has done i.e. rename setup.py to _setup.py and keep _setup.py as a fall-back for some time, maybe in 1.6, and remove it in scikit-learn 1.7.

@rgommers said on this:

we did keep _setup.py as a backup for ~2 minor releases, because of the known conda-forge issue, and because our build is very complex and we were the very first package to shift over to Meson, so we weren't 100% sure what surprises we'd encounter. I think there were a few other Linux distros that stayed with setup.py for at least one release cycle, but I think mostly that was a "this is less work right now" than anything fundamental.
@github-actions github-actions bot added the Needs Triage Issue requires triage label Jun 24, 2024
@lesteve lesteve added Build / CI RFC and removed Needs Triage Issue requires triage labels Jun 24, 2024
@mr-c
Copy link
Contributor
mr-c commented Jun 24, 2024

@lesteve Fine for Debian! I just tested removing our last setup.py call and we will switch to an entirely PEP-517 based builder in the next packaging upload. Therefore the switch to meson-python without setup.py should be fine

https://salsa.debian.org/science-team/scikit-learn/-/commit/799a3233792f07f1a75f48df5757dfa369b5338f#8756c63497c8dc39f7773438edf53b220c773f67_152_142

Thanks for asking!

@lesteve
Copy link
Member Author
lesteve commented Jun 25, 2024

@lesteve Fine for Debian! I just tested removing our last setup.py call and we will switch to an entirely PEP-517 based builder in the next packaging upload. Therefore the switch to meson-python without setup.py should be fine

Great, thanks a lot @mr-c for your feed-back!

@jeremiedbb
Copy link
Member

One motivation for not keeping both for too long is that currently adding a new cython extension requires to duplicate the work and increases the maintenance burden.

I can not think about OpenMP-specific issues but happy to hear other opinions. We have released wheels with Meson for 1.5 roughly one month ago, and it doesn't seem like there has been any issues.

One thing that I can think of is compilation with non default compilers that have their own OpenMP flags, like intel's dpc++ compiler. The helper we have to set the appropriate flags is used when building using setuptools but not when building with meson. But I'm not even sure it's still working cause we've not been testing it for a while. And anyway if the need comes, we can try to fix it in sklearn or open an issue in meson anyway. So that wouldn't be a blocker imo.

@lesteve
Copy link
Member Author
lesteve commented Jun 25, 2024 •
edited
Loading

There is scipy/scipy#17075 about Intel compiler support in Scipy, this seems like there is some kind of support. Not sure about OpenMP detection, someone would need to try it out ... my feeling right now is we are amongst the first to test OpenMP with Meson in the Scientific Python ecosystem so there may be issues (maybe?).

@sergiopasra
Copy link
Contributor

@lesteve It's fine for Fedora also, the macros in the spec file take care of calling the appropriate incantation. We are building, for example, scikit-image just fine.

This was referenced Jun 26, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Build / CI RFC
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BLD Remove support for setuptools build lesteve/scikit-learn
4 participants
@sergiopasra @mr-c @lesteve @jeremiedbb
0