[TRT][BYOC] handling dynamism in TensorRT to support OD models #6905

rohanmukh · 2020-11-12T15:57:02Z

refactoring test tensort code

added comments to dynamic check wrapper

log.warn changed to logger.info

TRT codegen taking slice_mode into account

TRT codegen to handle both types of stride_mode

refactoring TRT codegen

adding a test for dynamic offload

[TRT] bug in codegen for slice_mode=end

ctx determined from target in test + io test was missing

refactoring test tensort code added comments to dynamic check wrapper log.warn changed to logger.info TRT codegen taking slice_mode into account TRT codegen to handle both stride_mode refactoring TRT codegen adding a test for dynamic offload [TRT] bug in codegen for slice_mode=end ctx determined from target in test + io test was missing

rohanmukh · 2020-11-12T15:57:41Z

@trevor-m @anijain2305 @zhiics

trevor-m

Thanks @rohanmukh !

trevor-m · 2020-11-12T16:51:22Z

python/tvm/relay/op/contrib/tensorrt.py

+
+    for i in range(0, len(args[0].checked_type.shape)):
+        begin = int(attrs.begin[i])
+        end = (


We need to take slice mode into account here also

trevor-m · 2020-11-12T16:53:44Z

python/tvm/relay/op/contrib/tensorrt.py

@@ -715,6 +771,34 @@ def conv3d_transpose_annotate_fn(expr):  # pylint: disable=unused-variable
        return False
    return True

+_register_external_dynamic_check_func("add", add_annotate_fn)


I'm not sure if using the wrapper is better than adding a call to check_dynamism to each annotator. With the previous method, the annotator is all in one place with the decorator. Now, we have to remember to call register down here after writing the functions.

Will it also not lead to the same issue, that we have to remember to check for check_dynamism in each of the annotator?

I see, can we make this function a decorator then?

Thanks, I did that. Also tested its efficacy with test_dynamic_offload(). Can you please cross-verify the decorator usage?

trevor-m · 2020-11-12T16:54:52Z

src/relay/backend/contrib/tensorrt/codegen.cc

@@ -133,26 +133,34 @@ class TensorRTJSONSerializer : public backend::contrib::JSONSerializer {
    auto process_slice_index = [](Integer x, int default_value, int dim_value) {
      if (!x.defined()) return default_value;
      int value = x.as<IntImmNode>()->value;
-      if (value < 0) value += dim_value;
+      value = (value < 0 ) ? dim_value + value : value;


This line is the same as the previous code, can you change it back?

trevor-m · 2020-11-12T16:55:30Z

src/runtime/contrib/tensorrt/tensorrt_ops.cc

@@ -944,7 +944,7 @@ class ReduceOpConverter : public TensorRTOpConverter {
 #if TRT_VERSION_GE(5, 1, 5)
 class StridedSliceOpConverter : public TensorRTOpConverter {
 public:
-  StridedSliceOpConverter() : TensorRTOpConverter({kTensor, kWeight, kWeight, kWeight}) {}
+  StridedSliceOpConverter() : TensorRTOpConverter({kTensor}) {} // , kWeight, kWeight, kWeight}) {}


Remove comment

trevor-m · 2020-11-12T17:00:24Z

tests/python/contrib/test_tensorrt.py



-def test_tensorrt_serialize():
+def test_tensorrt_serialize(data_shape=(1, 3, 224, 224), data_type="float32"):


I think it would be good to split this into test_tensorrt_serialize_graph_runtime and test_tensorrt_serialize_vm

trevor-m · 2020-11-12T17:01:39Z

tests/python/contrib/test_tensorrt.py

@@ -841,6 +906,39 @@ def get_graph(
    run_and_verify_func(get_graph(strides=(2, 2, 2)))
    run_and_verify_func(get_graph(strides=(2, 2, 2), output_padding=(1, 1, 1)))

+def test_tensorrt_ops():


You can leave these in main

rohanmukh · 2020-11-12T17:28:23Z

Thanks for the comments @trevor-m.

anijain2305 · 2020-11-12T17:24:25Z

python/tvm/relay/op/contrib/tensorrt.py

@@ -18,11 +18,12 @@
 """TensorRT supported operators."""
 import logging
 import numpy as np
+import os


Do we need this?

anijain2305 · 2020-11-12T17:24:37Z

python/tvm/relay/op/contrib/tensorrt.py

            ),
            transform.FoldConstant(),
+            transform.InferType(),


Do we need this?

Thanks for pointing, we don't, was used for debugging purposes only.

anijain2305 · 2020-11-12T17:25:24Z

python/tvm/relay/op/contrib/tensorrt.py

+
+    Parameters
+    ----------
+    args: a TRT array of the arguments of the op


Please follow Python style for docstrings

anijain2305 · 2020-11-12T17:25:45Z

python/tvm/relay/op/contrib/tensorrt.py

+
+    Parameters
+    ----------
+    op_name: name of the op for debugging purposes only


Same as above

anijain2305 · 2020-11-12T17:41:47Z

python/tvm/relay/op/contrib/tensorrt.py

+_register_external_dynamic_check_func("nn.conv3d_transpose", conv3d_transpose_annotate_fn)
+
+
+


remove extra spaces everywhere

Lint formatting

python/tvm/relay/op/contrib/tensorrt.py

zhiics · 2020-11-12T22:53:36Z

tests/python/contrib/test_tensorrt.py

-    test_tensorrt_serialize()
-
-    # Op tests
+    test_tensorrt_serialize_graph_runtime()


you can remove all these test_xx and just use pytest.main([__file__]) because pytest is used for testing.

zhiics · 2020-11-12T22:56:01Z

tests/python/contrib/test_tensorrt.py

+    def save_vm(code, lib):
+        # save and load the code and lib file.
+        lib.export_library("path_lib.so")
+        with open("path_code.ro", "wb") as fo:


its probably better to use tmp dir because you sometimes may not have write permission at the current dir.

trevor-m · 2020-11-12T23:45:37Z

tests/python/contrib/test_tensorrt.py

+    # Get the expected relay graph and compare
+    mod_exp = get_expected()
+    tvm.ir.assert_structural_equal(mod_trt, mod_exp, map_free_vars=True)
+    return


Don't need this return

trevor-m · 2020-11-12T23:46:15Z

tests/python/contrib/test_tensorrt.py

+    test_densenet121()
+
+
+def test_dynamic_offload(data_shape=(1, 32, 8, 8), k_shape=(1, 32, 3, 3)):


Lets move the args to variables inside the test. Same for the serialization tests

skip_codegen added for test_tensorrt::test_dynamic_offload

zhiics

LGTM

trevor-m · 2020-11-13T18:09:33Z

tests/python/contrib/test_tensorrt.py

-    test_pool3d()
-    test_conv3d_transpose()
-
+def test_tensorrt_integration():


We can remove this function - these tests will already be ran

Thanks @trevor-m, sorry I missed that while importing the tests to pytest.

trevor-m

Thanks!

anijain2305 · 2020-11-13T23:31:42Z

Thanks @rohanmukh @zhiics @trevor-m This is merged

…e#6905) * handling dynamism in TensorRT to support OD models refactoring test tensort code added comments to dynamic check wrapper log.warn changed to logger.info TRT codegen taking slice_mode into account TRT codegen to handle both stride_mode refactoring TRT codegen adding a test for dynamic offload [TRT] bug in codegen for slice_mode=end ctx determined from target in test + io test was missing * Addressed the formatting/refactoring comments * Addressed comment in TRT codegen Lint formatting * Lint error * using slice_mode during strided slice registration in tensorrt.py * removed a few blank lines * addressing cli comment on elif-return * Added decorator for tensorrt functions with dynamism check skip_codegen added for test_tensorrt::test_dynamic_offload * addressed comments in PR + black linting * resolved import error in test_tensorrt * import mxnet location changed to pass CI * test_integration removed as components were run by pytest anyway

trevor-m requested changes Nov 12, 2020

View reviewed changes

anijain2305 reviewed Nov 12, 2020

View reviewed changes

rohanmukh added 6 commits November 12, 2020 19:15

Addressed the formatting/refactoring comments

416ea33

Addressed comment in TRT codegen

c33be64

Lint formatting

Lint error

99a767e

using slice_mode during strided slice registration in tensorrt.py

11a9968

removed a few blank lines

8fe7b62

addressing cli comment on elif-return

2210543

zhiics reviewed Nov 12, 2020

View reviewed changes

trevor-m requested changes Nov 12, 2020

View reviewed changes

rohanmukh added 4 commits November 13, 2020 03:09

Added decorator for tensorrt functions with dynamism check

b041eee

skip_codegen added for test_tensorrt::test_dynamic_offload

addressed comments in PR + black linting

282db6d

resolved import error in test_tensorrt

b6d8dc8

import mxnet location changed to pass CI

0b1ac24

zhiics approved these changes Nov 13, 2020

View reviewed changes

trevor-m requested changes Nov 13, 2020

View reviewed changes

test_integration removed as components were run by pytest anyway

3b8182a

trevor-m approved these changes Nov 13, 2020

View reviewed changes

anijain2305 approved these changes Nov 13, 2020

View reviewed changes

anijain2305 merged commit b4f99e5 into apache:main Nov 13, 2020

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TRT][BYOC] handling dynamism in TensorRT to support OD models #6905

[TRT][BYOC] handling dynamism in TensorRT to support OD models #6905

rohanmukh commented Nov 12, 2020

rohanmukh commented Nov 12, 2020

trevor-m left a comment

trevor-m Nov 12, 2020

trevor-m Nov 12, 2020

rohanmukh Nov 12, 2020

trevor-m Nov 12, 2020

rohanmukh Nov 13, 2020

trevor-m Nov 12, 2020

trevor-m Nov 12, 2020

trevor-m Nov 12, 2020

trevor-m Nov 12, 2020

rohanmukh commented Nov 12, 2020

anijain2305 Nov 12, 2020

anijain2305 Nov 12, 2020

rohanmukh Nov 12, 2020

anijain2305 Nov 12, 2020

anijain2305 Nov 12, 2020

anijain2305 Nov 12, 2020

zhiics Nov 12, 2020

zhiics Nov 12, 2020

trevor-m Nov 12, 2020

trevor-m Nov 12, 2020

zhiics left a comment

trevor-m Nov 13, 2020

rohanmukh Nov 13, 2020

trevor-m left a comment

anijain2305 commented Nov 13, 2020



		def test_tensorrt_serialize():
		def test_tensorrt_serialize(data_shape=(1, 3, 224, 224), data_type="float32"):

		_register_external_dynamic_check_func("nn.conv3d_transpose", conv3d_transpose_annotate_fn)

		test_densenet121()


		def test_dynamic_offload(data_shape=(1, 32, 8, 8), k_shape=(1, 32, 3, 3)):

[TRT][BYOC] handling dynamism in TensorRT to support OD models #6905

[TRT][BYOC] handling dynamism in TensorRT to support OD models #6905

Conversation

rohanmukh commented Nov 12, 2020

rohanmukh commented Nov 12, 2020

trevor-m left a 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

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

rohanmukh commented Nov 12, 2020

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

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

zhiics left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-m left a comment

Choose a reason for hiding this comment

anijain2305 commented Nov 13, 2020