-
Notifications
You must be signed in to change notification settings - Fork 660
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
feat: always separate lidar preprocessing from pointcloud_container #6091
Conversation
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
Signed-off-by: kminoda <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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?"
|
@miursh Yes, that is intended. From the performance perspective, it is better to gather as much as possible in one container even when
|
There was a problem hiding this 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 🙆♂️
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. |
@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: BTW the difference between 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Memo: Further investigation has been done |
…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>
…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>
…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]>
…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>
…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>
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 theuse_pointcloud_container
flag. Specifically,use_pointcloud_container
from LiDAR preprocessing node launcherspointcloud_container_name
andindividual_container_name
. Former one is used whenuse_pointcloud_container
is true./pointcloud_container
but rather to LiDAR container.MUST BE MERGED WITH
Note that the future work may include
use_pointcloud_container
flag: Current Autoware effectively assumes thatuse_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.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
feat: always separate lidar preprocessing from pointcloud_container autoware_launch#796
feat: always separate lidar preprocessing from pointcloud_container sample_sensor_kit_launch#85
feat: always separate lidar preprocessing from pointcloud_container tier4/aip_launcher#197
feat: always separate lidar preprocessing from pointcloud_container tier4/awsim_sensor_kit_launch#13
INTERNAL SLACK LINK FOR TIER IV
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.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.