Expose --params-file option to composable nodes

[dawonn-haval]

Fixes #1215

[ivanpauno]

What if the node container is in a different machine that the node that's sending the loading request?
How would the file be found?

Except the file exist in the target machine (where the node container is being run), this doesn't make much sense.

IMO, what makes more sense is to load the file in the machine that's sending the request, and generate the request appropriately.

@dawonn-haval what do you think?

[ivanpauno]

Why this argument is prefixed with -- and the one above (use_intra_process_comms) isn't.
That sounds weird to me.

[dawonn-haval]

I didn't notice and it worked fine. I'll try to remove the '--' and see if it still works.

Regardless, I suppose I should move it to a const since it's referenced more than once... :)

[dawonn-haval]

I think both use cases are valid; The existing functionality reads a parameter file locally at the launch file host, and this function reads them at the runtime host.

Honestly, I think the fact that launch_ros uses a different parameter file format than rclcpp should be considered a bug and should be resolved. Perhaps supporting both file formats should be supported since this logic has now been released and might already be in use by somebody?

In any case, exposing this flag enables developers to use the exact same code for parsing parameter files as when using ros2 run <...> and via component composition, which is advantageous for QA.

[ivanpauno]

I think both use cases are valid; The existing functionality reads a parameter file locally at the launch file host, and this function reads them at the runtime host.

I'm not sure if both should be valid, I don't see much value on loading a parameter file on the "runtime host".
If you have any use case where that is desired, please give more details.

Honestly, I think the fact that launch_ros uses a different parameter file format than rclcpp should be considered a bug and should be resolved. Perhaps supporting both file formats should be supported since this logic has now been released and might already be in use by somebody?

I agree that having two different file formats is weird.
I think this one is the real problem you're trying to solve.
I'm not sure why that's the case, maybe @hidmic @mjcarroll have more insights.

In any case, exposing this flag enables developers to use the exact same code for parsing parameter files as when using ros2 run <...> and via component composition, which is advantageous for QA.

We could use exactly the same parsing code if we would want.
We would need a proper wrapper of rcl_param_yaml_parser in rclpy (which I haven't checked if we have), and make use of it in launch_ros.

[hidmic]

The existing functionality reads a parameter file locally at the launch file host, and this function reads them at the runtime host.

I find that brittle and surprising.

Honestly, I think the fact that launch_ros uses a different parameter file format than rclcpp should be considered a bug and should be resolved. Perhaps supporting both file formats should be supported since this logic has now been released and might already be in use by somebody?

The reason why file formats are slightly different is because their recipients are different. Any given executable may contain one or more nodes. Therefore, when passing parameters in a file on execution, which parameter goes with which node must be specified. In contrast, when dynamically loading a composable node, it is clear that any parameters belong to said node and thus further specification is not required.

In any case, exposing this flag enables developers to use the exact same code for parsing parameter files as when using ros2 run <...> and via component composition, which is advantageous for QA.

If that's the use case, perhaps passing a parameters' file to the container executable or having a launch_ros composable nodes' loader that can mix and match nodes and parameters' files would be more appropriate.

[ivanpauno]

@hidmic maybe the launch LoadComposableNode actions should support both file formats, so the same file can be reused regardless of you're running the node alone in a process, composing it manually in a process with other nodes, or loading it in a node container.

For that to work, the node name and namespace are needed at launch time, so it won't work in all cases (it will only work when both are passed in the LoadComposableNode action constructor.
The simplified format can also be supported, and that one will always work, regardless if the node name and namespaces are known at launch time or not.

Does that make sense?

[hidmic]

Does that make sense?

Yeah, that's one option. It'd have to be applied to other use facing APIs too, such as ros2 component load. Also, the ability to disambiguate between file formats depends on current restrictions in parameter types, so perhaps it should be made explicit when passing either format (use these parameters vs. use parameters_from this parameter collection).

[ivanpauno]

Yeah, that's one option. It'd have to be applied to other use facing APIs too, such as ros2 component load. Also, the ability to disambiguate between file formats depends on current restrictions in parameter types, so perhaps it should be made explicit when passing either format (use these parameters vs. use parameters_from this parameter collection).

Yeah, it doesn't sound like we can distinguish between both formats, as the parameters files for components already accept a nested dictionary.

@dawonn-haval I'm closing this one, as supporting to load a file from the "runtime host" machine sounds like a fragile idea.
I will copy the alternative solution described in this comment to an issue in launch.