fx2trt converters aten::slice,aten::select and aten::matmul #1770

apbose · 2023-03-24T23:58:45Z

Description

Fixes #1709
Fixed #1714

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

frank-wei

It is nice to see many activation operators of aten IR are added. But could you minimize the changes on fx2trt side like changes of common_activation ?

frank-wei · 2023-03-25T06:03:09Z

py/torch_tensorrt/fx/converters/activation.py



-def common_activation(


Is there reason we want to drop this common_activation func and split it into many single functions?

Hi Wei, the common_activation function is not split, instead it is included in the add_activation function in activation.py. The idea is that the activation and the operator functions are split in two (activation.py and operator.py respectively) and the converter_utils.py is for the common functions across the two and other operations.

gs-olive

Looks good overall, with lots of critical operators and tests. Just made a few comments on in-place modification of layer objects and matmul test coverage.

gs-olive · 2023-03-27T22:35:20Z

py/torch_tensorrt/fx/converters/operator.py

+    output = layer.get_output(0)
+    output.name = output.name + "_" + target.__name__
+    return layer.get_output(0)


Does layer.get_output(0) make a copy of the underlying object? If so, line 547 may have no effect, since it only modifies the output object. Should this be return output instead?

So the layer.get_output(0) does not make an explicit copy and modifying the output.name causes inplace modification of the layer.get_output(0).name too. Yes it could have been return output too I assume, but any change would be inconsequential.

gs-olive · 2023-03-27T22:36:33Z

py/torch_tensorrt/fx/converters/operator.py

+    layer = network.add_matrix_multiply(
+        input_val, input_matrix_op, other_val, other_matrix_op
+    )
+    set_layer_name(layer, target, name)


Some layer name changes use this function and others directly modify layer.get_output(0).name. Consider standardizing the usage across functions.

That makes sense. Just that in the case of set_layer_name the name is changed directly for the layer, while appending acc_ops.target.__name__ to the layer name with the name passed in the above function. Whereas in case of the other we change the name of layer.get_output(0) with only the target.__name__. @frank-wei any thoughts on this to integrate the two?

gs-olive · 2023-03-27T22:41:20Z

py/torch_tensorrt/fx/test/converters/aten_op/test_matmul_aten.py

Consider adding additional tests for this function since it is used so often. For example, adding multi-dimensional tensors and single-dimensional vector matrix multiplications as well.

- Add backend adapted from previous `fx2trt_compiler` provided by Dynamo - Currently, the TRTSplitter needs work to fully support the `aten` path - Additionally, the existing `aten` pass was reworked to exclude the `torch._dynamo.export` call, which may be necessary here

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

apbose · 2023-04-19T15:04:01Z

Hi @wushirong, thanks for the review.

Yes, the changes are too big for a single review, hence could you please review - fx2trt converters - change of prototype and addition of activation operation #1745. I will then subsequently put the additional converter changes present in the current review after those changes are reviewed/merged in, so that the changes are done in small increments.
Regarding the second point, yes the purpose of PR fx2trt converters - change of prototype and addition of activation operation #1745, is to make the flow generic for acc and aten operations (Right now the implementation is in acc_ops_converters.py). For that reason the implementation of the converters have been moved from acc_ops_converters.py to a generic interface, activation.py for activation operations and operator.py for operations. acc_ops_converters.py and aten_ops_converters.py call the functions from these two files.
The changes for reorganization of files-
activation.py - functions for activation operations
operator.py - functions for converter operations
converter_utils.py - common functions for both
eg: since common_activation is required just for activation operations, this is moved to actovation.py from converter_utils.py

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

…onverters_transformer_encoder

…ed in lowering

…mer_encoder fx2trt converters transformer encoder

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

apbose added 5 commits March 22, 2023 15:18

Implementation of slice and select operations

96bdead

merging the bose_fx2trt_converter

85c755b

select test implementation

c8811cd

select aten test

4f18c0f

aten::matmul, aten::slice, aten::select converters

8303cd5

facebook-github-bot added cla signed fx labels Mar 24, 2023

github-actions bot added component: api [Python] Issues re: Python API component: fx labels Mar 24, 2023

github-actions bot requested a review from narendasan March 24, 2023 23:59

github-actions bot approved these changes Mar 24, 2023

View reviewed changes

apbose changed the title ~~Bose fx2trt converters slice select~~ fx2trt converters aten::slice,aten::select and aten::matmul Mar 24, 2023

apbose requested a review from gs-olive March 24, 2023 23:59

apbose added the WIP Work is in progress, pull request should not be merged yet label Mar 25, 2023

github-actions bot approved these changes Mar 25, 2023

View reviewed changes

github-actions bot requested a review from frank-wei March 25, 2023 00:00

frank-wei requested a review from wushirong March 25, 2023 05:58

github-actions bot approved these changes Mar 25, 2023

View reviewed changes

frank-wei reviewed Mar 25, 2023

View reviewed changes

gs-olive reviewed Mar 27, 2023

View reviewed changes

gs-olive and others added 5 commits March 28, 2023 20:47

Add decompositions to aot call

243bf9b

Mark FX2TRT converter as fake tensor unsupported

76fd3c8

Minor naming bugfix

6a8102c

Merge branch 'main' into bose_fx2trt_converters_slice_select

35cf89d

github-actions bot approved these changes Mar 29, 2023

View reviewed changes

matmul changes, bmm changes and adding broadcastable

09a52b9

github-actions bot approved these changes Apr 19, 2023

View reviewed changes

Correcting pre-commit hooks

d1fd1d7

narendasan changed the base branch from main to bose_fx2trt_converters April 19, 2023 18:43

feat: Add ts converter support for aten::all.dim (#1840)

1d78f43

github-actions bot approved these changes Apr 20, 2023

View reviewed changes

apbose added 11 commits April 19, 2023 22:42

Resolving merge conflicts with bose_fx2trt_converters

39e4da6

Merge branch 'main' into bose_fx2trt_converters_transformer_encoder

d9545be

Merge branch 'bose_fx2trt_converters_slice_select' into bose_fx2trt_c…

89f2fcf

…onverters_transformer_encoder

Correcting rsqrt and rsub operator

ce7f122

python linting issues and removing chunk test

30c5fd6

Correcting acc squeeze test

7ab071d

test_reshape expected ops aten.reshape since aten.view has been remov…

36ac0cf

…ed in lowering

removing aten.view in lowering pass

eb851b1

layer_norm test

6b234e0

correcting linting error

95c1ada

correcting dynamic shape layer norm

1a1b809

apbose removed the WIP Work is in progress, pull request should not be merged yet label Apr 21, 2023

Merge pull request #1814 from pytorch/bose_fx2trt_converters_transfor…

f721ed1

…mer_encoder fx2trt converters transformer encoder

github-actions bot added component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: core Issues re: The core compiler component: tests Issues re: Tests labels Apr 21, 2023

github-actions bot approved these changes Apr 21, 2023

View reviewed changes

apbose merged commit ae2706d into bose_fx2trt_converters Apr 21, 2023

frank-wei mentioned this pull request Apr 24, 2023

fx2trt converters - change of prototype and addition of activation operation #1745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fx2trt converters aten::slice,aten::select and aten::matmul #1770

fx2trt converters aten::slice,aten::select and aten::matmul #1770

apbose commented Mar 24, 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

frank-wei left a comment •

edited

Loading

frank-wei Mar 25, 2023

apbose Mar 27, 2023

gs-olive left a comment

gs-olive Mar 27, 2023

apbose Mar 30, 2023

gs-olive Mar 27, 2023

apbose Mar 30, 2023

gs-olive Mar 27, 2023

github-actions bot left a comment

github-actions bot left a comment

apbose commented Apr 19, 2023 •

edited

Loading

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



		def common_activation(

fx2trt converters aten::slice,aten::select and aten::matmul #1770

fx2trt converters aten::slice,aten::select and aten::matmul #1770

Conversation

apbose commented Mar 24, 2023

Description

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

frank-wei left a comment • edited Loading

Choose a reason for hiding this comment

frank-wei Mar 25, 2023

Choose a reason for hiding this comment

apbose Mar 27, 2023

Choose a reason for hiding this comment

gs-olive left a comment

Choose a reason for hiding this comment

gs-olive Mar 27, 2023

Choose a reason for hiding this comment

apbose Mar 30, 2023

Choose a reason for hiding this comment

gs-olive Mar 27, 2023

Choose a reason for hiding this comment

apbose Mar 30, 2023

Choose a reason for hiding this comment

gs-olive Mar 27, 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

apbose commented Apr 19, 2023 • edited Loading

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

frank-wei left a comment •

edited

Loading

apbose commented Apr 19, 2023 •

edited

Loading