Skip to content

TensorRT 6.0 support and PyTorch->ONNX->TRT6 unit test#26426

Closed
drnikolaev wants to merge 58 commits intopytorch:masterfrom
drnikolaev:pr_trt6
Closed

TensorRT 6.0 support and PyTorch->ONNX->TRT6 unit test#26426
drnikolaev wants to merge 58 commits intopytorch:masterfrom
drnikolaev:pr_trt6

Conversation

@drnikolaev
Copy link
Contributor

@drnikolaev drnikolaev commented Sep 18, 2019

This PR makes Caffe2 compatible with TensorRT 6. To make sure it works well, new unit test is added. This test checks PyTorch->ONNX->TRT6 inference flow for all classification models from TorhchVision Zoo.
Note on CMake changes: it has to be done in order to import onnx-tensorrt project. See #18524 for details.

@pytorchbot pytorchbot added caffe2 module: build Build system issues module: onnx Related to torch.onnx module: pybind Related to our Python bindings / interactions with other Python libraries module: third_party labels Sep 18, 2019
@pytorchbot pytorchbot added the module: ci Related to continuous integration label Sep 18, 2019
@soumith soumith requested a review from houseroad September 20, 2019 05:26
facebook-github-bot
facebook-github-bot reviewed Sep 20, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad
houseroad reviewed Sep 20, 2019
View reviewed changes
houseroad
houseroad reviewed Sep 20, 2019
View reviewed changes
houseroad
houseroad reviewed Sep 20, 2019
View reviewed changes
houseroad
houseroad reviewed Sep 20, 2019
View reviewed changes
houseroad
houseroad reviewed Sep 20, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Overall, looks pretty good. Please address the inline comments. Thanks!

houseroad
houseroad reviewed Oct 4, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

@drnikolaev could you rebase to the master?

I have introduced a CI for you already. :-)

@drnikolaev
Copy link
Contributor Author

drnikolaev commented Oct 4, 2019

@houseroad thank you! This is the first thing I'll do after vacation. :)

