Add param dump <node-name> #285

artivis · 2019-06-19T20:48:43Z

Add the param dump <node-name> which dumps the parameters of a given node to a yaml file.

From the test, the following

node.declare_parameter('bool_param', True)
node.declare_parameter('int_param', 42)
node.declare_parameter('double_param', 1.23)
node.declare_parameter('str_param', 'Hello World')
node.declare_parameter('foo.str_param', 'foo')
node.declare_parameter('foo.bar.str_param', 'foobar')

generates the yaml file (test_node.yaml) :

test_node:
  ros__parameters:
    bool_param: true
    double_param: 1.23
    foo:
      bar:
        str_param: foobar
      str_param: foo
    int_param: 42
    str_param: Hello World

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis · 2019-06-19T20:51:10Z

ros2param/ros2param/verb/dump.py

+        return value
+
+    def insert_dict(self, dict, key, value):
+        split = key.split("/", 1)


I'm not sure if namespaces are delimited with / or . as I have seen both :s .

Depends what you mean by namespace.

A node namespace (the thing set with __ns:=/foo/bar) has tokens delimited with /. Multiple levels in a parameter name are delimited with a .. You can get the character with this import from rclpy.parameter import PARAMETER_SEPARATOR_STRING

$ ros2 run demo_nodes_cpp parameter_blackboard __ns:=/foo/bar & [1] 25174 ... $ ros2 node list /foo/bar/parameter_blackboard $ ros2 param list /foo/bar/parameter_blackboard use_sim_time $ ros2 param set /foo/bar/parameter_blackboard ping.pong 5 Set parameter successful $ ros2 param list /foo/bar/parameter_blackboard ping.pong use_sim_time

If you're dumping to a yaml file, you can just use indentation.

$ ros2 run demo_nodes_cpp parameter_blackboard __ns:=/lidar_ns/lidar_1/lidar11 __node:=lidar111 __params:=/home/sloretz/ws/ros2/src/ros2/rcl/rcl_yaml_param_parser/test/multi_ns_correct.yaml & [1] 25358 ... $ ros2 param list /lidar_ns/lidar_1/lidar11/lidar111 driver1.driver11.bk_sensor_specs driver1.driver11.driver111.dx driver1.driver11.driver111.dy driver1.driver11.driver111.fr_sensor_specs driver2.driver21.dx driver2.driver21.dy id is_front name ports use_sim_time

I did mean "Multiple levels in a parameter name", didn't know about PARAMETER_SEPARATOR_STRING thanks.
This new verb does dump into a yaml file using multi-level indentation for multi-level parameters, see the example in my very first comment.

But how does one declare those multi-level parameters in python ?? The following does not work,

node.declare_parameter('foo.str_param', 'foo')

for topic name must not contain characters other than alphanumerics, '_', '~', '{', or '}'

That looks like a bug

https://github.com/ros2/rclpy/blob/a28b8258c2a1f6300bb407b2d7e10e4e98b60b48/rclpy/rclpy/validate_parameter_name.py#L31-L33

@jubeira FYI

@artivis Would you be willing to open an issue about declare_parameter('foo.str_param', 'foo') not working on the rclpy repo? https://github.com/ros2/rclpy/issues/new

Looks like an issue has been opened ros2/rclpy#373

Ok great, thanks.

ros2param/test/test_verb_dump.py

artivis · 2019-06-19T20:55:43Z

Before this PR to be ready for review,

make sure about namespaces delimiters -> depends on unable to namespace parameters rclpy#373
finish the main test

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis · 2019-06-20T12:52:54Z

PR is subject to changes once ros2/rclpy#373 is fixed.
In the meantime the overall script works and review can start.

kyrofa · 2019-06-27T16:25:01Z

@artivis ros2/rclpy#377 has been merged, can this be updated?

artivis · 2019-06-27T16:42:48Z

@kyrofa yep, I'll get to it 👍

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis · 2019-06-27T18:40:45Z

