[TF:TRT] Enable Dynamic Shape Support for ConvertResize by wraveane · Pull Request #51462 · tensorflow/tensorflow

wraveane · 2021-08-12T16:12:20Z

The Resize operation now supports full dynamic shapes. This is done by slicing the input tensor's shape, replacing the H and W dims with the corresponding Resize op scales, and concatenating the dims back to a shape tensor that can then be given as a second input to the TRT Resize layer.

In case of static shapes, the previous method remains, where the output dims are simply given as a parameter to the TRT Resize layer instead.

A new use case is now also supported, where the "size" may now be given as a tensor input instead of constant weights. This would often happen when working with dynamic shapes, as the output size may need to be calculated on the fly through shape ops. E.g., resizing a tensor by a scale factor of 2 would require the size to be calculated dynamically in the graph based on the input tensor's shape, thus not a constant input that is known during building.

The corresponding test was also updated to now expect all 3 TRT modes to pass: kImplicitBatch, kExplicitBatch and kDynamicShape.

Tracker: #45481

google-cla · 2021-08-12T16:12:30Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

bixia1 · 2021-08-13T15:32:40Z

+  // Set output shape.
+  if (params->use_implicit_batch || HasStaticShape(inputs_dims)) {
+    // If all shape dims are known at build time, pass them as a parameter.
+    layer->setOutputDimensions(output_shape_dims);
+  } else {
+    // Otherwise, the output shape tensor becomes an extra input to the layer.
+    layer->setInput(1, *output_shape_tensor->trt_tensor());
+  }
+


Let's merge this if-stmt with the previous if-stmt, by moving the code between the two if-stmts to before the first if-stmt.

Actually, it's not possible to merge them, in case of dynamic shape, the concatenation of the shape tensor needs to be done before the resize layer itself, but then setting the output dimensions and/or adding the secondary input can only be done after the resize layer is created.

I see what you meant. Can you (1) define "nvinfer1::IResizeLayer* layer = nullptr" before the if-stmt, and let the branches create the layer, and (2) move the relevant code from this block to after the if-stmt?

