Dynamic Strided Slice #6316

mbrookhart · 2020-08-20T20:28:14Z

This PR splits the dynamic-parameter version of strided slice from the constant parameter version, improves the shape funcs to pass more unit test, and supports executing the constant parameter version of the op on dynamically shaped inputs.

I was able to pass most tests, the only except is that on the static op with a dynamic input tensor, I can't pass tests with negative begin values.

There's a lot of te::compute being thrown around here, but I haven't seen a clean way to simplify it yet. I would love suggestions.

Thanks!

cc @zhiics @kevinthesun @yongwww @lixiaoquan @electriclilies

electriclilies

Overall, looks good to me! I added a few style suggestions.

PS that was a lot of relay.const calls!

electriclilies · 2020-08-20T22:30:35Z

python/tvm/relay/op/dyn/_transform.py

+
+
+@script
+def _strided_slice_shape_func_input_data(data, begin, end, strides,


What's the difference between _strided_slice_shape_func_input_shape and _strided_slice_shape_func_input_data?

For the static op, we only require the input shapes, since the tributes are static, thus the input_shape variant. For the dynamic op, we need the input data, since the attributes are coming in as values. Thus the input_data variant. Why the VM is set up to separate those instead of just sending in data all the time, I'm not sure. Optimization?

electriclilies · 2020-08-20T22:44:40Z

python/tvm/relay/op/transform.py

+            end = const(list(end))
+        if isinstance(strides, (tuple, list)):
+            strides = const(list(strides))
+        begin = _make.where(begin < cast_like(const(0), begin),


Can you rename this begin for clarity?

electriclilies · 2020-08-20T23:36:48Z

src/relay/op/dyn/tensor/transform.cc

@@ -430,6 +434,114 @@ RELAY_REGISTER_OP("dyn.full")
    .set_attr<FTVMCompute>("FTVMCompute", FullCompute)
    .set_attr<TOpPattern>("TOpPattern", kElemWise);

+bool StridedSliceRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+                     const TypeReporter& reporter) {
+  CHECK_EQ(types.size(), 5);


It would be nice to add a comment saying what each of the input types are (ie types = [type1_description, ... ret_type]

electriclilies · 2020-08-20T23:43:04Z

src/relay/op/dyn/tensor/transform.cc

+  te::Tensor end = inputs[2];
+  te::Tensor strides = inputs[3];
+  // Dynamic computation
+  int64_t attr_size = data->shape.size();


Does this stand for attribute size? If so, the name seems a bit inaccurate

electriclilies · 2020-08-20T23:48:31Z

src/relay/op/dyn/tensor/transform.cc

+        end->shape[0].as<IntImmNode>()->value == attr_size &&
+        strides->shape[0].as<IntImmNode>()->value == attr_size)
+      << "begin, end, and strides are required to have the same length"
+      << " if they are non-constant.";


The wording of this error is a bit confusing; "begin, end, and strides are required to have the same length or must all be constants" might be better

electriclilies · 2020-08-20T23:55:46Z

src/relay/op/tensor/transform.cc

-    for (int64_t i = 0; i < num_axis; ++i) {
-      oshape[i] = Any();
-    }
+    CHECK(false) << "strided_slice recieved invalid params";


You could state in this error that strided_slice received an incorrect beginning, end, or strides tensor.

electriclilies · 2020-08-21T00:05:52Z

src/relay/transforms/dynamic_to_static.cc

+        {Op::Get("dyn.strided_slice"),
+         [](const CallNode* call_node) {
+           if (const ConstantNode* begin = call_node->args[1].as<ConstantNode>()) {
+             if (const ConstantNode* end = call_node->args[2].as<ConstantNode>()) {


It would be cleaner to pull these definitions out of the if statements, and then check whether they are null or not in one if statement, though potentially slower

electriclilies · 2020-08-21T00:12:31Z

tests/python/relay/test_op_level4.py

@@ -343,7 +337,7 @@ def verify(dshape, begin, end, strides, output, slice_mode="end",
        text = func.astext()
        assert "begin=" in text
        assert "end=" in text
-
+        


white space!!

mbrookhart · 2020-08-21T16:49:08Z

The unit test error appears to be a bug with the cython backend, we're getting int64t overflow if we use cython, but not with ctypes.

mbrookhart · 2020-08-21T21:28:05Z

The failing unit test is a symptom of the bug fixed here: #6321

I'll rebase this PR once that's merged.

yongwww

Overall lgtm, thanks for migrating to dyn module.

yongwww · 2020-08-24T05:52:46Z

python/tvm/relay/op/_transform.py

@@ -165,6 +138,8 @@ def _strided_slice_shape_func_input_shape(data_shape, begin, end, strides, slice
            cstride = int64(strides[i])
        if len(begin) > i:
            cbegin = int64(begin[i])
+            if cbegin < 0:
+                cbegin += int64(data_shape[i])


how about cbegin is still less than 0 after adding data_shape[i]? probably raise error for invalid data input.

I attempted to do this with an assert in the hybrid script, but something seems a little off in the compiler, even after these lines it was still checking against the original value.

yongwww · 2020-08-24T05:59:53Z

python/tvm/relay/op/transform.py

+            end = const(list(end))
+        if isinstance(strides, (tuple, list)):
+            strides = const(list(strides))
+        normalized_begin = _make.where(begin < cast_like(const(0), begin),


we could consider moving the normalization step into strided_slice

Hmm, yeah, seems a little odd to produce a subgraph as part of the constructor of an op, which is why I put it here. That being said, this makes the op less useful from other frontends, so...

Any other votes?

How does this make it less useful?

We'd have to reproduce this on any frontend that creates ops.

yongwww · 2020-08-24T06:03:53Z

src/relay/op/tensor/transform.cc

-    for (int64_t i = 0; i < num_axis; ++i) {
-      oshape[i] = Any();
-    }
+    CHECK(param->begin) << "strided_slice recieved invalid begin";


print the received begin value in the message?

yongwww · 2020-08-24T06:07:03Z

cc @kevinthesun @zhiics

masahi · 2020-08-24T21:38:12Z

@mbrookhart #6314 introduced another strided slice usage, you might need to update that too.

mbrookhart · 2020-08-24T21:44:21Z

I don't think so, it's already targeting the dynamic op

        # squeeze the two outputs of nms for strided_slice
        size = get_relay_op("squeeze")(nms_ret[1], axis=[1])
        data_slice = get_relay_op("squeeze")(nms_ret[0], axis=[0])

        # strided slice to get the dynamic result
        return get_relay_op("strided_slice")(data_slice, begin=_expr.const([0]),
                                             end=size, slice_mode="size")

mbrookhart · 2020-08-26T15:54:36Z

@yongwww @electriclilies Can you take another look?

yongwww

lgtm

zhiics · 2020-09-03T03:10:58Z

@mbrookhart please rebase

zhiics · 2020-09-03T03:11:39Z

python/tvm/relay/frontend/mxnet.py

-                             _expr.const(begin, dtype="int32"),
-                             _expr.const(end, dtype="int32"))
+                             begin,
+                             end)


I think this could be one-liner

…the tests iin the dyn directory

zhiics

@mbrookhart #6337 has been merged. You can try to enable the GPU tests now

mbrookhart · 2020-09-03T17:27:37Z

@mbrookhart #6337 has been merged. You can try to enable the GPU tests now

I think I'd like to do it as a separate PR after we get this merged, I don't want to conflate the strided slice op with the others too much, but I'm excited to start on this :) I'll split a branch and start playing with that.

zhiics · 2020-09-03T17:32:54Z

@mbrookhart no problem

BTW, I would suggest moving the compute to topi and make dynamic version there in the next PR. Do we need to change the topi strided_slice definition to let it use dynamic/static version?

zhiics · 2020-09-03T17:34:26Z

@masahi @electriclilies could you take another look and approve/comment?

electriclilies

LGTM!

mbrookhart · 2020-09-08T16:00:44Z

@zhiics Could you take another look?

zhiics · 2020-09-08T16:48:16Z

Thanks @mbrookhart @yongwww @masahi @electriclilies

* Dynamic Strided Slice * fix clang-format lint * remove debug print * respond to review comments * respond to yongwww's comments * fix bad rebase * revert hybrid-script assert * reformat mxnet change * use new testing api * while getting test to work with the new testing API, refactor all of the tests iin the dyn directory

electriclilies suggested changes Aug 21, 2020

View reviewed changes

ZihengJiang added the status: need review label Aug 21, 2020

mbrookhart force-pushed the mbrookhart/dynamic_strided_slice branch from 8a1719b to ad7a37f Compare August 21, 2020 16:23

mbrookhart force-pushed the mbrookhart/dynamic_strided_slice branch from ad7a37f to 311e79b Compare August 21, 2020 16:53

yongwww approved these changes Aug 24, 2020

View reviewed changes

yongwww requested changes Aug 24, 2020

View reviewed changes

Matthew Brookhart added 5 commits August 24, 2020 09:36

Dynamic Strided Slice

625cc97

fix clang-format lint

570f6f8

remove debug print

24ac192

respond to review comments

f98a29d

respond to yongwww's comments

99ac920

mbrookhart force-pushed the mbrookhart/dynamic_strided_slice branch from 311e79b to 99ac920 Compare August 24, 2020 16:52

Matthew Brookhart added 2 commits August 24, 2020 09:53

fix bad rebase

3e0c042

revert hybrid-script assert

470819c

mbrookhart mentioned this pull request Aug 27, 2020

Dynamic ONNX Importer #6351

Merged

yongwww approved these changes Sep 3, 2020

View reviewed changes

zhiics reviewed Sep 3, 2020

View reviewed changes

Matthew Brookhart added 4 commits September 3, 2020 07:37

Merge branch 'master' into mbrookhart/dynamic_strided_slice

2f8e802

reformat mxnet change

c1bddce

use new testing api

ae6bf1f

while getting test to work with the new testing API, refactor all of …

d2ddad2

…the tests iin the dyn directory

zhiics reviewed Sep 3, 2020

View reviewed changes

masahi approved these changes Sep 3, 2020

View reviewed changes

Merge branch 'master' into mbrookhart/dynamic_strided_slice

742550e

electriclilies approved these changes Sep 4, 2020

View reviewed changes

j-paulus mentioned this pull request Sep 7, 2020

Bug: Strided N-D slicing in torch model ignoring the stride argument (strided_slice) #6414

Closed

zhiics approved these changes Sep 8, 2020

View reviewed changes

zhiics merged commit eee413f into apache:master Sep 8, 2020

zhiics added status: accepted and removed status: need review labels Sep 8, 2020

j-paulus mentioned this pull request Sep 10, 2020

ONNX strided slice ignoring stride argument #6441

Closed

masahi mentioned this pull request Sep 15, 2020

[Frontend][Pytorch] Improve Pytorch frontend for object detection models #6449

Merged



		@script
		def _strided_slice_shape_func_input_data(data, begin, end, strides,

Dynamic Strided Slice #6316

Dynamic Strided Slice #6316

Conversation

mbrookhart commented Aug 20, 2020 • edited Loading

electriclilies 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

Choose a reason for hiding this comment

mbrookhart commented Aug 21, 2020

mbrookhart commented Aug 21, 2020

yongwww 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

yongwww commented Aug 24, 2020

masahi commented Aug 24, 2020

mbrookhart commented Aug 24, 2020

mbrookhart commented Aug 26, 2020

yongwww left a comment

Choose a reason for hiding this comment

zhiics commented Sep 3, 2020

Choose a reason for hiding this comment

zhiics left a comment

Choose a reason for hiding this comment

mbrookhart commented Sep 3, 2020

zhiics commented Sep 3, 2020

zhiics commented Sep 3, 2020

electriclilies left a comment

Choose a reason for hiding this comment

mbrookhart commented Sep 8, 2020

zhiics commented Sep 8, 2020

mbrookhart commented Aug 20, 2020 •

edited

Loading