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

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

Merged
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?

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...

@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.

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.

@jeremiedbb
Copy link
Member

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

@lesteve
Copy link
Member

lesteve commented May 14, 2024

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

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

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants