Implement node parameters and parameter services by nuclearsandwich · Pull Request #214 · ros2/rclpy

nuclearsandwich · 2018-08-06T18:52:57Z

This adds parameter services to rclpy nodes. Like rclcpp nodes, parameter services are started by default and can be turned off with the create_node/Node constructor kwarg start_parameter_services=False.

Parameters are stored on the Node class via a _parameters dict attribute. The methods following methods have been added to the Node API.

get_parameter(name)
get_parameters(names)
set_parameters(parameters)
set_parameters_atomically(parameters)

A Parameter class provides a python interface to individual parameters that is (hopefully) a bit more ergonomic than using the Parameter-related message types from rcl_interfaces.

connects to #202

mikaelarguedas

a couple questions on the expected behavior before diving into actual code review

mikaelarguedas · 2018-08-06T19:13:44Z

rclpy/rclpy/parameter.py

+        return cls(rcl_param.name, type_, value)
+
+    def __init__(self, name, type_, value):
+        assert isinstance(type_, Parameter.Type)


should we raise a TypeError exception rather than asserting ?

I wasn't certain if TypeErrors were the correct exception and there were other assert isinstance(... instances throughout. I'd be inclined to flip these to TypeErrors if that's the usual pattern.

per today's offline discussion we'll move type assertions to raise TypeErrors in this PR and I'll try to send a followup PR migrating assertions to type errors in the existing code that I used as an example.

mikaelarguedas · 2018-08-06T19:15:14Z

rclpy/rclpy/parameter.py

+    def get_descriptor(self):
+        return ParameterDescriptor(name=self.name, type=self.type.value)
+
+    def get_parameter_value(self):


What happens if the parameter is NOT_SET ? it looks like I could do:

set_parameters([Parameter('not_set_param', Parameter.Type.NOT_SET, 42)]) get_parameter('not_set_param').value

And successfully retrieve 42. Is this expected?

If per your later question we end up enforcing parity between declared type and given value that closes this odd behavior.

I ended up creating a positional args constructor for Parameter but maybe I should change it to be a kwarg constructor to make it possible to omit the value for a 'not set' parameter. Deleting parameters isn't currently part of the rclcpp parameter API but we could make it part of the rclpy API if we want.

per today's offline discussion we'll add code to parameter initialization to prevent setting values that don't match declared type (including setting any value for a parameter of type NOT_SET.

This line of review is resolved by adding creation-time type checking in 39698fb.

mikaelarguedas · 2018-08-06T19:47:57Z

rclpy/rclpy/parameter.py

+    def get_parameter_value(self):
+        parameter_value = ParameterValue(type=self.type.value)
+        if Parameter.Type.BOOL == self.type:
+            parameter_value.bool_value = self.value


should we enforce types here (and below) ? it looks like is can do the following:

self.node.set_parameters([Parameter('not_set_param', Parameter.Type.BOOL, 'foo')]) self.assertIsInstance(self.node.get_parameter('not_set_param'), Parameter) self.assertEqual(self.node.get_parameter('not_set_param').value, 'foo')

I think it's possible to enforce types on the Parameters. An earlier implementation of the Parameter class determined the type from the value given to the constructor but when I added the Parameter.Type enum to wrap the interface enum values I dropped all of that.

per today's offline discussion we'll add code to parameter initialization to prevent setting values that don't match declared type (including setting any value for a parameter of type NOT_SET.

mikaelarguedas · 2018-08-06T21:52:36Z

rclpy/rclpy/parameter_service.py

