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.

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

feat(include_launch_description): only check required arguments in the local launch file #746

Open
wants to merge 9 commits into
base: rolling
Choose a base branch
from

Conversation

Owen-Liuyuxuan
Copy link

@Owen-Liuyuxuan Owen-Liuyuxuan commented Dec 4, 2023

Description

In a feature request issue on #745. It suggests that the argument checking mechanism in include_launch_description.py introduces unnecessary overhead for launching large systems.

In this PR, a mechanism is implemented so that the best-effort argument checking is only performed inside the local launch file instance and checks on each included sub-launch file are only performed once.

Methods

Add an argument to the get_launch_arguments_with_include_launch_description_actions function.

Testing

Tested with autoware. If the launch file is OK, we could now launch significantly faster. If the launch file has un-declared arguments even if it is hidden in the sub-sub included files, the launch system can identify the problem and stop the launch operation cleanly (it also keeps up with the cases in CI).

Feedback is welcome.

Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
@Owen-Liuyuxuan
Copy link
Author

@Owen-Liuyuxuan Could you check the failing CI?
https://build.ros2.org/job/Rpr__launch__ubuntu_jammy_amd64/311/testReport/launch.test.launch.actions/test_include_launch_description/test_include_launch_description_launch_arguments/

Working on methods to pass this case while keeping the complexity manageable.

Owen-Liuyuxuan added 5 commits December 12, 2023 13:13
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp>
@Owen-Liuyuxuan Owen-Liuyuxuan changed the title feat(include_launch_description): only check required arguments with the first inclusion feat(include_launch_description): only check required arguments in the local launch file Dec 12, 2023
@audrow
Copy link
Member

audrow commented Dec 12, 2023

For more context, how significant of a speedup are you seeing by only checking the required args?

@Owen-Liuyuxuan
Copy link
Author

For more context, how significant of a speedup are you seeing by only checking the required args?

This image showcases how it accelerates the logging simulation of the OSC [autoware] (https://github.com/autowarefoundation/autoware). Numbers are time spent in the execute function of include_launch_description.py in seconds.
image

To reproduce:

  1. Build Autoware.
  2. Manually update the launch file and log the time info.
  3. Analyse the launch.log and find the most time-consuming parts.

Intuition:
Autoware has become a rather huge system with many optional launchable sub-systems. The original code will unfold more than 100k parameters at checking the first launch script (and these parameters will be checked recursively many times by tons of subfiles/sub-subfiles). This update can significantly improve launch speed for huge launch system.

@Owen-Liuyuxuan
Copy link
Author

Owen-Liuyuxuan commented Mar 29, 2024

Any updates on the review process? Thanks a lot for the maintenance of the code.

The new commits from main for new the ROS version breaks the CI again. Need to figure out what is new.

@Owen-Liuyuxuan
Copy link
Author

Owen-Liuyuxuan commented Apr 2, 2024

Merge commit does not produce any new reports and the CI fails. I will have to push some dummy commit here.

by April 2. The CI errors does not look like to be related to the PR. Waiting for updates.

Would love to hear more information about the CI here, it does not look like related to the PR .

Sorry for the mention @mjcarroll @clalancette .

@Owen-Liuyuxuan
Copy link
Author

Added a branch on my personal repo
https://github.com/Owen-Liuyuxuan/launch/tree/feat/augment_check_skipping_install
for temporary installation.

git clone https://github.com/Owen-Liuyuxuan/launch.git
git checkout feat/augment_check_skipping_install
./install2ros.sh

@Owen-Liuyuxuan
Copy link
Author

Owen-Liuyuxuan commented Oct 1, 2024

@mjcarroll
I barely updated the branch and I got all CIs passed.

In our internal system, we have been installing the accelerated version instead when we conduct the system setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants