Add rosparam verb load by v-lopez · Pull Request #590 · ros2/ros2cli

v-lopez · 2021-01-26T12:03:02Z

Example execution:

ros2 param  load /controller_manager ~/controller_manager.yaml 
Set parameter diff_drive_controller.type successful
Set parameter forward_command_controller_position.type successful
Set parameter joint_state_controller.type successful

ros2 param  load /controller_manager ~/controller_manager.yaml  --use-wildcard 
Set parameter mystr successful
Set parameter diff_drive_controller.type successful
Set parameter diff_drive_controller.type successful
Set parameter forward_command_controller_position.type successful
Set parameter joint_state_controller.type successful

Using the following file:

controller_manager:
  ros__parameters:
    diff_drive_controller:
      type: diff_drive_controller/DiffDriveController
    forward_command_controller_position:
      type: forward_command_controller/ForwardCommandController
    joint_state_controller:
      type: joint_state_controller/JointStateController
/**:
  ros__parameters:
    mystr: some_string
    diff_drive_controller:
      type: overriden_string

The parameters in the wildcard namespace are only used if --use-wildcard is provided, and are loaded before the node parameters, as I assume that should be the logical behavior.

fixes #589

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

v-lopez · 2021-02-03T08:44:10Z

@clalancette could you please give this a look? I've got some other PR's that depend on this functionality.
Also could this be released to foxy?

audrow

This looks like a useful feature, thanks for the PR.

A few thoughts:

I'd like to see tests for the load verb.
I don't think that the wildcard behavior should be a part of this PR. There is some (currently stalled) discussion on parameter files and presidence - see ros2/design#289 and ros2/design#303 - that I think we should wait on. The main reason for this is that I think that we should keep the wildcard behavior of ros2param, ros2launch, and ros2run similar.

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

v-lopez · 2021-02-09T10:04:13Z

@audrow Added tests and removed wildcard behavior. I'm not sure why those unrelated tests are failing.

audrow · 2021-02-10T16:51:59Z

I'll give this a more thorough review soon (probably Friday).

By the way, I ran CI. There are a few linter errors.

Linux

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

v-lopez · 2021-02-10T17:18:57Z

Thanks, I pushed fixes for the linter. I apologise for that, but I cannot get those errors to show up on my machine. I'm running latest foxy and same flake8 version and never get the bad quotes errors.

audrow · 2021-02-10T17:58:34Z

I cannot get those errors to show up on my machine

It works for me when I use colcon test (Rolling with Ubuntu); for example, colcon test --packages-select ros2param from the root of my ROS2 workspace, and colcon test-result --verbose to view the test failures. If you're already doing that, I'm not sure why the linter wouldn't run. (I tend to use colcon rather than running the tests directly because it is better at resolving paths, in my experience.)

Also, if the other tests are passing but you're fixing linter warnings, the colcon test mixin linters-only is much more convenient, especially for packages with slow tests.

audrow · 2021-02-10T18:01:28Z

It would be nice to make a PR to document this new feature in the ROS2 documentation on parameters.

I believe it makes sense to add to the "Load parameter file" section of the Understanding ROS2 parameters tutorial.

If you make a PR there, I can review it.

v-lopez · 2021-02-10T20:42:39Z

I will add the documentation, probably on Friday as well.

audrow

Looks good to me. I'll run CI.

ros2param/test/test_verb_load.py

audrow · 2021-02-12T17:01:29Z

Here's CI. There are linter warnings.

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

audrow · 2021-02-12T23:51:23Z

@v-lopez, I'm not sure why but I wasn't able to push changes fixing the linter errors to your branch. Anyway, I've made a commit to fix the linter errors: https://github.com/audrow/ros2cli/commit/812a5be8b4a5f099a51d611fc0d6e7d9c213468c. Once you apply these changes, I'll run CI and, if it's green, we can merge in the two PRs.

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

v-lopez · 2021-02-13T09:16:52Z

Fixes applied!

audrow · 2021-02-13T17:47:18Z

Linux
Linux-aarch64
macOS
Windows

audrow · 2021-02-13T17:52:22Z

I've merged both PRs, thanks for the contribution @v-lopez!

v-lopez · 2021-02-13T17:55:34Z

Thanks to you! Should I make a PR for the foxy branch to backport it?

* Add rosparam verb load Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com> * Move load_parameter_file implementation to api, add test_verb_load Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com> * Apply fixes for linter Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com> * Remove TODO comment Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com> * Fix linter errors Signed-off-by: Audrow Nash <audrow.nash@gmail.com> Co-authored-by: Audrow Nash <audrow.nash@gmail.com>

ros2param/ros2param/api/__init__.py

ivanpauno · 2021-02-17T17:18:15Z

ros2param/ros2param/api/__init__.py

+    internal_node_name = node_name.split('/')[-1]
+    with open(parameter_file, 'r') as f:
+        param_file = yaml.safe_load(f)
+        if internal_node_name not in param_file:


what about /**?

I don't see how that is being handled

audrow · 2021-02-17T18:45:08Z

Thanks for pointing those cases out, @ivanpauno. @v-lopez, can you make another PR addressing @ivanpauno's comments, since this has already been merged?

By the way, I checked with the foxy ROS Boss and you can make a PR for a foxy backport. This probably makes sense to do after the new comments are addressed.

Feel free to @ me for both PRs.

v-lopez · 2021-02-18T17:10:34Z

I'll work on a PR tomorrow to address this.

@ivanpauno I will check for both with and without namespace.

Regarding /** it was in there originally but I removed it on 6e06195 as discussed in this PR. I'd like to have this or a similar behavior added, but I guess we are waiting for unique criteria on: ros2/design#289 and ros2/design#303

ivanpauno · 2021-02-18T17:23:34Z

@ivanpauno I will check for both with and without namespace.

You should only check for the fully qualified name.
e.g. namespace=/asd/bsd name=csd

you should check for /asd/bsd/csd and asd/bsd/csd (but not for csd or `/csd).

Regarding /** it was in there originally but I removed it on 6e06195 as discussed in this PR

Can you link the discussion? I wasn't able to find it.

I'd like to have this or a similar behavior added, but I guess we are waiting for unique criteria on: ros2/design#289 and ros2/design#303

IIRC the current behavior is that parameter overrides specific to a node will override the ones passed using the /** wildcard, so I think it's fine to imitate that behavior here without waiting for the other discussions.

v-lopez · 2021-02-18T17:58:34Z

Can you link the discussion? I wasn't able to find it.

Well not a big discussion, just this comment #590 (review)

IIRC the current behavior is that parameter overrides specific to a node will override the ones passed using the /** wildcard, so I think it's fine to imitate that behavior here without waiting for the other discussions.

@audrow Should I put the wildcard behavior back in then?

audrow · 2021-02-18T19:19:30Z

@v-lopez, yes, I think it would make sense to support wildcard behavior. Sorry for the misdirection.

My intention in #590 (review) was that this new CLI verb doesn't get ahead of the discussion I mentioned on presidence in parameter files, by adding the ability to prioritize for parameters defined with a wildcard. I didn't mean to eliminate handling the wildcard case, which I missed in my review.

It would also be great to update the tests to show that wildcards (and namespaces) are handled :)

v-lopez · 2021-02-19T10:17:45Z

You should only check for the fully qualified name.
e.g. namespace=/asd/bsd name=csd

you should check for /asd/bsd/csd and asd/bsd/csd (but not for csd or `/csd).

@ivanpauno Is this right? Didn't you mean to check for csd and /csd but not for the namespace?

As far as I see, the param files do not have the namespace. Only the node name, which could be with or without leading slash.
Using the test node of the ros2param tests, which has a namespace:
ros2 param dump /foo/test_node

test_node:
  ros__parameters:
    bool_array_param:
    - false
...

ivanpauno · 2021-02-19T13:45:16Z

As far as I see, the param files do not have the namespace. Only the node name, which could be with or without leading slash.

Well, ros2 param dump seems to be broken 😶 ....
But the original use case of a parameter file was passing it to a node:

node_executable --ros-args --params-file /path/to/my/parameter/file

and in that case it will look for /foo/test_node (ros2 param dump should be generating something compatible to that).

e.g.: rclcpp code here

clalancette · 2021-02-19T13:58:45Z

and in that case it will look for /foo/test_node (ros2 param dump should be generating something compatible to that).

Yikes, good call. It does seem to be broken. Do you mind opening an issue?

v-lopez · 2021-02-19T14:14:22Z

Leaving aside the implications of the changes, I believe it might be interesting to be able to load params for a node, regardless of the namespace.

Say I have navigation parameters for a robot, and I want to launch a simulation with multiple robots of the same type. Would I have to create multiple identical files, each with its own namespace? Do some magic at launch time to rename them?

ivanpauno · 2021-02-19T14:26:03Z

Yikes, good call. It does seem to be broken. Do you mind opening an issue?

I'm working on a PR for it.

Say I have navigation parameters for a robot, and I want to launch a simulation with multiple robots of the same type. Would I have to create multiple identical files, each with its own namespace? Do some magic at launch time to rename them?

That's a good question.
Ideally, my_robot would refer to any node named my_robot and /namespace/my_robot would refer specifically to one.
But we are already using my_robot as /my_robot in parameter files, so I don't know.

For the moment, I would focus on consistency and not in that particular use case (which I agree is useful).

v-lopez · 2021-02-19T14:32:22Z

Ideally, my_robot would refer to any node named my_robot and /namespace/my_robot would refer specifically to one.

That would make sense to us.

And I understand this is not the place to discuss this, could we move the discussion elsewhere? I think it's going to be important in the near future.

ivanpauno · 2021-02-19T14:58:02Z

I'm working on a PR for it.

See #600.

ivanpauno · 2021-02-19T17:11:40Z

And I understand this is not the place to discuss this, could we move the discussion elsewhere? I think it's going to be important in the near future.

Sounds good, maybe you can open an issue with the proposal in ros2/ros2, as it will involve changes in different places (rclcpp, rclpy, ros2param, ...).

v-lopez · 2021-02-19T17:14:25Z

I will, but I'd love to have some backing from you guys, otherwise this things get lost in a huge todo pile.

I'll ping you when I've opened it.

ivanpauno · 2021-02-19T17:45:18Z

why removing the namespace?

The name in a parameter file is a fully qualified name.
IIRC, both having a leading slash or not is accepted.

I finally solved this issue in #600, as the tests for ros2 param load depend on ros2 param dump behavior.

what about /**?

I don't see how that is being handled

That one is still pending.

ivanpauno · 2021-02-19T17:48:45Z

I will, but I'd love to have some backing from you guys, otherwise this things get lost in a huge todo pile.

Sounds good, I understand that pushing new features/enhancements is demanding.

I'll ping you when I've opened it.

👍 feel free to tag me.
I think the only point of discussion will be if the change of behavior for relative node names in parameter files is acceptable, or if we need to somehow handle that case in a backwards compatible fashion.

v-lopez · 2021-02-23T09:46:42Z

@ivanpauno Please see #602 for the wildcard support.

v-lopez · 2021-02-23T09:58:55Z

And the issue about node parameters regardless of namespaces: ros2/ros2#1092

* Add rosparam verb load Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com> * Move load_parameter_file implementation to api, add test_verb_load Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com> * Apply fixes for linter Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com> * Remove TODO comment Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com> * Fix linter errors Signed-off-by: Audrow Nash <audrow.nash@gmail.com> Co-authored-by: Audrow Nash <audrow.nash@gmail.com>

Add rosparam verb load

857724d

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

This was referenced Jan 26, 2021

Standard interface for parsing parameters from yaml files ros2/rclpy#671

Closed

Add spawner and unspawner scripts ros-controls/ros2_control#310

Merged

audrow reviewed Feb 5, 2021

View reviewed changes

audrow self-assigned this Feb 5, 2021

Move load_parameter_file implementation to api, add test_verb_load

6e06195

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

v-lopez force-pushed the add-param-load branch from 3e2bb2f to 6e06195 Compare February 9, 2021 09:56

Apply fixes for linter

f0c9c12

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

audrow mentioned this pull request Feb 12, 2021

Add ros2 param load documentation ros2/ros2_documentation#1101

Merged

audrow approved these changes Feb 12, 2021

View reviewed changes

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

audrow added the enhancement New feature or request label Feb 12, 2021

Remove TODO comment

1981292

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

Fix linter errors

fe35451

Signed-off-by: Audrow Nash <audrow.nash@gmail.com>

audrow merged commit 14eb345 into ros2:master Feb 13, 2021

ivanpauno reviewed Feb 17, 2021

View reviewed changes

v-lopez mentioned this pull request Feb 23, 2021

Add wildcard loading to ros2 param load #602

Merged

v-lopez mentioned this pull request Feb 23, 2021

Provide a way of loading node parameters from a file regardless of the node namespace ros2/ros2#1092

Open

jordan-palacios mentioned this pull request Mar 29, 2021

Hardware Interfaces with many parameters ros-controls/ros2_control#347

Closed

v-lopez mentioned this pull request Jun 8, 2021

Can't load non-absolute node name in parameter file #648

Closed

Conversation

v-lopez commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

v-lopez commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

audrow left a comment

Choose a reason for hiding this comment

Uh oh!

v-lopez commented Feb 9, 2021

Uh oh!

audrow commented Feb 10, 2021

Uh oh!

v-lopez commented Feb 10, 2021

Uh oh!

audrow commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

audrow commented Feb 10, 2021

Uh oh!

v-lopez commented Feb 10, 2021

Uh oh!

audrow left a comment • 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!

Uh oh!

audrow commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

audrow commented Feb 12, 2021

Uh oh!

v-lopez commented Feb 13, 2021

Uh oh!

audrow commented Feb 13, 2021

Uh oh!

audrow commented Feb 13, 2021

Uh oh!

v-lopez commented Feb 13, 2021

Uh oh!

Uh oh!

ivanpauno Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

audrow commented Feb 17, 2021

Uh oh!

v-lopez commented Feb 18, 2021

Uh oh!

ivanpauno commented Feb 18, 2021

Uh oh!

v-lopez commented Feb 18, 2021

Uh oh!

audrow commented Feb 18, 2021

Uh oh!

v-lopez commented Feb 19, 2021

Uh oh!

ivanpauno commented Feb 19, 2021

Uh oh!

clalancette commented Feb 19, 2021

Uh oh!

v-lopez commented Feb 19, 2021

Uh oh!

ivanpauno commented Feb 19, 2021

Uh oh!

v-lopez commented Feb 19, 2021

Uh oh!

ivanpauno commented Feb 19, 2021

Uh oh!

ivanpauno commented Feb 19, 2021

Uh oh!

v-lopez commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanpauno commented Feb 19, 2021

Uh oh!

ivanpauno commented Feb 19, 2021

Uh oh!

v-lopez commented Feb 23, 2021

Uh oh!

v-lopez commented Jan 26, 2021 •

edited

Loading

v-lopez commented Feb 3, 2021 •

edited

Loading

audrow commented Feb 10, 2021 •

edited

Loading

audrow left a comment •

edited

Loading

audrow commented Feb 12, 2021 •

edited

Loading

v-lopez commented Feb 19, 2021 •

edited

Loading