Use Pybind11 for name functions by sloretz · Pull Request #709 · ros2/rclpy

sloretz · 2021-03-10T22:19:41Z

Part of #665

This uses pybind11 for name-related functions. I also added RCUtilsError since it was useful for one of the functions, and relaxed test expectations on the TypeError human readable text since the text output by pybind11 is a little bit different when an argument is the wrong type.

sloretz · 2021-03-10T22:31:24Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-03-11T18:48:59Z

Rebased on master

cottsay

A few quick comments to address.

It's worth noting that several of the return types are changing from List to Tuple in this PR, though it's probably the right thing to do since it matches the documentation and is more performant.

cottsay · 2021-03-15T23:29:25Z

rclpy/src/rclpy/names.cpp

+  if (rcutils_ret == RCUTILS_RET_BAD_ALLOC) {
+    rcl_reset_error();
+    throw std::bad_alloc();
+  } else if (rcutils_ret != RCUTILS_RET_OK) {
+    throw RCLError("failed to initialize string map");
+  }


This should be rcutils error handling, not rcl.

The docs should be updated to reflect the other exception type, too.

Updated docs with RMWError, RCUtilsError, or RCLError in c269b6a

Oops, actually fixed this one in d7fd3b0

cottsay · 2021-03-15T23:48:54Z

rclpy/src/rclpy/names.hpp

+/// Validate a node name and return error message and index of invalidation.
+/**
+ * Raises MemoryError if memory could not be allocated
+ * Raises RuntimeError if an unexpected error happened while validating the node name


Suggested change

* Raises RuntimeError if an unexpected error happened while validating the node name

* Raises RCLError if an unexpected error happened while validating the node name

I think this doc got fixed in c269b6a too

cottsay · 2021-03-15T23:51:38Z

rclpy/src/rclpy/names.hpp

+
+/// Expand a topic name
+/**
+ * Raises ValueError if the topic name, node name, or namespace are not valid.


These last three functions can raise RCLError. Is that omitted intentionally?

Added note about raising RCLError in c269b6a

hidmic

Overall looks great!

hidmic · 2021-03-16T16:42:13Z

rclpy/src/rclpy/names.cpp

+  int validation_result;
+  size_t invalid_index;
+  rmw_ret_t ret = rmw_validate_namespace(namespace_, &validation_result, &invalid_index);
+  if (ret == RCL_RET_BAD_ALLOC) {


@sloretz in the same vein as @cottsay comments about rcutils error handling vs. rcl error handling, it'd be good to deal with rmw error handling using rmw API.

I think there a similar issue in get_validation_error_for_full_topic_name, get_validation_error_for_node_name.

On further investigation, there seems to be some handling for this in rcl. They seem to be translated to rcl error codes though?
https://github.com/ros2/rcl/blob/0ca8c6162136594e490ba69fbf288de1b5f7073d/rcl/src/rcl/expand_topic_name.c#L79

Both rmw and rcl error handling APIs are aliases of rcutils'. That's why people tend to use them interchangeably. Which means we'll have a huge body of code to migrate if that stops being true at some point.

Regardless, I think it is worthwhile to use the correct types (even if they are aliases of each other). Besides the fact that it makes migration possible (though that is unlikely), there is no question for future readers of the code on whether this is correct or not.

I added RMWError in fc32a0a and used rcl, rcutuils, or rmw constants and functions for each case in c269b6a

hidmic · 2021-03-16T16:44:13Z

rclpy/src/rclpy/names.cpp

+    // finalize the string map before returning
+    rcutils_ret = rcutils_string_map_fini(&substitutions_map);
+    if (rcutils_ret != RCUTILS_RET_OK) {
+      fprintf(


@sloretz FYI there's RCUTILS_SAFE_FWRITE_TO_STDERR too.

Oops, I skimmed over the fact you are actually formatting. Either disregard or drop the integer formatting and use two safe writes.

Uses two calls to RCUTILS_SAFE_FWRITE_TO_STDERR in 90f1052

hidmic · 2021-03-16T16:48:20Z

rclpy/src/rclpy/names.cpp

+    throw RCLError("failed to get node options");
+  }
+
+  char * output_cstr = NULL;


@sloretz nit:

Suggested change

char * output_cstr = NULL;

char * output_cstr = nullptr;

NULL -> nullptr in dcfc7e2

rclpy/src/rclpy/names.cpp

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2021-03-17T02:07:08Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

hidmic

LGTM but for a few minor nits I won't block on.

hidmic · 2021-03-17T14:10:51Z

rclpy/src/rclpy/names.cpp

+  auto resolved_name = std::unique_ptr<char, decltype(name_deleter)>(
+    output_cstr, name_deleter);
+
+  if (ret != RCL_RET_OK) {


@sloretz nit^N: flip operands, here and elsewhere.

Flipped conditionals in 193a026

hidmic · 2021-03-17T14:11:47Z

rclpy/src/rclpy/names.cpp

+      node_options->allocator.deallocate(name, node_options->allocator.state);
+    };
+  auto resolved_name = std::unique_ptr<char, decltype(name_deleter)>(
+    output_cstr, name_deleter);


@sloretz FYI, nit^N: RCPPUTILS_SCOPE_EXIT may be handy in these cases.

Noted, I'll use this on the next PR once the rcpputils dependency is added.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2021-03-17T16:29:46Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

sloretz self-assigned this Mar 10, 2021

sloretz mentioned this pull request Mar 10, 2021

Use Pybind11 for rclpy Python Bindings #665

Closed

34 tasks

sloretz requested review from azeey and cottsay March 11, 2021 16:07

sloretz force-pushed the pybind11_names branch from 9a6e823 to dbe25de Compare March 11, 2021 18:48

cottsay requested changes Mar 15, 2021

View reviewed changes

hidmic reviewed Mar 16, 2021

View reviewed changes

sloretz added 6 commits March 16, 2021 18:04

Add RCUtilsError

3edf168

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Make name related functions use pybind11

a17b9ab

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Add RMWError

fc32a0a

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

NULL -> nullptr

dcfc7e2

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Include names.hpp, not context.hpp

c4e5b6b

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Use rcl, rcutils, or rmw error handling

c269b6a

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz force-pushed the pybind11_names branch from dbe25de to c269b6a Compare March 17, 2021 01:52

sloretz added 3 commits March 16, 2021 18:58

Use RCUTILS_SAFE_FWRITE_TO_STDERR

90f1052

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Use RCUtilsError instead of RCLError

d7fd3b0

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Use unique_ptr to free memory if string constructor throws

f2d242a

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz mentioned this pull request Mar 17, 2021

Convert serialize/deserialize to pybind11 #712

Merged

sloretz requested review from cottsay, gbalke and hidmic March 17, 2021 04:00

hidmic approved these changes Mar 17, 2021

View reviewed changes

Switch order of conditionals

193a026

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz requested a review from hidmic March 17, 2021 16:55

hidmic approved these changes Mar 17, 2021

View reviewed changes

sloretz merged commit ea26f5f into master Mar 17, 2021

delete-merged-branch bot deleted the pybind11_names branch March 17, 2021 18:57

	* Raises RuntimeError if an unexpected error happened while validating the node name
	* Raises RCLError if an unexpected error happened while validating the node name

Conversation

sloretz commented Mar 10, 2021

Uh oh!

sloretz commented Mar 10, 2021

Uh oh!

sloretz commented Mar 11, 2021

Uh oh!

cottsay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sloretz commented Mar 17, 2021

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hidmic Mar 16, 2021 •

edited

Loading