@sloretz This PR now uses PARAMETER_SEPARATOR_STRING and the test is passing properly.
Ready for review.

artivis · 2019-07-16T15:36:08Z

@sloretz Please let me know if there is anything I can do to help push this forward.

sloretz

Added a few comments

sloretz · 2019-07-23T22:49:22Z

ros2param/ros2param/verb/dump.py

+
+        # extract type specific value
+        pvalue = response.values[0]
+        if pvalue.type == ParameterType.PARAMETER_BOOL:


It looks like this code could be shared with ros2 param get. In fact, here's a function with part of it. Mind moving this code to ros2param/api/__init__.py? I'll create a ticket to follow up with get.

sloretz · 2019-07-23T22:58:43Z

ros2param/ros2param/verb/dump.py

+            '--include-hidden-nodes', action='store_true',
+            help='Consider hidden nodes as well')
+        parser.add_argument(
+            '--file-path',


nit, recommend --output-dir. Despite the help text I mistakenly thought this was the name of a file. Alternatively outputing to stdout with the expectation that users will pipe to a file seems like another reasonable option.

How about having both options ? Adding a --print flag or such that take precedence over saving to a file.

Sounds good to me

Done 907f1be

ros2param/ros2param/verb/dump.py

sloretz · 2019-07-23T23:17:34Z

ros2param/ros2param/verb/dump.py

+                    "'{node_name.full_name}': {e}".format_map(locals()),
+                    file=sys.stderr)
+
+            print('Saving to: ', os.path.join(args.file_path, node_name.name + ".yaml"))


There are a few places that assume a fully qualified node name is unique, so making the output file name include the namespace seems appropriate. Recommend replacing node_name.name with absolute_node_name.replace('/', '__')

The output file name and only that ? Not sure how it works with namespaces now that the node name is the first entry in the yaml file...
Also should I filter for leading ' / ' in absolute_node_name ?

The output file name and only that ?

Yeah, I was just thinking it would make this case more convenient. It looks like the second call would overwrite the first if only using node_name.name for the file name.

ros2 param dump /left/camera --output-dir . ros2 param dump /right/camera --output-dir .

Also should I filter for leading ' / ' in absolute_node_name ?

Sure. The absolute name always begins with /, so that should be easy to remove.

Done e41232c

ros2param/test/test_verb_dump.py

artivis · 2019-07-24T12:40:10Z

@sloretz Thanks for the review, will address it asap.

Signed-off-by: artivis <jeremie.deray@canonical.com>

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

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

sloretz · 2019-07-25T22:38:22Z

@artivis This looks good to me! Would you mind checking that commits: 38944d9,
04d9112, and 0f1b534 look ok?

CI (Build up to ros2param, testing ros2param)

Linux
Linux-aarch64
macOS
Windows

sloretz · 2019-07-25T22:59:04Z

@artivis Looks like there are some linter complaints. Mind addressing them?

https://ci.ros2.org/job/ci_linux/7667/testReport/junit/ros2param.test/test_flake8/test_flake8/

artivis · 2019-07-26T12:07:58Z

ros2param/ros2param/verb/dump.py

@@ -127,5 +127,5 @@ def main(self, *, args):  # noqa: D102
                file_name = absolute_node_name.replace('/', '__')

            print('Saving to: ', os.path.join(args.output_dir, file_name + '.yaml'))
-            with open(os.path.join(args.output_dir, node_name.name + '.yaml'), 'w') as yaml_file:
+            with open(os.path.join(args.output_dir, file_name + '.yaml'), 'w') as yaml_file:


artivis · 2019-07-26T12:13:23Z

@artivis This looks good to me! Would you mind checking that commits: 38944d9,
04d9112, and 0f1b534 look ok?

Looks good to me 👍

artivis · 2019-07-26T12:52:33Z

@artivis Looks like there are some linter complaints. Mind addressing them?

https://ci.ros2.org/job/ci_linux/7667/testReport/junit/ros2param.test/test_flake8/test_flake8/

