[py] Make it easier to pass C++ node pointers in Python #285

EricCousineau-TRI · 2023-06-14T20:40:10Z

I would like to not get too creative when trying to bind C++ code that has rclcpp::Node arguments. This is one approach.
Relates ros2/rclpy#291

This change is

EricCousineau-TRI

+@sloretz Might you have time to review this?

\cc @dmcconachie-tri @calderpg-tri @IanTheEngineer

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

EricCousineau-TRI

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @sloretz)

drake_ros/drake_ros/core/cc_pybind.cc line 143 at r1 (raw file):

  py::class_<rclcpp::Node, std::shared_ptr<rclcpp::Node>>(m, "CppNode",
                                                          py::module_local())

Working: This seems to cause a segfault in Anzu. Removing py::module_local() makes things a bit happier.

EricCousineau-TRI

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @sloretz)

a discussion (no related file):
There seems to be a new CI failure, most likely unrelated to this PR:
https://github.com/RobotLocomotion/drake-ros/actions/runs/5272233626/jobs/9534218327?pr=285#step:7:345

  CMake Error at drake_ros/CMakeLists.txt:7 (target_link_libraries):
    Error evaluating generator expression:
  
      $<TARGET_PROPERTY:fastcdr,TYPE>
  
    Target "fastcdr" not found.

@sloretz Is this something we need to work around, or is there something else going awry here?

drake_ros/drake_ros/core/cc_pybind.cc line 143 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: This seems to cause a segfault in Anzu. Removing py::module_local() makes things a bit happier.

Done. For now, making this global; can take a step back if/when it interferes with another library's bindings.

adityapande-1995

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

drake_ros/drake_ros/test/core_test.py line 122 at r2 (raw file):

    assert not cpp_node_options.use_global_arguments
    node_cpp = CppNode("direct_node", node_options=cpp_node_options)
    assert node_cpp.get_name() == "direct_node"

If we are planning to expose existing rclcpp nodes to python, what APIs are we planning to connect ? i.e. can we add subscribers, publishers, callbacks, etc ?
It'd be nice to add tests for and document the features that we'll be exposing as a docstring, and also the limitations of this binding.
I'd be great if we can add a test that shows that this node can actually spin and receive / send msgs, and play alongside existing "vanilla" rclpy nodes. I'm curious to see how rclpy.spin() and rclcpp::spin() interact.

adityapande-1995

If we're planning to bind subscribers or publishers, this automatically requires a generic typecaster for data types : #257

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

EricCousineau-TRI

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @sloretz)

drake_ros/drake_ros/test/core_test.py line 122 at r2 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

If we are planning to expose existing rclcpp nodes to python, what APIs are we planning to connect ? i.e. can we add subscribers, publishers, callbacks, etc ?
It'd be nice to add tests for and document the features that we'll be exposing as a docstring, and also the limitations of this binding.

I'd be great if we can add a test that shows that this node can actually spin and receive / send msgs, and play alongside existing "vanilla" rclpy nodes. I'm curious to see how rclpy.spin() and rclcpp::spin() interact.

Working: Per Slack DM, the intent of this is to only bind the minimum amount of the C++ Node API in Python so we don't need too much creativity in writing downstream bindings.
I'm going to hold off on binding additional facets, but I will add a quick test.

