[Relay] Add shape check for ConcatenateRel and StackRel #3699

Lyken17 · 2019-08-02T20:50:21Z

As discussed in [Relay][Concatenate] Missing shape checking for non-concat axes, it is better to add shape checking in the relay. This pull request aims to complete the missing shape checking for non-concating / non-stacking axes.

slyubomirsky · 2019-08-02T22:48:34Z

Would it be possible to have test cases/regressions?

Lyken17 · 2019-08-03T01:06:42Z

Would it be possible to have test cases/regressions?

Yes, I can add test cases. Where can I find some examples (of test cases)?

Between, currently I am using reporter->AssertEQ when performing the check in Rel. But in topi:compute, there is no reporter. How can I compare two tvm::Expr?

Lyken17 · 2019-08-05T23:50:07Z

@slyubomirsky Did you mean tests like https://github.com/dmlc/tvm/tree/master/tests/cpp?

slyubomirsky · 2019-08-06T00:11:32Z

They could be like those, or tests in Python (see tests/python/relay/test_op_level[*].py)

Lyken17 · 2019-08-06T00:25:10Z

Concatenate is a fundamental operator so it should at level 1. Is my understanding correct?

Lyken17 · 2019-08-06T00:51:01Z

@slyubomirsky I have just added a test case. Please check.

Also, I note there is no check to stack relay. Consider the similar functionality of stack / concatenate, do you think it is necessary to add one?

zhiics · 2019-08-06T20:04:51Z

tests/python/relay/test_op_level1.py

+        y = relay.var('p2', shape=(2, 3))
+        c = relay.concatenate([x, y], axis=0)
+        func = relay.Function([x, y], c)
+        relay_module = relay.Module.from_expr(func)


Are you implicitly inferring type here? Should we have some assertions?

Can you explain more here? Current action here is similar to run_infer_type() used in other tests.

Yeah, I am saying that type inference implicit here when creating a module. We probably prefer to invoke run_infer_type directly and check the expected type.

Fixed. But one weird thing I met is, if I use run_infer_type(), then no error is thrown

# no error x = relay.var('p1', shape=(2, 5)) y = relay.var('p2', shape=(2, 3)) z = relay.concatenate([x, y], axis=1) zz = run_infer_type(z) # throws tvm.Error x = relay.var('p1', shape=(2, 5)) y = relay.var('p2', shape=(2, 3)) z = relay.concatenate([x, y], axis=1) func = relay.Function([x, y], c) zz = run_infer_type(func)

While run_infer_type() is

def run_infer_type(expr): mod = relay.Module.from_expr(expr) mod = transform.InferType()(mod) entry = mod["main"] return entry if isinstance(expr, relay.Function) else entry.body

It seems we have to pack it into a relay.Function then the error can be detected.

That seems very strange, since module.from_expr is supposed to pack a non-function into a function anyway (see https://github.com/dmlc/tvm/blob/master/src/relay/ir/module.cc#L232).

I checked out your branch and tried running the example you gave myself and did not get an error in either case, so I'm not sure what problem you might have had. Was there a problem before that you fixed?

src/relay/op/tensor/transform.cc

slyubomirsky · 2019-08-07T05:43:49Z

Thanks for writing tests, I will look it over when I get a chance.

slyubomirsky · 2019-08-08T01:56:59Z

Looks good to me, especially after Zhi's recommended changes. I think there should be a test for stack, since it was also modified by these changes (definitely an oversight that there wasn't one already).

tqchen · 2019-09-13T20:52:27Z

ping @Lyken17 can you look into the CI error?

Lyken17 · 2019-09-15T02:17:37Z

@tqchen Sorry the delay I will have a look at the CI error.

Seems the change lead to doc build failed on ssd example. But I cannot reproduce on local laptop.

generating gallery for tutorials/frontend... [  8%] from_onnx.py

generating gallery for tutorials/frontend... [ 16%] from_coreml.py

generating gallery for tutorials/frontend... [ 25%] deploy_ssd_gluoncv.py


Segmentation fault: 11



