You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2021/01/12 07:35:06 UTC
[GitHub] [incubator-mxnet] Kh4L opened a new pull request #19742: [v1.x] Simplify TRT build by adding dependency builder script
Kh4L opened a new pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742
## Description ##
Add a script to build the dependencies required by MXNet-TRT: ONNX and onnx_tensorrt.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-768253076
@Kh4L I retriggered the job manually. cc @ChaiBapchya
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-773376737
Thank you @Kh4L. Do you intend to port this PR to the master branch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-766533096
Jenkins CI successfully triggered : [sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu merged pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#discussion_r565274032
##########
File path: CMakeLists.txt
##########
@@ -239,29 +239,23 @@ if(USE_TENSORRT)
message(STATUS "Using TensorRT")
set(ONNX_PATH 3rdparty/onnx-tensorrt/third_party/onnx/build/)
set(ONNX_TRT_PATH 3rdparty/onnx-tensorrt/build/)
+ add_definitions(-DMXNET_USE_TENSORRT=1)
+ add_definitions(-DONNX_NAMESPACE=onnx)
+ add_definitions(-DONNX_ML=1)
+ set(BUILD_SHARED_LIBS_SAVED "${BUILD_SHARED_LIBS}")
+ set(BUILD_SHARED_LIBS ON)
+ add_subdirectory(3rdparty/onnx-tensorrt/ EXCLUDE_FROM_ALL)
+ set(BUILD_SHARED_LIBS "${BUILD_SHARED_LIBS_SAVED}")
Review comment:
Are you trying to prevent `add_subdirectory` from changing the global variables? The problem is a "bug" in `3rdparty/onnx-tensorrt/` (ie. they shouldn't change the global variables in the first place). A better workaround compared to manually saving `BUILD_SHARED_LIBS` is to call `add_subdirectory` inside a function. Each function in cmake has it's own scope, and `add_subdirectory` can then only modify the variables inside the function scope.
You can look at `load_omp` function inside this file to see how it's done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-766533096
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-767199711
@mxnet-bot run ci [sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-767200399
@Kh4L sanity has some issues I think. Current on call is looking into this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19742: [v1.x] Simplify TRT build by adding dependency builder script
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#discussion_r555718298
##########
File path: CMakeLists.txt
##########
@@ -237,6 +237,7 @@ endif()
if(USE_TENSORRT)
message(STATUS "Using TensorRT")
+ execute_process(COMMAND bash tools/dependencies/build_onnx_trt.sh)
Review comment:
Could you clarify why an external script is needed here? Usually we can use cmake `add_subdirectory(path EXCLUDE_ALL)`. Both onnx and onnx-tensorrt use cmake, and we can import their targets via `add_subdirectory` and then specify them as dependency for the mxnet target? The latter may be more robust
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #19742: [v1.x] Simplify TRT build by adding dependency builder script
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#discussion_r556311666
##########
File path: CMakeLists.txt
##########
@@ -237,6 +237,7 @@ endif()
if(USE_TENSORRT)
message(STATUS "Using TensorRT")
+ execute_process(COMMAND bash tools/dependencies/build_onnx_trt.sh)
Review comment:
Not sure if it's needed and why it was done like that (the original author left NVIDIA), let me try with `add_subdirectory`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-768151038
@mxnet-bot run ci [unix-gpu-cu110]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-773376737
Thank you @Kh4L. Do you intend to port this PR to the master branch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-767200399
@Kh4L sanity has some issues I think. Current on call is looking into this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-773722351
@leezu Thank you, yes I will do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-766533073
@mxnet-bot run ci [sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19742: [v1.x] Simplify TRT build by adding dependency builder script
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-758467119
Hey @Kh4L , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [sanity, windows-cpu, centos-gpu, centos-cpu, miscellaneous, unix-gpu, unix-cpu, clang, windows-gpu, website, edge]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-768151076
None of the jobs entered are supported.
Jobs entered by user: [unix-gpu-cu110]
CI supported Jobs: [windows-cpu, clang, edge, centos-cpu, unix-cpu, windows-gpu, unix-gpu, centos-gpu, website, miscellaneous, sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-767199763
Jenkins CI successfully triggered : [sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-766739504
Jenkins CI successfully triggered : [sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-773722351
@leezu Thank you, yes I will do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-766739438
@mxnet-bot run ci [sanity]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-767213271
@Zha0q1 ok, thank you!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#discussion_r565274032
##########
File path: CMakeLists.txt
##########
@@ -239,29 +239,23 @@ if(USE_TENSORRT)
message(STATUS "Using TensorRT")
set(ONNX_PATH 3rdparty/onnx-tensorrt/third_party/onnx/build/)
set(ONNX_TRT_PATH 3rdparty/onnx-tensorrt/build/)
+ add_definitions(-DMXNET_USE_TENSORRT=1)
+ add_definitions(-DONNX_NAMESPACE=onnx)
+ add_definitions(-DONNX_ML=1)
+ set(BUILD_SHARED_LIBS_SAVED "${BUILD_SHARED_LIBS}")
+ set(BUILD_SHARED_LIBS ON)
+ add_subdirectory(3rdparty/onnx-tensorrt/ EXCLUDE_FROM_ALL)
+ set(BUILD_SHARED_LIBS "${BUILD_SHARED_LIBS_SAVED}")
Review comment:
Are you trying to prevent `add_subdirectory` from changing the global variables? The problem is a "bug" in `3rdparty/onnx-tensorrt/` (ie. they shouldn't change the global variables in the first place). A better workaround compared to manually saving `BUILD_SHARED_LIBS` is to call `add_subdirectory` inside a function. Each function in cmake has it's own scope, and `add_subdirectory` can then only modify the variables inside the function scope.
You can look at `load_omp` function inside this file to see how it's done. But if `3rdparty/onnx-tensorrt` only overwrites `BUILD_SHARED_LIBS`, it's also fine to merge this PR first and improve later
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu merged pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on pull request #19742: [v1.x] Simplify TRT build by adding onnx_tensorrt targets in CMake
Posted by GitBox <gi...@apache.org>.
Kh4L commented on pull request #19742:
URL: https://github.com/apache/incubator-mxnet/pull/19742#issuecomment-766533073
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org