Parameter API enhancements #325

jubeira · 2019-04-26T23:23:19Z

Closes #321; refer to the issue for details.

jubeira · 2019-04-26T23:24:32Z

This is a draft for now; there are still multiple TODOs around the code and testing to be done before promoting it to a PR.
If anyone's interested in taking a preliminary look and providing feedback, it is more than welcome!

rclpy/rclpy/exceptions.py

rclpy/rclpy/node.py

jubeira · 2019-04-30T19:52:17Z

Code updated and rebased with the current master.
This is ready for review; the only pending tasks are adding proper test cases for the new API, and improving exception handling. Existing

Note that this PR should get in before the API freeze. The implementation reused the existing codebase as much as possible; the intention was to comply with the new API changing it as little as possible. Following that line, I would suggest to correct implementation details that may appear after the API freeze.

hidmic · 2019-04-30T20:00:55Z

CI (full run):

Linux
Linux-aarch64
macOS
Windows

rclpy/rclpy/exceptions.py

rclpy/rclpy/node.py

rclpy/test/test_create_node.py

jubeira · 2019-05-01T22:06:35Z

rclpy/rclpy/node.py

+        :raises: InvalidParameterValueException if the registered callback rejects the parameter.
+        """
+        for parameter in parameters:
+            parameter.name = namespace + parameter.name


Writing tests I just realized that this is not possible (name can't be set after creating Parameter object).
I can think of these alternatives, in order of preference:

Creating new parameters with same value, type and descriptor but with a new name containing the namespace.

Suppressing namespace parameter. This is introduced to match the behavior in rclcpp, but on the other hand the current set_parameters method in master doesn't have a namespace option.

Adding a setter to the name.

Using the private _name member (argh).

@hidmic thoughts?
/cc @wjwwood input on this is welcome!

See a proposed fix in a39ea62.

In rclcpp, declare_parameter takes a name, a value and a descriptor, but set_parameter takes a rclcpp::Parameter. Following that line, I left set_parameters with the same signature, and changed declare_parameters to take the pieces of the parameter as separate arguments in a tuple.
In other words, its @jacobperron 's second suggestion, considering ParameterDescriptor as well.

def declare_parameter(self, name: str, parameter: ParameterValue, descriptor: ParameterDescriptor) -> Parameter: ... def declare_parameters(self, namespace: str, parameters: List[Tuple[str, ParameterValue, ParameterDescriptor]]) -> List[Parameter]: ...

/cc @hidmic

For what it is worth, there's no reason C++ doesn't provide a set_parameters(vector<pair<string, ParameterValue>>) nor is there a reason against declare_parameters(string, vector<pair<Parameter, descriptor>>), other than that I ran out of time to keep adding convenient signatures. Many of the other signatures that seem less obvious were added by request before my big refactor.

rclpy/rclpy/node.py

+        :raises: InvalidParameterValueException if the registered callback rejects the parameter.
+        """
+        for parameter in parameters:
+            parameter.name = namespace + parameter.name


rclpy/rclpy/node.py

jacobperron

Partial review.

rclpy/rclpy/validate_parameter_name.py

rclpy/rclpy/node.py

