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: always separate lidar preprocessing from pointcloud_container #6091

Merged

Conversation

kminoda
Copy link
Contributor

@kminoda kminoda commented Jan 16, 2024

Description

Current Autoware has use_pointcloud_container flag, which is to define whether to gather necessary nodes into one composable node, namely /pointcloud_container. Currently, the flag is also applied to LiDAR preprocessing nodes, gathering all of those from multiple LiDARs into one composable node, which makes Autoware vulnerable to process failure in those nodes.

In order to improve redundancy towards the node failure in LiDAR preprocessing, we would like to separate LiDAR preprocessing nodes from /pointcloud_container regardless of the use_pointcloud_container flag. Specifically,

  • Changes to sensing: Remove use_pointcloud_container from LiDAR preprocessing node launchers
  • Changes to perception: Clarify the difference between pointcloud_container_name and individual_container_name. Former one is used when use_pointcloud_container is true.
  • Changes to localization: We need some modification in localization nodes as well since it will no longer be attached to /pointcloud_container but rather to LiDAR container.

image

MUST BE MERGED WITH

Note that the future work may include

  • Removing use_pointcloud_container flag: Current Autoware effectively assumes that use_pointcloud_container is true due to the performance capability. It is reasonable to remove the flag at least for now to improve the readability of launch files.
  • Reorganizing glog: With this modification, glog_component will no longer be applied to LiDAR preprocessing nodes. I will create another PR to organize glog so that it will be enabled for those nodes as well.

Related links

Tests performed

Performance evaluation (For XX1)

Since this PR will separate concatenation node from LiDAR preprocessing nodes, I have tested the PR with JPN TAXI to see if the performance regression is negligible or not.
The results are written here: TIER IV INTERNAL LINK. We observed a slight delay in the deriving topics from concatenated/pointcloud, which is around ~10ms in average.

Logging simulator w/ awf/autoware (sample_sensor_kit)

use_pointcloud_container = true

use_pointcloud_container = false

Logging simulator w/ tier4/pilot-auto (aip_xx1)

use_pointcloud_container = true

use_pointcloud_container = false

Notes for reviewers

None

Interface changes

This PR includes name change in composable node.
No topics nor node names are changed.

Effects on system behavior

Increase in CPU usage is expected since it will increase the inter-process communication (mostly in a single computer). However, we estimate this risk small enough given the above-mentioned delay estimation results.

Pre-review checklist for the PR author

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

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.

@github-actions github-actions bot added component:perception Advanced sensor data processing and environment understanding. (auto-assigned) component:launch Launch files, scripts and initialization tools. (auto-assigned) labels Jan 16, 2024
@github-actions github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label Jan 16, 2024
@kminoda kminoda added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jan 16, 2024
@kminoda kminoda marked this pull request as ready for review January 16, 2024 13:58
Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a3f90f0) 14.63% compared to head (7bc683b) 14.63%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6091   +/-   ##
=======================================
  Coverage   14.63%   14.63%           
=======================================
  Files        1858     1858           
  Lines      126848   126848           
  Branches    37173    37173           
=======================================
  Hits        18570    18570           
  Misses      87390    87390           
  Partials    20888    20888           
Flag Coverage Δ *Carryforward flag
differential 27.56% <ø> (?)
total 14.63% <ø> (ø) Carriedforward from a3f90f0

*This pull request uses carry forward flags. Click here to find out more.

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

@miursh
Copy link
Contributor

miursh commented Jan 17, 2024

When use_pointcloud_container is set to false, each node (ground_segmentation, concatenate, compare_map_filter, ) is placed in a separate container. Is this intended?"

$ ros2 component list
/perception/occupancy_grid_map/occupancy_grid_map_container
  1  /perception/occupancy_grid_map/occupancy_grid_map_node
/sensing/lidar/rear/pointcloud_preprocessor/pointcloud_container
  1  /sensing/lidar/rear/velodyne_driver_ros_wrapper_node
  2  /sensing/lidar/rear/crop_box_filter_self
  3  /sensing/lidar/rear/crop_box_filter_mirror
  4  /sensing/lidar/rear/distortion_corrector_node
  5  /sensing/lidar/rear/ring_outlier_filter
