[py] Add generic type casters

[adityapande-1995]

This PR adds generic typecasters for geometry messages. For now, I've substituted the individual conversions using a macro, still need to work on fixing the inputs for the macro.

Resolves #229

---

This change is 

[sloretz]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @adityapande-1995)

---

drake_ros/core/geometry_conversions_pybind.h line 27 at r1 (raw file):

  \
      object py_serialize = module::import("rclpy.serialization").attr("serialize_message");\
      bytes pybytes = py_serialize(source);\

Before we can serialize, we need to make sure the typesupport has been loaded. That's what the check_for_type_support here does.

---

drake_ros/core/geometry_conversions_pybind.h line 29 at r1 (raw file):

      bytes pybytes = py_serialize(source);\
      const std::string content = pybytes;\
      const auto content_size = content.size() + 1;\

What's the +1 for? The \0 byte appended below? If so, it might not be necessary. We don't seem to do it here:

drake-ros/drake_ros/drake_ros/core/cc_pybind.cc

Lines 58 to 64 in a6afd7e

	std::string bytes = overload();
	rclcpp::SerializedMessage serialized_message(bytes.size());
	rcl_serialized_message_t& rcl_serialized_message =
	serialized_message.get_rcl_serialized_message();
	std::copy(bytes.data(), bytes.data() + bytes.size(),
	rcl_serialized_message.buffer);
	rcl_serialized_message.buffer_length = bytes.size();

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 29 at r1 (raw file):

Previously, sloretz (Shane Loretz) wrote…

What's the +1 for? The \0 byte appended below? If so, it might not be necessary. We don't seem to do it here:

drake-ros/drake_ros/drake_ros/core/cc_pybind.cc

Lines 58 to 64 in a6afd7e

	std::string bytes = overload();
	rclcpp::SerializedMessage serialized_message(bytes.size());
	rcl_serialized_message_t& rcl_serialized_message =
	serialized_message.get_rcl_serialized_message();
	std::copy(bytes.data(), bytes.data() + bytes.size(),
	rcl_serialized_message.buffer);
	rcl_serialized_message.buffer_length = bytes.size();

Hmm I thought that was more of a safety measure in case the buffers contain garbage data at the end, or is just not null terminated. Given the buffer length specifically, this should not happen, but I see the same used in unit tests here : https://github.com/ros2/rclcpp/blob/33dae5d679751b603205008fcb31755986bcee1c/rclcpp/test/rclcpp/test_serialized_message.cpp#L78-L90.
On the other hand I could add the same null termination when converting from the C++ type to Python, and that doesn't change things.

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 27 at r1 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Before we can serialize, we need to make sure the typesupport has been loaded. That's what the check_for_type_support here does.

Done

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 29 at r1 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

Hmm I thought that was more of a safety measure in case the buffers contain garbage data at the end, or is just not null terminated. Given the buffer length specifically, this should not happen, but I see the same used in unit tests here : https://github.com/ros2/rclcpp/blob/33dae5d679751b603205008fcb31755986bcee1c/rclcpp/test/rclcpp/test_serialized_message.cpp#L78-L90.
On the other hand I could add the same null termination when converting from the C++ type to Python, and that doesn't change things.

Added the null termination character for the opposite conversion as well.

[EricCousineau-TRI]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

a discussion (no related file):
Is there a way to write a type trait (SFINAE) to check if a type is a ROS message IDL type?

