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

[BLD] Add support for NumPy 2.0 wheels #629

Merged

Conversation

matthewfeickert
Copy link
Contributor

@matthewfeickert matthewfeickert commented Jun 18, 2024

Types of changes

Resolves #626.

  AttributeError: `np.infty` was removed in the NumPy 2.0 release. Use `np.inf` instead.
  AttributeError: `np.Inf` was removed in the NumPy 2.0 release. Use `np.inf` instead.

Motivation and context / Related issue

How has this been tested (if it applies)

On my fork all tests (.github/workflows/build_tests.yml) and wheel builds (.github/workflows/build_wheels.yml) are passing.

Locally I tested by

uv pip install --upgrade -e . && python -c 'import ot'

PR checklist

  • I have read the CONTRIBUTING document.
  • The documentation is up-to-date with the changes I made (check build artifacts).
  • All tests passed, and additional code has been covered with new tests.
  • I have added the PR and Issue fix to the RELEASES.md file.

@matthewfeickert matthewfeickert force-pushed the build/numpy-1-x-2-x-compatible-wheel branch from 826b40c to d392abf Compare June 18, 2024 09:32
@matthewfeickert
Copy link
Contributor Author

@rflamary It looks like things are going to pass on my fork (thumbs pressed) so can you enable the CI on my PR (as I'm a first timer)? I'm going to be offline for the next several hours.

@matthewfeickert matthewfeickert marked this pull request as ready for review June 18, 2024 09:45
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.67%. Comparing base (cba9c7b) to head (f34309b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #629   +/-   ##
=======================================
  Coverage   96.67%   96.67%           
=======================================
  Files          85       85           
  Lines       16836    16836           
=======================================
  Hits        16276    16276           
  Misses        560      560           

Copy link
Collaborator

@rflamary rflamary left a comment

Choose a reason for hiding this comment

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

This is great thanks @matthewfeickert

Could you also update teh setup.py? I know the toml file is enough but it is importnant to remain synchronized

@rflamary
Copy link
Collaborator

Also if you put "build wheels" in the commit text it should build the wheels in the PR

@matthewfeickert matthewfeickert force-pushed the build/numpy-1-x-2-x-compatible-wheel branch from f6e3cdf to 49556f9 Compare June 18, 2024 15:21
@matthewfeickert matthewfeickert requested a review from rflamary June 18, 2024 15:22
@matthewfeickert
Copy link
Contributor Author

(Sorry, I force pushed and I forgot that doing so reset the approval of the CI permissions.)

@matthewfeickert matthewfeickert force-pushed the build/numpy-1-x-2-x-compatible-wheel branch from 49556f9 to 2923485 Compare June 18, 2024 15:37
* Add numpy>=2.0.0 to build-system requires to build NumPy 1.x and 2.x compatible
  wheels.
   - c.f. https://numpy.org/doc/stable/dev/depending_on_numpy.html#numpy-2-0-specific-advice
   - As NumPy 2.0 is Python 3.9+, also need to conditionally support 'oldest-supported-numpy'
     for Python 3.8.
* Remove wheel from build-system requires as it is never required and injected
  automatically by setuptools only when needed.
   - c.f. https://learn.scientific-python.org/development/guides/packaging-classic/
* Avoids:

  AttributeError: `np.infty` was removed in the NumPy 2.0 release.
  Use `np.inf` instead.

  AttributeError: `np.Inf` was removed in the NumPy 2.0 release.
  Use `np.inf` instead.
@matthewfeickert matthewfeickert force-pushed the build/numpy-1-x-2-x-compatible-wheel branch from 2923485 to f34309b Compare June 18, 2024 16:32
@@ -69,7 +69,6 @@
license='MIT',
scripts=[],
data_files=[],
setup_requires=["oldest-supported-numpy", "cython>=0.23"],
Copy link
Contributor Author

@matthewfeickert matthewfeickert Jun 18, 2024

Choose a reason for hiding this comment

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

Could you also update the setup.py? I know the toml file is enough but it is important to remain synchronized.

setup_requires is deprecated and indeed trying to use it actually breaks a NumPy 2.0 build, so this needs to get removed. The build-system information in pyproject.toml supersedes it.

c.f. https://learn.scientific-python.org/development/guides/packaging-classic/#pep-517518-support-high-priority

@matthewfeickert
Copy link
Contributor Author

Okay, as I now have all tests and wheel builds passing on my fork, this PR should be ready for CI and review.

@@ -69,7 +69,6 @@
license='MIT',
scripts=[],
data_files=[],
setup_requires=["oldest-supported-numpy", "cython>=0.23"],
install_requires=["numpy>=1.16", "scipy>=1.6"],
python_requires=">=3.6",
Copy link
Contributor Author

@matthewfeickert matthewfeickert Jun 18, 2024

Choose a reason for hiding this comment

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

The requires-python metadata here is incorrect compared to reality as pot v0.9.3 only provides wheels for Python 3.7+ and the tests are only for Python 3.8+

python-version: ["3.8", "3.9", "3.10", "3.11"]

As @henryiii has covered elsewhere (can't remember which blog) the purpose of requires-python (python_requires in setuptools) is to provide guards to keep older CPython versions from installing releases that could contain unrunnable code and to avoid backtracking unhelpfully in dependency resolves.

This should get updated, but this is also a separate (adjacent) scope issue than NumPy 2.0, so I'll open up PR #630 for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks we need to cleanup stuff

@matthewfeickert matthewfeickert changed the title build: Add support for NumPy 2.0 wheels [BLD] Add support for NumPy 2.0 wheels Jun 18, 2024
@rflamary rflamary merged commit 61d751b into PythonOT:master Jun 19, 2024
21 of 22 checks passed
@matthewfeickert matthewfeickert deleted the build/numpy-1-x-2-x-compatible-wheel branch June 19, 2024 07:39
@rflamary
Copy link
Collaborator

Wheels are building, thanks so much @matthewfeickert !

https://github.com/PythonOT/POT/actions/runs/9578068455

@matthewfeickert
Copy link
Contributor Author

That's great! :D

Thanks for the super speedy and responsive review. It was a great first time contributing to the project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Numpy 2.0 compatibility
3 participants