Stack trace:

  [bt] (0) /usr/local/lib/python3.6/dist-packages/mxnet/libmxnet.so(+0x2b64150) [0x7f71ead82150]

  [bt] (1) /lib/x86_64-linux-gnu/libc.so.6(+0x354b0) [0x7f72814114b0]

  [bt] (2) /workspace/build/libtvm.so(tvm::relay::GetValidCountRel(tvm::Array<tvm::relay::Type, void> const&, int, tvm::Attrs const&, tvm::relay::TypeReporter const&)+0x18b) [0x7f72651511eb]

  [bt] (3) /workspace/build/libtvm.so(std::_Function_handler<void (tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*), void tvm::runtime::TypedPackedFunc<bool (tvm::Array<tvm::relay::Type, void> const&, int, tvm::Attrs const&, tvm::relay::TypeReporter const&)>::AssignTypedLambda<bool (*)(tvm::Array<tvm::relay::Type, void> const&, int, tvm::Attrs const&, tvm::relay::TypeReporter const&)>(bool (*)(tvm::Array<tvm::relay::Type, void> const&, int, tvm::Attrs const&, tvm::relay::TypeReporter const&))::{lambda(tvm::runtime::TVMArgs const&, tvm::runtime::TVMRetValue*)#1}>::_M_invoke(std::_Any_data const&, tvm::runtime::TVMArgs&&, tvm::runtime::TVMRetValue*&&)+0xd4) [0x7f7265008dd4]

  [bt] (4) /workspace/build/libtvm.so(tvm::relay::TypeSolver::Solve()+0x4e1) [0x7f7265176571]

  [bt] (5) /workspace/build/libtvm.so(tvm::relay::TypeInferencer::Infer(tvm::relay::Expr)+0x55) [0x7f7265183cc5]

  [bt] (6) /workspace/build/libtvm.so(tvm::relay::InferType(tvm::relay::Function const&, tvm::relay::Module const&, tvm::relay::GlobalVar const&)+0x33f) [0x7f726518513f]

  [bt] (7) /workspace/build/libtvm.so(tvm::relay::ModuleNode::Add(tvm::relay::GlobalVar const&, tvm::relay::Function const&, bool)+0x344) [0x7f7264fffc04]

  [bt] (8) /workspace/build/libtvm.so(tvm::relay::ModuleNode::FromExpr(tvm::relay::Expr const&, tvm::Map<tvm::relay::GlobalVar, tvm::relay::Function, void, void> const&, tvm::Map<tvm::relay::GlobalTypeVar, tvm::relay::TypeData, void, void> const&)+0x18f) [0x7f7265001fbf]

make: *** [html] Error 255

Makefile:70: recipe for target 'html' failed

script returned exit code 2

tqchen · 2019-09-16T17:04:44Z

could it has something to do with the mxnet version? The error seems does correlates with the concat op if that helps

Lyken17 · 2019-09-17T04:08:20Z

@tqchen I have resolved the CI issue. Can you have a check?

Lyken17 · 2019-09-18T17:57:52Z

seems tianqi is busy. @zhiics @jroesch @slyubomirsky can u have a check?

src/relay/op/tensor/transform.h

src/relay/op/tensor/transform.cc

zhiics

LGTM

* [Relay] add shape check for concat * [Relay] add shape check for stack * add test case for shape mismatch * [typo] add the missing assert * fix lint errors. * replace int with size_t. * statically cast param->axis to size_t. * switch to run_infer_type. * fix checking for negative index * add static_cast for param->axis * merge to latest tvm * fix lint error * Fix an error with negative index. * Update transform.h * Update transform.cc

Lyken17 added 2 commits August 2, 2019 16:40

[Relay] add shape check for concat

b45fcb5

[Relay] add shape check for stack

742310f

add test case for shape mismatch

d49a76c

Lyken17 added 2 commits August 5, 2019 20:51

[typo] add the missing assert

45ea7f2

fix lint errors.

82fe6ee

zhiics requested changes Aug 6, 2019

View reviewed changes

Lyken17 added 3 commits August 6, 2019 16:47

replace int with size_t.

6c03ceb

statically cast param->axis to size_t.

582d089

switch to run_infer_type.

af3dc53

jroesch added status: need update need update based on feedbacks status: need review labels Sep 1, 2019

Lyken17 and others added 5 commits September 16, 2019 17:41

fix checking for negative index

c7b370d

Merge branch 'master' into master

9e6e2be

add static_cast for param->axis

2308290

merge to latest tvm

daeb7fe

fix lint error

754fded

zhiics reviewed Sep 18, 2019

View reviewed changes

src/relay/op/tensor/transform.h Outdated Show resolved Hide resolved

src/relay/op/tensor/transform.cc Outdated Show resolved Hide resolved

Lyken17 added 3 commits September 18, 2019 15:42

Fix an error with negative index.

115a187

Update transform.h

3d4e4a9

Update transform.cc

8641b3d

zhiics approved these changes Sep 18, 2019

View reviewed changes

zhiics merged commit cdbf4d8 into apache:master Sep 18, 2019

zhiics added status: accepted and removed status: need review status: need update need update based on feedbacks labels Sep 18, 2019

tqchen mentioned this pull request Nov 8, 2019

[RELEASE][DRAFT] TVM v0.6 Release candidate #4259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay] Add shape check for ConcatenateRel and StackRel #3699

[Relay] Add shape check for ConcatenateRel and StackRel #3699

Lyken17 commented Aug 2, 2019 •

edited

Loading

slyubomirsky commented Aug 2, 2019

Lyken17 commented Aug 3, 2019

Lyken17 commented Aug 5, 2019

slyubomirsky commented Aug 6, 2019 •

edited

Loading

Lyken17 commented Aug 6, 2019

Lyken17 commented Aug 6, 2019

zhiics Aug 6, 2019

Lyken17 Aug 6, 2019

zhiics Aug 6, 2019

Lyken17 Aug 6, 2019

slyubomirsky Aug 8, 2019 •

edited

Loading

slyubomirsky Aug 8, 2019 •

edited

Loading

slyubomirsky commented Aug 7, 2019

slyubomirsky commented Aug 8, 2019 •

edited

Loading

tqchen commented Sep 13, 2019

Lyken17 commented Sep 15, 2019

tqchen commented Sep 16, 2019

Lyken17 commented Sep 17, 2019

Lyken17 commented Sep 18, 2019

zhiics left a comment

[Relay] Add shape check for ConcatenateRel and StackRel #3699

[Relay] Add shape check for ConcatenateRel and StackRel #3699

Conversation

Lyken17 commented Aug 2, 2019 • edited Loading

slyubomirsky commented Aug 2, 2019

Lyken17 commented Aug 3, 2019

Lyken17 commented Aug 5, 2019

slyubomirsky commented Aug 6, 2019 • edited Loading

Lyken17 commented Aug 6, 2019

Lyken17 commented Aug 6, 2019

zhiics Aug 6, 2019

Choose a reason for hiding this comment

Lyken17 Aug 6, 2019

Choose a reason for hiding this comment

zhiics Aug 6, 2019

Choose a reason for hiding this comment

Lyken17 Aug 6, 2019

Choose a reason for hiding this comment

slyubomirsky Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

slyubomirsky Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

slyubomirsky commented Aug 7, 2019

slyubomirsky commented Aug 8, 2019 • edited Loading

tqchen commented Sep 13, 2019

Lyken17 commented Sep 15, 2019

tqchen commented Sep 16, 2019

Lyken17 commented Sep 17, 2019

Lyken17 commented Sep 18, 2019

zhiics left a comment

Choose a reason for hiding this comment

Lyken17 commented Aug 2, 2019 •

edited

Loading

slyubomirsky commented Aug 6, 2019 •

edited

Loading

slyubomirsky Aug 8, 2019 •

edited

Loading

slyubomirsky Aug 8, 2019 •

edited

Loading

slyubomirsky commented Aug 8, 2019 •

edited

Loading