soumith pushed a commit that referenced this pull request Oct 4, 2019
@houseroad @soumith
Turn Caffe2 CUDA 9.1 + py2 to CUDA 10.1 + py3 (#26835)
667deb9 
Summary:
For TensorRT test introduced in #26426
Pull Request resolved: #26835

Reviewed By: hl475

Differential Revision: D17580108

Pulled By: houseroad

fbshipit-source-id: c57fafec228b78c26b8a7946c92ad7434425bbd4
facebook-github-bot
facebook-github-bot reviewed Oct 22, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@drnikolaev
Copy link
Contributor Author

drnikolaev commented Oct 24, 2019

@houseroad please check it out

facebook-github-bot
facebook-github-bot reviewed Oct 24, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad
houseroad reviewed Oct 25, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Overall, looks legit, some inline comments, and also could you rebase to master?

facebook-github-bot
facebook-github-bot reviewed Oct 25, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad
houseroad approved these changes Oct 25, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

LGTM

@bddppq
Copy link
Contributor

bddppq commented Oct 25, 2019

This has broken master CI:

https://circleci.com/gh/pytorch/pytorch/3360955?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link/console

Oct 25 20:09:21 -- Found TENSORRT: /usr/include/x86_64-linux-gnu  
Oct 25 20:09:21 awk: cannot open /usr/include/x86_64-linux-gnu/NvInferVersion.h (No such file or directory)
Oct 25 20:09:21 awk: cannot open /usr/include/x86_64-linux-gnu/NvInferVersion.h (No such file or directory)
Oct 25 20:09:21 CMake Error at cmake/public/cuda.cmake:129 (string):
Oct 25 20:09:21   string sub-command STRIP requires two arguments.
Oct 25 20:09:21 Call Stack (most recent call first):
Oct 25 20:09:21   cmake/Dependencies.cmake:896 (include)
Oct 25 20:09:21   CMakeLists.txt:377 (include)
Oct 25 20:09:21 
Oct 25 20:09:21 
Oct 25 20:09:21 CMake Error at cmake/public/cuda.cmake:130 (string):
Oct 25 20:09:21   string sub-command STRIP requires two arguments.
Oct 25 20:09:21 Call Stack (most recent call first):
Oct 25 20:09:21   cmake/Dependencies.cmake:896 (include)
Oct 25 20:09:21   CMakeLists.txt:377 (include)
Oct 25 20:09:21

@drnikolaev
Copy link
Contributor Author

drnikolaev commented Oct 25, 2019

Could you try
[ -r "/usr/include/x86_64-linux-gnu/NvInferVersion.h"] && awk ...

zdevito pushed a commit to zdevito/ATen that referenced this pull request Oct 25, 2019
@drnikolaev @facebook-github-bot
TensorRT 6.0 support and PyTorch->ONNX->TRT6 unit test (#26426)
2c6dc51 
Summary:
This PR makes Caffe2 compatible with TensorRT 6. To make sure it works well, new unit test is added. This test checks PyTorch->ONNX->TRT6 inference flow for all classification models from TorhchVision Zoo.
Note on CMake changes: it has to be done in order to import onnx-tensorrt project. See pytorch/pytorch#18524 for details.
Pull Request resolved: pytorch/pytorch#26426

Reviewed By: hl475

Differential Revision: D17495965

Pulled By: houseroad

fbshipit-source-id: 3e8dbe8943f5a28a51368fd5686c8d6e86e7f693
@houseroad
Copy link
Member

houseroad commented Oct 25, 2019

The PR was reverted, please resubmit the PR and fix the issue.

@drnikolaev drnikolaev mentioned this pull request Oct 25, 2019
Closed
@drnikolaev
Copy link
Contributor Author

drnikolaev commented Oct 25, 2019

done. see #28715

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 26, 2019

@houseroad merged this pull request in 4996e3a.

facebook-github-bot pushed a commit that referenced this pull request Nov 1, 2019
@drnikolaev @facebook-github-bot
#26426 fixed (#28715)
1e2049c 
Summary:
This is the fix for reverted #26426
houseroad bddppq soumith
Pull Request resolved: #28715

Reviewed By: hl475

Differential Revision: D18146731

Pulled By: houseroad

fbshipit-source-id: 247366451a6334e84df82d00339521f797b33130
zdevito pushed a commit to zdevito/ATen that referenced this pull request Nov 1, 2019
@drnikolaev @facebook-github-bot
#26426 fixed (#28715)
eac3b9b 
Summary:
This is the fix for reverted pytorch/pytorch#26426
houseroad bddppq soumith
Pull Request resolved: pytorch/pytorch#28715

Reviewed By: hl475

Differential Revision: D18146731

Pulled By: houseroad

fbshipit-source-id: 247366451a6334e84df82d00339521f797b33130
pdlive215 pushed a commit to pdlive215/pytorch that referenced this pull request Nov 27, 2019
@houseroad @pdlive215
Turn Caffe2 CUDA 9.1 + py2 to CUDA 10.1 + py3 (pytorch#26835)
86a9b26 
Summary:
For TensorRT test introduced in pytorch#26426
Pull Request resolved: pytorch#26835

Reviewed By: hl475

Differential Revision: D17580108

Pulled By: houseroad

fbshipit-source-id: c57fafec228b78c26b8a7946c92ad7434425bbd4
thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
@drnikolaev
TensorRT 6.0 support and PyTorch->ONNX->TRT6 unit test (pytorch#26426)
8cc9788 
Summary:
This PR makes Caffe2 compatible with TensorRT 6. To make sure it works well, new unit test is added. This test checks PyTorch->ONNX->TRT6 inference flow for all classification models from TorhchVision Zoo.
Note on CMake changes: it has to be done in order to import onnx-tensorrt project. See pytorch#18524 for details.
Pull Request resolved: pytorch#26426

Reviewed By: hl475

Differential Revision: D17495965

Pulled By: houseroad

fbshipit-source-id: 3e8dbe8943f5a28a51368fd5686c8d6e86e7f693
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@houseroad houseroad houseroad approved these changes

@lara-hdr lara-hdr Awaiting requested review from lara-hdr

@spandantiwari spandantiwari Awaiting requested review from spandantiwari

@BowenBao BowenBao Awaiting requested review from BowenBao

+1 more reviewer

@csarofeen csarofeen csarofeen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

caffe2 Merged module: build Build system issues module: ci Related to continuous integration module: onnx Related to torch.onnx module: pybind Related to our Python bindings / interactions with other Python libraries module: third_party open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@drnikolaev @soumith @houseroad @yf225 @ezyang @bddppq @facebook-github-bot @csarofeen @pytorchbot @mruberry