Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Parameter management #66

Closed
3 of 6 tasks
takayuki5168 opened this issue Jul 26, 2022 · 14 comments
Closed
3 of 6 tasks

Parameter management #66

takayuki5168 opened this issue Jul 26, 2022 · 14 comments
Assignees
@takayuki5168 @soblin @shmpwk @kminoda

Comments

@takayuki5168
Copy link
Contributor

takayuki5168 commented Jul 26, 2022 •
edited
Loading

Checklist

  • I've read the contribution guidelines.
  • I've searched other issues and no duplicate issues were found.
  • I've agreed with the maintainers that I can plan this task.

Description

Currently, autoware has its parameters only in autoware.universe.
That means, when we want to integrate autoware to a specific vehicle and modify parameters, we have to fork autoware.universe which also has a lot of code other than parameters.

Now we have the autoware_launch repository referring to autoware.universe.
We, TIER IV, concluded that it would be better for autoware_launch to have parameters of autoware.universe, and modify parameters for integration.
With this integration procedure, we don't have to fork autoware.universe.

This idea is described here.
https://autowarefoundation.github.io/autoware-documentation/pr-171/contributing/coding-guidelines/ros-nodes/parameters/

Purpose

In order to easily modify autoware parameters (e.g. for integration to a specific vehicle)

Possible approaches

Definition of done

All checkboxes of the possible approaches are closed.
@takayuki5168 takayuki5168 self-assigned this Jul 26, 2022
This was referenced Jul 26, 2022
Closed
Closed
@takayuki5168
Copy link
Contributor Author

@xmfcx Hi, I'm Takayuki Murooka from the planning/control team at TIER IV

We, TIER IV, concluded how to manage parameters of autoware described in this issue and the following document.
https://autowarefoundation.github.io/autoware-documentation/pr-171/contributing/coding-guidelines/ros-nodes/parameters/

Currently, we are working on creating this parameters' management system described in "Possible approaches".

Miyake-san suggested to me that I should share this with you. Do you have any questions or opinions?

@takayuki5168 takayuki5168 mentioned this issue Jul 26, 2022
Closed
4 tasks
@xmfcx
Copy link
Contributor

xmfcx commented Aug 1, 2022 •
edited
Loading

@takayuki5168 @kenji-miyake

Hi Murooka-san, nice to meet you.

I have been thinking about similar solutions for following issues I've faced with Autoware:

Launch system and parameters

  • It's easy miss to set some parameters
  • Hard to figure out where the parameters are set
  • Launch system is too complicated
  • Hard to create a launch configuration
  • It's not clear which parameters are shared

Node management

  • It's not clear what nodes
    • need to run
    • have crashed
    • are running
    • are functioning ok
    • belong to what high level module
    • depend on what nodes
  • Hard to debug only a single node

This solution that you brought, solves the bold ones from the list about launch system and parameters.

I'm grateful to you for creating this solution and the automation of transferring of the param.yaml files.

Automatic launch file generation

I also had some ideas that are a bit broader but harder to achieve. I will try to list them:

  • We should have a script/tool that automatically generates launch files from the nodes.
  • We should have a list of shared parameters in the autoware.
    • If any parameter should be used in more than 1 node, it should be put to that list.
    • When creating a node, the developers should check that shared parameters list before declaring parameters.
  • For a single package:
    • A single package can have multiple executables/nodes with different parameter sets.
    • It will probe the nodes for declared parameters
    • It will check the parameter list yaml file for each node params/$NODE_NAME.yaml to make sure it has all the declared parameters
    • It will check if shared parameters exist in the $NODE_NAME.yaml
      • If so, it will ignore it and use the one from the shared parameters list.
    • It will copy rest of the parameters from the file to a single large yaml file. (along with parameters from other nodes.)
  • We will have a list of nodes that should be launched for a required configuration.
    • Like run whole stack, planning only, control test, lidar perception test etc.
    • These are just list of packages and nodes (without parameters).
    • Maybe we should first list all the possible nodes in the autoware workspace and mark the ones we see fit for these lists.
  • This launch file generator tool will go through the nodes in the list for a specific configuration.
    • It then collects the parameters required for these nodes and puts them into a single large yaml file.

This method could automate launch file creation and eliminate human error while creating launch configurations.

It also requires us to set each node's allow_undeclared_parameters to false, so that all the declared params are defined before starting the node, reference link.

There are things I haven't quite thought through:

  • How to handle topic remapping in autoware
    • We as system developers know what nodes should connect to what through topics but how do we formalize this and automate launch file generation?
    • How do we visualize plug&play nature of nodes?
    • How do we offer alternative nodes for a given role?
      • 2 or more nodes can do the same job, how do we present these alternatives to system dev?
  • How to handle composable nodes?
  • Is it possible to list declared parameters of a node through CLI?

