-
Notifications
You must be signed in to change notification settings - Fork 124
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
Port bindings to pybind11? #223
Comments
Users seem to expect binary wheels, not just Integrating something like this into the release process will probably take some effort and decision-making by the project owners. |
Comparison with selected other tools for wrapping C APIs:
For this particular project, in a quick up-front comparison, |
Does pybind11 have mypy-compatible type hints? That was another problem I encountered with the current ctypes code. |
It’s an important question. I can’t answer from experience. I’ve use ctypes, Cython, and CFFI in the past, but not pybind11—and all before the widespread use of typing in Python. A quick search turns up mypy.stubgen for generating draft stubs for arbitrary C extensions, with the expectation that they will be edited and maintained by hand. There is also a third-party pybind11-stubgen (discussion). I don’t see an obvious way to avoid some manual type hinting as @hobu suggested, but I could easily be missing something. |
An open question is, if type hints indeed need to be manually maintained, is it worth the switch, and particularly, worth introducing compiled code to the package? Using On the other hand, publishing binary wheels is a lot more annoying. |
We are already in the business of publishing binary wheels. Doing this with ctypes+libspatialindex is just as annoying as it would be with pybind11+libspatialindex. I would have expected some type hinting stuff coming out of pybind11 because the point of it is to have type checking when compiling wrappers 😄, but I can understand if it hasn't matured that far at this time. The case for pybind11 is indeed to do away with the manually maintained and matched API calls of ctypes <-> libspatialindex to avoid the discrepancies of things like @musicinmybrain found in #222. |
Yes, manual type hints are fine. It's just that with ctypes, it didn't seem to be possible to use manual type hints. Mypy forbids assignment of methods to override other instance methods. Also, the typeshed hints for ctypes seem to have the wrong number of arguments, so there were all kinds of issues. As long as pybind11 type hints are slightly simpler, I'll be happy with the change! |
That’s right, I forgot about the bundled Then the conversion is probably worth attempting even if the gain is somewhat less. If pybind11 isn’t offering automatic type hinting, it might mean that It might just come down to which tool’s style you like better. |
We don't have to migrate. Having done both pybind11 and ctypes, I like pybind11 better nowadays for the reasons I've stated. This codebase has been quite stable over the years, so I do not wish to upset that with a big subsystem change that has the potential for breakage for very little external benefit. A case for pybind11 is the opportunity to ditch the C API usage of libspatialindex for direct consumption of its C++ API. That has the potential to speed things up and make code simpler and easier to follow. The plan for now is to make the 1.0.0 release be based on ctypes to include all of the recent activity, and then we will evaluate pybind11 options if you or someone else has time to put in the effort. |
It was suggested by @hobu in #220 (comment) that the libspatialindex bindings should ideally be ported from ctypes to pybind11.
This could reduce the need for manual alignment of function prototypes and manual type hinting, and it could hopefully prevent future subtle bugs due to prototype mismatches (cf. #220).
It would, however, require new machinery to build and publish binary wheels for popular platforms.
I am filing this issue to provide a forum for discussion and investigation, and to track eventual progress, on this topic.
The text was updated successfully, but these errors were encountered: