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.

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

ARROW-14968: [Python] Pin numpy build dependency using oldest-supported-numpy #11606

Closed
wants to merge 4 commits into from
Closed

ARROW-14968: [Python] Pin numpy build dependency using oldest-supported-numpy #11606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a2dbd4
Experiment with oldest-supported numpy
kszucs Nov 4, 2021
0644383
Enfore numpy>=1.16.6
kszucs Nov 4, 2021
e235ffe
Revert "Enfore numpy>=1.16.6"
kszucs Nov 4, 2021
4f6c78c
Restore test numpy pins
kszucs Dec 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions python/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,8 @@
[build-system]
requires = [
"cython >= 0.29",
"numpy==1.16.6; python_version<'3.8'",
"numpy==1.17.3; python_version=='3.8'",
"numpy==1.19.4; python_version=='3.9'",
"numpy==1.21.3; python_version>'3.9'",
"setuptools < 58.5", # ARROW-14584
"oldest-supported-numpy",
Copy link
Member Author

Choose a reason for hiding this comment

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

We may need to add numpy>=1.16.6 here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there are older numpy versions pinned for python < 3.8: https://github.com/scipy/oldest-supported-numpy/blob/master/setup.cfg#L44-L46

Copy link
Member

Choose a reason for hiding this comment

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

We don't support numpy <= 1.16.6? Do you remember the reason?

Copy link
Member Author

@kszucs kszucs Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

oldest-supported-numpy should be >=1.17, so no need to enforce this?

Copy link
Member

Choose a reason for hiding this comment

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

I think we already worked around the issue: #8834

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I searched for PyArray_DescrCheck across the codebase and didn't find any occurrences, so hope that pinning is not necessary.

Copy link
Member Author

@kszucs kszucs Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder what do we need in install_requires: https://github.com/apache/arrow/blob/master/python/setup.py#L572

Just keep as is?

Copy link
Member

Choose a reason for hiding this comment

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

Let's not change it if it doesn't annoy anyone.

Copy link
Member

Choose a reason for hiding this comment

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

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

"setuptools_scm",
"setuptools >= 38.6.0",
"wheel"
]
4 changes: 2 additions & 2 deletions python/requirements-build.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cython>=0.29
numpy>=1.16.6
Copy link
Member Author

Choose a reason for hiding this comment

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

We may need to keep this.

setuptools>=38.6.0
oldest-supported-numpy
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK this shouldn't be added here, this is purely a dependency for the build environment that pip creates

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, seems like we use this for development.

setuptools_scm
setuptools>=38.6.0
15 changes: 2 additions & 13 deletions python/requirements-wheel-build.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,5 @@
cython>=0.29.11
setuptools>=58
oldest-supported-numpy
Copy link
Member

Choose a reason for hiding this comment

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

same here, this file shouldn't be change AFAIK

Copy link
Member Author

@kszucs kszucs Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It would be better if we could rely on oldest-supported-numpy here since previously we pinned newer then necessary versions.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep the requirements files for now and defer the pypa-build question to another issue.

setuptools_scm
setuptools>=58
wheel
numpy==1.19.4; platform_system == "Linux" and platform_machine == "aarch64" and python_version <= "3.9"
numpy==1.21.3; platform_system == "Linux" and platform_machine == "aarch64" and python_version > "3.9"
numpy==1.16.6; platform_system == "Linux" and platform_machine != "aarch64" and python_version < "3.9"
numpy==1.19.4; platform_system == "Linux" and platform_machine != "aarch64" and python_version == "3.9"
numpy==1.21.3; platform_system == "Linux" and platform_machine != "aarch64" and python_version > "3.9"
numpy==1.21.3; platform_system == "Darwin" and platform_machine == "arm64"
numpy==1.16.6; platform_system == "Darwin" and platform_machine != "arm64" and python_version < "3.8"
numpy==1.19.4; platform_system == "Darwin" and platform_machine != "arm64" and python_version >= "3.8" and python_version <= "3.9"
numpy==1.21.3; platform_system == "Darwin" and platform_machine != "arm64" and python_version > "3.9"
numpy==1.16.6; platform_system == "Windows" and python_version < "3.9"
numpy==1.19.4; platform_system == "Windows" and python_version == "3.9"
numpy==1.21.3; platform_system == "Windows" and python_version > "3.9"