If so, I would like us to offer a macro such as `

---

---

drake_ros/core/geometry_conversions_pybind.h line 14 at r3 (raw file):

namespace py = pybind11;

#define ROS_MSG_TYPECAST(PKG_NAME, PKG_SUBDIR, MSG_NAME)                       \

Writing longform code in macros is very painful to grok, edit, etc.

We should strive to write this code in actual C++ as much as possible, and have the macro be as small as possible.
Can you write a base type caster to handle this core stuff, add arguments via template arguments, and have the macro just be used for defining the type_caster<> specialization?

For example:
https://github.com/RobotLocomotion/drake/blob/v1.14.0/bindings/pydrake/common/wrap_pybind.h#L60-L61
https://github.com/RobotLocomotion/drake/blob/v1.14.0/bindings/pydrake/common/cpp_param_pybind.h#L155-L158

---

drake_ros/core/geometry_conversions_pybind.h line 14 at r3 (raw file):

namespace py = pybind11;

#define ROS_MSG_TYPECAST(PKG_NAME, PKG_SUBDIR, MSG_NAME)                       \

nit This macro does not indicate usage (for pybind11). This should be qualified, e.g.

ROS_MSG_PYBIND_TYPECAST(...)

---

drake_ros/core/geometry_conversions_pybind.h line 14 at r3 (raw file):

namespace py = pybind11;

#define ROS_MSG_TYPECAST(PKG_NAME, PKG_SUBDIR, MSG_NAME)                       \

nit Does ROS IDL offer static / constexpr access to package / subdir / message name as strings?

If so, ideally we extract that from type traits, rather than requiring users to decompose it in the macro itself.
e.g.
https://github.com/RobotLocomotion/drake/blob/v1.14.0/bindings/pydrake/systems/lcm_pybind.h#L32-L33

[EricCousineau-TRI]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Is there a way to write a type trait (SFINAE) to check if a type is a ROS message IDL type?

If so, I would like us to offer a macro such as `

Ah, incomplete thought:
... macro such as ROS_MSG_PYBIND_TYPECAST_ALL(), which will declare a type_caster<> that should bind to all type casters.
We should strongly caveat that this should only be used in translation units, not header files.

---

[adityapande-1995]

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, incomplete thought:
... macro such as ROS_MSG_PYBIND_TYPECAST_ALL(), which will declare a type_caster<> that should bind to all type casters.
We should strongly caveat that this should only be used in translation units, not header files.

IIUC, do you mean the final usage should be something like :

ROS_MSG_PYBIND_TYPECAST_ALL(geometry_msgs::msg::Quaternion);
ROS_MSG_PYBIND_TYPECAST_ALL(geometry_msgs::msg::Point);
.
.
.
(and so on for all the needed types)

The macro would check if it the supplied type is a valid ros msg, extract the text form of the msg type, and invoke the specific type_caster<> for that type ?

[adityapande-1995]

Previously, adityapande-1995 (Aditya Pande) wrote…

IIUC, do you mean the final usage should be something like :

ROS_MSG_PYBIND_TYPECAST_ALL(geometry_msgs::msg::Quaternion);
ROS_MSG_PYBIND_TYPECAST_ALL(geometry_msgs::msg::Point);
.
.
.
(and so on for all the needed types)

The macro would check if it the supplied type is a valid ros msg, extract the text form of the msg type, and invoke the specific type_caster<> for that type ?

We'll still be supplying the required binding types, right ? I assume theres no way to figure out at compile type what all type castings are required automatically.

[EricCousineau-TRI]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

a discussion (no related file):
Not exactly.

Users would still (maybe should?) specify ROS_MSG_PYBIND_TYPECAST(MyMessageType)
They should then use just ROS_MSG_PYBIND_TYPECAST() if they're writing a lot of bindings against message types and want everything to convert via serialization.

We'll still be supplying the required binding types, right ?

Not sure if I understand. These macros are meant to supply casting via serilaization, so as long as I have the C++ type and the Python type, I should be able to convert a message completely.
If the C++ type exposes something like ::package_name() and ::message_name(), then we can use that to dynamically look up the Python type.

I assume theres no way to figure out at compile type [...]

Also not sure if I understand.
pybind11 type casting works by effectively calling type_caster<MyType> and letting C++ specializations figure out where this ends up.
If there's a specialization that binds to the type (highest affinity), it will use that.
If there's no specialization, it will use type_caster_generic, and thus rely on the py::class_<> RTTI registry.