I'll take care of it no problem. However that linter test passes on my machine. Do I have something mis-configured or something ?

clalancette · 2019-07-26T12:58:45Z

I'll take care of it no problem. However that linter test passes on my machine. Do I have something mis-configured or something ?

Typically that is the case, yeah. Flake8 has an annoying behavior that it will only run the linters that are installed, so if you forget one, it just won't run it. Anyway, I'd suggest re-running through the installation instructions, which should fetch the ones you are missing.

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis · 2019-07-26T13:40:16Z

@clalancette Thanks, I was indeed missing some linters 👍.
However I am getting some warnings now like the following:

lib/python3.6/site-packages/_pytest/mark/structures.py:332: PytestUnknownMarkWarning: Unknown pytest.mark.copyright - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/latest/mark.html
    PytestUnknownMarkWarning,

lib/python3.6/site-packages/_pytest/mark/structures.py:332: PytestUnknownMarkWarning: Unknown pytest.mark.linter - is this a typo? ....

Any idea ?

clalancette · 2019-07-26T13:49:31Z

Any idea ?

Yeah, I see those as well. I think those are just warnings from Python itself, and can be safely ignored. At least, they don't seem to cause problems in CI or in the code itself.

artivis · 2019-07-26T14:01:44Z

Yeah, I see those as well. I think those are just warnings from Python itself, and can be safely ignored. At least, they don't seem to cause problems in CI or in the code itself.

Ok, thanks 👍

sloretz · 2019-07-26T15:38:58Z

CI (testing just ros2param)

Linux
Linux-aarch64
macOS
Windows -- failed to delete workspace

sloretz · 2019-07-26T16:06:34Z

ros2param/test/test_verb_dump.py

+
+            # Make sure the file was not create, thus '--print' did preempt
+            expected_err = '[Errno 2] No such file or directory: ' \
+                           "'{not_generated_param_file}'".format_map(locals())


Looks like the error text is slightly different on windows. Maybe relax this check to assert 'No such file or directory' in context.exception?

https://ci.ros2.org/job/ci_windows/7522/testReport/junit/ros2param.test.test_verb_dump/TestVerbDump/test_verb_dump_print/

That's surprising.
See 7b508a0.

Signed-off-by: artivis <jeremie.deray@canonical.com>

sloretz · 2019-07-26T16:32:22Z

CI (testing ros2param)

Linux
Linux-aarch64
macOS
Windows

artivis · 2019-07-26T16:34:34Z

@sloretz Genuine curiosity, does the ci kicks in from keywords ? Can anyone launch it ??

sloretz · 2019-07-26T16:42:26Z

@artivis The CI jobs for all platforms are started manually. The ros2 build farm supports PR jobs which would run an ubuntu job automatically, but it looks like this repo isn't yet set up for that

sloretz

CI looks good. Thanks for the PR @artivis !

artivis added 7 commits June 18, 2019 17:21

wip param dump

1a3a231

Signed-off-by: artivis <jeremie.deray@canonical.com>

default path & cleanup

98e0ead

Signed-off-by: artivis <jeremie.deray@canonical.com>

wip test verb dump

4468803

Signed-off-by: artivis <jeremie.deray@canonical.com>

rm spin_once

424cec3

Signed-off-by: artivis <jeremie.deray@canonical.com>

nested namespaces

6ce360b

Signed-off-by: artivis <jeremie.deray@canonical.com>

cleaning up

49708bb

Signed-off-by: artivis <jeremie.deray@canonical.com>

multithread the test

aee2ec8

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis commented Jun 19, 2019

View reviewed changes

ros2param/test/test_verb_dump.py Outdated Show resolved Hide resolved

artivis changed the title ~~Feature/param dump~~ Add param dump <node-name> Jun 19, 2019

artivis added 3 commits June 20, 2019 08:47

todo use PARAMETER_SEPARATOR_STRING

b6e7ede

Signed-off-by: artivis <jeremie.deray@canonical.com>

