refactor(lidar_centerpoint): rework parameters

[tby-udel]

Description

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

• Move the default parameter file to a standardized location. Use the JSON Schema for that.
• Updated readme file.

Tests performed

Package built and launched locally.

Notes for reviewers

Nothing inside the src code part was changed, only taking the parameters out.

Interface changes

Parameter values are now required to be passed to the node when launched.

Effects on system behavior

None

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.

[knzo25]

@tby-udel
I will paste the other PR you sent to keep track of their relation.
#7204

This one seems to fail on the same things :c

[tby-udel]

In line 105, the check passed, as centerpoint_tiny_ml_package.param.yaml should be compared with centerpoint_tiny_ml_package.schema.json. However, in line 106, it's centerpoint_tiny_ml_package.param.yaml comparing with centerpoint_tiny.schema.json, which shouldn't be happening, they are meant for different files.
The line 115 error has been corrected by deleting the 'has_twist' attribute from .schema.json file.

[knzo25]

@tby-udel
It seems that behavior is by design
https://github.com/autowarefoundation/autoware-github-actions/blob/main/json-schema-check/validate_json_schemas.py

What happens is that first perception/lidar_centerpoint/schema/centerpoint_tiny.schema.json is selected as the target for the check. Then, all .param.yaml files starting with centerpoint_tiny are checked, which end up with two parameter files being tested, among them one that should not be checked. We probably can not change the github action, so one approach would be not use centerpoint_tiny but centerpoint_tiny_base or another name that eliminates the base name conflict

[tby-udel]

I have renamed the files and made necessary changes to the yaml,schema and launch files.

[knzo25]

@tby-udel
I think that I did not correctly conveyed my intention in the previous post after seeing the latest changes.

The reason it fails is that
centerpoint.schema.json triggers checks for centerpoint.param.yaml and centerpoint_ml_package.param.yaml since they share the same centerpoint name.
What I proposed is to have centerpoint_base and centerpoint_ml_package so that there are no more conflicts and each json would only trigger one yaml check.

The change to "_base" should not propagate in more places that the name of the param file

[tby-udel]

If the parameter file's name is changed, then the launch file won't be able to find the correct parameter file, as the parameter file is used through:

arg name="data_path" default="$(env HOME)/autoware_data" description="packages data and artifacts directory path"
arg name="model_name" default="centerpoint_tiny" description="options: centerpoint or centerpoint_tiny"
arg name="model_path" default="$(var data_path)/lidar_centerpoint"
arg name="model_param_path" default="$(find-pkg-share lidar_centerpoint)/config/$(var model_name).param.yaml"
arg name="ml_package_param_path" default="$(var model_path)/$(var model_name)_ml_package.param.yaml"
arg name="class_remapper_param_path" default="$(var model_path)/detection_class_remapper.param.yaml"
arg name="build_only" default="false" description="shutdown node after TensorRT engine file is built"

In order to correctly launch this node, if the parameter file's name is changed to centerpoint_base.param.yaml, then inside the launch file, for the "model_name" part,it should be description="options: centerpoint_base or centerpoint_tiny , and only after changing the launch file, can the "model_param_path" become the correct /config/$(var model_name).param.yaml.

And in the current centerpoint.param.yaml file, the onnx paths and engine paths are organized as this:

encoder_onnx_path: "$(var model_path)/pts_voxel_encoder_$(var model_name).onnx"
encoder_engine_path: "$(var model_path)/pts_voxel_encoder_$(var model_name).engine"
head_onnx_path: "$(var model_path)/pts_backbone_neck_head_$(var model_name).onnx"
head_engine_path: "$(var model_path)/pts_backbone_neck_head_$(var model_name).engine"

So now that the model_name is changed from centerpoint to centerpoint_base, but these files' name are still centerpoint.onnx and centerpoint.engine, so instead of $(var model_name), they should all be changed to centerpoint.

If the json_schema_check cannot change, I think we can't solve this problem by changing the centerpoint.param.yaml only, so I changed centerpoint.param.yaml (renamed and changed the onnx/engine paths), centerpoint.schema.json(renamed) and the launch file.

[knzo25]

@tby-udel
I think we still have a misunderstanding
model_name should still be centerpoint or centerpoint_tiny.

