TFTRT: Add ExpandDims, Squeeze ops and unit tests. #23909

trevor-m · 2018-11-21T21:46:39Z

Add conversion for ExpandDims, Squeeze ops, and unit tests for both.

These ops will allow 1D convolutions to be converted.

tensorflow/contrib/tensorrt/convert/convert_nodes.cc

aaroey · 2018-11-29T22:09:37Z

tensorflow/contrib/tensorrt/convert/convert_nodes.cc

+  const int input_rank = input_dims.size();
+  // Mark axes to remove by setting them to 0.
+  TFAttrs attrs(node_def);
+  auto squeeze_dims = attrs.get<std::vector<int>>("squeeze_dims");


squeeze_dims is deprecated, we should use axis, see https://www.tensorflow.org/api_docs/python/tf/squeeze.

While the API now uses axis, it appears the NodeDef.attr() only contains squeeze_dims, even though when creating the node def attributes we use the .axis_ field: https://github.com/tensorflow/tensorflow/pull/23909/files#diff-5b5e9a9d60ab4ad5474d419d568fb6faR2110

See Op definition for Squeeze: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/ops/array_ops.cc#L1980

Thanks for pointing out!

tensorflow/contrib/tensorrt/convert/convert_nodes.cc

pooyadavoodi · 2018-12-05T18:47:08Z

tensorflow/contrib/tensorrt/convert/convert_nodes.cc