test comp generate<->expected param file

1de2121

Signed-off-by: artivis <jeremie.deray@canonical.com>

lipstick

a865449

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis force-pushed the feature/param_dump branch from 2503e1f to a865449 Compare June 20, 2019 12:50

artivis marked this pull request as ready for review June 20, 2019 12:53

jacobperron added the in review Waiting for review (Kanban column) label Jun 25, 2019

use proper PARAMETER_SEPARATOR_STRING

5dbe3de

Signed-off-by: artivis <jeremie.deray@canonical.com>

sloretz reviewed Jul 23, 2019

View reviewed changes

artivis force-pushed the feature/param_dump branch from d1eb7da to 64dad3f Compare July 24, 2019 13:35

artivis added 4 commits July 24, 2019 10:19

mv common code to api

07d9a46

Signed-off-by: artivis <jeremie.deray@canonical.com>

rename param output-dir

75498f1

Signed-off-by: artivis <jeremie.deray@canonical.com>

rm line breaks

1d6cc12

Signed-off-by: artivis <jeremie.deray@canonical.com>

rm useless import

1efc7c8

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis added 2 commits July 24, 2019 10:19

raise rather than print

802efed

Signed-off-by: artivis <jeremie.deray@canonical.com>

raise rather than print

60d5b85

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis force-pushed the feature/param_dump branch from 64dad3f to 60d5b85 Compare July 24, 2019 14:23

artivis and others added 5 commits July 25, 2019 16:11

add --print option

907f1be

Signed-off-by: artivis <jeremie.deray@canonical.com>

prepend node namespace to output filename

e41232c

Signed-off-by: artivis <jeremie.deray@canonical.com>

preempted -> preempt

38944d9

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

"w" -> 'w'

04d9112

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

Output file using fully qualified node name

0f1b534

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

sloretz force-pushed the feature/param_dump branch from 0ba7042 to 0f1b534 Compare July 25, 2019 22:34

artivis commented Jul 26, 2019

View reviewed changes

fix linter tests

f989bcf

Signed-off-by: artivis <jeremie.deray@canonical.com>

artivis force-pushed the feature/param_dump branch from d9d2b88 to f989bcf Compare July 26, 2019 13:39

sloretz reviewed Jul 26, 2019

View reviewed changes

relaxe --print preempt test

7b508a0

Signed-off-by: artivis <jeremie.deray@canonical.com>

sloretz approved these changes Jul 26, 2019

View reviewed changes

sloretz merged commit d804e11 into ros2:master Jul 26, 2019

artivis deleted the feature/param_dump branch July 26, 2019 20:33

kyrofa mentioned this pull request Sep 4, 2019

ROS 2 Eloquent Elusor ros2/ros2#734

Closed

34 tasks

Add param dump <node-name> #285

Add param dump <node-name> #285

Conversation

artivis commented Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artivis commented Jun 19, 2019 • edited Loading

artivis commented Jun 20, 2019 • edited Loading

kyrofa commented Jun 27, 2019

artivis commented Jun 27, 2019

artivis commented Jun 27, 2019

artivis commented Jul 16, 2019

sloretz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artivis commented Jul 24, 2019

sloretz commented Jul 25, 2019

sloretz commented Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

artivis commented Jul 26, 2019

artivis commented Jul 26, 2019

clalancette commented Jul 26, 2019

artivis commented Jul 26, 2019

clalancette commented Jul 26, 2019

artivis commented Jul 26, 2019

sloretz commented Jul 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented Jul 26, 2019

artivis commented Jul 26, 2019

sloretz commented Jul 26, 2019

sloretz left a comment

Choose a reason for hiding this comment

artivis commented Jun 19, 2019 •

edited

Loading

artivis commented Jun 19, 2019 •

edited

Loading

artivis commented Jun 20, 2019 •

edited

Loading

sloretz left a comment •

edited

Loading

sloretz commented Jul 25, 2019 •

edited

Loading