-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-39555: [Packaging][Python] Enable building pyarrow against numpy 2.0 #39557
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
Changes from all commits
d1a2df1
185ffba
fa80565
88daada
66748cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,12 @@ | |
| [build-system] | ||
| requires = [ | ||
| "cython >= 0.29.31", | ||
| "oldest-supported-numpy>=0.14", | ||
| # Starting with NumPy 1.25, NumPy is (by default) as far back compatible | ||
| # as oldest-support-numpy was (customizable with a NPY_TARGET_VERSION | ||
|
Comment on lines
+21
to
+22
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this always be the case?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean exactly with "always"? For all Python versions, or all numpy versions? Or the full API? It's a guarantee that is given by NumPy starting with version 1.25: https://numpy.org/devdocs/release/1.25.0-notes.html#compiling-against-the-numpy-c-api-is-now-backwards-compatible-by-default (see also some details I posted on the parent issue: #39532 (comment))
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean that oldest-supported-numpy also accounts for bugs that cropped up in the past, e.g. broken ARM compatibility.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I understand, oldest-supported-numpy pins to a newer numpy version for some platforms / architectures (like arm) because the older version that is theoretically supported has bugs. But so that means that if we always use the latest available version for a platform, that should be fine? Of course, there might be a new bug occurring in the latest version, and at that point, defaulting to "the latest available version" might temporarily give issues, compared to pinning to a slightly older but "known to be OK" version (for example, if we know 1.25 is fine, we could pin to that in case the newest 1.26.x would introduce a bug). cc @seberg
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds all exactly right. I suppose the docs linked could recommend to compile against the latest available version (unless there be bugs). This is required for major releases, i.e. NumPy 2 (you cannot build releases against it yet since there is no To continue supporting building against pre 2.0 releases, you may have to vendor |
||
| # define). For older Python versions (where NumPy 1.25 is not yet avaiable) | ||
| # continue using oldest-support-numpy. | ||
| "oldest-supported-numpy>=0.14; python_version<'3.9'", | ||
| "numpy>=1.25; python_version>='3.9'", | ||
| "setuptools_scm < 8.0.0", | ||
| "setuptools >= 40.1.0", | ||
| "wheel" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| cython>=0.29.31 | ||
| oldest-supported-numpy>=0.14 | ||
| oldest-supported-numpy>=0.14; python_version<'3.9' | ||
| numpy>=1.25; python_version>='3.9' | ||
| setuptools_scm<8.0.0 | ||
| setuptools>=38.6.0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| cython>=0.29.31 | ||
| oldest-supported-numpy>=0.14 | ||
| oldest-supported-numpy>=0.14; python_version<'3.9' | ||
| numpy>=1.25; python_version>='3.9' | ||
| setuptools_scm<8.0.0 | ||
| setuptools>=58 | ||
| wheel |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -449,7 +449,7 @@ def has_ext_modules(foo): | |
|
|
||
|
|
||
| install_requires = ( | ||
| 'numpy >= 1.16.6, <2', | ||
| 'numpy >= 1.16.6', | ||
| ) | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added this for testing now, but so eventually this should only be done when this is being run as a nightly job (for creating the nightly wheels), and not on a release.
I am not sure how to detect that inside this dockerfile (passing some env variable?). Now, in practice that is maybe also not needed, because since we don't plan to backport this to the 15.x branch, this commit should only live on main and by the time of the next 16.x release cycle, we should be able to remove this again (by that time, numpy 2.0 should be released)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use the same logic as in
ci/scripts/install_pandas.sh?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we open an issue labeled as blocker for 16.0 to remove this again (to build with (by then) released numpy), we won't forget to update this, and for now I think it is fine to just always build with numpy nightly on the
mainbranchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building with Numpy nightly doesn't seem like a tremendous idea, is it? It is not required functionally and may introduce instability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least should add a comment, and open an issue to remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment and opened a blocker issue -> #39848