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

Extend TF:TRT C++ API to handle non-frozen models #53082

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Extend TF:TRT C++ API to handle non-frozen models #53082

merged 1 commit into from
Mar 15, 2022

Conversation

tfeher
Copy link
Contributor

@tfeher tfeher commented Nov 16, 2021 •
edited

This PR extends the C++ converter API of TF-TRT #52012 to handle models that are not frozen (i.e. contains variables).

Depends on #53310 . Tracker #45481.

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Nov 16, 2021
@google-cla google-cla bot added the cla: yes label Nov 16, 2021
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Nov 16, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Nov 16, 2021
@tfeher tfeher force-pushed the trt_cpp_converter_variables branch from 495a2c2 to e8e7ccf Compare December 1, 2021 18:00
@tfeher tfeher mentioned this pull request Dec 5, 2021
Merged
@tfeher tfeher force-pushed the trt_cpp_converter_variables branch from e8e7ccf to c3db9ec Compare December 7, 2021 16:58
@tfeher tfeher marked this pull request as ready for review December 7, 2021 17:46
@rthadur
Copy link
Contributor

rthadur commented Dec 7, 2021

@tfeher could you please resolve conflicts ?

@tfeher tfeher force-pushed the trt_cpp_converter_variables branch from c3db9ec to 685920e Compare December 8, 2021 11:26
@tfeher
Copy link
Contributor Author

tfeher commented Dec 8, 2021

Conflicts resolved, tagging @bixia1 for review.

@rthadur rthadur requested a review from bixia1 December 8, 2021 17:41
@bixia1 bixia1 added the comp:gpu:tensorrt Issues specific to TensorRT label Dec 14, 2021
@gbaned gbaned requested review from bixia1 and removed request for bixia1 December 31, 2021 16:38
@gbaned
Copy link
Contributor

gbaned commented Feb 10, 2022

@bixia1 Can you please review this PR ? Thanks!

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Feb 14, 2022
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Feb 14, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 14, 2022
@gbaned gbaned removed awaiting review Pull request awaiting review ready to pull PR ready for merge process labels Feb 14, 2022
@gbaned
Copy link
Contributor

gbaned commented Feb 14, 2022

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

@tfeher
Copy link
Contributor Author

tfeher commented Feb 15, 2022

Addressed sanity errors. Also rebased and fixed missing argument that was introduced in #53507.

@gbaned gbaned requested review from bixia1 and removed request for sanjoy February 16, 2022 13:47
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Feb 16, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 1, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 1, 2022
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Mar 1, 2022
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Mar 3, 2022
@tfeher tfeher force-pushed the trt_cpp_converter_variables branch from 2c403ea to 8d12421 Compare March 7, 2022 12:17
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 7, 2022
@tfeher tfeher force-pushed the trt_cpp_converter_variables branch from 8d12421 to f29248f Compare March 7, 2022 18:55
tfeher
tfeher commented Mar 7, 2022
View reviewed changes
Copy link
Contributor Author

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

@bixia1 I have fixed the issue. Let me know if you have a different suggestion for resolving the cycle.

tensorflow/compiler/tf2tensorrt/BUILD Show resolved Hide resolved
@gbaned gbaned requested a review from bixia1 March 8, 2022 13:52
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Mar 8, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 8, 2022
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 8, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 8, 2022
@gbaned gbaned removed the awaiting review Pull request awaiting review label Mar 9, 2022
bixia1
bixia1 reviewed Mar 9, 2022
View reviewed changes
tensorflow/compiler/tf2tensorrt/BUILD
Comment on lines +140 to +146
filegroup(
name = "headers",
srcs = [
"trt_convert_api.h",
],
)

Copy link
Contributor

Choose a reason for hiding this comment

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

This filegroup is not used?

tensorflow/compiler/tf2tensorrt/BUILD
@@ -370,7 +384,6 @@ cc_library(
name = "trt_op_libs",
deps = [
":get_calibration_data_op_op_lib",
":trt_convert_api",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pass tensorflow/compiler/tf2tensorrt:convert_qdq_test_gpu?
I got this error: third_party/tensorflow/compiler/tf2tensorrt/convert/ops/quantization_ops_test.cc:33:10: fatal error: 'third_party/tensorflow/compiler/tf2tensorrt/trt_convert_api.h' file not found
#include "third_party/tensorflow/compiler/tf2tensorrt/trt_convert_api.h"
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

tensorflow/compiler/tf2tensorrt/trt_convert_api.h
@@ -19,6 +19,7 @@ limitations under the License.
#include <string>
#include <vector>

#include "tensorflow/cc/saved_model/loader.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to remove this line, and instead just add a forward declaration
struct SavedModelBundle;

@gbaned gbaned removed the ready to pull PR ready for merge process label Mar 10, 2022
@gbaned
Copy link
Contributor

gbaned commented Mar 11, 2022

@tfeher Can you please check @bixia1's comments and keep us posted ? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Mar 11, 2022
@copybara-service copybara-service bot merged commit a1214be into tensorflow:master Mar 15, 2022
@google-ml-butler google-ml-butler bot removed the stat:awaiting response Status - Awaiting response from author label Mar 15, 2022
@DEKHTIARJonathan DEKHTIARJonathan mentioned this pull request Mar 22, 2022
Merged
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

@joker-eph joker-eph Awaiting requested review from joker-eph

Assignees

@gbaned gbaned

Labels
cla: yes comp:gpu:tensorrt Issues specific to TensorRT size:L CL Change Size: Large
Projects
PR Queue
  
Approved by Reviewer
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tfeher @rthadur @gbaned @bixia1 @kokoro-team @tensorflowbutler