fix: Add special cases for `clone` and `to_copy` where input of graph is output #2265

gs-olive · 2023-08-25T17:44:40Z

Description

TRT does not allow inputs of graphs to be outputs as well, however many of the scenarios encountered in real models can have this situation come up, especially in cases where the input is cloned or copied and then returned
The current converters will register these operators as a no-op, causing TRT engine building to fail on such inputs
Instead of requiring creation of an identity layer for every case of a clone or copy node, we instead check if that node is the only operator on a placeholder (input) and then insert the identity layer or not, accordingly
Coalesce implementations of clone and to_copy, which are effectively the same operator
Add test cases to validate new behavior
Add new boilerplate converter validator utility to support this case

Addresses bug in #1565

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist:

[ x ] My code follows the style guidelines of this project (You can use the linters)
[ x ] I have performed a self-review of my own code
[ x ] I have commented my code, particularly in hard-to-understand areas and hacks
[ x ] I have made corresponding changes to the documentation
[ x ] I have added tests to verify my fix or my feature
[ x ] New and existing unit tests pass locally with my changes
[ x ] I have added the relevant labels to my PR in so that relevant reviewers are notified

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

py/torch_tensorrt/dynamo/conversion/aten_ops_converters.py

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

narendasan · 2023-08-29T16:53:19Z

@gs-olive is this only for graphs like:

graph(x: Tensor):
  return x

Is it worth even converting these into engines?

gs-olive · 2023-08-29T16:59:14Z

@narendasan - This change is primarily for cases encountered in detectron-style models where inputs are also outputs of the engine. It might be something like:

graph(x: Tensor, y: Tensor):
  # A lot of logic/operations applied to y
  ...
  return x, y_new

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

narendasan · 2023-08-29T17:22:10Z

I see

py/torch_tensorrt/dynamo/conversion/impl/elementwise/base.py

apbose · 2023-09-08T01:31:57Z

py/torch_tensorrt/dynamo/conversion/impl/elementwise/base.py

@@ -103,9 +104,13 @@ def convert_binary_elementwise(
    # dtype but we don't have a way to detect whether it makes sense for the
    # scalar to be float or half. Hence we go with the lhs dtype.
    if is_lhs_trt_tensor and isinstance(rhs_val, (float, int)):
-        rhs_val = torch.tensor([rhs_val], dtype=lhs_dtype)


Why is this getting converted to np array in place of the torch.Tensor. Is that because the torch.Tensor was leading to clones getting produced in the graph?

Creating torch.Tensor objects within the torch.compile scope causes them to be "Fake-ified" which removes the data contained within them.

apbose · 2023-09-08T01:38:52Z

py/torch_tensorrt/dynamo/conversion/converter_utils.py

+        and any(user.op == "output" for user in list(node.users.keys()))
+    )
+
+


Why is there any used here? I believe that the above is for to_copy and clone cases having one node arg only? Or are there different cases for this?

any is used here because we are checking if any of the users of the node are outputs, meaning that the node is the only function between a placeholder and an output. We are effectively search for subgraphs where an input is followed by a function is followed by an output.

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

- TRT does not allow inputs of graphs to be outputs as well, however many of the scenarios encountered in real models can have this situation come up, especially in cases where the input is cloned or copied and then returned - The current converters will register these operators as a no-op, causing TRT engine building to fail on such inputs - Instead of requiring creation of an identity layer for every case of a clone or copy node, we instead check if that node is the only operator on a placeholder (input) and then insert the identity layer or not, accordingly - Coalesce implementations of clone and to_copy, which are effectively the same operator - Add test cases to validate new behavior - Add new boilerplate converter validator utility to support this case

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

gs-olive self-assigned this Aug 25, 2023

facebook-github-bot added the cla signed label Aug 25, 2023

github-actions bot requested a review from apbose August 25, 2023 17:45

github-actions bot approved these changes Aug 25, 2023

View reviewed changes

gs-olive commented Aug 25, 2023

View reviewed changes

py/torch_tensorrt/dynamo/conversion/aten_ops_converters.py Outdated Show resolved Hide resolved

gs-olive commented Aug 25, 2023

View reviewed changes

py/torch_tensorrt/dynamo/conversion/aten_ops_converters.py Show resolved Hide resolved

gs-olive force-pushed the get_clone_fixes branch 2 times, most recently from 4f4556f to bcfa6c1 Compare August 25, 2023 20:56

github-actions bot approved these changes Aug 25, 2023

View reviewed changes

gs-olive force-pushed the get_clone_fixes branch from bcfa6c1 to cbd22ac Compare August 29, 2023 16:57

github-actions bot approved these changes Aug 29, 2023

View reviewed changes

gs-olive mentioned this pull request Aug 31, 2023

feat: support many elementwise dynamo converters #2263

Merged

7 tasks

apbose reviewed Sep 8, 2023

View reviewed changes

gs-olive force-pushed the get_clone_fixes branch from cbd22ac to a57c97f Compare September 8, 2023 05:57

gs-olive requested a review from apbose September 8, 2023 05:57

github-actions bot approved these changes Sep 8, 2023

View reviewed changes

gs-olive force-pushed the get_clone_fixes branch from a57c97f to a055858 Compare September 8, 2023 20:10

github-actions bot approved these changes Sep 8, 2023

View reviewed changes

apbose approved these changes Sep 20, 2023

View reviewed changes

gs-olive force-pushed the get_clone_fixes branch from a055858 to 892f7d0 Compare September 20, 2023 17:36

github-actions bot approved these changes Sep 20, 2023

View reviewed changes

gs-olive merged commit ac007ce into main Sep 20, 2023

gs-olive deleted the get_clone_fixes branch September 20, 2023 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add special cases for `clone` and `to_copy` where input of graph is output #2265

fix: Add special cases for `clone` and `to_copy` where input of graph is output #2265

gs-olive commented Aug 25, 2023

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

narendasan commented Aug 29, 2023

gs-olive commented Aug 29, 2023

github-actions bot left a comment

github-actions bot left a comment

narendasan commented Aug 29, 2023

apbose Sep 8, 2023

gs-olive Sep 8, 2023

apbose Sep 8, 2023

gs-olive Sep 8, 2023

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

		and any(user.op == "output" for user in list(node.users.keys()))
		)

fix: Add special cases for clone and to_copy where input of graph is output #2265

fix: Add special cases for clone and to_copy where input of graph is output #2265

Conversation

gs-olive commented Aug 25, 2023

Description

Type of change

Checklist:

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

narendasan commented Aug 29, 2023

gs-olive commented Aug 29, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

narendasan commented Aug 29, 2023

apbose Sep 8, 2023

Choose a reason for hiding this comment

gs-olive Sep 8, 2023

Choose a reason for hiding this comment

apbose Sep 8, 2023

Choose a reason for hiding this comment

gs-olive Sep 8, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

fix: Add special cases for `clone` and `to_copy` where input of graph is output #2265

fix: Add special cases for `clone` and `to_copy` where input of graph is output #2265