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.

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

TF-TRT C++ conversion #52012

Merged

Conversation

tfeher
Copy link
Contributor

@tfeher tfeher commented Sep 15, 2021

TF-TRT C++ interface to convert models.

Differences compared to the Python API:

  • It is assumed that the input graph is frozen. This assumption will be removed in a follow up PR.
  • convert_to_static_engine conversion param allows to convert dynamic engines to static engines.
  • Currently we do not provide a way to save the engine files as assets.

The steps for model conversion by ConvertAndBuild

  1. Inline functions
  2. Freeze the graph - omitted since we assume frozen input graph
  3. Run Grappler with TRTOptimizer pass
  4. Infer the graph to provide shape information (only in dynamic shape mode).
  5. Infer the graph to build the engines
  6. Convert the graph_def to have static engines

(On the Python side, steps 4-5 are done by a separate build function.)

Related PRs:

@google-ml-butler google-ml-butler bot added the size:XL CL Change Size:Extra Large label Sep 15, 2021
@google-cla google-cla bot added the cla: yes label Sep 15, 2021
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Sep 15, 2021
@gbaned gbaned self-assigned this Sep 15, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 15, 2021
@bixia1 bixia1 self-requested a review September 16, 2021 16:40
@bixia1 bixia1 added the comp:gpu:tensorrt Issues specific to TensorRT label Sep 16, 2021
@bixia1
Copy link
Contributor

bixia1 commented Sep 16, 2021

We can avoid the change to the utilities for freezing the graph by updating out SavedModelBundle with the converted GraphDef. Something like this:

MetaGraphDef* meta_graph_def = &saved_model_bundle->meta_graph_def; 
*meta_graph_def->mutable_graph_def() = graph_def;

@sanjoy sanjoy removed their request for review September 20, 2021 20:30
@tfeher tfeher force-pushed the trt_cpp_conversion_example branch 2 times, most recently from afaa5af to d876eae Compare September 26, 2021 22:49
@tfeher tfeher marked this pull request as ready for review September 26, 2021 22:51
@tfeher
Copy link
Contributor Author

tfeher commented Sep 26, 2021

@bixia1 ready for review.

tensorflow/core/BUILD Outdated Show resolved Hide resolved
Copy link
Contributor

@bixia1 bixia1 left a comment

Choose a reason for hiding this comment

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

I am still working on reviewing trt_convert.cc .

tensorflow/core/BUILD Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/BUILD Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.h Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.h Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.h Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert_test.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert_test.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert_test.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert_test.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@bixia1 bixia1 left a comment

Choose a reason for hiding this comment

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

Still working on trt_convert.cc.

tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
Comment on lines 62 to 66
// We could set item_conifg.feed_nodes and item_config.fetch_nodes to the
// nodes in the signature def. Alternatively, we could also set collection
// 'train_op'. Grappler can use these to determine the input and outputs.
// If none of these are set, then it will use the SignatereDef from
// the MetaGraphDef. See grappler_item_builder.cc for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we will go with the API where users provide input/output names, we should setup ItemConfig fetch_nodes/feed_nodes.
If we will go with the API where users provide SignatureDef, we need to clear the SignatureDef in the input meta_graph_def and keep only the one we need.

Also, if we will need to run grappler a few times, we probably want to avoid constructing GrapplerItem each time by somehow reusing the GrapplerItem, see TF code here.

Copy link
Contributor Author

@tfeher tfeher Oct 4, 2021

Choose a reason for hiding this comment

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

  • I have set up feed_nodes and fetch_nodes.
  • If a previous conversion had RunMetaOptimizer(item, ..., out_graph_def), can we just replace the item's graph item.graph = out_graph_def before the next RunMetaOptimizer call? For now we have only a single grappler pass, but it might matter in the follow up PR where we need to inline the graph before freezing it.

tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 4, 2021
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.

Thanks @bixia1 for the review I have addressed most of the issues. Pleas have a look.

tensorflow/core/BUILD Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/BUILD Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.h Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.h Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
Comment on lines 62 to 66
// We could set item_conifg.feed_nodes and item_config.fetch_nodes to the
// nodes in the signature def. Alternatively, we could also set collection
// 'train_op'. Grappler can use these to determine the input and outputs.
// If none of these are set, then it will use the SignatereDef from
// the MetaGraphDef. See grappler_item_builder.cc for details.
Copy link
Contributor Author

@tfeher tfeher Oct 4, 2021

Choose a reason for hiding this comment

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

  • I have set up feed_nodes and fetch_nodes.
  • If a previous conversion had RunMetaOptimizer(item, ..., out_graph_def), can we just replace the item's graph item.graph = out_graph_def before the next RunMetaOptimizer call? For now we have only a single grappler pass, but it might matter in the follow up PR where we need to inline the graph before freezing it.

tensorflow/compiler/tf2tensorrt/trt_convert.cc Outdated Show resolved Hide resolved
@tfeher tfeher force-pushed the trt_cpp_conversion_example branch 2 times, most recently from 09cd906 to b02e321 Compare October 7, 2021 17:30
@tfeher
Copy link
Contributor Author

tfeher commented Oct 7, 2021

@bixia1 I have removed the circular dependency, and added trt_convert_api as a dependency of trt_op_libs. Let me know if there are other issues that we want to address in this PR.

@bixia1
Copy link
Contributor

bixia1 commented Oct 7, 2021

Can you fix the PR description, such as to remove the information that is obsolete? The PR description will become part of the commit message.

@sherhut sherhut removed their request for review October 8, 2021 12:23
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.

Thanks @bixia1 for the review, I have addressed the issues.

tensorflow/compiler/tf2tensorrt/trt_convert_api.cc Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert_api.h Outdated Show resolved Hide resolved
tensorflow/compiler/tf2tensorrt/trt_convert_api.h Outdated Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Oct 11, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 11, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 11, 2021
@copybara-service copybara-service bot merged commit e8366cb into tensorflow:master Oct 11, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:gpu:tensorrt Issues specific to TensorRT size:XL CL Change Size:Extra Large
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

5 participants