ARROW-14968: [Python] Pin numpy build dependency using oldest-supported-numpy #11606
Conversation
| @@ -1,4 +1,4 @@ | |||
| cython>=0.29 | |||
| numpy>=1.16.6 | |||
| "numpy==1.19.4; python_version=='3.9'", | ||
| "numpy==1.21.3; python_version>'3.9'", | ||
| "setuptools", | ||
| "oldest-supported-numpy", |
There was a problem hiding this comment.
We may need to add numpy>=1.16.6 here as well.
There was a problem hiding this comment.
Yes, there are older numpy versions pinned for python < 3.8: https://github.com/scipy/oldest-supported-numpy/blob/master/setup.cfg#L44-L46
There was a problem hiding this comment.
We don't support numpy <= 1.16.6? Do you remember the reason?
There was a problem hiding this comment.
Based on the blame it points to numpy/numpy#17913.
I guess the latest crossbow builds should fail if the pin is still required.
cc @xhochy
There was a problem hiding this comment.
oldest-supported-numpy should be >=1.17, so no need to enforce this?
There was a problem hiding this comment.
I think we already worked around the issue: #8834
There was a problem hiding this comment.
Yeah, I searched for PyArray_DescrCheck across the codebase and didn't find any occurrences, so hope that pinning is not necessary.
There was a problem hiding this comment.
I wonder what do we need in install_requires: https://github.com/apache/arrow/blob/master/python/setup.py#L572
Just keep as is?
There was a problem hiding this comment.
Let's not change it if it doesn't annoy anyone.
There was a problem hiding this comment.
I wonder what do we need in install_requires: https://github.com/apache/arrow/blob/master/python/setup.py#L572
Yes, I don't think this needs to be changed. oldest-supported-numpy is about build dependencies, not runtime install dependencies
|
@github-actions crossbow submit -g wheel python-sdist We should also check the linux packaging before merging. |
|
Revision: 9ba0fdf Submitted crossbow builds: ursacomputing/crossbow @ actions-1086
|
|
@github-actions crossbow submit -g wheel python-sdist |
|
Revision: 8d346c1 Submitted crossbow builds: ursacomputing/crossbow @ actions-1088
|
kszucs
changed the title
kszucs
changed the title
| @@ -1,4 +1,4 @@ | |||
| cython>=0.29 | |||
| numpy>=1.16.6 | |||
| setuptools>=38.6.0 | |||
| oldest-supported-numpy | |||
There was a problem hiding this comment.
AFAIK this shouldn't be added here, this is purely a dependency for the build environment that pip creates
There was a problem hiding this comment.
Yes, seems like we use this for development.
| @@ -1,16 +1,5 @@ | |||
| cython>=0.29.11 | |||
| setuptools>=58 | |||
| oldest-supported-numpy | |||
There was a problem hiding this comment.
same here, this file shouldn't be change AFAIK
There was a problem hiding this comment.
It would be better if we could rely on oldest-supported-numpy here since previously we pinned newer then necessary versions.
There was a problem hiding this comment.
Ah, so we use this file to manually create the "build environment", from where we call python setup.py bdist_wheel. Then I suppose we could indeed use oldest-supported-numpy here.
Now, I think the "proper" solution would be to stop using python setup.py bdist_wheel, and use python -m build instead (https://pypa-build.readthedocs.io/en/stable/). Then that would create the build environment based on what is already specified in pyproject.toml, and it would follow the same workflow as when you build a wheel when installing an sdist.
Although that brings up an interesting question: arrow-cpp also requires numpy to build. Should this be the same version? Or that should also follow the idea of "build against oldest supported"? (in which case we need to keep numpy anyway in this requirements file)
There was a problem hiding this comment.
Let's keep the requirements files for now and defer the pypa-build question to another issue.
|
@github-actions crossbow submit wheel-manylinux2010-cp310-amd64 |
|
Revision: e235ffe Submitted crossbow builds: ursacomputing/crossbow @ actions-1232
|
|
@jorisvandenbossche the warning counts differ from the expected values in the pandas tests only for the python 3.7 builds. Could you please take a look at it? Do we need to pin a differrent pandas version there? |
python/requirements-wheel-test.txt
Outdated
| numpy==1.19.5; platform_system == "Darwin" and platform_machine != "arm64" and python_version < "3.9" | ||
| numpy==1.21.3; platform_system == "Darwin" and platform_machine != "arm64" and python_version >= "3.9" | ||
| numpy==1.19.5; platform_system == "Windows" and python_version < "3.9" | ||
| numpy==1.21.3; platform_system == "Windows" and python_version >= "3.9" |
There was a problem hiding this comment.
Maybe we can keep this, so we test with a numpy version of around the same "age" as the pandas version below
There was a problem hiding this comment.
Thanks for taking a look! I'm going to restore these pins, hopefully that'll fix the tests.
|
I suppose it is not specific to Python 3.7, but due to a combination of versions of other packages that get installed. One guess is that because of having an older pandas version (1.0.5) but the latest numpy (1.21.3), numpy raises some warnings that are not only fixed in newer pandas versions. For Python 3.6 this doesn't happen because an older version of numpy gets installed (I suppose there are no wheels for the latest numpy release for python 3.6) |
kszucs
changed the title
|
|
|
@github-actions crossbow submit -g wheel python-sdist |
|
Revision: 4f6c78c Submitted crossbow builds: ursacomputing/crossbow @ actions-1237
|
|
@github-actions crossbow submit -g python |
|
Revision: 4f6c78c Submitted crossbow builds: ursacomputing/crossbow @ actions-1238
|
|
Benchmark runs are scheduled for baseline = 8cebc49 and contender = a71c1e6. a71c1e6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
oldest-supported-numpyis a meta-package which defines the numpy version pins across platforms and python versions, this should make the python package maintenance easier and more future-proof.cc @jorisvandenbossche