-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-703] Update to TensorRT 5, ONNX IR 3. Fix inference bugs. #13310
Conversation
3a9b7e7
to
f663c47
Compare
e410dd8
to
f6133bf
Compare
06a47fa
to
6150f4a
Compare
@KellenSunderland please take a look at the failed CI build @mxnet-label-bot add [pr-work-in-progress] |
@mxnet-label-bot add [pr-work-in-progress] |
22f738f
to
2260f59
Compare
Note: will rebase this PR before merging. |
2260f59
to
5558ab4
Compare
// More information on ONNX versions and opsets can be found at: | ||
// https://github.com/onnx/onnx/blob/master/docs/IR.md | ||
|
||
auto opset_proto = model_proto.add_opset_import(); |
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.
is copy ok?
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.
Yeah I believe so, proto editing in C++ is a little strange but I've seen this pattern in several places. I've run model validation and it certainly failed for me right away if I did not have a properly set opset proto associated with my model proto for onnx v3 models.
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 % comments
Thanks for the review @larroy! Will update. @marcoabreu do you feel your comments were addressed sufficiently? Anything you'd still like to see changed? |
Lets wait until the Jenkins refactor has been finalized, ok? |
I have two minor comments:
|
@marcoabreu Is jenking refactor finalized now? @KellenSunderland Can you rebase this PR? |
@mxnet-label-bot Update [pr-awaiting-response] |
This works around a CUDA 10 cmake issue documented here: clab/dynet#1457 This fix is temporary; once an updated cmake package is published to Ubuntu's package repo it may be reverted.
b2cf02f
to
a264afc
Compare
Rebased (sorry took a while), I want to do one more pass addressing Pedro, Marco and Nathalie's comments. |
a1030a2
to
d4d9ba3
Compare
d4d9ba3
to
600bc1f
Compare
@NRauschmayr Totally agree with your comments here. Would it be ok if we make those changes in a follow up PR? |
@KellenSunderland Sure, that's ok! There is no hurry for the changes which I suggested. |
…che#13310) * [MXNET-703] Install CUDA 10 compatible cmake This works around a CUDA 10 cmake issue documented here: clab/dynet#1457 This fix is temporary; once an updated cmake package is published to Ubuntu's package repo it may be reverted. * [MXNET-703] Update to TensorRT 5 ONNX IR 3. Fix inference bugs. * [MXNET-703] Describe onnx opsets and major version
…che#13310) * [MXNET-703] Install CUDA 10 compatible cmake This works around a CUDA 10 cmake issue documented here: clab/dynet#1457 This fix is temporary; once an updated cmake package is published to Ubuntu's package repo it may be reverted. * [MXNET-703] Update to TensorRT 5 ONNX IR 3. Fix inference bugs. * [MXNET-703] Describe onnx opsets and major version
Description
This PR updates the IR which is passed to TensorRT to use version3 of the spec, which aligns much better with MXNet defaults and results in a decrease in boilerplate code. This update also fixes some bugs when building inference engines that were resulting in feature vectors that were very different from what they should have been.
Fixes #12598
Fixes #13113
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Marco:
Resolves #13459