Skip to content
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

REP 2007: Type Adaptation #303

Merged
merged 29 commits into from
May 26, 2021
Merged

REP 2007: Type Adaptation #303

merged 29 commits into from
May 26, 2021

Conversation

audrow
Copy link

@audrow audrow commented Jan 28, 2021

Adds a REP for supporting type adaptation (a.k.a. Type Masquerading) in rclcpp.


References and related links:

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, just a few comments

rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
rep-2007.rst Outdated Show resolved Hide resolved
@gavanderhoorn
Copy link
Contributor

As I wrote in ros2/examples#300, I do really like type masquerading. But as I also admitted, not because it's necessarily the standard terminology for this, but because of what it conveys: register some type pretending to be another type.

But thinking about it more, it's not necessarily actually true. As @wjwwood writes:

This interface will allow us to define methods for serializing directly to the user requested type, and/or using that type in intra-process communication without ever converting it. Whereas without this feature, even to send a cv::Mat from one publisher to a subscription in the same process would require converting it to a sensor_msgs::msg::Image first and then back again to cv::Mat.

this is no longer pretending, this is basically adding first-class support for non-msg types.

Would that perhaps need to be represented somehow in the name for the technique?

Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point of clarification - is this appropriate as a REP or does it belong in https://github.com/ros2/design? I ask because I haven't seen feature designs for ROS 2 in REP, to date - though there are some for ROS.

rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
@clalancette
Copy link
Contributor

Point of clarification - is this appropriate as a REP or does it belong in https://github.com/ros2/design? I ask because I haven't seen feature designs for ROS 2 in REP, to date - though there are some for ROS.