Sorry for the long post but I wanted to share my vision about the system configuration of the autoware. I think your solution is very precise and will work but I'd also like to hear your opinions about my thoughts too.

Thanks!

@takayuki5168
Copy link
Contributor Author

takayuki5168 commented Aug 2, 2022 •
edited
Loading

@xmfcx Thank you for your reply and a nice idea.

This is my personal opinion.
Rather than an automatic launch files generator, I prefer having arguments in autoware_launch and deciding packages to launch.

For example, there is obstacle_cruise_planner and obstacle_stop_planner which have a similar function. One of this is selected by cruise_planner argument, which is hard to find for Autoware users.

My idea is having this cruise_planner argument in tier4_planning_launch of autoware_launch/autoware.launch.xml.
Then, by changing arguments only in autoware_launch/autoware.launch.xml and also parameters in autoware_launch/config (, which do not exist currently), we can integrate Autoware to a specific vehicle.

I'm not sure if this fixed launch system (tier4_*_launch packages are fixed. Users will modify only autoware_launch package) is a good idea or not for OSS users considreing adding new packages, due to less flexibility of the launch system.
For more flexibility, the automatic launch generator will be much better.

@doganulus
Copy link
Member

It would be good if we could launch Autoware from a single config file. For example,

planning:
  default_param_path: "config/params.yaml" # loads default parameter values from the file
  some_common_planning_parameter: {value: 123 } # allows overriding the default value at top level

  obstacle_cruise_planner:
    default_param_path: "config/params.yaml" # loads default parameter values for the package
    some_obstacle_cruise_planner_parameter: {value: 45.6} # overrides the default value
    
control:
  default_param_path: "..."

perception:
  default_param_path: "..."

Surely there is much room for improvement, but a docker-compose-like experience would be a good target when managing parameters.

@kminoda kminoda mentioned this issue Sep 16, 2022
Closed
7 tasks
This was referenced Nov 10, 2022
Merged
Closed
@stale
Copy link

stale bot commented Nov 15, 2022

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the stale label Nov 15, 2022
@kminoda
Copy link
Contributor

kminoda commented Dec 8, 2022 •
edited
Loading

We, TIER IV (including @takayuki5168, @kenji-miyake ), have reached to an alternative solution to the above-mentioned issue. We would like to reflect this improvement if we can agree with this modification.

@xmfcx @.others who is interested in this topic
Let us know if you have any comments on this.

P.S. Please see #66 (comment) for the latest proposal

Current issue (a quick brushup)

Current Autoware has following issue:

  • If we want to customize the parameter depending on our vehicle, we need to take either of the approaches:
    1. Fork autoware.universe and change the parameter
    2. Fork autoware_launch and put a custom parameter in the repository
  • Approach i increases the maintenance cost of your autoware.universe since you need to fork all the packages which is frequently updated by developers.
  • Approach ii can confuse the developers since you are going to have three different parameters that look alike
    • one in package, one in tier4_*_launch, and one in forked autoware_launch

Proposal

Our proposal parameter configuration is as follows

  • Move all the parameter files in autoware.universe/launch/tier4_*_launch/config to autoware_launch/config (We name this “launch parameters”)
  • Create a parameter configuration file that lists all the “launch parameters” for each components (autoware_launch/launch/params/tier4_*_params.launch.xml in the figure)
  • In tier4_*_launch, the launch files loads the “launch parameter” if the argument is given in the parameter configuration file.

Please refer to these PRs for example

image

Pros & Cons

Pros

  • You can customize the Autoware parameters just by forking autoware_launch repository.

Cons

  • You would not be able to launch tier4_*_launch only within autoware.universe since it will require “launch parameters” which is stored in autoware_launch any longer.

Possible effects

  • Launch configuration of tier4_*_launch will be changed as described above.
    • If you are using original tier4_*_launch and your own customized autoware.launch.xml, make sure to incorporate the above change.
    • It won’t be a problem as long as you are using the same version of autoware.universe and autoware_launch.

@stale stale bot removed the stale label Dec 8, 2022
@kenji-miyake
Copy link
Contributor

@kminoda Thank you for your proposal! 😄 Generally it looks good to me, but some details are different from what I thought.

This was my assumption. What do you think?

// autoware_launch/launch/autoware.launch.xml
<include file="$(find-pkg-share autoware_launch)/launch/components/tier4_localization.launch.xml"/>
// autoware_launch/launch/components/tier4_localization.launch.xml
<include file="$(find-pkg-share tier4_localization_launch)/launch/localization.launch.xml">
  <arg name="foo_param_path" value="foo" />
  <arg name="bar_param_path" value="bar" />