+        :raises: InvalidParameterValueException if the registered callback rejects the parameter.
+        """
+        for parameter in parameters:
+            parameter.name = namespace + parameter.name


rclpy/rclpy/node.py

rclpy/rclpy/validate_parameter_name.py

jubeira · 2019-05-02T20:46:31Z

Thanks for the comments @wjwwood @jacobperron !
I've just pushed some more changes addressing them. I still need to add more test cases, but this seems to be taking shape. I'll rebase to fix the conflicts before proceeding.

- See issue #321 for details. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira · 2019-05-03T00:07:19Z

CI for linux:

Linux:
Linux-aarch64:
MacOS:

Windows was not available and didn't finish yet. Other than that CI looks good for this.
@wjwwood any more thoughts or comments? Do you think this can get in the API freeze?

sloretz · 2019-05-03T00:29:17Z

@jubeira It looks like those CI runs tested all packages. For the next run I recommend --packages-above rclpy in the tests args to reduce the time CI takes

jacobperron

It might be good to add a few tests exercising describe_parameter, describe_parameters, and undeclare_parameter.

jacobperron · 2019-05-02T21:40:36Z

rclpy/test/test_node.py

@@ -277,6 +286,255 @@ def test_node_has_parameter_services(self):
        )


+class Test(unittest.TestCase):


nit: rename this test fixture to be more descriptive. Maybe TestNodeParameters? Since the previous test fixture is not testing the node's parameter methods anyways, it could probably keep the name TestNode.

jacobperron · 2019-05-02T21:42:57Z

rclpy/test/test_node.py

+    def test_declare_parameter(self):
+        result_foo = self.node.declare_parameter(
+            'foo', ParameterValue(
+                type=Parameter.Type.INTEGER.value, integer_value=42), ParameterDescriptor())


nit: for readability I would put each argument to declare_parameter on a new line.

Same for all instances below.

jacobperron · 2019-05-02T21:48:46Z

rclpy/test/test_node.py

+        self.assertEqual(result_baz.value, 2.41)
+        self.assertEqual(self.node.get_parameter('foo').value, 42)
+        self.assertEqual(self.node.get_parameter('bar').value, 'hello')
+        self.assertEqual(self.node.get_parameter('baz').value, 2.41)


Might be good to test calls where only the name or name and value are provided.

rclpy/test/test_node.py

jacobperron · 2019-05-03T01:24:47Z

@jubeira My recent review is just minor nitpicks. Also, I'm okay if we want to put the additional tests in a follow-up PR.

wjwwood

lgtm with a suggestion that's non-blocking

rclpy/rclpy/node.py

jubeira · 2019-05-03T13:26:25Z

@jacobperron @wjwwood thanks again for your reviews!
If you are OK with it, I'd suggest merging this one and opening a ticket to track down the missing suggestions and test cases. I can do the follow-up.

/cc @hidmic

- Minor typos and naming fixes for test_node. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

rclpy/rclpy/__init__.py

jubeira · 2019-05-04T00:29:14Z

CI:

Linux:
Linux-aarch64:
Mac OS:
Windows

Overall looks good; the failing cases seem to be unrelated.

jubeira added the in progress Actively being worked on (Kanban column) label Apr 26, 2019

jacobperron reviewed Apr 27, 2019

View reviewed changes

rclpy/rclpy/exceptions.py Outdated Show resolved Hide resolved

rclpy/rclpy/node.py Outdated Show resolved Hide resolved

rclpy/rclpy/node.py Outdated Show resolved Hide resolved

rclpy/rclpy/node.py Show resolved Hide resolved

jubeira marked this pull request as ready for review April 30, 2019 19:14

jubeira changed the title ~~[WIP] Parameter API enhancements~~ Parameter API enhancements Apr 30, 2019

jubeira force-pushed the issue/#321_enhance_parameter_api branch from 9147c83 to 89eb719 Compare April 30, 2019 19:45

hidmic reviewed Apr 30, 2019

View reviewed changes

jubeira commented May 1, 2019

View reviewed changes

wjwwood reviewed May 1, 2019

View reviewed changes

jacobperron reviewed May 2, 2019

View reviewed changes

rclpy/rclpy/node.py Outdated Show resolved Hide resolved

rclpy/rclpy/node.py Outdated Show resolved Hide resolved

jacobperron reviewed May 2, 2019

View reviewed changes

rclpy/rclpy/node.py Outdated Show resolved Hide resolved

rclpy/rclpy/node.py Outdated Show resolved Hide resolved

rclpy/rclpy/validate_parameter_name.py Outdated Show resolved Hide resolved

rclpy/rclpy/validate_parameter_name.py Outdated Show resolved Hide resolved

Implementing new parameter API.

8bfc19c

- See issue #321 for details. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira force-pushed the issue/#321_enhance_parameter_api branch from 0925fe4 to 8bfc19c Compare May 2, 2019 20:55

Improving tests for parameters.

5abf1dc

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jacobperron reviewed May 3, 2019

View reviewed changes

wjwwood approved these changes May 3, 2019

View reviewed changes

rclpy/rclpy/node.py Show resolved Hide resolved

jacobperron approved these changes May 3, 2019

View reviewed changes

Juan Ignacio Ubeira added 2 commits May 3, 2019 15:54

Small fix for getting initial parameters when undeclared are allowed.

8b35742

- Minor typos and naming fixes for test_node. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

Adding automatic declaration for initial parameters.

8e7ebfe

Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>

jubeira mentioned this pull request May 3, 2019

Using new parameter API: allowing undeclared parameters. ros2/system_tests#356

Merged

wjwwood approved these changes May 3, 2019

View reviewed changes

jacobperron reviewed May 3, 2019

View reviewed changes

rclpy/rclpy/__init__.py Show resolved Hide resolved

jubeira merged commit d340737 into master May 4, 2019

jubeira removed the in progress Actively being worked on (Kanban column) label May 4, 2019

jubeira deleted the issue/#321_enhance_parameter_api branch May 4, 2019 00:52

This was referenced May 5, 2019

Setting automatic declaration for initial parameters to False. #339

Merged

Follow-up on parameter API #341

Closed

jubeira mentioned this pull request May 13, 2019

Parameter handling improvements. #345

Merged

This was referenced May 23, 2019

Adding section for changes in parameter API for rclpy in Dashing. ros2/ros2_documentation#221

Merged

Enforcing parameter ranges. #357

Merged

Rename parameter options #363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter API enhancements #325

Parameter API enhancements #325

jubeira commented Apr 26, 2019

jubeira commented Apr 26, 2019

jubeira commented Apr 30, 2019

hidmic commented Apr 30, 2019

jubeira May 1, 2019

This comment was marked as resolved.

This comment was marked as outdated.

jubeira May 2, 2019 •

edited

Loading

wjwwood May 2, 2019

This comment was marked as resolved.

jacobperron left a comment

This comment was marked as outdated.

jubeira commented May 2, 2019

jubeira commented May 3, 2019 •

edited

Loading

sloretz commented May 3, 2019

jacobperron left a comment •

edited

Loading

jacobperron May 2, 2019

jacobperron May 2, 2019

jacobperron May 2, 2019

jacobperron commented May 3, 2019

wjwwood left a comment

jubeira commented May 3, 2019

jubeira commented May 4, 2019 •

edited

Loading

		@@ -277,6 +286,255 @@ def test_node_has_parameter_services(self):
		)


		class Test(unittest.TestCase):

Parameter API enhancements #325

Parameter API enhancements #325

Conversation

jubeira commented Apr 26, 2019

jubeira commented Apr 26, 2019

jubeira commented Apr 30, 2019

hidmic commented Apr 30, 2019

jubeira May 1, 2019

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

jubeira May 2, 2019 • edited Loading

Choose a reason for hiding this comment

wjwwood May 2, 2019

Choose a reason for hiding this comment

This comment was marked as resolved.

jacobperron left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

jubeira commented May 2, 2019

jubeira commented May 3, 2019 • edited Loading

sloretz commented May 3, 2019

jacobperron left a comment • edited Loading

Choose a reason for hiding this comment

jacobperron May 2, 2019

Choose a reason for hiding this comment

jacobperron May 2, 2019

Choose a reason for hiding this comment

jacobperron May 2, 2019

Choose a reason for hiding this comment

jacobperron commented May 3, 2019

wjwwood left a comment

Choose a reason for hiding this comment

jubeira commented May 3, 2019

jubeira commented May 4, 2019 • edited Loading

jubeira May 2, 2019 •

edited

Loading

jubeira commented May 3, 2019 •

edited

Loading

jacobperron left a comment •

edited

Loading

jubeira commented May 4, 2019 •

edited

Loading