refactor(pose_initializer): rework parameters

[PhoebeWu21]

Description

Implement the ROS Node configuration layout described in https://github.com/orgs/autowarefoundation/discussions/3371 for the pose_initializer package.

• Create the schema

Tests performed

Package build and launch locally.
colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release --packages-up-to pose_initializer

Effects on system behavior

More reliable and faster parameter configuration file creation.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.
• The PR has been properly tested.
• The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

• There are no open discussions or they are tracked via tickets.
• The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

[KYabuuchi]

@PhoebeWu21 Thanks for making the PR!
Unfortunately, we want to avoid managing ndt_enabled and others in pose_initializer.param.yaml.
For example, this launch file rewrites ndt_enabled based on the runtime arguments.

Therefore, I would like you to revert the changes to pose_initializer.param.yaml and pose_initializer.launch.xml and make the PR just add a schema.json file. 🙏

(Also, I am not familiar with the syntax of json.scheme, but I think the default in json.scheme needs to be removed if my request is to be reflected.)

[PhoebeWu21]

@KYabuuchi Got it. But if remove ndt_enabled and others from the pose_initializer.param.yaml file, they should also be removed from the schema file to pass the build-and-test check.

[KYabuuchi]

@PhoebeWu21 Hmm, I'm not familier in JSON schema validation, but is the only way to pass the build-and-test to remove parameters not included in param.yaml from the json.schema?
In my understanding, the purpose of json.schema is to define the parameters that nodes require, so I thought that json.schema should include ndt_enabled and others as much as possible.

If we have no choice except to remove them, I would like you to proceed in that manner.

[PhoebeWu21]

@PhoebeWu21 Thanks for making the PR! Unfortunately, we want to avoid managing ndt_enabled and others in pose_initializer.param.yaml. For example, this launch file rewrites ndt_enabled based on the runtime arguments.

Therefore, I would like you to revert the changes to pose_initializer.param.yaml and pose_initializer.launch.xml and make the PR just add a schema.json file. 🙏

(Also, I am not familiar with the syntax of json.scheme, but I think the default in json.scheme needs to be removed if my request is to be reflected.)

@KYabuuchi Got it. But if remove ndt_enabled and others from the pose_initializer.param.yaml file, they should also be removed from the schema file to pass the build-and-test check.

@PhoebeWu21 Hmm, I'm not familier in JSON schema validation, but is the only way to pass the build-and-test to remove parameters not included in param.yaml from the json.schema? In my understanding, the purpose of json.schema is to define the parameters that nodes require, so I thought that json.schema should include ndt_enabled and others as much as possible.

If we have no choice except to remove them, I would like you to proceed in that manner.

Hi @kaspermeck-arm @ambroise-arm, I am not sure if there is other methods to deal with this situation. Please give me some advice, thank you.

[ambroise-arm]

Unfortunately, we want to avoid managing ndt_enabled and others in pose_initializer.param.yaml.

@KYabuuchi The goal of the change is not to manage the parameters in pose_initializer.param.yaml. The goal is to move the default values of the parameters from the launch file of the package to the config file of the package.

Looking at another launch file from the same tier4_localization_launch package you linked to, I see https://github.com/autowarefoundation/autoware.universe/blob/main/launch/tier4_localization_launch/launch/pose_twist_fusion_filter/pose_twist_fusion_filter.launch.xml#L41 simply references the default values from the config file.

So @PhoebeWu21 it may just be about adding a <arg name="config_file" value="$(find-pkg-share pose_initializer)/config/pose_initializer.param.yaml"/> line to https://github.com/autowarefoundation/autoware.universe/blob/main/launch/tier4_localization_launch/launch/pose_twist_estimator/pose_twist_estimator.launch.xml#L14 (or something like that)

EDIT: and also double-checking whether other launch files are affected in the same way

[KYabuuchi]

