Skip to content

BLD Use NPY_NO_DEPRECATED_API in meson.build to mirror setup.py #29357

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 2 commits into from
Jun 27, 2024
Merged

BLD Use NPY_NO_DEPRECATED_API in meson.build to mirror setup.py #29357

merged 2 commits into from
Jun 27, 2024

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Jun 27, 2024 •
edited
Loading

Looks like this was not ported from setup.py which is likely an oversight. It is not clear to me what the impact is to be fully honest if any, for example on the released scikit-learn 1.5 wheels.

Probably more details are available at here and here.

The comment is from Scipy meson.build, feel free to suggest improvements.

This was noticed in #29352, see in particular #29352 (comment).

@lesteve lesteve changed the title BUILD Use NPY_NO_DEPRECATED_API to mirror setup.py BLD Use NPY_NO_DEPRECATED_API in meson.build to mirror setup.py Jun 27, 2024
@lesteve lesteve mentioned this pull request Jun 27, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 27, 2024 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 25d618b. Link to the linter CI: here

jjerphan
jjerphan approved these changes Jun 27, 2024
View reviewed changes
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.

LGTM. Thank you, @lesteve!

@lesteve
Copy link
Member Author

lesteve commented Jun 27, 2024

@jjerphan just to be clear if we ever start using a deprecated Numpy C API, this would be a compilation error with the current setting in this PR, right?

@jjerphan
Copy link
Member

I think that depending the version of the C API which is used, the compilation of Cython-generated C translation can fail if they are using deprecated pieces of API.

Supposedly, NPY_1_9_API_VERSION is stricter than NPY_1_7_API_VERSION, but would ask for confirmation to people which are more knowledgeable.

Gentle ping to @seberg: is this remark true?

@lesteve
Copy link
Member Author

lesteve commented Jun 27, 2024

One thing I am a bit confused about: should the no deprecated flag match our minimum supported Numpy version (currently Numpy 1.19.5) and use NPY_1_19_API_VERSION 🤔

@seberg
Copy link
Contributor

seberg commented Jun 27, 2024

I think the higher is stricter works, but nothing was really ever added to the deprecations due to lack of interest in the existing ones mostly (i.e. without cython 3 it was all rather useless anyway).

@jeremiedbb jeremiedbb merged commit d8185db into scikit-learn:main Jun 27, 2024
30 checks passed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 2, 2024
@lesteve @jeremiedbb
BLD Use NPY_NO_DEPRECATED_API in meson.build to mirror setup.py (scik…
f85980c 
…it-learn#29357)
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
Merged
11 tasks
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 2, 2024
@lesteve @jeremiedbb
BLD Use NPY_NO_DEPRECATED_API in meson.build to mirror setup.py (scik…
fd3ea92 
…it-learn#29357)
jeremiedbb pushed a commit that referenced this pull request Jul 2, 2024
@lesteve @jeremiedbb
BLD Use NPY_NO_DEPRECATED_API in meson.build to mirror setup.py (#29357)
ce52cf4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjerphan jjerphan jjerphan approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lesteve @jjerphan @seberg @jeremiedbb