Repository navigation
[TFTRT] Add Dynamic Shape Testing for ConvertConv3D - #46940
copybara-service[bot] merged 1 commit into
Conversation
bixia1
left a comment
There was a problem hiding this comment.
Looks good to me, thank you! I see some formatting issues. Can you please run clang format on the code as described here https://www.tensorflow.org/community/contribute/code_style ?
| Scope s = Scope::NewRootScope(); | ||
| auto input = ops::Placeholder(s.WithOpName("input"), DT_FLOAT); | ||
| auto filter = ops::Placeholder(s.WithOpName("weights"), DT_FLOAT); | ||
| // Get the NodeDef for Pack. |
| NodeDef node_def = GetConv3DNodeDef(p.strides, p.padding, p.data_format, | ||
| p.dilations, p.is_conv3d_backprop); | ||
|
|
||
| test->AddTestTensor("input", p.input_dims, test->get_tf_type(), p.input); |
There was a problem hiding this comment.
Does this test work in dynamic shape mode? AFAIK TRT requires static (known) values for the channel dimension. By default AddTestTensor replaces all input dims with -1, therefore this should fail the test in dynamic shape mode.
For the Conv2D test, I needed to fix the input dims like this, and define an extra test for the condition on the channel value .
There was a problem hiding this comment.
It does work so far on my end. Let me know if you have any issue trying to reproduce this behavior
There was a problem hiding this comment.
Thanks Jonathan, you are right, it does work. Unfortunately, I am also right: in general it should not work in dynamic shape mode because TRT requires that the channel dimension is known. The following happens here: TRT is clever enough to deduce that setting min=max shape optimization profile means that the channel dims is known at engine creation time, and treats it as if the corresponding input dim was set to that known value (instead of -1 that we provide).
If you define a profile where min<max for the channel dimension, then TRT gives the following error message:
DefaultLogger /my_conv3d-conv:CONVOLUTION: number of channels in input tensor to a convolution layer must not be dynamic
DefaultLogger Builder failed while analyzing shapes.
I recommend the following:
- In the converter, check that the channel is known. We can relax this a little bit: allow conversion if
kOptimalorkImplicitBatchModeCompatibleprofiles are used, because they have min=max for non batch dimension, reject converting the op otherwise. - Adapt the test use give concrete channel dimensions.
Most importantly, I think we should adapt the unit test to check that the converters work correctly for a profile where min < max. This will be a general case in the future, and we want to be prepared for that. One option would be the following:
-
Rebase the PR so that it contains the profile management logic from Add TF-TRT optimization profiles for testing #45588
-
Define a
ProfileStrategy::kUnitTestprofile by setting min = opt = input_shape, max = min+1. Here is a sketch:
void SetUnitTestProfile(
const std::vector<nvinfer1::Dims>& dimvec, std::vector<nvinfer1::Dims>* min,
std::vector<nvinfer1::Dims>* opt, std::vector<nvinfer1::Dims>* max) {
*min = dimvec;
*opt = dimvec;
*max = dimvec;
for (auto& dim : *max) {
for (int k = 0; k < dim.nbDims; k++) dim.d[k]++;
}
}This would come into trt_shape_optimization_profiles.cc, and a new case should be added switch statement here.
Make the kUnitTest profile as a default profile for unit tests. (Currently it is set here.)
- Check if the correct error is returned.
Step 3-4 could be implemented in a separate PR.
My motivation to introduce a specific profile for unit test is the following:
BuildAndRunsees a single input shape for the network.- From this single shape we want to create a non-trivial profile (min!=max).
kImplicitBatchModeCompatibleonly changes the batch dim, but we want to a profile that also changes other dims. - Other profile options that we were considering for future implementations are derived from multiple input shapes, therefore they are not applicable.
@bixia1 please let us know if you have other ideas / preferences.
There was a problem hiding this comment.
I am fine with putting step 3-4 in a separated PR. However, instead of adding kUnitTest strategy, I suggest you to open a backdoor in TrtShapeOptimizationProfile, such as using a special test_only_field to indicate the need to modify the kOptimal mode, to serve similar purpose (similar to what we have in the V2 converter python class to support the switch of implicit/explicit batch mode). The ProfileStrategy enums will be hoisted out to a bigger scope to support the API change for dynamic shape. I don't feel that I can get pass the google code interview if we have kUnitTest in the enums.
There was a problem hiding this comment.
Thanks @DEKHTIARJonathan for the update, it looks good to me.
|
@bixia1 : PR is squashed and good to go ;) |
|
Hi, there is a comment you haven't addressed yet, https://github.com/tensorflow/tensorflow/pull/46940/files#r571100867. I am approving it so that I can get it for testing but please feel free to push another commit and squash it. |
bixia1
left a comment
There was a problem hiding this comment.
Approving it for the purpose of testing.
@bixia1 @tfeher for review
Feature Tracker: #45481