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

Numpy 2.0 Wheels #15

Closed
wants to merge 16 commits into from
Closed

Numpy 2.0 Wheels #15

wants to merge 16 commits into from

Conversation

mikedh
Copy link

@mikedh mikedh commented May 11, 2024

Thanks for maintaining these bindings!! I'm working on supporting the Numpy 2.0 release and I noticed that the current wheels + Numpy 2.0 do some funky stuff. I was able to fix it with a simple re-build, although on further investigation I'm not sure why that worked (maybe it used a new pybind11 from my environment?) haha.

It appears that the minimal thing we need to do is pin pybind11>=2.12 which supports both Numpy 1.x and 2.x and re-build. However I was poking and it looks like tests may not have been running on the wheels so I ended up also changing:

  • I moved the cibuildwheel logic and project information into pyproject.toml and only left the extension stuff in setup.py.
  • I don't think cibuildwheel was running tests previously? I added a test-command that runs pytest with a default install (Numpy 1.x), and then also tests with a Numpy 2.0 prerelease if one is available for that platform (this did previously fail).
  • I changed the build version pin to pybind11==2.12.0
  • Wheels are currently building.

If the infrastructure changes are too much I'd be also happy to re-open this as the minimal PR which would (I think) be:

  • pin pybind11==2.12.0
  • bump version

@ChrisBarker-NOAA
Copy link

NOTE: we have it building on conda-forge now. Not the same, but it's something, and shows it can work.

By the way, it would be good to put the tests in the sdist -- or better yet, in the package (they're small). That way, the buitl wheels/conda packages can be easily tested.

Copy link

@ChrisBarker-NOAA ChrisBarker-NOAA left a comment

Choose a reason for hiding this comment

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

thanks for getting this started.

@skogler: any change you're still active and can merge some of this?

pyproject.toml Outdated
@@ -2,6 +2,49 @@
requires = [
"setuptools>=42",
"wheel",
"pybind11~=2.6",
"pybind11==2.12.0",

Choose a reason for hiding this comment

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

maybe put a >= in here?

pyproject.toml Outdated
[project]
name = "mapbox_earcut"
version = "1.0.2"
requires-python = ">=3.7"

Choose a reason for hiding this comment

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

I think this should be >=3.9:

  1. 3.7 is getting pretty old

  2. numpy 2.0 isn't available for < 3.9 anyway.

pyproject.toml Outdated
authors = [{name = "Samuel Kogler", email = "samuel.kogler@gmail.com"}]
license = {file = "LICENSE.md"}
description = "Python bindings for the mapbox earcut C++ polygon triangulation library."
dependencies = ["numpy>=1.19.0"]

Choose a reason for hiding this comment

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

maybe > 1.24? -- not entirely sure how far back the 2.0 compatibility goes -- but no on e needs an ancient numpy :-)

"pip install --force-reinstall --upgrade --pre numpy",
"pytest {package}/tests"]

# don't test on PyPy as it will re-build numpy

Choose a reason for hiding this comment

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

does this workin PyPy at all? I give up on that on the conda-forge build.

pyproject.toml Outdated
# Run the package tests using `pytest`
# also test against pre-release Numpy
# TODO : when numpy 2.0 releases this can be reduced to just one pytest
test-command = ["pytest {package}/tests",

Choose a reason for hiding this comment

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

does this work? the tests aren't in the sdist -- I had to put acopy of the test fijles in the conda recipe to get the tests to run.

really, they should be installed with the package -- they are small, and it's realy good to be able to directly test the installed distro with a compiled package like this.

@@ -3,40 +3,44 @@
from setuptools import setup

Choose a reason for hiding this comment

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

we really should move to a "modern" build -- e.g. not putting code in setup.py.

maybe even scikit build?

@ChrisBarker-NOAA
Copy link

How important is it to change the module name?

That's going to break downstream packages -- how many? I have no idea, but mine, anyway :-)

If you do decide to change it, please put in an alias to the old name that raises a deprecation warning ...

@mikedh mikedh closed this Jul 29, 2024
@mikedh
Copy link
Author

mikedh commented Jul 29, 2024

Hey, yeah I needed wheels on PyPi for something today so I forked it into pip install earcutx, the name change is just because in the past I've not done that and ended up extremely confused as to which fork I was installing 😆. I think the minimal things the project here needs to change are just pybind11>=2.12.0 and possibly un-pin the OS versions for the CI runners.

FWIW earcut.hpp wins the triangulator wars pretty thoroughly for most casual users when I did some quick benchmarks.

@ChrisBarker-NOAA
Copy link

Ahh -- so no word from @skogler yet? darn.

OK -- for now, I'm using my conda-forge packages, so no need for any changes.

Thanks for the benchmarks -- interesting, and good to know this is worth using.

I wonder what's up with shapely on your "quality" check -- what the heck?

@skogler
Copy link
Owner

skogler commented Jul 30, 2024

Hey there, sorry for being absent, I do not currently use Python anymore, so I don't check this often!

Thanks for the PR, I will take a look this week!

@skogler skogler reopened this Jul 30, 2024
@ChrisBarker-NOAA
Copy link

ChrisBarker-NOAA commented Jul 30, 2024 via email

@skogler
Copy link
Owner

skogler commented Jul 30, 2024

@mikedh Are you okay with me just integrating some of your changes manually? This PR includes a lot, and I would rather go step-by-step and include only what is necessary.

@mikedh
Copy link
Author

mikedh commented Jul 30, 2024

That would be great! I think you just need to bump the pybind11 version and re-build the wheels.

@ChrisBarker-NOAA
Copy link

Can I put a plug in for adding the tests to the sdist, too?

That way, I can run them in a conda-build script -- which is a really good way to make sure it's actually built correctly.

@skogler
Copy link
Owner

skogler commented Jul 31, 2024

@ChrisBarker-NOAA Tests are now included in the sdist

@ChrisBarker-NOAA
Copy link

Great, thanks! When you get a new release out, I'll update the conda-forge feedstock.

https://github.com/conda-forge/mapbox_earcut-feedstock
Let me now if either of you want to be involved with that.

@skogler
Copy link
Owner

skogler commented Aug 3, 2024

Release with new builds is out! Could you check if it fits your needs?
I had to remove some variants from the wheels, but I tried to match whatever numpy publishes as close as possible.

@skogler
Copy link
Owner

skogler commented Aug 4, 2024

Closing this for now, feel free to open issues/PRs if you have problems!

@skogler skogler closed this Aug 4, 2024
@mikedh
Copy link
Author

mikedh commented Aug 13, 2024

Awesome thank you!! Tested in docker and locally and looks like it's working great!

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.

3 participants