Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

enable TensorRT integration with cpp api#15335

Merged
KellenSunderland merged 1 commit intoapache:masterfrom
haohuanw:master
Jul 4, 2019
Merged

enable TensorRT integration with cpp api#15335
KellenSunderland merged 1 commit intoapache:masterfrom
haohuanw:master

Conversation

@haohuanw
Copy link
Contributor

@haohuanw haohuanw commented Jun 23, 2019

Description

This PR enables using TensorRT through cpp api

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • [ x ] Changes are complete (i.e. I finished coding on this PR)
  • [ x ] All changes have test coverage:
  • [ x ] Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
    • I have updated the inference test but I don't think this is part of the ci task since I am not sure what is the easiest way to add this test in ci.
  • [ x ] Code is well-documented:
  • [ x ]For new C++ functions in header files, their functionalities and arguments are documented.
  • [ x ]For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • [ x ] To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • [ X ] inception_inference.cpp : update example for bind with TRT
./inception_inference --symbol "./model/Inception-BN-symbol.json" --params "./model/Inception-BN-0126.params" --synset "./model/synset.txt" --mean "./model/mean_224.nd" --image "./model/dog.jpg" --enableTRT
...
[17:53:45] /opt/workspace/incubator-mxnet/cpp-package/example/inception_inference.cpp:377: The model predicts the input image to be a [ pug, pug-dog ] with Accuracy = 0.987554

root@0f0b3e022627:/opt/workspace/incubator-mxnet/build/private/cmake/cpp-package/example# LD_LIBRARY_PATH=/opt/workspace/incubator-mxnet/build/lib/:$LD_LIBRARY_PATH ./inception_inference --symbol "./model/Inception-BN-symbol.json" --params "./model/Inception-BN-0126.params" --synset "./model/synset.txt" --mean "./model/mean_224.nd" --image "./model/dog.jpg"            
...
[17:53:52] /opt/workspace/incubator-mxnet/cpp-package/example/inception_inference.cpp:377: The model predicts the input image to be a [ pug, pug-dog ] with Accuracy = 0.986955

root@0f0b3e022627:/opt/workspace/incubator-mxnet/build/private/cmake/cpp-package/example# LD_LIBRARY_PATH=/opt/workspace/incubator-mxnet/build/lib/:$LD_LIBRARY_PATH ./inception_inference --symbol "./model/Inception-BN-symbol.json" --params "./model/Inception-BN-0126.params" --synset "./model/synset.txt" --mean "./model/mean_224.nd" --image "./model/dog.jpg" --gpu
...
[17:54:03] /opt/workspace/incubator-mxnet/cpp-package/example/inception_inference.cpp:377: The model predicts the input image to be a [ pug, pug-dog ] with Accuracy = 0.986955
  • [ x ] symbol.hpp: added some api, these are tested when running inception_inference
  • [ x ] contrib.h: new file, added functions for binding TRT, these are tested when running inception_inference

Comments

  • If this change is a backward incompatible change, why must this change be made.
    • This change is for enabling ability to use TensorRT integration through C++ API

@haohuanw haohuanw requested a review from nswamy as a code owner June 23, 2019 18:32
@haohuanw
Copy link
Contributor Author

haohuanw commented Jun 23, 2019

@KellenSunderland @Caenorst

@Roshrini Roshrini added the pr-awaiting-review PR is waiting for code review label Jun 23, 2019
@haohuanw
Copy link
Contributor Author

haohuanw commented Jun 28, 2019

ping! anyone could do a quick review?

Caenorst
Caenorst reviewed Jun 30, 2019
View reviewed changes
cpp-package/include/mxnet-cpp/contrib.h
static const std::string TENSORRT_SUBGRAPH_PARAM_IDENTIFIER = "subgraph_params_names";
// needs to be same with
// https://github.com/apache/incubator-mxnet/blob/master/src/operator/subgraph/tensorrt/tensorrt.cc#L244
static const std::string TENSORRT_SUBGRAPH_PARAM_PREFIX = "subgraph_param_";
Copy link
Contributor

@Caenorst Caenorst Jun 30, 2019

Choose a reason for hiding this comment

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

We should probably add those constants in src/operator/subgraph/tensorrt/tensorrt-inl.h to keep track

Copy link
Contributor Author

@haohuanw haohuanw Jul 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You mean duplicate them in the tensorrt-inl.h as well?

Copy link
Contributor

@Caenorst Caenorst Jul 2, 2019

Choose a reason for hiding this comment

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

I was more thinking putting them in tensorrt-inl.h and including the file in this file (in case we need it somewhere).

Copy link
Contributor Author

@haohuanw haohuanw Jul 2, 2019

Choose a reason for hiding this comment

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

hmmmm, tensorrt-inl.h is not in the default header folder. if we do that we probably need an new header file?

Copy link
Contributor

@Caenorst Caenorst Jul 2, 2019

Choose a reason for hiding this comment

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

I see, ignore my original comment then

@haohuanw haohuanw force-pushed the master branch 3 times, most recently from b92fb25 to 2b8b640 Compare July 1, 2019 06:02
KellenSunderland
KellenSunderland reviewed Jul 3, 2019
View reviewed changes
cpp-package/include/mxnet-cpp/contrib.h

} // namespace details

namespace contrib {
Copy link
Contributor

@KellenSunderland KellenSunderland Jul 3, 2019

Choose a reason for hiding this comment

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

@szha Hey Sheng, have we had contrib namespaces in C++ before? The intent of this code basically aligns with the intent of having a contrib level python API. Does namespacing it out like this make sense to you?

@KellenSunderland KellenSunderland merged commit fc54781 into apache:master Jul 4, 2019
@KellenSunderland KellenSunderland mentioned this pull request Oct 3, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nswamy nswamy Awaiting requested review from nswamy nswamy is a code owner

2 more reviewers

@Caenorst Caenorst Caenorst left review comments

@KellenSunderland KellenSunderland KellenSunderland approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@haohuanw @KellenSunderland @Caenorst @Roshrini