(I think functionality that consumes DrakeRos effectively shows it, but yes, it'd be nice to just test directly.)

Useful when writing Python bindings against C++ code drake_ros: Switch to shared_ptr to eleverage enable_shared_from_this core: Ensure we import DrakeRos; add CppNode and CppNodeOptions

EricCousineau-TRI

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

drake_ros/drake_ros/test/core_test.py line 122 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Per Slack DM, the intent of this is to only bind the minimum amount of the C++ Node API in Python so we don't need too much creativity in writing downstream bindings.
I'm going to hold off on binding additional facets, but I will add a quick test.

(I think functionality that consumes DrakeRos effectively shows it, but yes, it'd be nice to just test directly.)

Done.

adityapande-1995

with the linter error resolved !

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @sloretz)

EricCousineau-TRI · 2023-06-21T22:51:29Z

Done, I think. I had an odd failure of CMake-based ROS failing, but will see if CI shows the same issue.

EricCousineau-TRI · 2023-06-21T23:09:18Z

@adityapande-1995 CI seems to have failed.

Can I ask if you can reproduce this (Bazel passes, CMake fails) and, if so, what might be the problem?

# Bazel
cd drake-ros
cd drake_ros
bazel run //drake_ros:core_test
# Passes

# CMake
cd ros_ws
colcon build --packages-up-to drake_ros --symlink-install
# "Shortcut" - not sure how to run single test
source install/setup.bash
( cd build/drake_ros && ctest -V -R core_test_py; )
# Fails

EricCousineau-TRI

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

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

There seems to be a new CI failure, most likely unrelated to this PR:
https://github.com/RobotLocomotion/drake-ros/actions/runs/5272233626/jobs/9534218327?pr=285#step:7:345
  CMake Error at drake_ros/CMakeLists.txt:7 (target_link_libraries):
    Error evaluating generator expression:
  
      $<TARGET_PROPERTY:fastcdr,TYPE>
  
    Target "fastcdr" not found.
@sloretz Is this something we need to work around, or is there something else going awry here?

Done (I think) - I did not see this error once I rebased onto latest main

EricCousineau-TRI

This appears to be an RMW implementation issue. I've addressed it with a workaround, will file an issue.

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

EricCousineau-TRI · 2023-06-27T19:45:11Z

Core bazel testing of drake_ros is done. I'm going to go ahead and skip drake_ros_examples because our caching is still broken.

…tLocomotion#285)" This reverts commit 4ca38a8.

EricCousineau-TRI assigned sloretz Jun 14, 2023

EricCousineau-TRI commented Jun 14, 2023

View reviewed changes

EricCousineau-TRI force-pushed the feature-rclcpp-py branch from a2e94fd to 43c13c0 Compare June 14, 2023 21:21

EricCousineau-TRI commented Jun 14, 2023

View reviewed changes

EricCousineau-TRI closed this Jun 21, 2023

EricCousineau-TRI reopened this Jun 21, 2023

adityapande-1995 reviewed Jun 21, 2023

View reviewed changes

EricCousineau-TRI force-pushed the feature-rclcpp-py branch from 43c13c0 to b8884d4 Compare June 21, 2023 20:51

EricCousineau-TRI commented Jun 21, 2023

View reviewed changes

[py] Make it easier to pass C++ node pointers in Python

f5868b9

Useful when writing Python bindings against C++ code drake_ros: Switch to shared_ptr to eleverage enable_shared_from_this core: Ensure we import DrakeRos; add CppNode and CppNodeOptions

EricCousineau-TRI force-pushed the feature-rclcpp-py branch from b8884d4 to f5868b9 Compare June 21, 2023 21:32

EricCousineau-TRI commented Jun 21, 2023

View reviewed changes

adityapande-1995 approved these changes Jun 21, 2023

View reviewed changes

fixup! appease CMake

0bfbd86

EricCousineau-TRI commented Jun 22, 2023

View reviewed changes

fixup! add workaround

5e8b52e

EricCousineau-TRI commented Jun 27, 2023

View reviewed changes

EricCousineau-TRI mentioned this pull request Jun 27, 2023

[rmw] rmw_fastrtps_dds does not work well for C++ <-> Python pub/sub testing? #286

Open

EricCousineau-TRI added 2 commits June 27, 2023 14:16

fixup! rosdep keys

ad84812

fixup! ref issue

667ee88

EricCousineau-TRI merged commit 4ca38a8 into RobotLocomotion:main Jun 27, 2023

tervay-bdai added a commit to tervay-bdai/drake-ros that referenced this pull request Jul 10, 2023

Revert "[py] Make it easier to pass C++ node pointers in Python (Robo…

291f39e

…tLocomotion#285)" This reverts commit 4ca38a8.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[py] Make it easier to pass C++ node pointers in Python #285

[py] Make it easier to pass C++ node pointers in Python #285

EricCousineau-TRI commented Jun 14, 2023 •

edited

Loading

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

adityapande-1995 left a comment

adityapande-1995 left a comment •

edited

Loading

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

adityapande-1995 left a comment

EricCousineau-TRI commented Jun 21, 2023

EricCousineau-TRI commented Jun 21, 2023 •

edited

Loading

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI commented Jun 27, 2023

[py] Make it easier to pass C++ node pointers in Python #285

[py] Make it easier to pass C++ node pointers in Python #285

Conversation

EricCousineau-TRI commented Jun 14, 2023 • edited Loading

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

adityapande-1995 left a comment

Choose a reason for hiding this comment

adityapande-1995 left a comment • edited Loading

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

adityapande-1995 left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Jun 21, 2023

EricCousineau-TRI commented Jun 21, 2023 • edited Loading

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Jun 27, 2023

EricCousineau-TRI commented Jun 14, 2023 •

edited

Loading

adityapande-1995 left a comment •

edited

Loading

EricCousineau-TRI commented Jun 21, 2023 •

edited

Loading