[TensorRT] Fixed access to pad_value by akmaru · Pull Request #8131 · apache/tvm

akmaru · 2021-05-25T19:53:40Z

Fixed the omission modify pad_value from Attr to Args of `relay.nn.pad ' by the following commit.

78657e1

akmaru · 2021-05-25T19:59:19Z

By the way, are there suitable codeplaces of dynamic_cast util functions to relay ops from python?
If any, it should modularize the sequence of check and take constant from Relay.Constant as a function, for example, as_const_scalar.

comaniac

Can you add a test case?
Also cc @trevor-m

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

trevor-m · 2021-05-25T20:36:25Z

Thanks @akmaru
Were you able to test the runtime with this change in place? I think we also need to edit the input signature for the pad converter otherwise there will be a runtime error.

https://github.com/apache/tvm/blob/main/src/runtime/contrib/tensorrt/tensorrt_ops.cc#L1060
Should now be {kTensor, kWeight} instead of {kTensor}

akmaru · 2021-05-25T20:56:44Z

tests/python/contrib/test_tensorrt.py

+    run_and_verify_func(get_graph((1, 8, 16, 16), [[0, 0], [0, 0], [0, 1], [2, 0]], pad_value=-1.0e30))
    run_and_verify_func(get_graph((1, 8, 3, 16, 16), [[0, 0], [0, 0], [0, 0], [0, 0], [0, 0]]))
-
+    run_and_verify_func(get_graph((1, 8, 3, 16, 16), [[0, 0], [0, 0], [0, 0], [0, 0], [0, 0]], pad_value=-1.0e30))


Added the test cases.

akmaru · 2021-05-25T21:11:51Z

@trevor-m

Were you able to test the runtime with this change in place? I think we also need to edit the input signature for the pad converter otherwise there will be a runtime error.

Umm, I haven't encountered runtime error in my running.
(I have encountered this error compiling Tiny-YOLOv3 DarkNet and I have confirmed it is passed by this change.)

Though, it should be fix, so I'll do it!

akmaru · 2021-05-25T21:25:19Z

src/runtime/contrib/tensorrt/tensorrt_ops.cc

 class PadOpConverter : public TensorRTOpConverter {
 public:
-  PadOpConverter() : TensorRTOpConverter({kTensor}) {}
+  PadOpConverter() : TensorRTOpConverter({kTensor, kWeight}) {}


Added pad_value input into PadOpConverter.

fixstars-fujinami · 2021-05-26T04:52:33Z

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

+    assert isinstance(args[1], relay.Constant)
+    if len(args[1].checked_type.shape) == 0 and args[1].data.numpy().item() != 0.0:


I think it's better to check type of pad_value with if instead of assert because it may be Expr.

I suggest the following code, including the fix of checking the shape of pad_value,

Suggested change

assert isinstance(args[1], relay.Constant)

if len(args[1].checked_type.shape) == 0 and args[1].data.numpy().item() != 0.0:

if (not isinstance(args[1], relay.Constant) or

len(args[1].checked_type.shape) != 0 or

args[1].data.numpy().item() != 0.0):

It shouldn't be Expr anyways if the pad mode is constant.

I think pad_mode="constant" just means padding by pad_value, not type of pad_value is relay.Constant.

nn.pad allows Expr for pad_value.

tvm/python/tvm/relay/op/nn/nn.py

Lines 1657 to 1658 in 03c8a6f

pad_value: float, or tvm.relay.Expr, optional, default=0

The value used for padding

Hmm you're right. I didn't notice that constant mode doesn't enforce constant node. @akmaru please change accordingly.

Fixed by e50bc43, please review again.

tqchen · 2021-06-04T12:47:18Z

gentle ping @akmaru to resolve the lint error

comaniac · 2021-07-06T16:50:29Z

@akmaru not sure if the CI failure is related to this PR. Please fix it or retrigger the CI otherwise.

comaniac · 2021-07-27T18:05:36Z

Gentle ping @akmaru

trevor-m · 2021-09-24T00:47:06Z

@akmaru Could you resolve the conflict?

comaniac · 2021-10-11T16:23:06Z

Gentle ping @akmaru please resolve the conflict and pass the CI.

masahi · 2022-01-09T22:32:15Z

This was partially addressed in #9858. @akmaru can you update?

areusch · 2022-04-11T23:17:02Z

closing due to inactivity, but feel free to reopen!

Fixed access to pad_value

5fb7d13

masahi assigned comaniac May 25, 2021

comaniac reviewed May 25, 2021

View reviewed changes

python/tvm/relay/op/contrib/tensorrt.py Outdated Show resolved Hide resolved

Added test cases

6e0bbc0

akmaru commented May 25, 2021

View reviewed changes

akmaru added 2 commits May 26, 2021 06:22

Added pad_value into PadOpConverter inputs

d7af28e

Check pad_value is Relay.Constant by assertion

9cc90d8

akmaru commented May 25, 2021

View reviewed changes

fixstars-fujinami reviewed May 26, 2021

View reviewed changes

Fixed checking if constant pad_value

e50bc43

comaniac approved these changes May 31, 2021

View reviewed changes

Formatted

827797a

ylc added a commit to ylc/tvm that referenced this pull request Sep 28, 2021

changes from apache#8131

363d384

ylc mentioned this pull request Sep 28, 2021

changes from apache/tvm#8131 neo-ai/tvm#228

Merged

trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 28, 2021

changes from apache#8131 (#228)

561ac46

comaniac added the status: accepted label Oct 11, 2021

jroesch added the needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it label Jan 19, 2022

areusch closed this Apr 11, 2022

		assert isinstance(args[1], relay.Constant)
		if len(args[1].checked_type.shape) == 0 and args[1].data.numpy().item() != 0.0:

-    assert isinstance(args[1], relay.Constant)
-    if len(args[1].checked_type.shape) == 0 and args[1].data.numpy().item() != 0.0:
+    if (not isinstance(args[1], relay.Constant) or
+        len(args[1].checked_type.shape) != 0 or
+        args[1].data.numpy().item() != 0.0):

	pad_value: float, or tvm.relay.Expr, optional, default=0
	The value used for padding

Conversation

akmaru commented May 25, 2021

Uh oh!

akmaru commented May 25, 2021

Uh oh!

comaniac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trevor-m commented May 25, 2021

Uh oh!

akmaru May 25, 2021

Choose a reason for hiding this comment

Uh oh!

akmaru commented May 25, 2021

Uh oh!

akmaru May 25, 2021

Choose a reason for hiding this comment

Uh oh!

fixstars-fujinami May 26, 2021

Choose a reason for hiding this comment

Uh oh!

comaniac May 26, 2021

Choose a reason for hiding this comment

Uh oh!

fixstars-fujinami May 26, 2021

Choose a reason for hiding this comment

Uh oh!

comaniac May 26, 2021

Choose a reason for hiding this comment

Uh oh!

akmaru May 27, 2021

Choose a reason for hiding this comment

Uh oh!

tqchen commented Jun 4, 2021

Uh oh!

comaniac commented Jul 6, 2021

Uh oh!

comaniac commented Jul 27, 2021

Uh oh!

trevor-m commented Sep 24, 2021

Uh oh!

comaniac commented Oct 11, 2021

Uh oh!

masahi commented Jan 9, 2022

Uh oh!

areusch commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants