Dynamo converter cat #2343

apbose · 2023-09-25T19:58:36Z

This PR moves the aten::cat implementation to the dynamo impl library.

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 C++ style guidelines

gs-olive · 2023-09-25T21:25:17Z

py/torch_tensorrt/dynamo/conversion/aten_ops_converters.py

+        SourceIR.ATEN,
+        name,
+        tensors=args[0],
+        dim=args_bounds_check(args, 2, 1),


Switch to dim=args_bounds_check(args, 1, 0), since the default should be 0 at the args[1] position, as here.

gs-olive · 2023-09-25T21:26:16Z

py/torch_tensorrt/dynamo/conversion/impl/cat.py

+    if dim < 0:
+        dim = len(input[0].shape) + dim


Switch to get_positive_dim, as here:

TensorRT/py/torch_tensorrt/dynamo/conversion/converter_utils.py

Lines 337 to 339 in 4cffd6e

def get_positive_dim(

dim: Union[int, Sequence[int]], dim_size: int

) -> Union[int, Tuple[int, ...]]:

See above comment

gs-olive · 2023-09-25T21:45:10Z

py/torch_tensorrt/dynamo/conversion/impl/cat.py

+    if any(not isinstance(t, TRTTensor) for t in input):  # type: ignore[union-attr]
+        raise RuntimeError(
+            f"cat received inputs {input} that is not part " "of the TensorRT region!"
+        )


Instead of throwing an error, instead check the inputs and use get_trt_tensor for any inputs which are not already TRTTensor objects.

Alternatively, #2324 can help with this

Consider using @enforce_tensor_types here

gs-olive · 2023-09-25T21:46:03Z

py/torch_tensorrt/dynamo/conversion/impl/cat.py

+    target: Target,
+    source_ir: Optional[SourceIR],
+    name: str,
+    input: TRTNetwork,


This should likely be something like Sequence[TRTTensor] or Sequence[Union[TRTTensor, torch.Tensor, np.ndarray]]

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

github-actions

Code conforms to C++ style guidelines

github-actions

There are some changes that do not conform to Python style guidelines:

--- /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/conversion/impl/cat.py	2023-10-04 20:10:10.328968+00:00
+++ /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/conversion/impl/cat.py	2023-10-04 20:13:03.260979+00:00
@@ -16,11 +16,11 @@
    name: str,
    input: Union[TRTTensor, Sequence[TRTTensor]],
    dim: int,
) -> Union[TRTTensor, Sequence[TRTTensor]]:
    for each_input in input:
-        if(not isinstance(each_input, TRTTensor)):
+        if not isinstance(each_input, TRTTensor):
            each_input = get_trt_tensor(each_input)
    concat_layer = ctx.net.add_concatenation(input)
    if dim < 0:
        dim = len(input[0].shape) + dim

gs-olive · 2023-10-04T21:52:29Z

py/torch_tensorrt/dynamo/conversion/impl/cat.py

+    target: Target,
+    source_ir: Optional[SourceIR],
+    name: str,
+    input: Union[TRTTensor, Sequence[TRTTensor]],


Switch to Sequence[Union[TRTTensor, np.ndarray, torch.Tensor]]

gs-olive · 2023-10-04T21:54:17Z

py/torch_tensorrt/dynamo/conversion/impl/cat.py

+    for each_input in input:
+        if(not isinstance(each_input, TRTTensor)):
+            each_input = get_trt_tensor(each_input)


Switch to:

trt_tensor_input = [get_trt_tensor(ctx, each_input, name + "_tensor{i}") for i, each_input in enumerate(input)]

gs-olive · 2023-10-04T21:54:26Z

py/torch_tensorrt/dynamo/conversion/impl/cat.py

+    if dim < 0:
+        dim = len(input[0].shape) + dim


See above comment

github-actions

Code conforms to C++ style guidelines

github-actions

There are some changes that do not conform to Python style guidelines:

--- /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/conversion/impl/cat.py	2023-10-04 22:36:19.760280+00:00
+++ /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/conversion/impl/cat.py	2023-10-04 22:38:58.599050+00:00
@@ -21,11 +21,11 @@
    name: str,
    input: Sequence[Union[TRTTensor, torch.Tensor, np.ndarray]],
    dim: int,
) -> Union[TRTTensor, Sequence[TRTTensor]]:
    for each_input in input:
-        if(not isinstance(each_input, TRTTensor)):
+        if not isinstance(each_input, TRTTensor):
            each_input = get_trt_tensor(each_input)
    concat_layer = ctx.net.add_concatenation(input)
    if dim < 0:
        dim = get_positive_dim(dim, len(input[0].shape))

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

