Rclcpp: 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, https://github.com/ros2/rclpy/pull/547 and https://github.com/ros2/rclcpp/pull/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 https://github.com/ros2/design/pull/272 is relevant here.

[ivanpauno]

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

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.

[hidmic]

One more thought about this. At the time, best match-based precedence made sense to me. It made it easy to specialize configuration with no regard for order. But I could also imagine someone trying to mass apply a change after all other configuration has been provided.

So, what about respecting order but adding a CSS' !important rule equivalent? I get to mass apply changes using wildcards, and I get to apply some important configuration with no regard for order.

[clalancette]

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.

I think I still stand behind my previous thought that specificity should override more general terms. That is, a specific node name should override the wildcard, because a common use case is to have a parameter that is the same across nodes, but one that is different. While using command-line ordering is also valid (assuming it is consistent), I think it is more surprising.

If we assume that that logic is what we want, it sounds like the technical issue here is that we are applying the ordering too early in the process. We may have to change it so that we delay the applying of parameters until much later in the process, until we know what the fully-qualified node name is. That may be easier said than done, but I think we should shoot for a good user experience.

[ivanpauno]

We may have to change it so that we delay the applying of parameters until much later in the process, until we know what the fully-qualified node name is.

I think that's technically impossible. If the user doesn't specify the node name and namespace at launch time, Node action will have to use a wildcard.

The only thing that can be done in launch to improve the situation was proposed in https://github.com/ros2/launch_ros/pull/154.

[clalancette]

I think that's technically impossible. If the user doesn't specify the node name and namespace at launch time, Node action will have to use a wildcard.

I don't know the specifics here, but how can this be the case? At some point during the startup of a process, we have to know the fully-qualified node name. Otherwise how would we get that information for ros2 node list (and other things)?

[ivanpauno]

I don't know the specifics here, but how can this be the case? At some point during the startup of a process, we have to know the fully-qualified node name. Otherwise how would we get that information for ros2 node list (and other things)?

When executing the command, the node name and namespace can't be known except remapping rules for then (_ns, _name) are passed, as they are hard-coded in the executable.
We need the node fqn before executing the command, if not we can't add the correct arguments to set the parameters (or we have to use wildcards to pass them).

---

Parameters can also be modified at runtime with services.
Those services can't be opted-out, and launch actions shouldn't rely on using them IMO.

Passing parameters through command line or at runtime will not necessarily produce the same behavior.

---

(edit) To sum up: we must know the node fqn before running the executable to be able to pass the correct arguments without using a wildcard. Replacing passing arguments through the command line with runtime service calls won't result in the same behavior.

[jacobperron]

I'm still trying to wrap my head around the issue. Is it possible to give wildcard params passed on the command line higher precedence than wildcard params passed in a YAML file? Then launch_ros could pass dictionary params as CLI arguments instead of writing to a temporary YAML file.

[hidmic]

Is it possible to give wildcard params passed on the command line higher precedence than wildcard params passed in a YAML file?

It isn't impossible, but rcl would have to change. Also, wouldn't it be somewhat unintuitive?

[jacobperron]

wouldn't it be somewhat unintuitive?

I don't know if it's intuitive, but I don't think it's counter-intuitive :smile:

[ivanpauno]

Is it possible to give wildcard params passed on the command line higher precedence than wildcard params passed in a YAML file?

Maybe that's not a bad idea:

my_node:
    ros__parameters:
        my_int: 42
/**:
    ros__parameters:
        my_int: 43

In that case, I would be really surprised if the parameter using a wildcard overrides the one using a fqn.

ros2 run my_pkg my_exec --ros-args --params-file /path/to/above/file -p my_int:=45

In that case, I'm really not surprised. It feels pretty natural besides not using my_node:my_int:=45

[jacobperron]

ros2 run my_pkg my_exec --ros-args --params-file /path/to/above/file -p my_int:=45

In that case, I'm really not surprised. It feels pretty natural besides not using my_node:my_int:=45

@ivanpauno To clarify, are you saying that it's not that surprising if the assigned value was 45? I would agree that it seems reasonable.
I do think it's kind of arbitrary though, to say that CLI params always override YAML params. At least it would be a relatively easy rule to remember.

[clalancette]

Can I suggest here that we instead change this to a design document (or modification to one) now? I think I've seen at least 3 or 4 different proposals here (though some of them overlap), and it would be good to consolidate all of that down to one document. Once we have agreement on the design document, then we can think about how to implement it. What do people think?

[ivanpauno]

:+1: to @clalancette proposal.
I think that the current design document is not clear about ordering between wildcard and not wildcard arguments, so it would be good to clarify that.