+tensorflow::Status ConvertExpandDims(OpConverterParams* params) {
+  const auto& inputs = params->inputs;
+  const auto& node_def = params->node_def;
+  if (inputs.size() != 2) {


Isn't axis an attribute?

pooyadavoodi · 2018-12-05T18:56:34Z

@azaks2 @smit-hinsu
Could you please review.

googlebot · 2018-12-05T19:23:31Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

alextp · 2018-12-05T19:32:29Z

Can you rebase this PR wrt master? It's a little hard to review as-is

… Fix formatting

googlebot · 2018-12-05T19:39:17Z

CLAs look good, thanks!

aaroey · 2018-12-05T21:33:07Z

tensorflow/contrib/tensorrt/convert/convert_nodes.cc

+  }
+  // Convert negative axis to corresponding positive axis.
+  if (axis < 0) axis += input_rank + 1;
+  if (input_tensor.is_tensor() && axis == 0) {


input_tensor.is_tensor() must be true now.

Is it ok to leave it for now? We will need to support strided slice for weights later for dynamic reshaping.

If so, then please leave a comment.

Oh whoops, got this mixed up with the other PR. Removing these.

tensorflow/contrib/tensorrt/convert/convert_nodes.cc

aaroey · 2018-12-05T21:34:25Z

tensorflow/contrib/tensorrt/convert/convert_nodes.cc

+  const int input_rank = input_dims.size();
+  // Mark axes to remove by setting them to 0.
+  TFAttrs attrs(node_def);
+  auto squeeze_dims = attrs.get<std::vector<int>>("squeeze_dims");


Thanks for pointing out!

aaroey

Thanks for the PR!

aaroey · 2018-12-06T15:35:35Z

Hi @trevor-m, would you help to fix this error:

error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  TensorShapeUtils::MakeShape(shape, &tensor_shape);

Thanks.

trevor-m · 2018-12-06T17:36:18Z

Hi @trevor-m, would you help to fix this error:

error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  TensorShapeUtils::MakeShape(shape, &tensor_shape);

Thanks.

Done. Thanks for reviewing!

aaroey · 2018-12-06T23:04:20Z

Thanks @trevor-m. I found that rank_two_test and unary_test are failing, probably because they're using squeeze ops, would you help to double check? If you want to add an op that is unsupported you may use self.trt_incompatible_op.

…eze_ops PiperOrigin-RevId: 224471142

trevor-m requested a review from aaroey as a code owner November 21, 2018 21:46

googlebot added the cla: yes label Nov 21, 2018

trevor-m changed the title ~~Add ExpandDims, Squeeze ops and unit tests.~~ TFTRT: Add ExpandDims, Squeeze ops and unit tests. Nov 21, 2018

tensorflowbutler assigned yifeif Nov 22, 2018

Harshini-Gadige assigned Harshini-Gadige and unassigned yifeif Nov 26, 2018

Harshini-Gadige added the awaiting review Pull request awaiting review label Nov 26, 2018

aaroey requested changes Nov 29, 2018

View reviewed changes

tensorflowbutler removed the awaiting review Pull request awaiting review label Nov 30, 2018

trevor-m force-pushed the tmorris_tftrt_expanddims_squeeze_ops branch from f51f7b8 to 9284cda Compare November 30, 2018 21:34

Harshini-Gadige added the awaiting review Pull request awaiting review label Dec 4, 2018

pooyadavoodi reviewed Dec 5, 2018

View reviewed changes

tensorflow/contrib/tensorrt/convert/convert_nodes.cc Show resolved Hide resolved

pooyadavoodi reviewed Dec 5, 2018

View reviewed changes

trevor-m requested review from alextp, asimshankar, azaks2, mrry and smit-hinsu as code owners December 5, 2018 19:22

googlebot added cla: no and removed cla: yes labels Dec 5, 2018

trevor-m added 6 commits December 5, 2018 11:33

Add ExpandDims, Squeeze ops and unit tests.

91fbda2

Add missing VectorToTrtDims function

9d3b176

Fix compilation error with BuildAndRun, Use ExpectTrtDimsEqualsArray,…

dbe1a6b

… Fix formatting

Remove debug LOG statement. Use TestParams array for Ok tests

23a1bf7

Restrict ops to tensors only. Renamed VectorToTrtDims.

293bc93

Check that axis is a scalar

06fe3c8

trevor-m force-pushed the tmorris_tftrt_expanddims_squeeze_ops branch from 4f742ca to 06fe3c8 Compare December 5, 2018 19:39

googlebot added cla: yes and removed cla: no labels Dec 5, 2018

alextp removed their request for review December 5, 2018 19:47

aaroey reviewed Dec 5, 2018

View reviewed changes

aaroey added kokoro:force-run Tests on submitted change and removed awaiting review Pull request awaiting review labels Dec 5, 2018

aaroey removed request for asimshankar, mrry and azaks2 December 5, 2018 21:41

kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 5, 2018

trevor-m added 2 commits December 5, 2018 14:28

Fix clang-format

1680520

Remove is_tensor() since inputs are required to be tensors

9546b52

aaroey previously approved these changes Dec 5, 2018

View reviewed changes

aaroey added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 5, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 5, 2018

smit-hinsu removed their request for review December 6, 2018 08:13

Use return value of TensorShapeUtils::MakeShape

f83053f

trevor-m dismissed aaroey’s stale review via f83053f December 6, 2018 17:35

aaroey previously approved these changes Dec 6, 2018

View reviewed changes

Update tests to reflect that expanddims and squeeze are now converted

fc4b184

trevor-m dismissed aaroey’s stale review via fc4b184 December 6, 2018 23:51

aaroey approved these changes Dec 7, 2018

View reviewed changes

tensorflow-copybara merged commit fc4b184 into tensorflow:master Dec 7, 2018

tensorflow-copybara pushed a commit that referenced this pull request Dec 7, 2018

Merge pull request #23909 from trevor-m:tmorris_tftrt_expanddims_sque…

f18170f

…eze_ops PiperOrigin-RevId: 224471142

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TFTRT: Add ExpandDims, Squeeze ops and unit tests. #23909

TFTRT: Add ExpandDims, Squeeze ops and unit tests. #23909

trevor-m commented Nov 21, 2018

aaroey Nov 29, 2018

trevor-m Nov 30, 2018 •

edited

trevor-m Dec 5, 2018

aaroey Dec 5, 2018

pooyadavoodi Dec 5, 2018

pooyadavoodi commented Dec 5, 2018

googlebot commented Dec 5, 2018

alextp commented Dec 5, 2018

googlebot commented Dec 5, 2018

aaroey Dec 5, 2018

trevor-m Dec 5, 2018

aaroey Dec 5, 2018

trevor-m Dec 5, 2018

aaroey Dec 5, 2018

aaroey left a comment

aaroey commented Dec 6, 2018

trevor-m commented Dec 6, 2018

aaroey commented Dec 6, 2018

TFTRT: Add ExpandDims, Squeeze ops and unit tests. #23909

TFTRT: Add ExpandDims, Squeeze ops and unit tests. #23909

Conversation

trevor-m commented Nov 21, 2018

Choose a reason for hiding this comment

trevor-m Nov 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pooyadavoodi commented Dec 5, 2018

googlebot commented Dec 5, 2018

alextp commented Dec 5, 2018

googlebot commented Dec 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaroey left a comment

Choose a reason for hiding this comment

aaroey commented Dec 6, 2018

trevor-m commented Dec 6, 2018

aaroey commented Dec 6, 2018

trevor-m Nov 30, 2018 •

edited