+        nodename = node.get_name()
+
+        describe_parameters_service_name = '/'.join((nodename, 'describe_parameters'))
+        node.create_service(


I think we should use the parameter qos profile by default for all these services (I believe that would match rclcpp's implementation):

rclpy/rclpy/rclpy/qos.py

Line 179 in 98cdade

qos_profile_parameters = _rclpy.rclpy_get_rmw_qos_profile(

And the parameter event publisher should use the corresponding profile as well (but I think this will be in a follow-up PR)

mikaelarguedas · 2018-08-06T21:57:23Z

This looks great !

I didn't get a chance to thoroughly review it yet so just dropped a note for the questions I had. I'll give a review tomorrow morning if noone gets to it by then

mikaelarguedas

the overall logic looks good to me 👍
I added a few comments for what's not covered by previous review.

2 questions:

Do you have a set of prefixed parameters used for testing ?
Where will the testing of the callbacks happen ? in a separate PR here ? or just in a higher level integration test?

mikaelarguedas · 2018-08-07T21:59:19Z

rclpy/rclpy/parameter_service.py

+        if not 0 == request.depth:
+            # A depth of zero indicates infinite depth
+            names_with_prefixes = filter(
+                lambda name: name.count('.') + 1 <= request.depth, names_with_prefixes


Nit: name.count('.') < request.depth

mikaelarguedas · 2018-08-07T22:00:03Z

rclpy/rclpy/parameter_service.py

+    def _list_parameters_callback(self, request, response):
+        names_with_prefixes = []
+        for name in self._node._parameters.keys():
+            if '.' in name:


'.' should be made a constant and reused everywhere in the code so that we need to change only one place if we ever use a different separator (related to ros2/ros2#402)

It looks like this comment is still pending (same for other uses of '.' below)

Whoops, thought I got all of them. Addressed by 67436b0

mikaelarguedas · 2018-08-07T22:07:44Z

rclpy/rclpy/parameter_service.py

+            return response
+
+        if not 0 == request.depth:
+            # A depth of zero indicates infinite depth


Does this refer to DEPTH_RECURSIVE ?
If yes we should use that constant

mikaelarguedas · 2018-08-07T22:13:12Z

rclpy/rclpy/parameter_service.py

+            if request.prefixes:
+                for prefix in request.prefixes:
+                    prefix_with_trailing_dot = '%s.' % (prefix)
+                    if name.startswith(prefix_with_trailing_dot):


Nit:
if name.startswith(prefix + PARAMETER_SEPARATOR_STRING)

mikaelarguedas · 2018-08-07T22:18:24Z

rclpy/rclpy/parameter_service.py

+            response.types.append(self._node.get_parameter(name).get_parameter_type())
+        return response
+
+    def _list_parameters_callback(self, request, response):


Question: do you have examples of parameters that prompted the logic in this function ?
It looks to me like it could be simplified but I'd like to make sure I don't miss some test cases you used for development.

See #214 (comment) for test cases used.

nuclearsandwich · 2018-08-07T23:05:34Z

Where will the testing of the callbacks happen ? in a separate PR here ? or just in a higher level integration test?

During our sync meeting we talked about unit tests going here and integration tests happening in system tests (I haven't written any there yet either) but I guess there's nothing stopping me from unit testing the callback functions.

mikaelarguedas · 2018-08-07T23:16:00Z

During our sync meeting we talked about unit tests going here and integration tests happening in system tests (I haven't written any there yet either) but I guess there's nothing stopping me from unit testing the callback functions.

Good point, I remember that now.
Looking at rclcpp it looks like the only test that's performed is ensuring that it throws on invalid service name. But that the actual functionality testing of the callbacks is done at the test_rclcpp level.
So let's do this in system_test/test_rclpy to match what is done for rclcpp 👍

mikaelarguedas · 2018-08-08T00:58:27Z

rclpy/rclpy/parameter_service.py

+                prefix = '.'.join(name.split('.')[0:-1])
+                if prefix not in response.result.prefixes:
+                    response.result.prefixes.append(prefix)
+                response.result.names.append(name)


The definition of the result message is ambiguous IMO: https://github.com/ros2/rcl_interfaces/blob/f00fe22a36c8fdf6145d873173a4f9de0fdff131/rcl_interfaces/msg/ListParametersResult.msg.

It's unclear to me if the name of the "parameters under the prefix" should be:

the full parameter names (of the parameters that have a prefix matching one of the requested ones)

parameter names stripped of the prefix (of the parameters that have a prefix matching one of the requested ones)

The current implementation seems to match the implementation of rclcpp so I think it's good 👍. But maybe it would be worth rephrasing or addressing the todo in the message definition. Or clarify what we expect the prefixes field in the response to be used for.

maybe @wjwwood has more context regarding the use of the prefixes field in the result message?

I don't know for sure, but my initial feeling is that returning the absolute names and prefixes would be best.

maybe @wjwwood has more context regarding the use of the prefixes field in the result message?

The prefixes field contains additional nested prefixes as opposed to actual parameters. You can think of them as the "directories" when doing ls in a directory, as opposed to the actual files in the directory.

nuclearsandwich · 2018-08-10T19:07:19Z

@mikaelarguedas this is ready for another round of review.

nuclearsandwich · 2018-08-10T19:08:27Z

Running some CI:

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2018-08-13T22:39:42Z

rclpy/rclpy/parameter.py

+        return self._name
+
+    @property  # noqa: A003
+    def type(self):


not sure what to do about this, it feels like we should avoid using builtin names here.
@ros2/team how do you feel about using builtin names as method names ?

I think a property is similar enough to a public attribute and a builtin is similar enough to a reserved keyword that a trailing underscore would be appropriate

https://legacy.python.org/dev/peps/pep-0008/#names-to-avoid

If your public attribute name collides with a reserved keyword, append a single trailing underscore to your attribute name. This is preferable to an abbreviation or corrupted spelling.

We could lengthen the field name to parameter_type. It's redundant and burns a few extra milliseconds typing but avoids the ambiguity.

mikaelarguedas · 2018-08-13T22:44:22Z

rclpy/rclpy/parameter_service.py

+        self._node = node
+        nodename = node.get_name()
+
+        describe_parameters_service_name = '/'.join((nodename, 'describe_parameters'))


Same as for the parameter separator, it would be better to not redefine this hardcoded separator in every service name definition. Though it looks like that's what's done currently in rclcpp so it's not a blocker for getting this merged

Constantizing this is straightforward and we've got to start it somewhere. What's a good name for the constant? TOPIC_SEPARATOR_STRING?

That sounds good to me. It's consistent with the other constant defined in this PR and the naming used in otherp laces like logging https://github.com/ros2/rcutils/blob/4cbd05d9c09886754f41747afa705f15d02c6818/include/rcutils/logging.h#L34

mikaelarguedas · 2018-08-13T22:47:13Z

rclpy/test/test_node.py

        finally:
            node.destroy_node()

+    def test_node_set_parameters(self):


Based on feedback from #216 (comment) it looks like we now prefer using directly the assert statement rather than the ones provided by unittest (same multiple times below). Though not a blocker for this PR IMO

mikaelarguedas

last version looks good to me. Just added one question to the team below about method naming everything else is non-blocking remarks.

Note that I didn't test extensively, just reviewed the new changes

nuclearsandwich · 2018-08-14T16:49:41Z

I sent an earlier version of this to @mikaelarguedas directly but here is the node I've been testing parameter services on and an equivalent params.yaml file for matching behavior to the rclcpp implementation. https://gist.github.com/nuclearsandwich/8753121711671bfa8d9cb5f718ce09bc

There are some differences in the order of results when listing parameters due to the different traversal techniques between implementations but that service has no order-dependent cases (as opposed to setting parameters non-atomically where the results are ordered based on the inputs).

nuclearsandwich · 2018-08-14T17:11:05Z

rclpy/rclpy/validate_topic_name.py

 from rclpy.exceptions import InvalidTopicNameException
 from rclpy.impl.implementation_singleton import rclpy_implementation as _rclpy

+TOPIC_SEPARATOR_STRING = '/'


I tried to find a "sensible" place for the topic separator constant because it seems like it could be used for more than just parameter services and this was the best I could come up with based on module names. But I can either put it in parameter_services for now or if there's a place more "sensible" than this I can move it to I'm happy to field suggestions.

nuclearsandwich · 2018-08-14T17:13:18Z

rclpy/rclpy/parameter.py

+        return self._name
+
+    @property
+    def type_(self):


@mikaelarguedas @sloretz the property is now accessed as type_. I had initially let this slide since the generated interface code uses type in python currently and I know that surfaced in discussion recently but can't find it to figure out where we came down.

My recollection is that we cannot really make changes to ROS1 messages that use builtins.
But that anything not being legacy ROS1 code (and that will result in disruptive changes) should avoid using builtins names.

Part of the discussion is at ros2/build_farmer#104, bu I don't think it discuss anything outside of generated messages

Makes sense. And the change is in so all's well.

nuclearsandwich · 2018-08-14T17:14:50Z

New round of CI with increased timeout for rclpytests.

Linux
Linux-aarch64
macOS
Windows

nuclearsandwich · 2018-08-15T19:03:45Z

I'm able to reproduce the hang that is happening on ARM and am now investigating.

This adds parameter services to rclpy nodes. Like rclcpp nodes, parameter services are started by default and can be turned off with the create_node/Node constructor kwarg `start_parameter_services=False`. Parameters are stored on the Node class via a `_parameters` dict attribute. The methods following methods have been added to the Node API. * `get_parameter(name)` * `get_parameters(names)` * `set_parameters(parameters)` * `set_parameters_atomically(parameters)` A Parameter class provides a python interface to individual parameters that is (hopefully) a bit more ergonomic than using the Parameter-related message types from rcl_interfaces. Changes forthcoming in later pull requests: - Add the parameters_changed_callback API - Take initial parameters from params file argument.

When checking for valid arguments in the parameters API raise TypeErrors rather than asserting on failure.

This commit adds a Parameter.Type.check method which is used to enforce the declared parameter type at runtime.

nuclearsandwich · 2018-08-16T18:39:23Z

Linux
Linux-aarch64
macOS
Windows

A full CI run with the timeout I originally set and changes to the node creation tests so they clean up properly.

mikaelarguedas · 2018-08-16T19:08:11Z

full CI run with the timeout I originally set

Now that we know that the timeout issues on arm were related to a hang and not because we were getting close to the timeout limit. Is bumping the existing timeout still necessary ?
As the last timeout bump introduced a significant margin, I would expect it to be sufficient

nuclearsandwich · 2018-08-16T20:14:36Z

Is bumping the existing timeout still necessary ?

I could be mistaken about failures like: https://ci.ros2.org/job/ci_linux/5031/testReport/junit/(root)/projectroot/rclpytests/

Which I thought you had mentioned were related to earlier repeats of a test run timing out. It seems like we should comfortably get by with no timeout. What's a good way to test that well? Run just the rclpy tests with a retest until fail?

Avoids collusion/confusion with type builtin.

The test teardown method is calling rclpy.shutdown() before the nodes created here are destroyed. With the inclusion of parameter services this is causing the tests to hang after printing the following exceptions: ========================================================================================================================== 7 passed in 0.11 seconds Exception ignored in: <bound method Node.__del__ of <rclpy.node.Node object at 0x7fc321e0fd30>> Traceback (most recent call last): File "/tmp/scree/src/rclpy-mount/rclpy/rclpy/node.py", line 410, in __del__ self.destroy_node() File "/tmp/scree/src/rclpy-mount/rclpy/rclpy/node.py", line 353, in destroy_node _rclpy.rclpy_destroy_node_entity(srv.service_handle, self.handle) RuntimeError: Failed to fini 'rcl_service_t': rcl node is invalid, rcl instance id does not match, at /tmp/scree/src/ros2/rcl/rcl/src/rcl/node.c:461 Exception ignored in: <bound method Node.__del__ of <rclpy.node.Node object at 0x7fc320597828>> Traceback (most recent call last): File "/tmp/scree/src/rclpy-mount/rclpy/rclpy/node.py", line 410, in __del__ self.destroy_node() File "/tmp/scree/src/rclpy-mount/rclpy/rclpy/node.py", line 353, in destroy_node _rclpy.rclpy_destroy_node_entity(srv.service_handle, self.handle) RuntimeError: Failed to fini 'rcl_service_t': rcl node is invalid, rcl instance id does not match, at /tmp/scree/src/ros2/rcl/rcl/src/rcl/node.c:461 Exception ignored in: <bound method Node.__del__ of <rclpy.node.Node object at 0x7fc320597b70>> Traceback (most recent call last): File "/tmp/scree/src/rclpy-mount/rclpy/rclpy/node.py", line 410, in __del__ self.destroy_node() File "/tmp/scree/src/rclpy-mount/rclpy/rclpy/node.py", line 353, in destroy_node _rclpy.rclpy_destroy_node_entity(srv.service_handle, self.handle) RuntimeError: Failed to fini 'rcl_service_t': rcl node is invalid, rcl instance id does not match, at /tmp/scree/src/ros2/rcl/rcl/src/rcl/node.c:461 Destroying the nodes before the teardown runs resolves both the indefinite hang and the exceptions.

mikaelarguedas · 2018-08-16T20:30:14Z

Which I thought you had mentioned were related to earlier repeats of a test run timing out.

Yeah that was the initial thought as the original console output was indicating a Timeout.
After we looked closer at it we noticed that the tests were actually hanging with Fast-RTPS causing a timeout.
e.g. the console output from the job linked above:

1: ========================= 155 passed in 30.38 seconds ==========================
1/1 Test #1: rclpytests .......................***Timeout 200.01 sec

0% tests passed, 1 tests failed out of 1

Label Time Summary:
pytest    = 200.01 sec*proc (1 test)

The actual tests take 30 seconds to run and then hang.

One way to check how close we get to the timeout is to run the tests with Connext only and --retest-until-fail 30 or something like that.

nuclearsandwich · 2018-08-16T20:33:40Z

Running just rclpy and tests with connext checking that the unmodified timeout length is still appropriate.

Edit: CI did not want to be run without FastRTPS

mikaelarguedas · 2018-08-16T21:06:37Z

Job with only connext using ros2/ci#214:

mikaelarguedas

still looks good 👍

nuclearsandwich added the in progress Actively being worked on (Kanban column) label Aug 6, 2018

nuclearsandwich self-assigned this Aug 6, 2018

nuclearsandwich requested a review from mikaelarguedas August 6, 2018 18:52

nuclearsandwich added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 6, 2018

mikaelarguedas reviewed Aug 6, 2018

View reviewed changes

nuclearsandwich added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Aug 7, 2018

mikaelarguedas reviewed Aug 7, 2018

View reviewed changes

mikaelarguedas reviewed Aug 8, 2018

View reviewed changes

nuclearsandwich force-pushed the parameter-services branch from a356f77 to 4cd964a Compare August 10, 2018 18:41

nuclearsandwich added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 10, 2018

mikaelarguedas reviewed Aug 13, 2018

View reviewed changes

mikaelarguedas approved these changes Aug 13, 2018

View reviewed changes

nuclearsandwich commented Aug 14, 2018

View reviewed changes

nuclearsandwich force-pushed the parameter-services branch from 7a163bd to 3eea643 Compare August 14, 2018 18:29

mikaelarguedas removed the in review Waiting for review (Kanban column) label Aug 15, 2018

mikaelarguedas added the in progress Actively being worked on (Kanban column) label Aug 15, 2018

nuclearsandwich added 5 commits August 16, 2018 11:17

Raise TypeErrors instead of asserting types.

9dc54a8

When checking for valid arguments in the parameters API raise TypeErrors rather than asserting on failure.

Enforce parameter types.

1a2a4aa

This commit adds a Parameter.Type.check method which is used to enforce the declared parameter type at runtime.

Use constants for parameter separator and request depth.

8adc3d6

Make a constant for the topic separator and use it.

7350e26

nuclearsandwich force-pushed the parameter-services branch from 55dbbc1 to 4631f7a Compare August 16, 2018 18:37

nuclearsandwich added 3 commits August 16, 2018 13:21

Use parameters qos profile for parameter services.

869db6c

Use type_ for Parameter type attribute.

a8231e0

Avoids collusion/confusion with type builtin.

nuclearsandwich force-pushed the parameter-services branch from 4631f7a to e6c7125 Compare August 16, 2018 20:31

mikaelarguedas mentioned this pull request Aug 16, 2018

fix ignoring fastrtps ros2/ci#214

Merged

nuclearsandwich added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 16, 2018

mikaelarguedas approved these changes Aug 16, 2018

View reviewed changes

nuclearsandwich changed the title ~~Implement parameter services~~ Implement node parameters and parameter services Aug 16, 2018

nuclearsandwich merged commit 5b74d24 into master Aug 16, 2018

nuclearsandwich removed the in review Waiting for review (Kanban column) label Aug 16, 2018

nuclearsandwich deleted the parameter-services branch August 16, 2018 23:21

nuclearsandwich mentioned this pull request Aug 17, 2018

Take initial parameters from parameters yaml files and constructor arguments. #225

Merged

dirk-thomas mentioned this pull request Aug 17, 2018

Support for parameters #202

Closed

6 tasks

nuclearsandwich mentioned this pull request Aug 20, 2018

Add Node API method for setting the parameters_callback. #228

Merged

dhood mentioned this pull request Sep 20, 2018

Respect the use_sim_time parameter for rclpy nodes #240

Closed

2 tasks

Conversation

nuclearsandwich commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas 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!

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!

mikaelarguedas commented Aug 6, 2018

Uh oh!

mikaelarguedas 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!

nuclearsandwich commented Aug 7, 2018

Uh oh!

mikaelarguedas commented Aug 7, 2018

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!

nuclearsandwich commented Aug 10, 2018

Uh oh!

nuclearsandwich commented Aug 10, 2018

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!

mikaelarguedas left a comment

Choose a reason for hiding this comment

nuclearsandwich commented Aug 6, 2018 •

edited

Loading

nuclearsandwich commented Aug 16, 2018 •

edited

Loading