-
Notifications
You must be signed in to change notification settings - Fork 652
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(autoware_tensorrt_rtmdet): add tensorrt rtmdet model #8165
base: main
Are you sure you want to change the base?
feat(autoware_tensorrt_rtmdet): add tensorrt rtmdet model #8165
Conversation
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
2fb9f5c
to
d1149a4
Compare
132ff68
to
cf09221
Compare
Hi, @mitsudome-r This PR looks ready to review. |
perception/autoware_tensorrt_rtmdet/include/tensorrt_rtmdet/calibrator.hpp
Outdated
Show resolved
Hide resolved
perception/autoware_tensorrt_rtmdet/include/tensorrt_rtmdet/calibrator.hpp
Outdated
Show resolved
Hide resolved
perception/autoware_tensorrt_rtmdet/src/trt_batched_nms/batched_nms/trt_batched_nms.cpp
Outdated
Show resolved
Hide resolved
perception/autoware_tensorrt_rtmdet/include/tensorrt_rtmdet/tensorrt_rtmdet.hpp
Outdated
Show resolved
Hide resolved
Thank you for your PR. I am considering whether tensorrt_batched_nms should be a standalone package or add to the tensorrt_common. Any ideas? |
Hi @Owen-Liuyuxuan, thanks for your thoughts, I am not sure if it would be beneficial to use it as a separate package. When we organize it as a separate package, I would expect it to be used by multiple packages or models. Normally, the batchedNMSPlugin is a plugin published within the TensorRT library, and the version we are using has been modified by mmdeploy. Therefore, I don't think it will be used by another model or package in the future. If this plugin is used, it will most likely be in its original form and I guess we can directly reach the original form from TensorRT headers. Apart from that, if we want to gather all currently used and future plugins within |
@StepTurtle |
perception/autoware_tensorrt_rtmdet/src/trt_batched_nms/common_impl/nms/sortScoresPerImage.cu
Outdated
Show resolved
Hide resolved
perception/autoware_tensorrt_rtmdet/src/tensorrt_rtmdet_node.cpp
Outdated
Show resolved
Hide resolved
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.
@StepTurtle
As an honest opinion from a reviewer, reviewing a PR with more than 5000 lines is quite challenging.
May I suggest one of the following actions? 🙏
- Add unit tests and ensure that they pass in CI.
- The reviewer will then verify whether the tests are appropriate.
- Split the PR into smaller parts.
cc: @mitsudome-r @kminoda @mojomex
Please give your comments.
@StepTurtle
This is best handled in the launch stage. ROS 2 provides substitutions like Loading plugins should also definitely be covered by unit tests to ensure they work reliably during runtime. Thank you for your consideration. |
f2fd0db
to
d86d2c3
Compare
Hey, @mojomex @Shin-kyoto thanks for reviews. At first, I thought we wouldn’t focus much on the plugin since it was developed by others, but it's nice that you reviewed the plugin code. If we want to add the plugin as a separate package, should we put it under I am working on the other topics you mentioned like NOTE: Since there are a lot of TO DO, I switched the PR status as draft |
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
Hi again @mojomex @Shin-kyoto, I’ve updated the PR and made it ready for review.
Thanks! |
Signed-off-by: Barış Zeren <[email protected]>
@StepTurtle Thank you for the great work! |
Signed-off-by: Barış Zeren <[email protected]>
@Shin-kyoto, I just realized I forgot to mention something in this comment. I also removed the related commit: ccbd818 |
Hey @Shin-kyoto, If we decide on the way to import the Also we should add |
@StepTurtle Hi, thank you for your work (and also to all the reviewers). Please check, thanks! |
Signed-off-by: Barış Zeren <[email protected]>
Closing the discussion and PRs as |
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.
Thank you for your contribution! I just did general review, more comments regarding source code will come soon. I can confirm it works, but I wonder if NMS works as expected (please, check attached image). What do you think about it? Also played with parameters and I didn't get expected results.
EDIT:
Missing words for internal dictionary you can add via workflow. Source code itself does not require major changes and can be merged to autoware.universe. However, to improve code quality and save your time, please consider use clang tooling - there are multiple unused variables and headers which can be highlighted by automated tools.
ament_lint_auto_find_test_dependencies() | ||
|
||
ament_add_ros_isolated_gtest(test_rtmdet test/test_rtmdet.cpp) | ||
set_tests_properties(test_rtmdet PROPERTIES TIMEOUT 300) # It could take a long time on the first run to create the engine |
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.
[Review placeholder]
AFAIK, CI is limited to 60 s for each unit test, we need to check how it behaves at the end.
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.
Changed as 60 sec, but I cannot complete model build process in 60 sec.
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
Signed-off-by: Barış Zeren <[email protected]>
…tle/autoware.universe into feat/add_tensorrt_rtmdet
Signed-off-by: Barış Zeren <[email protected]>
Thanks @amadeuszsz for your review.
I never seen such problem. Can you share your bag file, so I can reproduce and try to fix it?
Here are the remaining words which creates spelling error:
I could not able to create a PR with workflow. I could not see the run workflow button, I think it because I am not a member for TIER IV repositories. |
Description
This PR adds a instance segmentation method RTMDet to perception pipeline.
Test Video: https://youtu.be/dBJdZtc4BC8
Process Times
1.61101 ms
0.470295 ms
1.71506 ms
0.757723 ms
13.4203 ms
0.628055 ms
23.4338 ms
6.98011 ms
Following table shows the total time for
preprocess
,inference
andpostprocess
processes. It don't containvisualization
15.791 ms
1.58901 ms
Process Times for 8 camera on single GPU
43.5821 ms
17.0037 ms
16.2328 ms
102.521 ms
42.7286 ms
15.8802 ms
22.3733 ms
98.7796 ms
41.4004 ms
15.0862 ms
15.0169 ms
87.0995 ms
42.3738 ms
15.7069 ms
21.1178 ms
105.505 ms
36.4766 ms
13.2308 ms
20.3997 ms
81.181 ms
42.2531 ms
16.1687 ms
16.265 ms
91.1761 ms
35.9258 ms
13.5001 ms
19.7352 ms
76.6217 ms
36.4776 ms
15.4815 ms
21.2165 ms
80.9634 ms
Computer Specifications
Related links
Parent Issue:
PR for message type which is used in this PR:
Repository for Plugin:
How was this PR tested?
Testers can follow these steps to test PR.
1) Download the pre-trained model:
2) For
autoware_internal_msgs
use following PR:3) For
trt_batched_nms
use https://github.com/autowarefoundation/trt_batched_nms:universe/extarnal
4) Build the package and other dependencies of package
cd autoware colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release --packages-up-to tensorrt_rtmdet
5) Update the topic parameters from
launch/rtmdet.launch.xml
and run the launch fileNotes for reviewers
🟨 Plugin Loading
Autoware has the same logic that I used to load the TensorRT plugin.
After compilation, a file with the extension '.so' is created. This file stored in
build
and it should be parameter ofdlopen()
function.Is there any information about can we handle this in Cmake. If we cannot, how can I provide the path to the file located inside the 'build' folder?
I was able to load the plugin using the file paths below:
./build/tensorrt_rtmdet/libtensorrt_rtmdet_plugin.so
(relative path from workspace)/home/user/projects/workspace/build/tensorrt_rtmdet/libtensorrt_rtmdet_plugin.so
(absolute path)🟨
TRTBatchedNMS
CodebaseThe RTMDet model uses the
TRTBatchedNMS
plugin (modified version of originalTRTBatchedNMS
by TensorRT) and I put all code base of plugin intosrc/trt_batched_nms
andinclude/trt_batched_nms
but I am not is it suitable or not?🟨
int8
Precision OptionThere are three precision option (
fp16
,fp32
andint8
) and one of them was not work. In the current situation, it is working, but the result is not entirely correct. Watch video to see problem:🟨 Message Type
Interface changes
None.
Effects on system behavior
None.