zewenli98

The current change looks good overall. A small issue could be fixed, as I commented below.

zewenli98 · 2023-10-04T23:34:24Z

py/torch_tensorrt/dynamo/conversion/impl/cat.py

+    if dim < 0:
+        dim = get_positive_dim(dim, len(input[0].shape))


Since get_positive_dim will internally check if dim is less than 0, the condition could be deleted.

gs-olive · 2023-10-04T23:44:12Z

py/torch_tensorrt/dynamo/conversion/impl/cat.py

+    for each_input in input:
+        if not isinstance(each_input, TRTTensor):
+            each_input = get_trt_tensor(each_input)


This loop will not save the get_trt_tensor objects anywhere, since the each_input object gets reset on every loop iteration. Consider replacing with something like:

trt_inputs = [] for i, each_input in enumerate(input): each_input = get_trt_tensor(ctx, each_input, name + "_tensor{i}") trt_inputs.append(each_input) ... # Replace all uses of input with trt_inputs

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

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

gs-olive

Looks good to me!

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

zewenli98

Thanks! LGTM.

gs-olive · 2023-10-06T23:19:08Z

Verified tests passing locally.

apbose requested a review from gs-olive September 25, 2023 19:58

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

github-actions bot added component: api [Python] Issues re: Python API component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths labels Sep 25, 2023

github-actions bot requested a review from narendasan September 25, 2023 19:58

github-actions bot approved these changes Sep 25, 2023

View reviewed changes

gs-olive reviewed Sep 25, 2023

View reviewed changes

gs-olive added the priority: high label Sep 29, 2023

github-actions bot requested a review from zewenli98 September 29, 2023 18:52

apbose force-pushed the dynamo_converter_cat branch from 31a2cba to afc9b5d Compare October 4, 2023 00:43

github-actions bot approved these changes Oct 4, 2023

View reviewed changes

apbose force-pushed the dynamo_converter_cat branch from afc9b5d to 79e7e13 Compare October 4, 2023 20:06

github-actions bot approved these changes Oct 4, 2023

View reviewed changes

github-actions bot requested changes Oct 4, 2023

View reviewed changes

gs-olive requested changes Oct 4, 2023

View reviewed changes

github-actions bot approved these changes Oct 4, 2023

View reviewed changes

github-actions bot requested changes Oct 4, 2023

View reviewed changes

apbose force-pushed the dynamo_converter_cat branch from 80300d3 to 94535d2 Compare October 4, 2023 22:42

github-actions bot approved these changes Oct 4, 2023

View reviewed changes

zewenli98 reviewed Oct 4, 2023

View reviewed changes

gs-olive reviewed Oct 4, 2023

View reviewed changes

zewenli98 mentioned this pull request Oct 4, 2023

feat: support flatten and reshape via shuffle_layer #2354

Merged

7 tasks

apbose force-pushed the dynamo_converter_cat branch 2 times, most recently from 57544fc to 1a6e814 Compare October 5, 2023 17:29

github-actions bot approved these changes Oct 5, 2023

View reviewed changes

apbose requested review from zewenli98 and gs-olive October 5, 2023 17:32

github-actions bot approved these changes Oct 5, 2023

View reviewed changes

gs-olive approved these changes Oct 5, 2023

View reviewed changes

apbose force-pushed the dynamo_converter_cat branch from 1a6e814 to 83e4f3c Compare October 5, 2023 22:21

github-actions bot approved these changes Oct 5, 2023

View reviewed changes

apbose added 4 commits October 5, 2023 15:25

aten::cat converter moving to impl

944278e

aten::cat converter moving to impl

c2ac3b4

aten::cat rebase changes

759b260

Addressing review comments

83e4f3c

zewenli98 approved these changes Oct 6, 2023

View reviewed changes

gs-olive merged commit 83176fe into main Oct 6, 2023

gs-olive deleted the dynamo_converter_cat branch October 6, 2023 23:19

	def get_positive_dim(
	dim: Union[int, Sequence[int]], dim_size: int
	) -> Union[int, Tuple[int, ...]]:

Dynamo converter cat #2343

Dynamo converter cat #2343

Conversation

apbose commented Sep 25, 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

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

Choose a reason for hiding this comment

gs-olive Sep 25, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

gs-olive Oct 4, 2023 • edited Loading

Choose a reason for hiding this 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

zewenli98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

github-actions bot left a comment

Choose a reason for hiding this comment

gs-olive 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

zewenli98 left a comment

Choose a reason for hiding this comment

gs-olive commented Oct 6, 2023

gs-olive Sep 25, 2023 •

edited

Loading

gs-olive Oct 4, 2023 •

edited

Loading