Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Address design review and feedback from MXNet commiters #8

Merged
merged 25 commits into from
Aug 9, 2018
Merged

Address design review and feedback from MXNet commiters #8

merged 25 commits into from
Aug 9, 2018

Conversation

KellenSunderland
Copy link

@KellenSunderland KellenSunderland commented Aug 9, 2018 •
edited
Loading

Combining the feedback from the PR, the dev list, and in-person communication, this PR attempts to:

  • Maintain all interface const'ness (a major concern).
  • Encapsulate TRT logic using OO mechanisms. Only include TRT objects in TRT builds.
  • Minimize the code change for non-TRT users to ensure they can’t run into regressions.
  • Move all user python code to contrib to indicate it’s a work in progress.
  • Add more validation logic, earlier in execution, with more descriptive error messages.
  • Add negative case testing for incorrect usage attempts.

Follow up improvements which I’ll leave to other PRs are:

  • Remove some duplicated code between TRT and non-TRT classes (it’s there, but pretty minimal).
  • Migrate to the sub-graph API.
  • Migrate from shared-buffer to sub-graph work, or a temporary third approach.

@KellenSunderland
WIP Refactored TRT integration and tests
d88ad8b
@KellenSunderland
Add more build guards, remove unused code
87ebdce
@KellenSunderland
Remove ccache report
83fa475
@KellenSunderland
Remove redundant const in declaration
4a3772f
@KellenSunderland
Clean Cmake TRT files
f779537
@KellenSunderland
Remove TensorRT env var usage
b0748ef 
We don't want to use environment variables with TensorRT yet, the
logic being that we want to try and have as much fwd compatiblity as
possible when working on an experimental feature.  Were we to add
env vars they would have to be gaurenteed to work in the future until
a major version change.  Moving the functionality to a contrib call
reduces this risk.
@KellenSunderland
Use contrib optimize_graph instaed of bind
35e1367
@KellenSunderland
Clean up cycle detector
6338e45
@KellenSunderland
Convert lenet test to contrib optimize
21d0239
@KellenSunderland
Protect interface with trt build flag
f30dbef
@KellenSunderland
Fix whitespace issues
eaba593
@KellenSunderland
Add another build guard to c_api
f870a3f
@KellenSunderland
Move get_optimized_symbol to contrib area
e40d6b3
@KellenSunderland
Ignore gz files in test folder
7fa6a4a
@KellenSunderland
Make trt optimization implicit
e777ab5
@KellenSunderland
Remove unused declaration
3ea9b89
@KellenSunderland
Replace build guards with runtime errors
d6d2cac
@KellenSunderland
Change default value of TensorRT to off
2d04aee 
This is change applies to both TensorRT and non-TensorRT builds.
@KellenSunderland
Warn user when TRT not active at runtime
449a195
@KellenSunderland
Move TensorRTBind declaration, add descriptive errors
ed36739
@KellenSunderland
Test TensorRT graph execution, fix bugs
882d8e5
@KellenSunderland
Fix lint and whitespace issues
95a7955
@KellenSunderland
Fix typo
0307467
@KellenSunderland
Removed default value for set_use_tensorrt
8504319
@KellenSunderland
Improved documentation and fixed spacing issues
55dd422
@KellenSunderland KellenSunderland changed the title Address design review and freedback from MXNet commiters Address design review and feedback from MXNet commiters Aug 9, 2018
@mkolod
Copy link
Owner

mkolod commented Aug 9, 2018

Thanks @KellenSunderland!

@mkolod mkolod merged commit f7ff036 into mkolod:tensorrt_integration Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KellenSunderland @mkolod