nanobind #537
nanobind #537
Conversation
musllinux_1_1 is too old
speed up compilation a bit
|
My python is a bit rusty, haven't used it in years, so I can't immediately comment on the changes. |
|
Ahh sorry, I should tag @axel-angel |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #537 +/- ##
=======================================
Coverage 91.50% 91.50%
=======================================
Files 35 35
Lines 4521 4521
=======================================
Hits 4137 4137
Misses 384 384
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This looks great, especially since nanobind is reducing how much binding code we need to maintain. Considering we haven't made an official Python release yet, I'm not particularly concerned about potential breaking changes here, as long as it's easy for people to use.
| if (manifolds.size() >= 1) { | ||
| // for some reason using Manifold() as the initial object | ||
| // will cause failure for python specifically | ||
| // unable to reproduce with c++ directly |
There was a problem hiding this comment.
I wonder if this was a pybind11 bug? Does it still repro?
There was a problem hiding this comment.
oh I did not check this
| @@ -116,19 +153,18 @@ PYBIND11_MODULE(manifold3d, m) { | |||
| "Compute the convex hull enveloping a set of 3d points.") | |||
| .def( | |||
| "transform", | |||
| [](Manifold &self, py::array_t<float> &mat) { | |||
| auto mat_view = mat.unchecked<2>(); | |||
There was a problem hiding this comment.
Nice that we can remove this - I take it nanobind doesn't do any checking by default?
There was a problem hiding this comment.
it does check, the problem is that they only allows the use of ndarrays, e.g. numpy/tensorflow arrays, instead of doing explicit cast of python lists.
There was a problem hiding this comment.
That seems pretty reasonable to me.
| [](Manifold &self, py::array_t<float> &t) { | ||
| auto t_view = t.unchecked<1>(); | ||
| if (t.ndim() != 1 || t.shape(0) != 3) | ||
| throw std::runtime_error("Invalid vector shape"); |
There was a problem hiding this comment.
What happens now if the wrong shape array is input?
There was a problem hiding this comment.
it will say that there is no appropriate function to call and print the type signature. (Vec3 requires a sequence with length 3, which is checked in the type caster I wrote above)
|
This looks like a good direction; should we go ahead and merge this? |
|
yeah I think we can use this instead of #521, but let me limit the python build wheel CI to tag/release first. |
This reverts commit b3ede2a.
* python stuff * do not choose musllinux_1_2 * skip musl build musllinux_1_1 is too old * try musllinux_1_2 * pep-0632 * fix * update * fix CI * skip emulated tests on mac * use build frontend * use scikit-build-core * re-enable action * minor changes speed up compilation a bit * use nanobind * don't use SABIModule for older python versions * fix build error * fix find and replace error * Revert "re-enable action" This reverts commit b3ede2a.
Use nanobind instead of pybind11, which should support python ABI3 (with Python 3.12).
API changes: Python list is in general not accepted for int/float array arguments, users will need to use numpy (or tensorflow etc.) arrays, except for Vec3.
@Loosetooth what do you think about this? will this break your existing code?