Odd parameter overrides' ordering when using wildcards

[hidmic]

Bug report

Required Info:

• Operating System:
  • Ubuntu 18.04
• Installation type:
  • Binaries
• Version or commit hash:
  • Eloquent (mostly 0.8.1)
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

1. Write a parameter YAML file, explicit_params.yaml, setting use_sim_time explicitly referring to a node:

   talker:
       ros__parameters:
           use_sim_time: false    

2. Write a parameter YAML file, wildcard_params.yaml, setting use_sim_time implicitly using wildcards:

   /**:
       ros__parameters:
           use_sim_time: true    

3. Run a talker node:

   • Passing the parameter YAML file using wildcards first:

     ros2 run demo_nodes_cpp talker --ros-args --params-file path/to/wildcard_params.yaml --params-file path/to/explicit_params.yaml

   • Passing the parameter YAML file being explicit first:

     ros2 run demo_nodes_cpp talker --ros-args --params-file path/to/explicit_params.yaml --params-file path/to/wildcard_params.yaml

Expected behavior

I'm not sure if we've ever fully specified behavior when using wildcards.

• If wildcards have lower precedence than fully qualified names, command line order should not matter i.e. use_sim_time always false.

• If command line order is respected, use_sim_time should be false in the first case, true in the second case.

Actual behavior

First given override wins.

Additional information

This is inconsistent with how it works for rclpy nodes, where command line order is respected. Looking at both rclpy and rclcpp implementations, it looks behavior emerged by pure chance. We should probably be matching wildcards where we can enforce a consistent precedence, down in rcl.

[clalancette]

* If wildcards have lower precedence than fully qualified names, command line order should not matter i.e. `use_sim_time` always `false`.

My opinion is that this should be the rule. use_sim_time is a bad example, but I can definitely imagine use-cases where you want to set the same parameter on a bunch of nodes, but have one that is different.

We should probably be matching wildcards where we can enforce a consistent precedence, down in rcl.

Yep, totally agreed.

[hidmic]

I was discussing a larger, breaking API change in rcl with @ivanpauno earlier today and he brought up a good point: fixing this in rclpy and rclcpp will have the same effect and can land with Foxy.

Ideally, and if everyone agrees, I'd much like to revamp rcl_yaml_param_parser API and move all wilcard matching there. In the meantime, ros2/rclpy#547 and #1094 will do.

[ivanpauno]

This behavior change introduced a problem in launch files:

...
Node(
  ...,
  parameters: [parameter_file_path, {param1: value1, param2: value2}],
  ...,
)
...

If the user doesn't explicitly pass the node name and namespace, we can't know the fully qualified node name in advance.
Thus, we generate a temporal parameter file using a wildcard.
Alternatively, we could pass a wildcard parameter argument --ros-args -p <p_name>:=<p_value>.

In both cases, the issue is that we cannot override a parameter passed in the parameter file (which was using the fully qualified node name), no matter the ordering in which they are passed.

IMO, we should change behavior again, so both wildcards and parameters with explicitly specified node fqn have the same priority. Last passed argument will always be the valid one.

The other question is if we should backport that fix, as it's a behavior change.
Considering that it is now impossible to override a parameter in a parameter file from a launch file, I would say that we should.

[jacobperron]

If we categorize this as buggy behavior (which I think it is), then I think backporting a fix should be okay.

[hidmic]

So, it's important to stick to a precedence in all client libraries. That's what the PRs that were aiming to address this issue did. Before that, it was UB.

---

@ivanpauno Your argument is a valid one. I think we may get around it in an ABI compatible way by performing wildcard matching down in rcl_yaml_param_parser, so that we don't have to do so in rclpy or rclcpp where that order information is not available.

But I will say that:

Considering that it is now impossible to override a parameter in a parameter file from a launch file, I would say that we should.

isn't strictly true. It is impossible as long as you combine exact node FQNs in the parameter YAML file with command line parameter overrides without a node FQN. One could argue that if you knew enough about the executable to hardcode node FQNs in that file, you know enough to provide it in the launch file as well. That'd require an API change in launch_ros though.

[ivanpauno]

@clalancette @hidmic friendly ping.
Do we want to recheck this decision considering the issue it causes in launch?

[hidmic]

I'm fine with using command line order instead of a wildcard precedence. It's not necessarily worst. But I do think that launch_ros should provide means to specify a node name for a given parameter. It's too tied to the notion of one node per process still. Perhaps ros2/design#272 is relevant here.

[ivanpauno]

Sorry, I missed the previous comments and I pinged again unnecessarily 😁.

isn't strictly true. It is impossible as long as you combine exact node FQNs in the parameter YAML file with command line parameter overrides without a node FQN. One could argue that if you knew enough about the executable to hardcode node FQNs in that file, you know enough to provide it in the launch file as well. That'd require an API change in launch_ros though.

We can pass the node name and namespace to Node action, that should be enough (as passed parameters apply to that node only).

@ivanpauno Your argument is a valid one. I think we may get around it in an ABI compatible way by performing wildcard matching down in rcl_yaml_param_parser, so that we don't have to do so in rclpy or rclcpp where that order information is not available.

Sounds good.