ROS_MSG_PYBIND_TYPECAST(Type) will provide a specialization for a specific type, and should have highest affinity.
If there is a SFINAE such that we can write is_ros_message_type_v<Type>, then we can write enable_if_t<is_ros_message_type_v<Type>>, and thus ensure type_caster<Type> binds to our serialization caster for all ROS message types.

Does this make sense?

---

[EricCousineau-TRI]

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

---

drake_ros/core/geometry_conversions_pybind.h line 39 at r3 (raw file):

      const auto content_size = content.size() + 1;                            \
                                                                               \
      rclcpp::SerializedMessage serialized_message(content_size);              \

When you carve this apart from the macro, can you see if it's easy to factor some of these into functions that can also be used by PySerializerInterface?

I had tried to do something to that effect in my prototype #169
Can you take a look there?

[adityapande-1995]

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Not exactly.

Users would still (maybe should?) specify ROS_MSG_PYBIND_TYPECAST(MyMessageType)
They should then use just ROS_MSG_PYBIND_TYPECAST() if they're writing a lot of bindings against message types and want everything to convert via serialization.

We'll still be supplying the required binding types, right ?

Not sure if I understand. These macros are meant to supply casting via serilaization, so as long as I have the C++ type and the Python type, I should be able to convert a message completely.
If the C++ type exposes something like ::package_name() and ::message_name(), then we can use that to dynamically look up the Python type.

I assume theres no way to figure out at compile type [...]

Also not sure if I understand.
pybind11 type casting works by effectively calling type_caster<MyType> and letting C++ specializations figure out where this ends up.
If there's a specialization that binds to the type (highest affinity), it will use that.
If there's no specialization, it will use type_caster_generic, and thus rely on the py::class_<> RTTI registry.

ROS_MSG_PYBIND_TYPECAST(Type) will provide a specialization for a specific type, and should have highest affinity.
If there is a SFINAE such that we can write is_ros_message_type_v<Type>, then we can write enable_if_t<is_ros_message_type_v<Type>>, and thus ensure type_caster<Type> binds to our serialization caster for all ROS message types.

Does this make sense?

Got it, added the ROS_MSG_PYBIND_TYPECAST_ALL() macro. The only caveat is that the second parameter to the PYBIND_TYPE_CASTER() macro, which is supposed to be a docstring for the method, needs to be a constexpr. rosidl does not provide that currently. Ideally the string should be a constexpr form of say "geometry_msgs.msg.Quaternion". I've left it as a blank string for now.

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 14 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Writing longform code in macros is very painful to grok, edit, etc.

We should strive to write this code in actual C++ as much as possible, and have the macro be as small as possible.
Can you write a base type caster to handle this core stuff, add arguments via template arguments, and have the macro just be used for defining the type_caster<> specialization?

For example:
https://github.com/RobotLocomotion/drake/blob/v1.14.0/bindings/pydrake/common/wrap_pybind.h#L60-L61
https://github.com/RobotLocomotion/drake/blob/v1.14.0/bindings/pydrake/common/cpp_param_pybind.h#L155-L158

Done.

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 14 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This macro does not indicate usage (for pybind11). This should be qualified, e.g.

ROS_MSG_PYBIND_TYPECAST(...)

Changed to the TYPECAST_ALL style macro.

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 14 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Does ROS IDL offer static / constexpr access to package / subdir / message name as strings?

If so, ideally we extract that from type traits, rather than requiring users to decompose it in the macro itself.
e.g.
https://github.com/RobotLocomotion/drake/blob/v1.14.0/bindings/pydrake/systems/lcm_pybind.h#L32-L33

I used the rosidl type traits for the actual conversions, but couldn't use for the docstring. Left that section as a black for now.

[adityapande-1995]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Ah, incomplete thought: ... macro such as ROS_MSG_PYBIND_TYPECAST_ALL(), which will declare a type_caster<> that should bind to all type casters. We should strongly caveat that this should only be used in translation units, not header files.