I think I understand what you're asking, but I'm not sure this would result in cleaner code (unless I'm overlooking something obvious). The resize layer would still have to be created the same way on both branches -- basically because the addResize() method only takes a single input tensor anyway, so the second input would still need to be provided through a separate statement regardless. Please correct me if I missed the point of what you asked of course.

Please let me know if there was anything else to do for this comment, as I think there's not much else that can be done with the way the layer is created in the current version.

bixia1 · 2021-08-13T15:33:43Z

  // Given input dimensions [N, C, H, W] and output size [H_out, W_out],
  // output dimensions equals [N, C, H_out, W_out]


This comment should go inside the if branch below

The comment applies to both branches, as it's a similar logic in both cases: H_out and W_out are overridden by the scale attribute in both cases.

I was thinking this comment was talking about calculating output_shape_dims, the else branch actually calculates a output shape tensor. Let's do this:
s/Calculate output dimensions./Calculate the output shape as static dimensions or a shape tensor:/

Ah right, I guess keeping the word "dimensions" could be mis-interpreted given the two different methods there are now. I updated this.

bixia1 · 2021-08-13T15:48:06Z


+  // Set output shape.
+  if (params->use_implicit_batch || HasStaticShape(inputs_dims)) {
+    // If all shape dims are known at build time, pass them as a parameter.


Let's revise this comment:
If the input shape is known at build time, pass the static output shape to the resize layer as expected output dimensions.

bixia1 · 2021-08-13T15:50:25Z

+    // If all shape dims are known at build time, pass them as a parameter.
+    layer->setOutputDimensions(output_shape_dims);
+  } else {
+    // Otherwise, the output shape tensor becomes an extra input to the layer.


Let's revise the comment:
Otherwise, pass the output shape tensor to the resize layer as an input.

bixia1 · 2021-08-13T15:53:18Z

+  const auto inputs_dims = inputs_tensor->getDimensions();
+  nvinfer1::Dims output_shape_dims;
+  ITensorProxyPtr output_shape_tensor;
+  if (params->use_implicit_batch || HasStaticShape(inputs_dims)) {


Shall we remove params->use_implicit_batch here?

This branch should also run when running in implicit batch mode, as in that case the output dimensions need to be explicitly set as well. However if you think it's cleaner -- I could change the conditionals to be more "descriptive" by checking for whether output_shape_dims or output_shape_tensor have been set in the if statement above, and run the corresponding branch in either case.

I have two questions:
(1) the block of code inside this if-branch also works and executes when (use_implicit_batch==false && HasStaticShape(inputs_dims)) before your PR, see code. Am I right here? If not, what are the reason that this code don't work for use_implicit_batch=false? I couldn't see that...
(2) This is not introduced by your PR, but when I read this code, I think it has a problem for use_implicit_batch=true as we remove the batch dimension and should only have three dimensions in the input tensor. Do you agree?

Sure, so:
(1) When running in implicit batch mode, all non-batch dimensions are known at build time. So that is enough to be able to calculate the output shape dimensions. So the implicit batch check there acts as a flag that allows this dimension calculation to happen at build time. In any case, this logic is slightly modified in the commit I'm pushing soon for dynamic output size support.
(2) Kind of -- So in implicit batch mode, during build time, the batch dimension is skipped and all build-time calculations must happen ignoring that dimension. However at run-time, the batch dimension does exist obviously, so any runtime computations must take the N batch dimension into account. So the transpose layer that you pointed out must be configured with how the layer will operate at runtime, and so the transpose dims must be configured taking the batch dimension in account.

We discussed this offline, I think the current version now addresses your concern with how implicit batch is checked for.

google-cla · 2021-08-13T17:29:03Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

wraveane · 2021-08-19T17:25:00Z

@googlebot I signed it!

bixia1 · 2021-08-23T15:21:53Z

+    if (inputs.at(1).weights().count() != 2) {
+      return errors::Unimplemented("Resize to shape=[] is not supported, at ",
+                                   node_def.name());
+    }


This code seems to imply is that if the rank is not 2, it is 0 ("shape=[]" in the error message). The error message it NOT added by this PR, so this is not your fault. Can we change the error message to "Resize requires 2D values for the new size"?

This error message was updated in a commit yesterday.

bixia1 · 2021-08-23T15:25:09Z

+      return errors::Unimplemented("Resize to shape=[] is not supported, at ",
+                                   node_def.name());


Similar to the error message in the if-branch.

This error message was updated in a commit yesterday.

bixia1 · 2021-08-23T15:43:16Z

I couldn't figure out a way to reply to #51462 (comment), so I am putting my response here. I don't feel you have answered my two questions, we need to schedule a meeting to discuss this.

bixia1

Would you please squash all the commits into one? This is to help simplify the auto generated message that goes with the commit, and it is usually done right before the PR is approved.

bixia1 · 2021-08-23T23:36:04Z

-    return errors::Unimplemented("Resize to shape=[] is not supported, at ",
-                                 node_def.name());
+  const auto inputs_dims = inputs_tensor->getDimensions();
+  const bool static_input_shape = HasStaticShape(inputs_dims);


NIT: we can replace the use of static_input_shape && const_output_size with
const bool static_output_shape = HasStaticShape(inputs_dims) && const_output_size

the put this new variable closer to where it is used.

Yes, this might be a bit cleaner indeed. Updated it as per your suggestion.

bixia1 · 2021-08-24T15:52:18Z

Looks good to me. Thanks for working on this!
When you address the last minor comment, please also squash the commits into one as request here.

wraveane · 2021-08-24T17:08:20Z

Looks good to me. Thanks for working on this!
When you address the last minor comment, please also squash the commits into one as request here.

@bixia1 : Ok great. I think I updated everything as per all the comments. Please let me know if anything else is still needed.

As for squashing, because I had to merge master into my branch to handle a conflict, squashing the commits now would require me to do a force push of the branch I think. Is that ok? Otherwise, I think github always allows the option to squash before merging once the review is complete.

bixia1 · 2021-08-24T18:09:07Z

Is this something I have a way to indicate on github "I think github always allows the option to squash before merging once the review is complete" ? I don't actually do anything to merge, but let the "automatic process to do the job".

If I can't figure out how to indicate "squash before merging", I need you to squash your commits. I have reviewed your new changes, you are free to do the merge and squash.

wraveane · 2021-08-24T18:53:25Z

Ok, all good now, commits have been squashed.

google-ml-butler Bot added the size:M CL Change Size: Medium label Aug 12, 2021

google-ml-butler Bot requested review from joker-eph, sanjoy and sherhut August 12, 2021 16:12

google-ml-butler Bot added the awaiting review Pull request awaiting review label Aug 12, 2021

google-cla Bot added the cla: no label Aug 12, 2021

bixia1 self-requested a review August 12, 2021 19:24

tfeher mentioned this pull request Aug 12, 2021

TF-TRT Dynamic Shapes Feature Tracker #45481

Closed

rthadur self-assigned this Aug 13, 2021

bixia1 added the comp:gpu:tensorrt Issues specific to TensorRT label Aug 13, 2021

bixia1 changed the title ~~[TFTRT - Dynamic Shape Phase 3+] Enable Dynamic Shape Support for ConvertResize~~ [TF:TRT] Enable Dynamic Shape Support for ConvertResize Aug 13, 2021

bixia1 reviewed Aug 13, 2021

View reviewed changes

tensorflowbutler removed the awaiting review Pull request awaiting review label Aug 15, 2021

gbaned assigned gbaned and unassigned rthadur Aug 17, 2021

sanjoy removed their request for review August 19, 2021 04:32

google-cla Bot added cla: yes and removed cla: no labels Aug 19, 2021

wraveane marked this pull request as ready for review August 23, 2021 14:01

bixia1 reviewed Aug 23, 2021

View reviewed changes

[TFTRT] Dynamic Shape Support for ConvertResize

46eef46

wraveane force-pushed the tftrt/resize/dev branch from bd6e9f5 to 46eef46 Compare August 24, 2021 18:52

bixia1 approved these changes Aug 24, 2021

View reviewed changes

google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 24, 2021

kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 24, 2021

christopherbate mentioned this pull request Aug 25, 2021

[TF:TRT] Reorganize testing utilities. #51235

Merged

copybara-service Bot merged commit 2cfb918 into tensorflow:master Aug 25, 2021

google-ml-butler Bot removed the ready to pull PR ready for merge process label Aug 25, 2021

		// Given input dimensions [N, C, H, W] and output size [H_out, W_out],
		// output dimensions equals [N, C, H_out, W_out]

		return errors::Unimplemented("Resize to shape=[] is not supported, at ",
		node_def.name());

Conversation

wraveane commented Aug 12, 2021 • edited by bixia1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla Bot commented Aug 12, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-cla Bot commented Aug 13, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

wraveane commented Aug 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bixia1 commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bixia1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bixia1 commented Aug 24, 2021

Uh oh!

wraveane commented Aug 24, 2021

Uh oh!

bixia1 commented Aug 24, 2021

Uh oh!

wraveane commented Aug 24, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

wraveane commented Aug 12, 2021 •

edited by bixia1

Loading

bixia1 commented Aug 23, 2021 •

edited

Loading