We're not quite there yet, but the long term goal is to retire design.ros2.org and move that content elsewhere. We haven't thought through all of the details (which is why we haven't announced it), but we're going to try to reduce the number of new things over there.

Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>
Copy link

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC the proposal is that the adapter will have some functions to convert from a native type to a ros type and the opposite.
On the other hand, ROS 1 needed functions to serialize/deserialize the custom message to a stream (see here).

I think that the ROS 1 feature was way more efficient because of avoiding an extra copy, the proposal here will only improve zero-copy intraprocess communication.

There are some disadvantages of the ROS 1 approach though:

  • It's harder to implement an adapter.
    Ideally, it should be possible to implement an adapter in the two ways (the simpler one, and the one that allows better performance).
  • In the current rmw architecture, it's extremely hard to implement type masquerading the "ROS 1 way".

About the second point, the problem is that though in ROS 2 you can publish/take a serialized message, there's no "rmw" interface for the serializer/deserializer (so you have to use the one your rmw vendor uses).
e.g.: "typesupports" aren't "too different" from type masquerading, it's conceptually the same, allowing more than one representation for the message (e.g. C and CPP).
But unfortunately, because of not having an standard serializer/deserializer API, we generate C/CPP typesupport code for each rmw vendor (e.g. std_msgs connext C/CPP typesupport, fastrtps C/CPP typesupport, etc; except of a rmw vendor independent std_msgs C/CPP typesupport).
The effort of improving that situation is unfortunately big.

rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
@wjwwood
Copy link
Contributor

wjwwood commented Feb 25, 2021

IIUC the proposal is that the adapter will have some functions to convert from a native type to a ros type and the opposite.
On the other hand, ROS 1 needed functions to serialize/deserialize the custom message to a stream (see here).

The plan is to support both convert and (de)serialize functions, and require at least one or the other, but also allow both.

The reason for this is that convert is superior for intra-process communication and the (de)serialization functions are better for inter-process communication.

We plan to start with the "convert" functions only because it gets the UX benefit of this feature, at the cost of extra conversions for inter-process communications, but no worse than what the user can do now.

Adding support for the (de)serialize functions is in scope for this work, but will be done in a second step most likely. But I think this REP will eventually cover a plan for both cases.

As you said, it will require either extending the rmw API to have a generic serialization API similar to Fast-CDR or something, or it will require allowing users to define custom typesupport structures, which would probably be rmw specific.

Currently we are working on the machinery to handle all of this at the rclcpp API level (template metaprogramming).

@ivanpauno
Copy link

The plan is to support both convert and (de)serialize functions, and require at least one or the other, but also allow both.

That's super cool!!!!

The reason for this is that convert is superior for intra-process communication and the (de)serialization functions are better for inter-process communication.

Yes, that's a good point.

Copy link
Contributor

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

rep-2007.rst Outdated Show resolved Hide resolved
audrow and others added 5 commits March 15, 2021 09:01
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow audrow changed the title REP 2007: Type Masquerading REP 2007: Type Adaptation Mar 16, 2021
@audrow audrow requested a review from wjwwood March 16, 2021 22:11
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
@audrow audrow marked this pull request as ready for review May 11, 2021 22:04
rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
rep-2007.rst Outdated Show resolved Hide resolved
paudrow and others added 3 commits May 11, 2021 15:28
Signed-off-by: Audrow Nash <audrow@hey.com>

Co-authored-by: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
@audrow audrow requested a review from wjwwood May 13, 2021 17:27
Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@audrow can you make a discourse post about this so we can get more eyes on it before merging it as accepted.

We've already started the reference implementation, but we have not finished all parts so it will remain "accepted" but not yet "final".

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/looking-for-feedback-on-ros-2-type-adaptation-rep/20465/1

@ruffsl
Copy link
Contributor

ruffsl commented May 19, 2021

I just read through the doc and I'm a bit concerned about the points raised in section Adding this feature in rclcpp.

What impacts would Type Adaptation bring to interoperability within the ROS software ecosystem? Could this have any negative side effects in allowing language and library specific types to bleed over into wire level data objects?

Currently, any language client library that at least supports the basic types listed in the Interface Definition and Language Mapping can easily interoperate with existing message types in the present ROS ecosystem, allowing users to abstract away what language or library is used on the other end of a topic, service or action.

For example: if using OpenCV matrixes becomes a common pattern for exchanging messages between sensor drivers or machine learning pipelines written in C++, how would downstream users integrate community packages requiring such type adaptation when the user's language and client library have no such equivalent data types or library support?

@mosteo
Copy link

mosteo commented May 19, 2021

Chiming in as an author of a third-party language client library, and seconding @ruffsl concern. I have not sufficient expertise on ROS2 internals (my interaction is purely via rcl) to comment on the feature, but every time that anything is added to rclcpp instead of rcl I feel uneasy.

If someone has the time to explain a bit the consequences for other languages I'd be grateful. From my reading, as there's the need to provide a conversion, in the end this should be transparent in the sense that non-rclcpp clients would see the ROS2-converted representation. But I'm not really sure.

@ivanpauno
Copy link

What impacts would Type Adaptation bring to interoperability within the ROS software ecosystem? Could this have any negative side effects in allowing language and library specific types to bleed over into wire level data objects?

The language or library specific type isn't actually sent on the wire.
You need to adapt it to a ROS interface in order to be able to send it on the wire, so interoperability with other languages isn't a problem.

(see the examplles in the REP, of std::string being adapted to std_msgs::msg::String)

For example: if using OpenCV matrixes becomes a common pattern for exchanging messages between sensor drivers or machine learning pipelines written in C++, how would downstream users integrate community packages requiring such type adaptation when the user's language and client library have no such equivalent data types or library support?

e.g. in that case, a ROS interface would be needed to adapt the opencv matrix to, so the other languages could use the bindings generated for that interface.

@ruffsl
Copy link
Contributor

ruffsl commented May 19, 2021

The language or library specific type isn't actually sent on the wire.
You need to adapt it to a ROS interface in order to be able to send it on the wire, so interoperability with other languages isn't a problem.

Ah, re-reading doc in the morning I see I was mistaken on that point.

(see the examplles in the REP, of std::string being adapted to std_msgs::msg::String)

Ok, would the following be the equivalent example for the more involved cv::Mat?

using AdaptedType = rclcpp::adapt_type<cv::Mat>::as<sensor_msgs::msg::Image>;

What would this then look like for rclpy, or if the matrix dimensionality >3, or if a ROI is defined? Would these be a lossy adaptation that the adapter designer have to consider? Is there way to ensure adapters in different languages are consistent with each other, or is that already captured by a separate REP standard?

@ivanpauno
Copy link

What would this then look like for rclpy, or if the matrix dimensionality >3, or if a ROI is defined? Would these be a lossy adaptation that the adapter designer have to consider?

I think that those decisions depends on the adapter, though I would only write adapters for things that have a reasonable 1 to 1 conversion.
FYI, this was already a thing in ROS 1 (e.g. http://wiki.ros.org/cv_bridge, https://github.com/ros-perception/vision_opencv/blob/906d326c146bd1c6fbccc4cd1268253890ac6e1c/cv_bridge/include/cv_bridge/cv_bridge.h#L336).

Is there way to ensure adapters in different languages are consistent with each other, or is that already captured by a separate REP standard?

ROS messages have representations on multiple languages, but the types that can be adapted to a message ROS don't necessarely have.
If they happen to have, it would be a responsability to the person writing the adapter to make sure they are consistent.

@wjwwood
Copy link
Contributor

wjwwood commented May 19, 2021

What would this then look like for rclpy

There's nothing in rclpy to handle this, and at this point, I'd argue should be a separate REP (which does not exist yet).

This is unfortunate, but it's not something that we had time for in this work. I'll also point out there was no special logic in rospy (ROS 1) to help with this either.


Separately, we could create a REP (or extend this REP), to include technical details on how different client libraries could share logic and/or avoid copies by working together via rcl or the middleware. However, as this REP said:

Placing this feature in ROS 2's client support library, rcl, would allow this feature to be used in other client libraries, such as rclcpp and rclpy.
However, we believe that the concrete benefits for C++ currently outweigh the potential benefits for existing or theoretical client libraries in other languages.

And also:

Perhaps we can support a form of this feature in other languages in rcl or rmw in the future.
One challenge in doing this is that it may require custom type support, which may be middleware specific.
This possibility will be further explored in the future.

Additionally, this enhancement (which is rclcpp exclusive) does not prevent us from doing the other described enhancements in the future. We had the resources to do this, but not the other, so we took this incrementally better step.


if the matrix dimensionality >3, or if a ROI is defined? Would these be a lossy adaptation that the adapter designer have to consider?

All of these things would be up to the author of the adapter. I think you're getting hung up on cv::Mat specifically. This REP is about allowing users to define mappings between user-defined types and ROS interface types. It is not about ensuring the mapping is correct, ideal, lossless, or that it handles various corner cases.

Is there way to ensure adapters in different languages are consistent with each other, or is that already captured by a separate REP standard?

No. Inherently, adapters in different languages will look different because the types they are adapting (at least on the custom type side) are likely different. I.e., the mat object in pyopencv is subtly different from the c++ cv::Mat.

If you want to ensure the a) an adapter exists for a specific language, or b) that it is consistent with other languages, then I think that should be handled on a case by case basis. At the moment, this adapter will probably live in the cv_bridge package, as it did in ROS 1, as the CvImage class:

