Skip to content

Refactor ExecuteTrtEngine. - #38118

Merged
tensorflow-copybara merged 5 commits into
tensorflow:masterfrom
tfeher:trt_refactor_execute_engine
Apr 8, 2020
Merged

Refactor ExecuteTrtEngine.#38118
tensorflow-copybara merged 5 commits into
tensorflow:masterfrom
tfeher:trt_refactor_execute_engine

Conversation

@tfeher

@tfeher tfeher commented Apr 1, 2020

Copy link
Copy Markdown
Contributor

This PR refactors ExecuteTrtEngine so that its components can be reused from BuildAndRun.

BuildAndRun is used by the TF-TRT op converter tests to initialize TRT bindings and execute inference. Its role is roughly equivalent to ExecuteTrtEngine. Recently ExecuteTrtEngine was modified to handle inference using dynamic shapes (PRs #36379, #36434, #36435), the same was not done for the opconverter's BuildAndRun. To allow unit testing of op converters in dynamic shape mode, one needs the extend BuildAndRun with similar changes.

This PR refactors ExecuteTRTEngine, so that both BuildAndRun and ExecuteTrtEngie can use a common set of utility functions to run inference. While doing these changes, the logging is streamlined: we return a status with the log message if an error occurs.

For easier comparison, one could do something like:

git show master:tensorflow/compiler/tf2tensorrt/kernels/trt_engine_op.cc | sed -n '619,868p' > ExecuteTrtEngine_master.cc && diff ExecuteTrtEngine_master.cc tensorflow/compiler/tf2tensorrt/utils/trt_engine_utils.cc

This PR does not add new tests, nor does it modify any existing behavior. This PR is a prerequisite of op converter tests in dynamic shape mode.

An alternative solution to this PR would be to copy the relevant code to BuildAndRun, but this would further increase the already existing code duplication between ExecuteTrtEngine and BuildAndRun. Furthermore, the exact way of specifying the input shapes is subject to change (for example currently we do not specify input shape bindings, and it might be required in the future). Therefore it is preferred to reuse code between BuildAndRun and ExecuteTrtEngine.

Tagging @DEKHTIARJonathan and @bixia1 for review.

@tensorflow-bot tensorflow-bot Bot added the size:L CL Change Size: Large label Apr 1, 2020
@tfeher tfeher mentioned this pull request Apr 1, 2020
Merged
@gbaned gbaned self-assigned this Apr 1, 2020
@gbaned gbaned added the comp:gpu:tensorrt Issues specific to TensorRT label Apr 1, 2020
@gbaned
gbaned requested a review from bixia1 April 1, 2020 15:04
bixia1
bixia1 reviewed Apr 1, 2020
View reviewed changes

@bixia1 bixia1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

Comment thread tensorflow/compiler/tf2tensorrt/utils/trt_engine_utils.h Resolved
@tfeher tfeher mentioned this pull request Apr 1, 2020
Merged
bixia1
bixia1 previously approved these changes Apr 1, 2020
View reviewed changes
@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 1, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 1, 2020
@gbaned gbaned added the kokoro:force-run Tests on submitted change label Apr 2, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 2, 2020
@gbaned

gbaned commented Apr 2, 2020

Copy link
Copy Markdown
Contributor

@tfeher Can you please address Ubuntu Sanity errors? Thanks!

@gbaned gbaned removed the ready to pull PR ready for merge process label Apr 2, 2020
@tfeher

tfeher commented Apr 2, 2020

Copy link
Copy Markdown
Contributor Author

Fixed the sanity error.

bixia1
bixia1 previously approved these changes Apr 2, 2020
View reviewed changes
@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 2, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 2, 2020
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Apr 6, 2020
@bixia1

bixia1 commented Apr 6, 2020

Copy link
Copy Markdown
Contributor

The merge process is blocked due to the error below, can you please fix this?
tensorflow/compiler/tf2tensorrt/convert/utils.cc:169:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
TensorShapeUtils::MakeShape(trt_dims.data(), trt_dims.size(), &shape);
^~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tensorflow/compiler/tf2tensorrt/convert/utils.cc:179:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
TensorShapeUtils::MakeShape(trt_dims.d, trt_dims.nbDims, &shape);
^~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

@gbaned gbaned removed the ready to pull PR ready for merge process label Apr 7, 2020
@tfeher

tfeher commented Apr 7, 2020

Copy link
Copy Markdown
Contributor Author

Thanks @bixia1, I have fixed the problems with the unused results.

@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 7, 2020
@kokoro-team kokoro-team removed kokoro:force-run Tests on submitted change labels Apr 7, 2020
@tensorflow-copybara
tensorflow-copybara merged commit 1057da9 into tensorflow:master Apr 8, 2020
@tfeher tfeher mentioned this pull request Dec 8, 2020
Closed
@tfeher
tfeher deleted the trt_refactor_execute_engine branch May 16, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bixia1 bixia1 bixia1 approved these changes

Assignees

@gbaned gbaned

Labels

cla: yes comp:gpu:tensorrt Issues specific to TensorRT ready to pull PR ready for merge process size:L CL Change Size: Large

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tfeher @gbaned @bixia1 @googlebot @kokoro-team @tensorflow-copybara