</include>
// tier4_localization_launch/launch/localization.launch.xml
<arg name="foo_param_path" />
<arg name="bar_param_path" />

<include file="$(find-pkg-share foo)/launch/foo.launch.xml">
  <arg name="foo_param_path" value="$(var foo_param_path)" />
</include>

<include file="$(find-pkg-share bar)/launch/bar.launch.xml">
  <arg name="bar_param_path" value="$(var bar_param_path)" />
</include>

@kminoda
Copy link
Contributor

kminoda commented Dec 10, 2022

@kenji-miyake Thank you! After internal discussion, let me modify the above proposal a bit.

--

Proposal

Our proposal parameter configuration is as follows

  • Move all the parameter files in autoware.universe/launch/tier4_*_launch/config to autoware_launch/config (We name this “launch parameters”)
  • Create a component launch script in autoware_launch in which we list all the “launch parameters” in the arguments.
    • autoware_launch/launch/components/tier4_localization_component.launch.xml in the figure.
  • tier4_*_launch in autoware.universe will use the parameter file designated in the above component launch script.
    image

Please refer to these PRs for example

Pros & Cons

Pros

  • You can customize the Autoware parameters just by forking autoware_launch repository.

Cons

  • You would not be able to launch tier4_*_launch only within autoware.universe since it will require “launch parameters” which is stored in autoware_launch any longer. (You can still use the default parameters in each packages to launch tier4_*_launch within autoware.universe)

Possible effects

  • Launch configuration of tier4_*_launch will be changed as described above.
    • If you are using original tier4_*_launch and your own customized autoware.launch.xml, make sure to incorporate the above change.
    • It won’t be a problem as long as you are using the same version of autoware.universe and autoware_launch.

@miursh miursh mentioned this issue Dec 14, 2022
Merged
7 tasks
@kminoda
Copy link
Contributor

kminoda commented Dec 15, 2022

We are going to start implementing the above proposal from December 19th.
Let us know if you have any comments until then. (cc. @xmfcx)

@xmfcx
Copy link
Contributor

xmfcx commented Dec 16, 2022

@kminoda @takayuki5168 will there be any automated mechanism that would check these launch files work as expected?

And they are kept updated? (Since they are now going to be more separated.)

I am glad we are reducing the number of launch files, it is very annoying to track how launch files are connected to each other.

@kminoda
Copy link
Contributor

kminoda commented Dec 19, 2022

@xmfcx Thank you for the reply!
As for now, we do not have any concrete plans for automated mechanisms as you have proposed in the previous comment, since we are focusing solely on reducing the number of parameter and launch files for better maintenance capability.

And they are kept updated? (Since they are now going to be more separated.)

Sorry I could not get what you meant here, but if you asked if the tier4_localization_components.launch.xml files are going to be kept updated, yes they are. We are also going to add some descriptions about how to update these files in the Autoware Documentation.

@xmfcx
Copy link
Contributor

xmfcx commented Dec 19, 2022

We are also going to add some descriptions about how to update these files in the Autoware Documentation.

@kminoda that's great, thanks for working on this!

This was referenced Dec 20, 2022
Closed
Closed
Merged
Merged
Merged
This was referenced Dec 20, 2022
Merged
Merged
Merged
Merged
Merged
@kminoda
Copy link
Contributor

kminoda commented Dec 20, 2022 •
edited
Loading

Progress:

This was referenced Dec 20, 2022
Merged
Merged
Merged
Merged
This was referenced Dec 21, 2022
Merged
Merged
@kminoda
Copy link
Contributor

kminoda commented Dec 21, 2022

The proposed modification has been merged to autoware.universe and autoware_launch.

@kminoda kminoda mentioned this issue Dec 27, 2022
Merged
8 tasks
@mitsudome-r mitsudome-r mentioned this issue Dec 27, 2022
Merged
4 tasks
@kminoda kminoda mentioned this issue Jan 6, 2023
Merged
8 tasks
@kminoda kminoda closed this as completed Jan 23, 2023
@kminoda kminoda mentioned this issue Feb 10, 2023
Open
3 tasks
badai-nguyen pushed a commit to badai-nguyen/autoware_launch that referenced this issue Oct 31, 2023
@kenji-miyake
fix(ansible/docker): replace lineinfile with copy not to write du…
55d58c3 
…plicated entries (autowarefoundation#66)

* fix(setup): remove apt ansible before installing by pip

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* refactor: change task order

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* fix(ansible/docker): replace `lineinfile` with `copy` not to write duplicated entries

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

* chore: remove paths condition to make it required for PRs

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@takayuki5168 takayuki5168

@soblin soblin

@shmpwk shmpwk

@kminoda kminoda

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@doganulus @xmfcx @takayuki5168 @soblin @kenji-miyake @shmpwk @kminoda