It would be reasonable to have a REP about how to handle the adaption in different languages, specifically for OpenCV's Mat class and the ROS interface sensor_msgs/msg/Image, but that's not the goal of this REP.

@wjwwood
Copy link
Contributor

wjwwood commented May 19, 2021

Chiming in as an author of a third-party language client library, and seconding @ruffsl concern. I have not sufficient expertise on ROS2 internals (my interaction is purely via rcl) to comment on the feature, but every time that anything is added to rclcpp instead of rcl I feel uneasy.

If someone has the time to explain a bit the consequences for other languages I'd be grateful.

I'll try to clarify this.

We would have liked to have some functionality in rcl to help other client libraries to get some of these features, but we decided that implementing it purely in C++ was a better option because (quoting the REP as to not repeat ourselves):

placing this feature in rclcpp allows us to avoid type erasure (which would be necessary to place this functionality into rcl) and to use ownership mechanics (unique and shared pointer) to ensure it is safely implemented.
Another added advantage of placing this feature in rclcpp is that it reduce the number of function calls and calls that potentially are to functions in separate shared libraries.

As I said above in a previous comment, taking this route does not prevent us from having similar functionality in rcl or rmw in the future, but we didn't have time for that this go around.

I also would love to have as much logic in rcl as possible (I was the person who you can thank/blame for the rmw/rcl/rclXXX architecture in the first place), but for some things it doesn't make sense. This deals with ownership, memory allocation, type safety, and very specifically C++ in nature. So it makes sense to me that it needs to live in rclcpp, in part or whole.

