10000 MAINT: specify C17 as C standard in meson.build by ngoldbaum · Pull Request #28980 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MAINT: specify C17 as C standard in meson.build #28980

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
Merged
merged 1 commit into from
May 13, 2024
Merged

MAINT: specify C17 as C standard in meson.build #28980

merged 1 commit into from
May 13, 2024

Conversation

ngoldbaum
Copy link
Contributor

Reference Issues/PRs

#28977

What does this implement/fix? Explain your changes.

Bumps the required C standard. Doing this as a PR to see how impactful it is in CI.

Any other comments?

@github-actions GitHub Actions
Copy link
github-actions bot commented May 8, 2024

✔️ Linting Passed

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

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

@ngoldbaum
Copy link
Contributor Author

Hmm, the pylatest_conda_mkl_no_openmp job failed with this error:

FAILED: sklearn/neighbors/_ball_tree.cpython-312-darwin.so.p/sklearn/neighbors/_ball_tree.pyx.c
  cython -M --fast-fail -3 '-X language_level=3' '-X boundscheck=False' '-X wraparound=False' '-X initializedcheck=False' '-X nonecheck=False' '-X cdivision=True' '-X profile=False' --include-dir /Users/runner/work/1/s/build/cp312 sklearn/neighbors/_ball_tree.pyx -o sklearn/neighbors/_ball_tree.cpython-312-darwin.so.p/sklearn/neighbors/_ball_tree.pyx.c

  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  from libc.string cimport memcpy

  import numpy as np
  import warnings

  from ..metrics._dist_metrics cimport (
  ^
  ------------------------------------------------------------

  sklearn/neighbors/_binary_tree.pxi:148:0: 'sklearn/metrics/_dist_metrics.pxd' not found

Not sure why the C standard would matter for that. If that's familiar to anyone reading along as unrelated to this PR please let me know, otherwise I'll try to reproduce this...

@ngoldbaum ngoldbaum mentioned this pull request May 8, 2024
Closed
@lesteve
Copy link
Member
lesteve commented May 13, 2024

I re-triggered the failing run that should fix the CI. You are right that this is another instance of #28820 I need to take a closer look to fix this.

lesteve
lesteve approved these changes May 13, 2024
View reviewed changes
Copy link
Member
@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@adrinjalali adrinjalali merged commit 2d51510 into scikit-learn:main May 13, 2024
33 checks passed
@adrinjalali
Copy link
Member

@jeremiedbb not sure if we need to have a similar change on the conda-forge side.

@glemaitre glemaitre mentioned this pull request May 13, 2024
Closed
@jeremiedbb
Copy link
Member

I don't think that we need it for 1.5 since 1.5 only supports python <= 3.12

@lesteve lesteve mentioned this pull request May 14, 2024
Closed
@lesteve
Copy link
Member
lesteve commented May 14, 2024 •
edited
Loading

Turns out this broke Pyodide build (Pyodide build is not run in PRs unless explicitly asked for via a commit message) see #29013 for more details. Insights more than welcome!

This likely will affect Scipy inside Pyodide as well.

@ogrisel ogrisel added the free-threading PRs and issues related to support for free-threaded CPython (a.k.a. nogil or no-GIL, PEP 703) label May 14, 2024
@ngoldbaum
Copy link
Contributor Author

This likely will affect Scipy inside Pyodide as well.

Unless I'm misunderstanding the discussions about this, it looks like scipy currently doesn't build under pyodide because of missing fortran support. That must be why it was missed when scipy moved to C17.

@agriyakhetarpal
Copy link
Contributor

SciPy does build under Pyodide, though: https://github.com/pyodide/pyodide/tree/main/packages/scipy (out-of-tree support isn't implemented yet, however)

@ngoldbaum
Copy link
Contributor Author

Thanks for clarifying! I wasn't aware of the package in pyodide, just going off of issues on the scipy issue tracker.

It looks like pyodide is packaging scipy 1.12.0, but Ralf changed the C standard to C17 in the 1.14 dev cycle, so presumably the pyodide package would hit this whenever it gets updated or whenever someone tried to add pyodide CI to scipy.

@agriyakhetarpal
Copy link
Contributor

Ah, I do have plans to add out-of-tree Pyodide support to SciPy, but given the complexity, it's the last on my list right now, and other Scientific Python packages are being prepared first. I am not sure how these C standard changes will be navigated, because an extensive amount of patching is required already for the in-tree builds...

jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request May 20, 2024
@ngoldbaum @jeremiedbb
MAINT: specify C17 as C standard in meson.build (scikit-learn#28980)
a9fa632
@jeremiedbb jeremiedbb mentioned this pull request May 20, 2024
Merged
14 tasks
jeremiedbb pushed a commit that referenced this pull request May 21, 2024
@ngoldbaum @jeremiedbb
MAINT: specify C17 as C standard in meson.build (#28980)
a660d89
@rgommers
Copy link
Contributor

For completeness: this was an issue with Emscripten support in Meson. It was fixed by mesonbuild/meson#13221, but that fix isn't released yet. So using C11 for now (as done in gh-29015) seems like the pragmatic thing to do indeed. Upgrading to C17 should be possible after the next Meson release.

agriyakhetarpal added a commit to agriyakhetarpal/scikit-learn that referenced this pull request Jul 12, 2024
@agriyakhetarpal
Set C standard to C17 again
e34690b 
See discussion on scikit-learn#29015 and scikit-learn#28980
@agriyakhetarpal agriyakhetarpal mentioned this pull request Jul 12, 2024
Closed
@agriyakhetarpal
Copy link
Contributor
agriyakhetarpal commented Jul 12, 2024 •
edited
Loading

Meson 1.5.0 has now been released, and it includes the fix – we should now be able to set the standard as C17 again, I have opened gh-29481 to revert the change made in gh-29015.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrinjalali adrinjalali adrinjalali approved these changes

@lesteve lesteve lesteve approved these changes

Assignees
No one assigned
Labels
free-threading PRs and issues related to support for free-threaded CPython (a.k.a. nogil or no-GIL, PEP 703)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ngoldbaum @lesteve @adrinjalali @jeremiedbb @agriyakhetarpal @rgommers @ogrisel
0