ros2 / rclpy

rclpy (ROS Client Library for Python)
Apache License 2.0
276 stars 224 forks source link

Interoperability between C / C++ and other Python binding libraries? #291

Open EricCousineau-TRI opened 5 years ago

EricCousineau-TRI commented 5 years ago

Haven't yet hit a need for this, but wanted to ask anywho:

We use pybind11 in Drake, and it'd be nice if we had the ability to get interop between rclpy and rclcpp within our own binding layer.

For some external examples, people have done this with packages like OpenCV, VTK, etc.: https://github.com/virtuald/pybind11_opencv_numpy https://github.com/EricCousineau-TRI/repro/tree/b9e02d6d5a71f6315b80759ba1628b4bb383c0b8/python/vtk_pybind (my stuff, but points to the motivating examples / questions)

Granted, VTK has a rich object model which makes it relatively easy to do the interop. OpenCV is implemented in pure C++, hence the ease of use with pybind C++.

My technical guesses:

As an alternative, we can do what we do for LCM, where we introduce our own wrapper interface... but this has given me personally many sad faces for API interop + decoupling (heavy frameworks...), utilities, generalization, etc.: https://drake.mit.edu/pydrake/pydrake.lcm.html https://drake.mit.edu/doxygen_cxx/classdrake_1_1lcm_1_1_drake_lcm.html (doc produced at or around drake@361223416)

mjcarroll commented 5 years ago

There was a proof-of-concept developed, but it's probably quite stale at this point: https://github.com/ros2/rclpy/tree/proof_of_concept_pybind11

@sloretz (the original author of this PoC) would probably be the best person to weigh in here.

dirk-thomas commented 5 years ago

When you say "interoperability between rclpy and rclcpp" can you please elaborate what exactly you mean / expect?

EricCousineau-TRI commented 5 years ago

There was a proof-of-concept developed [...]

Gotcha, thanks! It looks like that is namely tailored towards providing a subset of functionality (rclpy._rclpy_wait_set, which is consumed by rclpy.impl)?

When you say "interoperability between rclpy and rclcpp" can you please elaborate what exactly you mean / expect?

I'd like to define an API in C++, and bind it in pybind11 and have it interoperable with (some implementation of) rclpy.

As a toy example, "combining" both examples/rclcpp/minimal_publisher/lambda.cpp + examples/rclpy/topics/minimal_publisher/examples_rclpy_minimal_publisher/publisher_local_function.py

// pybind bindings of `MinimalPublisher`
PYBIND11_MODULE(cpp_pub, m) {
  m.class_<MinimalPublisher, rclcpp::Node>(m, "MinimalPublisher")
     .def(py::init());
}

and then in Python:

import cpp_pub

rclpy.init(args=args)
node = cpp_pub.MinimalPublisher()
rclpy.spin(node)
node.destroy_node()
rclpy.shutdown()

(assuming the GIL issues are avoided by only dispatching using python Thread)

Disclaimer: I haven't yet used ROS2 messages; if there's a nice generic type-erasure mechanism for C++ & Python generated classes via static or dynamic polymorphism, then I'm sure it'd be easy to resolve that via type_caster<> shindiggery.

wjwwood commented 5 years ago

When you say "interoperability between rclpy and rclcpp" can you please elaborate what exactly you mean / expect?

I'd like to define an API in C++, and bind it in pybind11 and have it interoperable with (some implementation of) rclpy.

I think maybe what he's asking is "to what end?" Are you trying to do intra-process communication between them, or mix C++ class instances with "pure" Python Node's and spin them in the same threads (if so why?), etc... What's the use case?

(assuming the GIL issues are avoided by only dispatching using python Thread)

Not sure you can rely on that.

Disclaimer: I haven't yet used ROS2 messages; if there's a nice generic type-erasure mechanism for C++ & Python generated classes via static or dynamic polymorphism, then I'm sure it'd be easy to resolve that via type_caster<> shindiggery.

Honestly I cannot answer this, without knowing what you intend to use the type erasure for, because it can be type erased, but the underlying storage for Python is our C message class rather than our C++ message class, so type erasing it will not allow you to use the messages in both systems without conversion, i.e. Python stores strings as char * in the end and C++ stores them as std::string.

EricCousineau-TRI commented 5 years ago

[...] or mix C++ class instances with "pure" Python Node's and spin them in the same threads (if so why?) [...]

This one! My goal is to avoid this (in overview) when providing C++ and Python interfaces for Drake:

As an alternative, we can do what we do for LCM, where we introduce our own wrapper interface... but this has given me personally many sad faces for API interop + decoupling (heavy frameworks...), utilities, generalization, etc.: [... links ...]

Short-term, sure, we can duplicate stuff if it's the best short-term path; long-term, it would be nice to minimize API + implementation duplication, especially in light of rigorous testing.

Not sure you can rely on that. [...]

Sure, there needs to be an understood contract in the implementation (e.g. do not threading dispatch in the implementation, but rather let the consumer do the handling, as is done by the spin interface / scheduling?).

Honestly I cannot answer this, without knowing what you intend to use the type erasure for [...]

Sorry, didn't clarify; I don't expect to access the direct C++ data from Python. I'm completely fine with there being a duplication of data; e.g. if passing a Python message to C++, serialization happens at that boundary. Simple enough if the interface handles it at some layer; if not, then whatevs, we can wrap it.

Example (but blech that this was needed in the first place...): https://github.com/RobotLocomotion/drake/blob/4c6246197/bindings/pydrake/systems/lcm_pybind.h#L26 https://github.com/RobotLocomotion/drake/blob/4c6246197ac615a335cf7302d2d95f921303af47/bindings/pydrake/systems/test/lcm_test.py#L74

peterdavidfagan commented 2 years ago

Hi everyone,

I see it has been a while since this thread was active but nonetheless I thought it may be worthwhile contributing to the discussion.

I am working on creating a Python library for the MoveIt project as part of GSoC this year and I am also using pybind11. As part of this project I was hoping that users would be able to leverage the rclpy library to create nodes which could be passed to MoveIt through the Python API.

I am currently encountering issues in doing so, I have a class whose constructor accepts a shared pointer reference to a node instance. I wish to create this node using rclpy and pass it to MoveIt as follows.

I believe this relates to this subject of interoperability; the moveit library I am binding is dependent on the rclcpp library.

Any comments or suggestions would be appreciated, at the moment I am considering adding utilities to the library I am creating in order to allow users to create nodes using rclcpp library via python bindings.

sloretz commented 2 years ago

I am currently encountering issues in doing so, I have a class whose constructor accepts a shared pointer reference to a node instance. I wish to create this node using rclpy and pass it to MoveIt as follows.

It looks like your API expects an rclcpp::Node::SharedPtr &, but your example is passing it the Python class rclpy.node.Node. Those are different types.

I was hoping that users would be able to leverage the rclpy library to create nodes which could be passed to MoveIt through the Python API.

Note that rclpy doesn't use rclcpp, so it has no conversions to rclcpp types. It does have a way to get the rcl type rcl_node_t. If moveit has APIs using rcl types, then that's your starting point. Otherwise you'd need rclcpp to offer a way to create an rclcpp::Node instance from a borrowed rcl_node_t *, which I don't think exists.

peterdavidfagan commented 2 years ago

Thanks @sloretz this is an awesome response I appreciate the detail.

It looks like your API expects an rclcpp::Node::SharedPtr &, but your example is passing it the Python class rclpy.node.Node. Those are different types.

Agreed on the above point.

Note that rclpy doesn't use rclcpp, so it has no conversions to rclcpp types. It does have a way to get the rcl type rcl_node_t. If moveit has APIs using rcl types, then that's your starting point. Otherwise you'd need rclcpp to offer a way to create an rclcpp::Node instance from a borrowed rcl_node_t *, which I don't think exists.

Thanks @sloretz I wasn't already aware of rcl_node_t being available, I appreciate this additional information. After further inspection it seems you are correct in that there doesn't appear to be a straight forward way to create a rclcpp::Node instance from a borrowed rcl_node_t *.

peterdavidfagan commented 9 months ago

I am currently revising my implementation for moveit_py. During GSoC I went with the approach of initializing and creating a rclcpp node in a custom constructor for my class (seen here). I got this working by the end of GSoC but it leads to a suboptimal development pattern as users of the Python API never handle node creation execution using the python client library rclpy. Also it lead to the need for logic when parsing node parameters from the Python command line arguments which was not ideal.

A new approach would be to decouple the MoveIt C++ implementation from the rclcpp library where possible and bind this while handling ROS specific bindings separately. I don't think this is currently a feasible solution given the extent of the C++ codebase for MoveIt and its userbase. It looks like this is what the Drake library currently does (pydrake & drake_ros).