@ambroise-arm Thank you for clarifying the goal. 😄
I believe the issue is that the launch file dynamically change the ndt_enabled based on the runtime argument 'pose_source'.
For example, ros2 launch autoware_launch autoware_launch.xml pose_source:=ndt will result in ndt_enabled=true, ros2 launch ... pose_source:=eagleye will result in ndt_enabled=false.
Are you suggesting placing distinct config files for each condition in pose_initializer/config/*.param.yaml?

[ambroise-arm]

Are you suggesting placing distinct config files for each condition in pose_initializer/config/*.param.yaml?

Now that you mention it, it sounds like an excellent idea! It would provide a tidier code in pose_twist_estimator.launch.xml, and it would simplify the interface between the two launch files with only the path to the config file to be used. And it matches the design described in https://autowarefoundation.github.io/autoware-documentation/main/contributing/coding-guidelines/ros-nodes/parameters/#parameter-file under "Launch parameter file". Yes, that's probably the best way to go.

However, I was suggesting a more modest change: keeping the <arg name="..."/> interface as it currently exists for all the parameters, but still adding the missing parameters to the config file as part of this PR. It shouldn't affect any packages that include the launch file from this package but satisfy the new schema linting. What I suggested above with config_file didn't make much sense on its own.

The first option sounds like a bigger change than the second. I don't know if we want the scope of this PR to be this big.

[KYabuuchi]

I see. The approach I suggested, involving multiple file placements, is a big changes.
I think it would be better to keep it a modest change that leaves <arg name="..." /> as ambroise-arm says.
As for me, I believe the current commit, in which all parameters independent on runtime arguments are included in param.yaml and passes the json-schema-check, is approvable.

@PhoebeWu21 Are you ok with the current style?
If OK, the code owner will review and approve.

[PhoebeWu21]

@KYabuuchi Ok. Thank you.
I made a new commit please check if my understanding is correct.

[KYabuuchi]

@PhoebeWu21 Oh? I apologize. I was approvable in the last state.
So if you agree with me, you did not have to commit 095de6b.

As for me, I believe the current commit, in which all parameters independent of runtime arguments are included in param.yaml and passes the json-schema-check, is approvable.

What I mean here is to include only parameters independent of runtime arguments in param.yaml, excluding dependent parameters (such as ndt_enabled) from both param.yaml and json.schema.

[PhoebeWu21]

@KYabuuchi Ahh got it. Sorry for the misunderstanding.

[KYabuuchi]

@YamatoAndo Now it is ready to review. Please review and approve 🙏

[ambroise-arm]

So if you agree with me, you did not have to commit 095de6b.

I disagree. The point of https://github.com/orgs/autowarefoundation/discussions/3371 (that this PR implements) is to have a 1 to 1 match between the parameters used in the C++ node and the schema/parameter files. I don't see the issue with having this commit in the PR. And having the PR without it doesn't fulfill the goal of the configuration dojo. (at least not my understanding of it)

cc @kaspermeck-arm

[KYabuuchi]

@ambroise-arm Sorry for the misunderstanding. I would like to confirm again, do you intend to put both <param from="$(var config_file)"> and <param name="ndt_enabled"> in the launch file?
And since <param name="ndt_enabled"> takes priority (I'm not sure), we can pass the parameters corresponding to the runtime arguments as before?

[isamu-takagi]

@PhoebeWu21 @KYabuuchi @ambroise-arm

Autoware allows parameters to be split into multiple parameter files and launch files. Therefore, there is no guarantee that the parameter file and schema file will match exactly. (To be more precise, schema matches the temporary merged parameter file that the ROS launch system gives to the node.)

It has been pointed out that due to file splitting, schema and parameter file validation cannot be done one-on-one.

• docs(parameters): add DevOps Dojo: ROS Node Config documentation autoware-documentation#367 (comment)

  Apply parameters to my_node by two parameter files (e.g. common part and others)

Therefore, possible solutions are as follows (some are temporary solutions)

• Define only the schema file and do not validate the parameter file
• Split the schema file into expected division units of parameter file
• Create a dummy parameter file that is not actually used but for validation (because it is unclear which parameter in the launch file or the parameter file will be used)
• Create a mechanism to validate the temporary merged parameter file given to the node by the ROS launch system

Edit:

• Remove "required" field from schema file

[ambroise-arm]

do you intend to put both <param from="$(var config_file)"> and <param name="ndt_enabled"> in the launch file?

@KYabuuchi Yes, in the pose_initializer.launch.xml launch file. But it is already like this, so it is more about keeping it this way.

And since <param name="ndt_enabled"> takes priority (I'm not sure), we can pass the parameters corresponding to the runtime arguments as before?

@KYabuuchi Exactly. I also assume it takes priority.

Create a dummy parameter file that is not actually used but for validation (because it is unclear which parameter in the launch file or the parameter file will be used)

@isamu-takagi I think it will result in something a bit like this, but I don't think the issue your are linking to is a problem here. What will happen is that we will have a schema file in this package that has all its fields as "required", and a config file that list all the fields. So it should be validated correctly. The launch file of this package then uses this config file as a default, while also requiring some of its arguments to be passed directly. So in practice the ndt_enabled, gnss_enabled, ekf_enabled, yabloc_enabled and stop_check_enabled arguments will always be overridden. At least until changes as described in #5773 (comment) are done.

[KYabuuchi]

For this package, I think the approach mentioned by ambroise-arm is fine.
However, as isamu-takagi says, some packages have split multiple parameter files, so it will be an issue that will have to be tackled eventually.

[isamu-takagi]

So in practice the ndt_enabled, gnss_enabled, ekf_enabled, yabloc_enabled and stop_check_enabled arguments will always be overridden.

@KYabuuchi @ambroise-arm
Is this behavior guaranteed by the launch_ros specification?

[KYabuuchi]

i am not sure😞

[isamu-takagi]

@KYabuuchi
I found a description that the last parameter has priority. So be careful about the order of parameters in the launch file. Also, note that namespace also affects priority.
https://github.com/ros2/launch_ros/blob/humble/launch_ros/launch_ros/actions/node.py#L175-L182

[KYabuuchi]

@isamu-takagi Thank you for finding the description. Very useful information!

[KYabuuchi]

I realized that the override rules are complex, and the current approach might actually introduce more bugs.
(I have created tests to verify the parameter override rules, so please take a look. https://github.com/KYabuuchi/ros2_launch_learn/tree/feat/minimum_case?tab=readme-ov-file#the-result)

I also posted this issue in the github-actions thread. I don't think we should merge PRs with parameter overrides at this time.

[KYabuuchi]

@PhoebeWu21 Sorry to keep you waiting for so long.
As a method to pass parameters from the launch file, the chosen approach is to use substitution within YAML.
reference , example

If it's not too inconvenient, would you mind revising this PR to incorporate that method? If it is troublesome, I am willing to work on it and create a new PR.
Thank you

[PhoebeWu21]

@KYabuuchi Sure, I can deal with the modifying. But I am a bit confused about the revising of the pose_initializer.launch.xml file. Should I revise it to the format like pose_twist_estimator.launch.xml?

[KYabuuchi]

@PhoebeWu21 Thanks for your cooperation. What I want you to do is the following:

• Remove all <param name=... /> from pose_initializer.launch.xml.
• Replace <param from="$(var config_file)" allow_substs="true"/> in pose_initializer.launch.xml. ref
• Add ndt_enabled: $(var ndt_enabled) etc. in pose_initializer.param.yaml. ref
• Add ndt_enabled etc. to pose_initializer.schema.json.
• Create a PR to change param.yaml in autoware_launch, too. ref

You do not need to refer to pose_twist_estimator.launch.xml to make this change.

[KYabuuchi]

Thank you for your help!

Since autoware_launch has the same parameter.yaml, please create a PR to modify that as well. Sorry for bothering you.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (765a596) 15.32% compared to head (0359901) 7.56%.
Report is 580 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #5773       +/-   ##
==========================================
- Coverage   15.32%   7.56%    -7.76%     
==========================================
  Files        1721      10     -1711     
  Lines      118559     185   -118374     
  Branches    37995       7    -37988     
==========================================
- Hits        18169      14    -18155     
+ Misses      79657     165    -79492     
+ Partials    20733       6    -20727     

Flag	Coverage Δ
differential	7.56% <ø> (?)
total	?

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KYabuuchi]

Thank you for your patient support!

I have confirmed that pose_initializer runs correctly in logging_simulator.launch.xml & planning_simulator.launch.xml.