/perception/obstacle_segmentation/ground_segmentation_container
  1  /perception/obstacle_segmentation/crop_box_filter
  2  /perception/obstacle_segmentation/common_ground_filter
  3  /perception/obstacle_segmentation/occupancy_grid_map_outlier_filter
/sensing/lidar/top/pointcloud_preprocessor/pointcloud_container
  1  /localization/util/crop_box_filter_measurement_range
  2  /sensing/lidar/top/velodyne_driver_ros_wrapper_node
  3  /localization/util/voxel_grid_downsample_filter
  4  /sensing/lidar/top/crop_box_filter_self
  5  /localization/util/random_downsample_filter
  6  /sensing/lidar/top/crop_box_filter_mirror
  7  /sensing/lidar/top/distortion_corrector_node
  8  /sensing/lidar/top/ring_outlier_filter
/perception/object_recognition/detection/pointcloud_map_filter_container
  1  /perception/object_recognition/detection/voxel_grid_downsample_filter
  2  /perception/object_recognition/detection/voxel_based_compare_map_filter
/perception/object_recognition/detection/clustering/euclidean_cluster_container
  1  /perception/object_recognition/detection/clustering/low_height_crop_box_filter
  2  /perception/object_recognition/detection/clustering/euclidean_cluster
/map/map_container
  1  /map/lanelet2_map_loader
  2  /map/lanelet2_map_visualization
  3  /map/pointcloud_map_loader
  4  /map/vector_map_tf_generator
/sensing/lidar/concatenate_container
  1  /sensing/lidar/concatenate_data

@kminoda
Copy link
Contributor Author

kminoda commented Jan 17, 2024

@miursh Yes, that is intended. From the performance perspective, it is better to gather as much as possible in one container even when use_pointcloud_container is false, but I didn't made that modification from the following reasons:

  • Doing the modification would make this PR larger (e.g. launching /perception_container when use_pointcloud_container=false)
  • use_pointcloud_container will be true in most of the cases, and thus the above implementation would make little difference for Autoware users

Copy link
Contributor

@KYabuuchi KYabuuchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tier4_localization_launch looks good to me 🙆‍♂️

@mebasoglu
Copy link
Contributor

Hello, could you share some metrics about this? How much performance do we lose with separating nodes? Maybe this PR could be merged after #6056 so that we can see delays.

@kminoda
Copy link
Contributor Author

kminoda commented Jan 18, 2024