Do we need a macro in this case ? We could just have the code section as it is.

[adityapande-1995]

Converting to a draft until a couple of things are addressed :

• Splitting serializing and deserializing into smaller functions
• Do we need a macro at all ?
• Is it okay to ignore the docstring in PYBIND11_TYPE_CASTER() macro ?

[EricCousineau-TRI]

Splitting serializing and deserializing into smaller functions

I think this is done? (or at least in a state that I'd be happy to merge at present)

Do we need a macro at all ?

Yup! I would like both.

Is it okay to ignore the docstring in PYBIND11_TYPE_CASTER() macro?

I'd be fine with that, but I would just like a more concrete pointer to the generated method (see note below).

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

a discussion (no related file):

rosidl does not provide that currently

Is that truly the case with ::name()? Can you provide me a link to a gist / or a repo with generated code where I can see the signature of this generated method?

---

---

drake_ros/core/geometry_conversions_pybind.h line 39 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

When you carve this apart from the macro, can you see if it's easy to factor some of these into functions that can also be used by PySerializerInterface?

I had tried to do something to that effect in my prototype #169
Can you take a look there?

OK Sweet, much better!

---

drake_ros/core/geometry_conversions_pybind.h line 18 at r4 (raw file):

// Generic (C++ <-> Python) typecaster for all ROS 2 messages.
#define ROS_MSG_PYBIND_TYPECAST_ALL()                                         \

Sorry for not being clearer, but I think we should provide users two entry points:

• The *_ALL() flavor
• A message-specific flavor

The guts of the ROS 2 IDL <-> pybind11 should happen inside of our own namespace drake_ros::core, e.g.

Does this make sense?

---

drake_ros/core/geometry_conversions_pybind.h line 38 at r4 (raw file):

// Get the name of the message as a string.
template <typename T>

For succinctness / clarity of authoring, most of our code should live in our own namespace.
We should then make thin shells in pybind11::detail that shell out to our code.

[EricCousineau-TRI]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

a discussion (no related file):
This needs small testing examples that aren't just geometry messages (e.g. using ALL and the per-message macro in separate translation units).

---

---

drake_ros/core/geometry_conversions_pybind.h line 1 at r4 (raw file):

#include <regex>

This is now much more general. Can you rename this to ros_idl_pybind.h or something to that effect?

You should still make a header, geometry_conversions_pybind.h, that would do per-message declaration.
Alternatively, for where you bind the message types, you could use the *_ALL variant.

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 18 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Sorry for not being clearer, but I think we should provide users two entry points:

• The *_ALL() flavor
• A message-specific flavor

The guts of the ROS 2 IDL <-> pybind11 should happen inside of our own namespace drake_ros::core, e.g.

Does this make sense?

Split the namespaces, done.

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 38 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

For succinctness / clarity of authoring, most of our code should live in our own namespace.
We should then make thin shells in pybind11::detail that shell out to our code.

Done, split namespaces.

[adityapande-1995]

Hi @EricCousineau-TRI !

Yes, splitting the functions into separate ones is done. One question : Now that we have both macros, how do you want the test to work ? i.e. The typecaster ultimately is a part of cc_pybind.cc (and exposed using import drake_ros.core). IIUC, we want a the python test to run twice : once using the ALL() typecaster, and once using the per msg typecasters. So ultimately, these 2 sections are equivalent as far as the test is concerned :

In ros_idl_pybind.h :

namespace PYBIND11_NAMESPACE {
namespace detail {
// Generic typecaster for all ROS 2 messages.
ROS_MSG_PYBIND_TYPECAST_ALL();
}
}

In geometry_conversions_pybind.h :

namespace PYBIND11_NAMESPACE {
namespace detail {
// Generic typecaster for specific ROS 2 messages.
// This method can be used instead of the ROS_MSG_PYBIND_TYPECAST_ALL() macro.
ROS_MSG_PYBIND_TYPECAST(geometry_msgs::msg::Quaternion);
ROS_MSG_PYBIND_TYPECAST(geometry_msgs::msg::Point);
ROS_MSG_PYBIND_TYPECAST(geometry_msgs::msg::Vector3);
ROS_MSG_PYBIND_TYPECAST(geometry_msgs::msg::Twist);
ROS_MSG_PYBIND_TYPECAST(geometry_msgs::msg::Accel);
ROS_MSG_PYBIND_TYPECAST(geometry_msgs::msg::Wrench);
ROS_MSG_PYBIND_TYPECAST(geometry_msgs::msg::Pose);
ROS_MSG_PYBIND_TYPECAST(geometry_msgs::msg::Transform);
}
}

Do you want like an #ifdef flag in cc_pybind.cc that selects the appropriate #include during compile time ? So by default it can go with the ALL() style, and if we set the flag it goes for the per msg style.

[adityapande-1995]

For the constexpr name, this is the generated function for say the Quaternion msg:

template<>
inline const char * name<geometry_msgs::msg::Quaternion>()
{
  return "geometry_msgs/msg/Quaternion";
}

This is in the generated file quaternion__traits.hpp. You can do a find -L . | grep quaternon__traits inside drake_ros after it is built. The compiler complains that const char* cannot be converted to a const char[].

Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @EricCousineau-TRI and @sloretz)

[EricCousineau-TRI]

Reviewed 1 of 1 files at r6.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

a discussion (no related file):

For the constexpr name, this is the generated function for say the Quaternion msg:

Yes, that makes sense that const char* cannot be used in a truly contexpr sense.
It seems defective to not provide contexpr access to generated types, though.

Can you file an upstream issue, and track it here via TODO?

---

a discussion (no related file):

One question : Now that we have both macros, how do you want the test to work ?

Let's keep it simple. ros_idl_pybind.h only declares the macros, it does not use them.
geometry_conversions_pybind.h only uses the per-type conversion marcos.

For testing, we should just create two entirely separate translation units (*_pybind.cc or _py.cc).
One that briefly tests a per-message macro, one that tests the *_ALL() macro.

---

---

drake_ros/core/geometry_conversions_pybind.h line 1 at r4 (raw file):

You should still make a header, geometry_conversions_pybind.h, that would do per-message declaration.

Can I ask if you can address this?

---

drake_ros/core/ros_idl_pybind.h line 21 at r6 (raw file):

  template <typename T>                                                     \
  struct type_caster<                                                       \
      T, std::enable_if_t<rosidl_generator_traits::is_message<T>::value>> { \

You should be able to inherit form another typecaster; that should reduce the repeated code.
See prior example pointed out:
https://github.com/RobotLocomotion/drake/blob/v1.14.0/bindings/pydrake/common/cpp_param_pybind.h#L155-L158

[adityapande-1995]

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

For the constexpr name, this is the generated function for say the Quaternion msg:

Yes, that makes sense that const char* cannot be used in a truly contexpr sense.
It seems defective to not provide contexpr access to generated types, though.

Can you file an upstream issue, and track it here via TODO?

Done

[adityapande-1995]

drake_ros/core/ros_idl_pybind.h line 21 at r6 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

You should be able to inherit form another typecaster; that should reduce the repeated code.
See prior example pointed out:
https://github.com/RobotLocomotion/drake/blob/v1.14.0/bindings/pydrake/common/cpp_param_pybind.h#L155-L158

Done

[adityapande-1995]

drake_ros/core/geometry_conversions_pybind.h line 1 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

You should still make a header, geometry_conversions_pybind.h, that would do per-message declaration.

Can I ask if you can address this?

Added a geometry_conversions_pybind.h that uses the message specific declaration.

[adityapande-1995]

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

One question : Now that we have both macros, how do you want the test to work ?

Let's keep it simple. ros_idl_pybind.h only declares the macros, it does not use them.
geometry_conversions_pybind.h only uses the per-type conversion marcos.

For testing, we should just create two entirely separate translation units (*_pybind.cc or _py.cc).
One that briefly tests a per-message macro, one that tests the *_ALL() macro.

Done !

[adityapande-1995]

drake_ros/drake_ros/core/cc_pybind_mock.cc line 38 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Same as above.

Done

[adityapande-1995]

drake_ros/drake_ros/core/cc_pybind_mock.cc line 20 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Per GSG, can you use CamelCase here?

Done

[adityapande-1995]

drake_ros/drake_ros/test/generic_typecaster_test.py line 10 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you move these comments to the function, using docstring """ ... """?

Done

[adityapande-1995]

drake_ros/drake_ros/BUILD.bazel line 128 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Name generic_typecaster is unclear.

I recommend renaming the test to something more specific, e.g. ros_message_type_caster_test

Done

[adityapande-1995]

drake_ros/drake_ros/BUILD.bazel line 43 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit These functions are for testing.

Can you place them in the appropriate test/ folder, and collocate them?

Done

[adityapande-1995]

drake_ros/drake_ros/BUILD.bazel line 41 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This should be testonly = 1.

Can you add that attribute here?

Done

[adityapande-1995]

drake_ros/drake_ros/BUILD.bazel line 35 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Purpose of this binding library unclear.

Can you place this closer to generic_typecaster_test, and perhaps change name to something as follows?
ros_message_type_caster_test_cc_py

You should also adjust cc_so_name = "ros_message_type_caster_test_cc"

Done

[adityapande-1995]

drake_ros/core/ros_idl_pybind.h line 118 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Blank string for type will make it very hard to reason about error messages.

Can you perhaps put something like RosTypeCaster[Unknown] for the name, so users can search for it?

Done.

[adityapande-1995]

drake_ros/core/ros_idl_pybind.h line 116 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This caster type should live in drake_ros::drake_ros_py.

Can you move it there?

Done

[adityapande-1995]

drake_ros/core/ros_idl_pybind.h line 113 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you move this TODO closer to the usage of the name for PYBIND11_TYPE_CASTER?

Done

[adityapande-1995]

drake_ros/core/ros_idl_pybind.h line 116 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Name unclear. Should be qualified by ros_message somewhere in name.

Consider ros_message_type_caster.

Done

[adityapande-1995]

drake_ros/core/ros_idl_pybind.h line 26 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Single-message declaration should ideally declare a name.

Can you change the macro to look something like this?

ROS_MESSAGE_PYBIND_TYPECASTER(T) \
   namespace pybind11 { namespace detail {
   template <> \
   struct type_caster<T> : public ros_message_type_caster<T> { \
     public: \
       static constexpr auto name = _("TypeCaster[" #T "]"); \
   } \

When pybind11 error messages crop up, then they should see something like:

incompatible function arguments... The following types are supported:
   1. (msg: RosTypeCaster[geometry_msgs::Vector3])

How should we handle this now the we are deriving from a common struct for the typecaster ?

[adityapande-1995]

drake_ros/drake_ros/cc_py_generic_typecaster.cc line 14 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This should have two translation units, and two functions. It should have functions that looks something like:

DefTypecastingViaSpecificTypes(m);
DefTypecastingViaAll(m);

I would recommend renaming those translation units to

test/ros_message_type_caster_test_via_specific_types_pybind.cc
test/ros_message_type_caster_test_via_all_pybind.cc

So in the module ultimately, we'll have both the specific and the generic typecaster at the same time ? How do we know if the module is internally using the all or the specific typecasters in that case ? The actual drake_ros geometry conversion has test for the specific types. I mean, in the test here :

import drake_ros.ros_message_all_typecaster_test_module as all_typecaster

def foo():
  assert all_typecaster.TestTypecasting(.....) == expected_msg

this assumes the module only contains the all style typecaster. Should we separate the modules if we want to make that clear ?

[jwnimmer-tri]

BTW @adityapande-1995 when using Reviewable, there is a big green "Publish" in the upper right, which publishes all of your responses in a batch. The little in the corner of each discussion thread should generally not be used, as it sends an email all on its own, leading to an email flood.

[adityapande-1995]

drake_ros/drake_ros/cc_py_generic_typecaster.cc line 14 at r7 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

So in the module ultimately, we'll have both the specific and the generic typecaster at the same time ? How do we know if the module is internally using the all or the specific typecasters in that case ? The actual drake_ros geometry conversion has test for the specific types. I mean, in the test here :

import drake_ros.ros_message_all_typecaster_test_module as all_typecaster

def foo():
  assert all_typecaster.TestTypecasting(.....) == expected_msg

this assumes the module only contains the all style typecaster. Should we separate the modules if we want to make that clear ?

I added a temp commit here : 918db4b which adds 2 separate translation units, one with the all-style typecaster and one with the specific ones. For the test, the Polygon msg typecaster is not declared using the specific types, and the corresponding test should fail. Instead, see the test pass, probably using the all-typecaster internally. That is, in this arrangement, we can't have a test that would fail for specific typecaster but pass for the all() one.

Just added this temp commit to demonstrate, I can revert if we can't resolve this.

Internally, we're probably polluting the py::detail namespace with both typecasters, so AFA the python module is concerned, the translation units are not separating the typecasters.

[sloretz]

Reviewed 2 of 5 files at r9, all commit messages.
Reviewable status: 5 of 9 files reviewed, 7 unresolved discussions (waiting on @adityapande-1995 and @EricCousineau-TRI)

---

drake_ros/core/geometry_conversions_pybind.h line 8 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

These macros should be called at top-level scope.

For example, it should function as PYBIND11_MAKE_OPAQUE does.
https://github.com/pybind/pybind11/blob/ed466da571fbc1d711351eb818e4bf82adb99eca/include/pybind11/cast.h#L1659-L1665
Or PYBIND11_NUMPY_DTYPE
https://github.com/pybind/pybind11/blob/ed466da571fbc1d711351eb818e4bf82adb99eca/include/pybind11/numpy.h#L1509-L1512

Are you saying the namespace ... declarations should live inside the macros?

---

drake_ros/core/ros_idl_pybind.h line 26 at r7 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

How should we handle this now the we are deriving from a common struct for the typecaster ?

Given the lack of a way to get a constexpr name for a ROS message type, I'd recommend adding a TODO above this macro definition referencing ros2/rosidl#734 as the resolution for this thread for now.

---

drake_ros/drake_ros/cc_py_generic_typecaster.cc line 14 at r7 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

I added a temp commit here : 918db4b which adds 2 separate translation units, one with the all-style typecaster and one with the specific ones. For the test, the Polygon msg typecaster is not declared using the specific types, and the corresponding test should fail. Instead, see the test pass, probably using the all-typecaster internally. That is, in this arrangement, we can't have a test that would fail for specific typecaster but pass for the all() one.

Just added this temp commit to demonstrate, I can revert if we can't resolve this.

Internally, we're probably polluting the py::detail namespace with both typecasters, so AFA the python module is concerned, the translation units are not separating the typecasters.

Based on your findings Aditya, it sounds we can't be sure the test is actually using the typecaster from the "type specific" macro when the "alll types" macro is used, even if it was used in a different translation unit. I'd recommend moving forward with tests for each macro being in different CPython extensions.

---

drake_ros/drake_ros/test/generic_typecaster_test.py line 27 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you explicitly test what an incorrect / mismsatched type looks like?

Ideally, the error message should provide the user with enough information so they can kind of figure out what's going on.

I think we get that for free from pytest. IIRC it will print the values of all parts of the expression used in the assert statement.

[EricCousineau-TRI]

Reviewed 1 of 5 files at r8, 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

---

drake_ros/core/geometry_conversions_pybind.h line 8 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Are you saying the namespace ... declarations should live inside the macros?

Sorry for the delay - yup!

---

drake_ros/drake_ros/test/ros_message_type_caster_test_via_specific_types_pybind.cc line 36 at r9 (raw file):

  m.doc() = "Python bindings for testing the generic typecaster";

  // py::class_<DrakeRos>(m, "DrakeRos");

nit Unused?

---

drake_ros/drake_ros/cc_py_generic_typecaster.cc line 14 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Based on your findings Aditya, it sounds we can't be sure the test is actually using the typecaster from the "type specific" macro when the "alll types" macro is used, even if it was used in a different translation unit. I'd recommend moving forward with tests for each macro being in different CPython extensions.

Working: Hm... that doesn't sound right. type_caster<T> for a non-registered type should only be able to bind through a translation unit.
Will briefly test that out.

---

drake_ros/drake_ros/test/generic_typecaster_test.py line 27 at r7 (raw file):

Previously, sloretz (Shane Loretz) wrote…

I think we get that for free from pytest. IIRC it will print the values of all parts of the expression used in the assert statement.

OK SGTM for now!

[EricCousineau-TRI]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

a discussion (no related file):
Working: I have a few other fixes incoming.

---

[EricCousineau-TRI]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

---

drake_ros/core/ros_idl_pybind.h line 30 at r9 (raw file):

  struct type_caster<T> : public drake_ros::drake_ros_py::ros_message_type_caster<T> {};

namespace drake_ros {

nit This file should ideally only define things in drake_ros::drake_ros_py.

---

drake_ros/core/ros_idl_pybind.h line 112 at r9 (raw file):

namespace drake_ros {
namespace drake_ros_py {
using namespace std;

We should not do using namespace if it can be avoided (and I believe we can avoid it).

---

drake_ros/core/ros_idl_pybind.h line 113 at r9 (raw file):

namespace drake_ros_py {
using namespace std;
using handle = py::handle;

nit These other using statemetns can be placed in a macro.

[EricCousineau-TRI]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

---

drake_ros/drake_ros/cc_py_generic_typecaster.cc line 14 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Hm... that doesn't sound right. type_caster<T> for a non-registered type should only be able to bind through a translation unit.
Will briefly test that out.

Yeah, I'm finding the same behavior. In this case, we should just create separate modules.

[EricCousineau-TRI]

Reviewable status: 3 of 12 files reviewed, 4 unresolved discussions (waiting on @adityapande-1995 and @sloretz)

---

.gitignore line 2 at r10 (raw file):

bazel-*
/.vscode/

Working: Oops should remove this.

---

drake_ros/core/geometry_conversions_pybind.h line 8 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Sorry for the delay - yup!

Working: Will fix.

---

drake_ros/core/ros_idl_pybind.h line 30 at r9 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This file should ideally only define things in drake_ros::drake_ros_py.

Done.

---

drake_ros/core/ros_idl_pybind.h line 112 at r9 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

We should not do using namespace if it can be avoided (and I believe we can avoid it).

Done.

---

drake_ros/core/ros_idl_pybind.h line 113 at r9 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit These other using statemetns can be placed in a macro.

Done.

---

drake_ros/drake_ros/cc_py_generic_typecaster.cc line 14 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Yeah, I'm finding the same behavior. In this case, we should just create separate modules.

Done.

My guess is that the linker was able to find specializations of type_caster<T>... though I'm not exactly sure how that works.

[EricCousineau-TRI]

w/ adjustments and pending CI, thanks!

Reviewable status: 2 of 12 files reviewed, all discussions resolved (waiting on @sloretz)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: I have a few other fixes incoming.

Done.

---

---

drake_ros/core/geometry_conversions_pybind.h line 8 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Will fix.

Done.

---

.gitignore line 2 at r10 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Oops should remove this.

Done.

---

drake_ros/drake_ros/test/ros_message_type_caster_test_via_specific_types_pybind.cc line 36 at r9 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Unused?

Done.

[EricCousineau-TRI]

Reviewed 6 of 9 files at r10, 3 of 3 files at r11, 1 of 2 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @adityapande-1995)