fix loading parameter behavior from yaml file.

[fujitatomoya]

address ros2/ros2cli#863

in this PR, it tries to address a few things as following.

• parameter_dict_from_yaml_file do not use temporary list, it now parses parameter yaml file and directly stores the contents in the dictionary.
• parameter_dict_from_yaml_file is now capable of parsing the yaml file with multiple keys with namespace and base nodename.
• load_parameter_file of Parameter Client should specifies the target node to parse the parameter yaml file. this should be done to avoid the original problem reported address ros2 param load sets the unexpected parameter value from yaml ros2cli#863
• add more unit tests to parameter_dict_from_yaml_file accordingly.

[sloretz]

@clalancette assigned for you to review 🧇

[clalancette]

I think we can skip doing this split at all if abs_name is in the param_file.keys(). That is, first check to see if the abs_name exists, and only if it doesn't, go ahead and do the split and look for the namespace.

[fujitatomoya]

we can do this but this is the different behavior from rcl parameter yaml parser, to keep the same behavior i think we need to keep this. (that said, if this is not designed behavior, i think we need to fix rcl parameter yaml parser 1st with deprecation period, because this would break the user space... it is likely that user application starts loading parameter via ros2 command line argument.)

here is an example of ros2 command line argument.

root@tomoyafujita:~/ros2_ws/colcon_ws# cat params.yaml
/foobar/parameter_blackboard:
  ros__parameters:
    test-abs: foobar-absolute
/foobar:
  parameter_blackboard:
    ros__parameters:
      test-rel: foobar-relative
parameter_blackboard:
  ros__parameters:
    test-ns: no-namespace
/parameter_blackboard:
  ros__parameters:
    test-abs-ns: no-namespace-abs
/demo/parameter_blackboard:
  ros__parameters:
    test-abs: demo-absolute
/demo:
  parameter_blackboard:
    ros__parameters:
      test-rel: demo-relative
/**:
  ros__parameters:
    debug: true
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 run demo_nodes_cpp parameter_blackboard --ros-args --params-file params.yaml
[INFO] [1701194971.305007151] [parameter_blackboard]: Parameter blackboard node named '/parameter_blackboard' ready, and serving '9' parameters already!
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 param dump parameter_blackboard
/parameter_blackboard:
  ros__parameters:
    debug: true
    qos_overrides:
      /parameter_events:
        publisher:
          depth: 1000
          durability: volatile
          history: keep_last
          reliability: reliable
    start_type_description_service: true
    test-abs-ns: no-namespace-abs
    test-ns: no-namespace
    use_sim_time: false

[clalancette]

I agree that we should maintain the same behavior as rcl_param_yaml_parser.

But I'd actually like to back-up a bit and look at parameter_dict_from_yaml_file. In particular, it has 1 required argument (parameter_file), and 3 optional arguments (use_wildcard, target_nodes, and namespace). I took a look through the core and through all of the code released into Rolling, and there are only a handful of non-test calls to this function:

• https://github.com/ros2/ros2cli/blob/419afea1282ae4daa43ea446a2c0bcc7394f856c/ros2param/ros2param/api/__init__.py#L42
• https://github.com/ros2/demos/blob/a29e65623e8ace15b8253ab33f19f3d8165c3cfe/demo_nodes_py/demo_nodes_py/parameters/async_param_client.py#L76

• rclpy/rclpy/rclpy/parameter_client.py

  Line 312 in e97c41b

  param_dict = parameter_dict_from_yaml_file(parameter_file, use_wildcard)

• rclpy/rclpy/rclpy/parameter_client.py

  Line 334 in e97c41b

  param_dict = parameter_dict_from_yaml_file(parameter_file, use_wildcard)

We never use the target_nodes or the namespace arguments at all.

So my question is: should we just remove these parameters and the functionality around them? It would make this function much easier to understand and test. But maybe I am missing something, I'd like to hear your thoughts.

[fujitatomoya]

@clalancette thanks!

• https://github.com/ros2/ros2cli/blob/419afea1282ae4daa43ea446a2c0bcc7394f856c/ros2param/ros2param/api/__init__.py#L42

ros2 param load <node> <parameter_file> case, i think we need to use target_nodes parameter to specify <node> with namespace from the yaml file. this is the original issue described ros2/ros2cli#863, wrong parameters are loaded to the <node>.

• https://github.com/ros2/demos/blob/a29e65623e8ace15b8253ab33f19f3d8165c3cfe/demo_nodes_py/demo_nodes_py/parameters/async_param_client.py#L76

this can work at this moment, only because it depends on the bug which this PR trying to address.

we can see, https://github.com/ros2/demos/blob/a29e65623e8ace15b8253ab33f19f3d8165c3cfe/demo_nodes_py/demo_nodes_py/parameters/params.yaml#L1

parameter yaml file only has /param_test_target absolute node name, which should never be loaded to

https://github.com/ros2/demos/blob/a29e65623e8ace15b8253ab33f19f3d8165c3cfe/demo_nodes_py/demo_nodes_py/parameters/async_param_client.py#L28

parameter_blackboard, because yaml file does not have parameter_blackboard parameters at all.

• rclpy/rclpy/rclpy/parameter_client.py

  Line 312 in e97c41b

  param_dict = parameter_dict_from_yaml_file(parameter_file, use_wildcard)

• rclpy/rclpy/rclpy/parameter_client.py

  Line 334 in e97c41b

  param_dict = parameter_dict_from_yaml_file(parameter_file, use_wildcard)

ParameterClient created on remote node, so when loading we can validate the parameters from the yaml file what needs to be loaded against remote node.

We never use the target_nodes or the namespace arguments at all.

parameter_dict_from_yaml_file can just convert yaml into dictionary, and then caller side can do filtering based on those target_nodes, namespace and wildcard, but i do not think that is really useful. filtering logic should be implemented in parameter_dict_from_yaml_file. so that we can get what needs to be loaded based on target node names, namespace and wildcard. (i guess that was original intention for this parameter_dict_from_yaml_file but not tested very good...)

[clalancette]

OK, I see. Sorry, I was looking at the original code and not this PR when I wrote my previous comment.

Let me back up even further and ask a question. Is the intention for parameter_dict_from_yaml_file to be the Python equivalent of https://github.com/ros2/rcl/blob/246fd1de5c073e60573abb014a787f012a241419/rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h#L96-L98 ?

[fujitatomoya]

I am not sure the original intention, sorry. i was just trying to fix ros2/ros2cli#863, and also check if it has the same behavior with rcl_yaml_param_parser. not sure if this answers your question though...

[fujitatomoya]

@clalancette friendly ping. can we resolve this comment?

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@clalancette can you check my comments above?

[fujitatomoya]

@clalancette friendly ping.

[fujitatomoya]

@clalancette the last comment #1193 (comment), could you check? CI and related changes are good to go.

• fix loading parameter behavior from yaml file. ros2cli#864
• Revert "Revert "fix loading parameter behavior from yaml file. (#656)… demos#661

[fujitatomoya]

@clalancette friendly ping, please have a look at #1193 (comment) the only one left unresolved.