Another new approach is to enable interoperability between rclpy and rclcpp as given in the example here. I am less familiar with the client library codebases but is there anything that can be done in this direction to help enable better usability of Python bindings for ROS libraries?

rhaschke commented 3 months ago

I think, creating interoperability of nodes between rclpy and rclcpp is not possible to achieve as both Node classes are surprisingly different from each other: A rclpy node essentially just wraps a Context and a rcl_node_t within a new C++ Node class: https://github.com/ros2/rclpy/blob/070132a6bb73a006f0cdecf362626a1098b92c05/rclpy/src/rclpy/node.hpp#L216-L217

while a rclcpp Node includes much more functionality. rclcpp's NodeBase class seems much closer to rclpy's Node class, but still has more members. Unfortunately, rclpy and rclcpp were not developed with interoperability in mind - causing a lot of code duplication between both code bases :disappointed: Why, instead of wrapping a new Node class for rclpy, you didn't simply wrap the existing rclcpp class?

While both implementations essentially build on the rcl_node_ type, they don't yet provide a way to convert both into each other and, even worse, I don't see a way to achieve this by any means.

peterdavidfagan commented 3 months ago

Thanks for the detailed overview and links @rhaschke.

Why, instead of wrapping a new Node class for rclpy, you didn't simply wrap the existing rclcpp class?

I was reluctant to create my own wrapper for the rclcpp Node class separate to the official python client library, this being said this suggestion is entirely feasible. The main benefit I saw of having interoperability was ensuring the developer experience for using the MoveIt Python library would align with existing ROS python development patterns (instantiation of Nodes etc.), in the end I simply abandoned this notion when interoperability was not possible through official client libraries and settled for instantiating nodes on C++ side.

This unfortunately could lead to confusion if end-users are not aware of how nodes are being handled by the library.

rhaschke commented 3 months ago

My comment was not targeting you, @peterdavidfagan, but the developers of rclpy.

peterdavidfagan commented 3 months ago

My comment was not targeting you, @peterdavidfagan, but the developers of rclpy.

Ah ok my bad and understood.

clalancette commented 3 months ago

Why, instead of wrapping a new Node class for rclpy, you didn't simply wrap the existing rclcpp class?

We didn't go that way for the original design for a couple of reasons:

  1. The original design was meant to concentrate more of the functionality in the rcl layer, so that it would be easier to add in different languages over time.
  2. If rclpy wrapped rclcpp, then it is more likely to be stuck with C++-isms. One of the goals of the design was to have common functionality in the rcl layer, but then to be "language-native" in the API that users interact with. That is, we are more free to implement things in a Pythonic way here in rclpy.
  3. The original implementation of rclpy didn't use pybind11, but instead used the Python C API directly.

You can argue whether the design is successful, and whether those design goals are worth it. But that's why the current implementation is like it is.

Unfortunately, rclpy and rclcpp were not developed with interoperability in mind

Yes, this is exactly it. ROS 2 tends to view interoperability at the message level, not the API level, which is why the design is like this.

While both implementations essentially build on the rcl_node_ type, they don't yet provide a way to convert both into each other and, even worse, I don't see a way to achieve this by any means.

Given enough motivation, it would of course be possible by making changes to both the rclcpp and rclpy layers to make them interoperable. But someone would need to have that motivation and the time to put in the large amount of work to get there.

rhaschke commented 3 months ago

Thanks for this explanation, Chris. I also had in mind, that a major goal of ROS2 was to concentrate functionality in the rcl layer. However, the current design of rclpy and rclcpp - adding more (and different) functionality on top of rcl_node_ - kind of counteracts that goal. Establishing interoperability, in my view, would mean a major rewrite of rclpy - to wrap rclcpp's classes. Even if one would have that motivation and time to do that work, will such a large rewrite be considered for merging at all?

clalancette commented 3 months ago

Even if one would have that motivation and time to do that work, will such a large rewrite be considered for merging at all?

It's unclear to me. Personally, I like the current design, and I think the problem is that we have implemented too much at the rclpy/rclcpp layers. So my personal hope would be that we would move more of the functionality down into rcl to remove some of the duplication (which would also make other rcl* libraries easier to write).

But I'll admit I am not the expert in this repository, so I'd be interested to hear what @sloretz had to say about it.