What should change is
<arg name="model_param_path" default="$(find-pkg-share lidar_centerpoint)/config/$(var model_name).param.yaml/>"
to
<arg name="model_param_path" default="$(find-pkg-share lidar_centerpoint)/config/$(var model_name)_base.param.yaml/>

and so on.

If you think this is going to deep and out of scope for the contribution / time you expected, let us know and we can continue this from here

[knzo25]

@tby-udel
If you need me to run the CI/CD please drop me a mention. Otherwise, there is a chance I will not see that you made a commit and want to try it (I just ran it, and it is not passing the tests 😢 )

[github-actions]

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

• You've checked our contribution guidelines.
• Your PR follows our pull request guidelines.
• All required CI checks pass before marking the PR ready for review.

[knzo25]

Parameters section
should be instead a link in markdown style

[knzo25]

The change in this section is not adequate in my opinion. In the previous version we explained the process of creating a new model/version of centerpoint (i.e., how to create new parameters, modify launchers ,etc). This idea should be kept in this PR.

Expressions including exclamations are not adequate in this kind of documents

[knzo25]

Suggested change

	The `lidar_centerpoint` node has `build_only` option to build the TensorRT engine file from the ONNX file, and the `build_only` option is inside the launch file of `lidar_centerpoint` node, and all the ROS parameters are moved in `.param.yaml` files.
	The `lidar_centerpoint` node has a `build_only` option to build the required TensorRT engine file from an ONNX file. This parameter is set in the launch file of this package, whereas all other parameters are set via `.param.yaml` files.

[knzo25]

Left some comments that need addressing

[knzo25]

@kminoda @YoshiRi
It seems that there are some param files missing for the sigma centerpoint. How should we proceed with this?
Also please do not forget to review the companion / sister PR

[tby-udel]

All the requested changes are made.

[knzo25]

Sorry, this lacks a little in both writing and content.
The idea of the section is to describe the process of creating the files for a new model. In addition to some grammatical errors, instead of giving examples of parameters of the config file explain what kind of parameters they have. This links to the the ml config file, which is not mentioned here

[tby-udel]

Sorry, this lacks a little in both writing and content. The idea of the section is to describe the process of creating the files for a new model. In addition to some grammatical errors, instead of giving examples of parameters of the config file explain what kind of parameters they have. This links to the the ml config file, which is not mentioned here

@knzo25 Thank you for your instructions, I have updated the README.md file, please check if this meets your expectation. If you can give proposal for me to accept, it would be of great help for me. I will click the button commit suggestion afterwards. I really learned a lot with your help for this PR, thank you so much.

[knzo25]

@tby-udel
Since you mentioned you are new to git/github I think it is not uncalled for to mention that when a reviewer presents some code suggestions, and you agree with those, it is customary to accept the changes instead of making a new commit with those changes. It makes reviewing easier. In the same fashion, when you address a comment, you should paste the hash of the related commit in the conversation, to the reviewer can check that his/her concern was handled.

There are some parts of the documentation that are not very good and I left the explanation as a review. If you prefer that I give a proposal for you to accept it let me now ! (and in that case please click the button commit suggestion)

Also, remember to mention people, since otherwise we do not receive notifications and only check PRs when we remember it or give a thorough list at the pending PRs

[knzo25]

@tby-udel
There are some conflicts

[tby-udel]

@tby-udel There are some conflicts

I have merged the main branch into the current branch, there doesn't seem to be conflicts after merging.

[kminoda]

Would you fix the name of this file? (schemal.json to schema.json)

[tby-udel]

@kminoda I have fixed the name of the file.

[Shin-kyoto]

@tby-udel

Thank you for your PR!
This PR causes conflicts with main branch, Could you please resolve them?

[Shin-kyoto]

@tby-udel
Thank you for creating the PR.
Would it be possible for you to resolve the conflicts?

To keep Autoware healthy, we close PRs if there is no response for more than a week after requesting conflict resolution. If this PR is still active, please let us know. Even if this PR is closed, you are welcome to reopen it once the conflicts have been resolved.

[Shin-kyoto]

@tby-udel

friendly ping

[Shin-kyoto]

@tby-udel

friendly ping

[Shin-kyoto]

@tby-udel

friendly ping

[Shin-kyoto]

@tby-udel

We did not get the reply from author for a month, so I close this PR.
@tby-udel, thank you so much for your PR and feel free to reopen it.