Migrate from pybind11 to nanobind - #1707
Conversation
5ae45f6 to
3c25f73
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
884952a to
21f8a4d
Compare
|
Interesting stuff. Did you see any performance improvements out of the gate with this? |
Yes! I kicked off a pretty rough vibe-benchmark while working on the PR and the general takeaway was twofold:
Other less performance related improvements to reiterate are that binary size is smaller and .pyi stub generation is automatic. |
|
I think we would absolutely consider merging this if the benefits are significant.
For a change this big, I would recommend against backporting, but it could go into Rolling and beyond. Lyrical and Rolling currently share target platforms. Looking at the Lyrical target platforms, do these platforms all have a new enough nanobind version?
If stub generation is automatic, do we need |
|
When I find some more time I will post some numbers on perf and binary size. In the meantime...
Yes on the first 4 -- I believe nanobind 2.0 is the cutoff, so I see 2.5.0 and newer on all the others. How do I check OpenEmbedded? My cursory search only had EDIT: Looking at rosdistro we may just need to copy how pybind11-dev did it, by adding this to the nanobind-dev key, and we should be good. However, there are still a bunch of other platforms like rhel, etc. that don't yet have a
It is generated! But it's also versioned with the code. This way people get IDE support more easily. Additionally, it lends itself to testing. For example, in my repo that uses nanobind we have a CI check that builds the code and runs the tests (as normal) and then does a |
|
Update, I left Claude running a benchmark in the background and it produced a nice report: https://claude.ai/code/artifact/f5514863-a0c8-42fe-81a7-e7bb6442eb04 These are the "headline numbers" I got in my CLI: quite encouraging TBH! |
Signed-off-by: Sebastian Castro <sebas.a.castro@gmail.com>
Signed-off-by: Sebastian Castro <sebas.a.castro@gmail.com>
Signed-off-by: Sebastian Castro <sebas.a.castro@gmail.com>
21f8a4d to
6ec8dcc
Compare
|
@skyegalaxy to put on the client library WG agenda |
|
We discussed this and we are all in favor of moving forward with this. |
|
Awesome! I could take a look. It is this repo? https://github.com/ros2/rosidl/tree/rolling/rosidl_buffer_py |
|
https://github.com/ros2/rosidl_python/tree/rolling/rosidl_generator_py That is the generator. It generates the bindings. ros2/rosidl_python#193 I also have some experimental code around that speed up the existing implementation by ~3x if using pybind that is not that intrusive... |
|
I guess the same optimizations might work for nanobind as well. |
|
Cool! Would it make sense to land those 2 PRs to I guess we probably don't want to merge this until both repos are in workable states w nanobind? |
|
Sorry, I just glanced at those repos and I don't think I understand. Is there anything there that currently uses pybind11 which would need porting to nanobind? I don't see any binding code at all in the repo or the linked PR. Just CPython stuff which is a level of abstraction down. Is the idea to change the CPython usage there to nanobind instead? |
|
Pulls: #1707 |
|
You are right. I was under the assumption that the 'other' python bindings would use the same framework, but this obviously is not the case. |
|
The CI does not find nanobind, are we missing nanobind-dev as a key for rosdep ? |
|
I added Here's the rosdistro entries: https://github.com/ros/rosdistro/blob/82eefa281fd5702f1869408c66606d395cd635ee/rosdep/base.yaml#L8554 Also re: finding it as a key there may be some missing OSes in our non tier 1 platforms. See: #1707 (comment) |
|
@mjcarroll @claraberendsen any idea what's going on with the nanobind dependency ? |
|
These new autogenerated stubs have some regressions around generics. I will check out if nanobind has a nice way of handling it. |
@InvincibleRMC maybe we can use https://nanobind.readthedocs.io/en/latest/api_core.html#_CPPv4N8nanobind3sigE Combined with this if needed: https://nanobind.readthedocs.io/en/latest/typing.html#pattern-files |
Yeah nb::sig will work, but it means a large amount of the .pyi file is going to be hand maintained (just in cpp instead of .pyi) |
Signed-off-by: Sebastian Castro <sebas.a.castro@gmail.com>
|
A large amount is better than the whole thing, I guess? I've just pushed up a commit that proves this out, and it's... not bad, and also not that substantial a portion of the CPP binding code. There are some gaps still, compared to the manual stub curation, but I really would like to hold on to the hope that this can be somewhat automated. Let me know your thoughts? |
--- See ros2/rclpy#1707 for more information
--- See ros2/rclpy#1707 for more information
|
@sea-bass @jmachowinski CI uses a custom set of dependencies defined on ros2/ci and pixi.toml for Windows. It doesn't use rosdep so there is some manual steps to test this: Two things to note: on conda-forge there isn't available the exact version as on Resolute so I bumped it to the next one (feel free to modify/point me to what version we want). Is this package available for RHEL? Otherwise CI for it will fail. And a fresh run/rebuild of ci with those branches as base: |
Description
Following a thread on ROS Discourse, where I suggested upgrading from
pybind11tonanobind, I figured I'd give this a try.Relevant context: https://nanobind.readthedocs.io/en/latest/why.html
Some additional changes besides pure porting:
None; however, nanobind provides for-free bindings for this if you just make it astd::optionalon the C++ side.Note that
nanobind-devas a dependency only transitioned from pip to apt on Ubuntu 24.04, but the 24.04 version is too old for stub generation. So I think on Lyrical onwards this PR is good to go. My recommendation would be not to backport this, though, and simply go forward-looking.Is this user-facing behavior change?
It shouldn't be for the end user, unless there's a bug (or an improvement!) that is noticeable.
Did you use Generative AI?
Yes, Claude Fable 5.
I of course read through all the changes (except for the autogenerated
.pyistub files), identified areas of simplification/inefficiency, and iterated.