Conversation
|
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. |
ChrisBarker-NOAA
left a comment
ChrisBarker-NOAA
left a comment
There was a problem hiding this comment.
thanks for getting this started.
@skogler: any change you're still active and can merge some of this?
pyproject.toml
Outdated
| "setuptools>=42", | ||
| "wheel", | ||
| "pybind11~=2.6", | ||
| "pybind11==2.12.0", |
pyproject.toml
Outdated
| [project] | ||
| name = "mapbox_earcut" | ||
| version = "1.0.2" | ||
| requires-python = ">=3.7" |
There was a problem hiding this comment.
I think this should be >=3.9:
-
3.7 is getting pretty old
-
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"] |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
we really should move to a "modern" build -- e.g. not putting code in setup.py.
maybe even scikit build?
|
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 ... |
|
Hey, yeah I needed wheels on PyPi for something today so I forked it into FWIW |
|
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? |
|
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! |
|
great, thanks!
Perhaps you want to give one of us permissions on the rep for the future?
…-CHB
On Tue, Jul 30, 2024 at 2:03 PM Samuel Kogler ***@***.***> wrote:
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!
—
Reply to this email directly, view it on GitHub
<#15 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG7YYETAIG5TY6J5UGX3CDZO75SVAVCNFSM6AAAAABHRM7Y4CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGIYDKMZVGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
Christopher Barker, Ph.D.
Oceanographer
Emergency Response Division
NOAA/NOS/OR&R (206) 526-6959 voice
7600 Sand Point Way NE (206) 526-6329 fax
Seattle, WA 98115 (206) 526-6317 main reception
***@***.***
|
|
@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. |
|
That would be great! I think you just need to bump the pybind11 version and re-build the wheels. |
|
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. |
|
@ChrisBarker-NOAA Tests are now included in the sdist |
|
Great, thanks! When you get a new release out, I'll update the conda-forge feedstock. https://github.com/conda-forge/mapbox_earcut-feedstock |
|
Release with new builds is out! Could you check if it fits your needs? |
|
Closing this for now, feel free to open issues/PRs if you have problems! |
|
Awesome thank you!! Tested in docker and locally and looks like it's working great! |
3 participants
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.12which 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:pyproject.tomland only left the extension stuff insetup.py.test-commandthat 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).pybind11==2.12.0If the infrastructure changes are too much I'd be also happy to re-open this as the minimal PR which would (I think) be:
pybind11==2.12.0