From my reading, as there's the need to provide a conversion, in the end this should be transparent in the sense that non-rclcpp clients would see the ROS2-converted representation. But I'm not really sure.

This feature of rclcpp does not change that fact that it always calls rcl_publish() and rcl_take...() with a ROS message, or possibly uses the loaned message APIs or serialized message APIs. But at any rate the data passed into and out of the "rcl layer" has not changed. So no, this will not impact interoperability with other client libraries or tooling. In fact, from the outside, a tool like ros2 topic echo doesn't know (or care) if the node publishing is using the ROS message type or not.

At the moment, only the conversion methods (convert_to_ros_message() and convert_to_custom()) are supported, but in the future a matching pair of serialization methods will also be supported, allowing you to serialize directly to and from your custom type (as was possible in ROS 1), but again that will not impact how other client libraries or tools work with rclcpp users that use that feature.

Hopefully that answers your questions?

@ruffsl
Copy link
Contributor

ruffsl commented May 19, 2021

Thanks @wjwwood and @ivanpauno for the clarifications. This type adaption looks like it could improve the ergonomics quite a bit in using rclcpp with common message types.

At the moment, only the conversion methods (convert_to_ros_message() and convert_to_custom()) are supported, but in the future a matching pair of serialization methods will also be supported, allowing you to serialize directly to and from your custom type (as was possible in ROS 1), but again that will not impact how other client libraries or tools work with rclcpp users that use that feature.

Interesting. Would there be any other past/present docs or examples that go over this that I could check out. I'd just like to learn more.

@wjwwood
Copy link
Contributor

wjwwood commented May 19, 2021

Interesting. Would there be any other past/present docs or examples that go over this that I could check out. I'd just like to learn more.

You could read this comment and the preceding discussion:

#303 (comment)

Also, this serialization/deserialization is how it worked in ROS 1:

https://wiki.ros.org/roscpp/Overview/MessagesSerializationAndAdaptingTypes#Serializer_Specialization

There was never a "convert to and from" API in ROS 1, so only the serialization thing was an option. The convert to/from is still useful though, since it is potentially more ergonomic for the user and can be more efficient when the publisher and subscriber are in the same process.

@mosteo
Copy link

mosteo commented May 20, 2021

(I was the person who you can thank/blame for the rmw/rcl/rclXXX architecture in the first place)

Truly a great decision, I'd say. I only wish it was exploited even more :)

So no, this will not impact interoperability with other client libraries or tooling.

Thanks for taking the time to explain, @wjwwood. I understand better now the pros and cons of doing it at the cpp level.

This was commented before in this thread (by @ivanpauno):

because of not having an standard serializer/deserializer API, we generate C/CPP typesupport code for each rmw vendor (e.g. std_msgs connext C/CPP typesupport, fastrtps C/CPP typesupport, etc; except of a rmw vendor independent std_msgs C/CPP typesupport).

[My following questions are off-topic. Excuse me, and feel free to ignore, but this thread is directly touching matters I've chased for
a while.] I'm currently relying on the C typesupports and introspection to access message data in place. I could benefit from having an Ada-specific typesupport, for better language-specific representation. The first half of the quote makes me think that's too much effort (I'm not going to write vendor-specific code), but the second part in bold confuses me. Is that referring to the C introspection information that I already use?

The crux of the issue is that I'd like to avoid doing unnecessary copying of data. If one copy is unavoidable and being done by the C typesupport, I'd like to replace that by my own typesupport [if there's a vendor-agnostic way]. If the C typesupports do not need to make copies as they use data in place, then my question is moot and I need another way to make the data more friendly for my language [using pointer wizardry].

Thanks to everybody participating, very informative thread.

@audrow
Copy link
Author

audrow commented May 26, 2021

Thanks everyone for the discussion.

@audrow audrow merged commit 2ae42e7 into ros-infrastructure:master May 26, 2021
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-08-17/33061/14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.