Migrate from pybind11 to nanobind#198
Conversation
There was a problem hiding this comment.
Pull request overview
Migrates the project’s Python bindings from pybind11 to nanobind, updating the C++ binding layer, CMake integration, and CI/test setup accordingly.
Changes:
- Switch Python binding implementation from pybind11 APIs/macros to nanobind equivalents across structured/unstructured bindings.
- Update build configuration to locate and build against nanobind (CMake + pyproject build requirements).
- Adjust test/CI workflows and packaging metadata to reflect the new binding backend.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/bindings/python/CMakeLists.txt | Uses Python_EXECUTABLE and refactors test target dependencies. |
| pyproject.toml | Replaces pybind11 build requirement with nanobind. |
| include/ghex/structured/regular/halo_generator.hpp | Adds missing <vector> include. |
| cmake/ghex_python.cmake | Switches to FindPython + nanobind discovery via python module + CMake config. |
| bindings/python/src/ghex/pyghex/init.py | Updates documentation comment to nanobind. |
| bindings/python/src/ghex/CMakeLists.txt | Adds pyghex_files target to refresh Python files for tests. |
| bindings/python/src/_pyghex/unstructured/pattern.cpp | Ports bindings from pybind11 to nanobind. |
| bindings/python/src/_pyghex/unstructured/halo_generator.cpp | Ports bindings from pybind11 to nanobind; adjusts constructors. |
| bindings/python/src/_pyghex/unstructured/field_descriptor.cpp | Reworks buffer/ndarray handling using nanobind::ndarray. |
| bindings/python/src/_pyghex/unstructured/domain_descriptor.cpp | Ports bindings and adds NB v1/v2 constructor handling. |
| bindings/python/src/_pyghex/unstructured/communication_object.cpp | Ports bindings + CUDA stream extraction to nanobind. |
| bindings/python/src/_pyghex/structured/regular/pattern.cpp | Ports bindings from pybind11 to nanobind. |
| bindings/python/src/_pyghex/structured/regular/halo_generator.cpp | Ports bindings; adjusts __call__ return handling for nanobind. |
| bindings/python/src/_pyghex/structured/regular/field_descriptor.cpp | Reworks buffer/ndarray handling using nanobind::ndarray. |
| bindings/python/src/_pyghex/structured/regular/domain_descriptor.cpp | Ports bindings from pybind11 to nanobind. |
| bindings/python/src/_pyghex/structured/regular/communication_object.cpp | Ports bindings from pybind11 to nanobind. |
| bindings/python/src/_pyghex/register_class.hpp | Updates helper to return nanobind::class_ and properties. |
| bindings/python/src/_pyghex/py_dtype_to_cpp_name.cpp | Reimplements dtype mapping using nanobind + NumPy dtype canonicalization. |
| bindings/python/src/_pyghex/mpi_comm_shim.hpp | Switches shim API from pybind11 to nanobind. |
| bindings/python/src/_pyghex/mpi_comm_shim.cpp | Ports MPI shim registration and conversion logic to nanobind. |
| bindings/python/src/_pyghex/module.cpp | Replaces PYBIND11_MODULE with NB_MODULE and updates registrations. |
| bindings/python/src/_pyghex/context_shim.cpp | Ports context bindings to nanobind. |
| bindings/python/src/_pyghex/config.cpp | Ports config bindings to nanobind and updates version reporting. |
| bindings/python/src/_pyghex/CMakeLists.txt | Builds Python extension via nanobind_add_module. |
| README.md | Updates dependency list to Nanobind. |
| .github/workflows/test_pip.yml | Removes pybind11 system package install in pip workflow. |
| .github/workflows/CI.yml | Replaces python3-pybind11 with nanobind-dev in CI apt dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return MPI_Comm_f2c(o.attr("py2f")().cast<MPI_Fint>()); | ||
| if (!nanobind::hasattr(o, "py2f")) | ||
| throw nanobind::type_error("Argument must be `mpi4py.MPI.Comm`"); | ||
| return MPI_Comm_f2c(nanobind::cast<MPI_Fint>(o.attr("py2f")())); |
There was a problem hiding this comment.
convert_to_mpi_comm does not actually call py2f(). o.attr("py2f") returns the bound method object, so casting it to MPI_Fint will fail at runtime. Call the method and cast its result (i.e., invoke py2f before converting to MPI_Comm).
| return MPI_Comm_f2c(nanobind::cast<MPI_Fint>(o.attr("py2f")())); | |
| auto py2f_method = o.attr("py2f"); | |
| auto f_comm_obj = py2f_method(); | |
| MPI_Fint f_comm = nanobind::cast<MPI_Fint>(f_comm_obj); | |
| return MPI_Comm_f2c(f_comm); |
| const auto protocol_version = nanobind::cast<std::size_t>(cuda_stream_protocol[0]); | ||
| if (protocol_version == 0) | ||
| { | ||
| std::stringstream error; | ||
| error << "Expected `__cuda_stream__` protocol version 0, but got " | ||
| << protocol_version; | ||
| throw pybind11::type_error(error.str()); | ||
| throw nanobind::type_error(error.str().c_str()); | ||
| } |
There was a problem hiding this comment.
The __cuda_stream__ protocol version check is inverted: version 0 is the expected value per the CUDA stream protocol, but the code throws when protocol_version == 0. This will reject valid stream objects. Flip the condition so that only non-zero protocol versions raise an error.
There was a problem hiding this comment.
@philip-paul-mueller this note looks very legit. Can you comment if it's right to complain or if the error message itself is wrong?
There was a problem hiding this comment.
Fixed by @philip-paul-mueller in #202. I'll apply the change here as well.
This is still work in progress, but functional, at least according to GHEX tests.
This was heavily assisted by LLMs (various models). All bugs are mine.