@mebasoglu Thank you for the comment! We indeed did a performance evaluation on our real vehicle, mostly with the same approach in the above PR (we've also got an advice from @.xmfcx about the accumulated time based evaluation last week). It's in description of this PR. I will just copy-and-paste it here as well for your convenience.

In short, we observed a negligible increase in topic delay, possibly because this PR only separate one single node (=concatenation node), so we estimate the degradation due to this PR fairly limited. We measured it in two approaches: ros2 topic delay and the accumulated time based approach (as in #6056). So I believe we can just merge this PR without #6056, although this is not a hard requirement and thus don't mind waiting for #6056 if you still see a concern in this PR.

BTW the difference between ros2 topic delay approach and the accumulated time based approach was very small in our case.


Performance evaluation (For XX1)

Since this PR will separate concatenation node from LiDAR preprocessing nodes, I have tested the PR with JPN TAXI to see if the performance regression is negligible or not.
The results are written here: TIER IV INTERNAL LINK. We observed a slight delay in the deriving topics from concatenated/pointcloud, which is around ~10ms in average.

Copy link
Contributor

@miursh miursh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kminoda
Copy link
Contributor Author

kminoda commented Jan 30, 2024

Memo: Further investigation has been done
INTERNAL LINK FOR JIRA TICKET

kminoda added a commit to tier4/autoware.universe that referenced this pull request Feb 2, 2024
…utowarefoundation#6091)

* feat!: replace use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* feat: remove from planning

Signed-off-by: kminoda <[email protected]>

* fix: fix to remove all use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* revert: revert change in planning.launch

Signed-off-by: kminoda <[email protected]>

* revert: revert rename of use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* fix: fix tier4_perception_launch to enable use_pointcloud_contaienr

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* refactor: remove trailing whitespace

Signed-off-by: kminoda <[email protected]>

* revert other changes in perception

Signed-off-by: kminoda <[email protected]>

* revert change in readme

Signed-off-by: kminoda <[email protected]>

* feat: move glog to pointcloud_container.launch.py

* revert: revert glog porting

Signed-off-by: kminoda <[email protected]>

* style(pre-commit): autofix

* fix: fix pre-commit

Signed-off-by: kminoda <[email protected]>

---------

Signed-off-by: kminoda <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 26, 2024
…utowarefoundation#6091)

* feat!: replace use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* feat: remove from planning

Signed-off-by: kminoda <[email protected]>

* fix: fix to remove all use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* revert: revert change in planning.launch

Signed-off-by: kminoda <[email protected]>

* revert: revert rename of use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* fix: fix tier4_perception_launch to enable use_pointcloud_contaienr

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* refactor: remove trailing whitespace

Signed-off-by: kminoda <[email protected]>

* revert other changes in perception

Signed-off-by: kminoda <[email protected]>

* revert change in readme

Signed-off-by: kminoda <[email protected]>

* feat: move glog to pointcloud_container.launch.py

* revert: revert glog porting

Signed-off-by: kminoda <[email protected]>

* style(pre-commit): autofix

* fix: fix pre-commit

Signed-off-by: kminoda <[email protected]>

---------

Signed-off-by: kminoda <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 28, 2024
…utowarefoundation#6091)

* feat!: replace use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* feat: remove from planning

Signed-off-by: kminoda <[email protected]>

* fix: fix to remove all use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* revert: revert change in planning.launch

Signed-off-by: kminoda <[email protected]>

* revert: revert rename of use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* fix: fix tier4_perception_launch to enable use_pointcloud_contaienr

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* refactor: remove trailing whitespace

Signed-off-by: kminoda <[email protected]>

* revert other changes in perception

Signed-off-by: kminoda <[email protected]>

* revert change in readme

Signed-off-by: kminoda <[email protected]>

* feat: move glog to pointcloud_container.launch.py

* revert: revert glog porting

Signed-off-by: kminoda <[email protected]>

* style(pre-commit): autofix

* fix: fix pre-commit

Signed-off-by: kminoda <[email protected]>

---------

Signed-off-by: kminoda <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: karishma <[email protected]>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request May 28, 2024
…utowarefoundation#6091)

* feat!: replace use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* feat: remove from planning

Signed-off-by: kminoda <[email protected]>

* fix: fix to remove all use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* revert: revert change in planning.launch

Signed-off-by: kminoda <[email protected]>

* revert: revert rename of use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* fix: fix tier4_perception_launch to enable use_pointcloud_contaienr

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* refactor: remove trailing whitespace

Signed-off-by: kminoda <[email protected]>

* revert other changes in perception

Signed-off-by: kminoda <[email protected]>

* revert change in readme

Signed-off-by: kminoda <[email protected]>

* feat: move glog to pointcloud_container.launch.py

* revert: revert glog porting

Signed-off-by: kminoda <[email protected]>

* style(pre-commit): autofix

* fix: fix pre-commit

Signed-off-by: kminoda <[email protected]>

---------

Signed-off-by: kminoda <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
…utowarefoundation#6091)

* feat!: replace use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* feat: remove from planning

Signed-off-by: kminoda <[email protected]>

* fix: fix to remove all use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* revert: revert change in planning.launch

Signed-off-by: kminoda <[email protected]>

* revert: revert rename of use_pointcloud_container

Signed-off-by: kminoda <[email protected]>

* fix: fix tier4_perception_launch to enable use_pointcloud_contaienr

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* fix: fix unnecessary change

Signed-off-by: kminoda <[email protected]>

* refactor: remove trailing whitespace

Signed-off-by: kminoda <[email protected]>

* revert other changes in perception

Signed-off-by: kminoda <[email protected]>

* revert change in readme

Signed-off-by: kminoda <[email protected]>

* feat: move glog to pointcloud_container.launch.py

* revert: revert glog porting

Signed-off-by: kminoda <[email protected]>

* style(pre-commit): autofix

* fix: fix pre-commit

Signed-off-by: kminoda <[email protected]>

---------

Signed-off-by: kminoda <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:launch Launch files, scripts and initialization tools. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants