Rewritten params yaml ignores duplicate node/namespace keys

[bpwilcox]

Bug report

Steps to reproduce issue

Add parameters in nav2_params.yaml as such:

top_node:
  ros_parameters:
    foo: True

top_node:
  extended_node:
    ros_parameters:
      bar: True

Expected behavior

• generated yaml file includes parameters for /top_node and top_node/extended_node

Actual behavior

• only one of the nodes' parameters is generated in the tmp yaml file

Additional information

Right now the configured_params created via the rewritten_yaml.py does not rewrite node parameters from duplicate key values. This limits the launch from using a params file yaml where there are duplicate keys, for instance if we want to specify parameters for a nested node. I wanted to do this in #1211 for /planner_server/global_costmap and controller_server/local_costmap, but the parameters weren't being set on the nodes because of this issue.

Implementation considerations

After some investigation, it looks like the problem might be that the keys being iterated over in getYamlLeafKeys in rewritten_yaml.py do not include duplicate values, thus the key mappings from yamlData.keys() are unique.

I think that when you add multiple yaml file arguments to the node, the new one will overwrite the previous one if there are any duplicates, so one solution could be to pass in the newly generated yaml in addition to the the original one. For cleanliness/efficiency, we might only include the substituted parameters in the generated yaml.

[orduno]

@bpwilcox based on your recent findings, is this still an issue?

[crdelsey]

This is invalid YAML. The yaml parser used in the RCL layer accepts this input, but it shouldn't if it enforced the standard.

I'm not sure we should fix this in the RewrittenYaml component. Rather, the YAML library used by the RCL layer should error out when it receives an invalid file like this.

[bpwilcox]

I'm not opposed to leaving as is if we consider the current version of the rcl yaml parser as having this as a bug, however, I'm also in favor of bringing this issue upstream in case they decide that this is an intended feature for ROS2.

[AlexeyMerzlyakov]

I've checked the rewritten_yaml.py code on given example. Indeed, during the getYamlLeafKeys() execution there only one parameter tree available to this method.
After insertion the following line to rewritten_yaml.py:

@@ -82,6 +87,7 @@ class RewrittenYaml(launch.Substitution):
     rewritten_yaml = tempfile.NamedTemporaryFile(mode='w', delete=False)
     param_rewrites, keys_rewrites = self.resolve_rewrites(context)
     data = yaml.safe_load(open(yaml_filename, 'r'))
+    print("Dumping ", yaml_filename, ":\n", yaml.dump(data))
     self.substitute_params(data, param_rewrites)
     self.substitute_keys(data, keys_rewrites)
     if self.__root_key is not None:

I've discovered that newly loaded yaml-file contains only one parameter tree:

Dumping  /home/amerzlyakov/navigation2_ws/install/nav2_bringup/share/nav2_bringup/params/nav2_params.yaml :
...
top_node:
  extended_node:
    ros_parameters: {bar: true}

This means that PyYAML loading mechanism is keeping only one unique parameter key and no reporting about error. It might be related to yaml/pyyaml#165.

In the end, to keep both of trees in one yaml and process them correctly it might need to have a written our own ROS2 yaml-loader instead of PyYAML.

[SteveMacenski]

Got it. To be clear, is this issue because of the ros__parameters namespace or because of 2 top_nodes?

I suppose what we could do is run each tree separately. I'm sure somehow we can find the lines with root items and then run each section of the params file over that and concatenate them?

Second, larger question, is whether we "should". How often is it that people have 1 yaml file with multiple identical roots? I can't think that's a very common issue. I dont see any places in our code that we do or did that. Maybe worth closing as a will not fix?

What do you think?

[AlexeyMerzlyakov]

This is initially the issue of duplicating keys (in this example: 2 duplicating top_node) in YAML-file. Duplicating keys in YAML-file are restricted by the standard - latest YAML spec https://yaml.org/spec/1.2/spec.html#mapping// says:

The content of a mapping node is an unordered set of key: value node pairs, with the restriction that each of the keys is unique.

As I understand, rcl_yaml_param_parser uses its own parser that makes the work with duplicating keys in YAML-s to be out-of-standard. If we will make our own parser, as you suggested by cutting YAML-file to sub-YAMLs based on top keys, load them separately and then merge parameters, this might resolve the issue.

But regarding the large question, I would rather agree with you that the problem is not so common to go out-of-standards and batter the loading mechanism in rewritten_yaml.py for all yaml-files. It also might have a negative performance effect on loading stage because of many python-based logic should be introduced there.

[SteveMacenski]

I think that’s a good analysis. Closing ticket as won’t fix, the core concept is invalid