[Relay] add fatal by MarisaKirisame · Pull Request #3911 · apache/tvm

MarisaKirisame · 2019-09-06T21:37:50Z

@jroesch @junrushao1994 @tqchen @wweic @vinx13 @slyubomirsky can you guys review?

slyubomirsky · 2019-09-10T01:55:40Z

Mostly looks good and it is good to have a more explicit solution to the problem of incomplete matches. A couple of issues besides the questions I've labeled:

Test cases for hashing and alpha-equal would be good too (they're dull but if anything goes wrong with those, it could be a problem in many areas)
We'd have to update various interpreters to handle fatal nodes and have test cases for those

slyubomirsky · 2019-09-11T00:38:42Z

Thanks for addressing the feedback. I think these changes are good (pending linting fixes)

MarisaKirisame · 2019-09-11T02:13:57Z

@slyubomirsky can you approve explicitly?

icemelon · 2019-09-11T04:01:20Z

+TVM_REGISTER_API("relay._make.Fatal")
+.set_body_typed(FatalNode::make);
+
+std::string NoMatchMsg() {


I feel we can just use constexpr or an enum to define some common error messages in both C++ and python. Not necessary to define such an API.
otherwise lgtm

I have to copy the string twice if it is a constant though. I feel like api is cleaner and not cumbersome to use too.

MarisaKirisame · 2019-09-12T00:33:44Z

Hi ppl, I add pytest.ini and updated tests/lint/check_file_type.py, is it ok?

slyubomirsky · 2019-09-14T00:45:39Z

Should there be an AST header on the pytest.ini? The linter is complaining about it, but should the .ini file be an exception?

wweic

lgtm.

wweic · 2019-09-18T17:09:06Z


 RELAY_DEFINE_NODE_REF(RefWrite, RefWriteNode, Expr);

+/*! \brief A fatal error has occured. Stop all execution and report with a message. */


Suggested change
/*! \brief A fatal error has occured. Stop all execution and report with a message. */
/*! \brief A fatal error has occurred. Stop all execution and report with a message. */

wweic · 2019-09-18T18:11:12Z

+ x = Var("x", l(a))
+ y = Var("y")
+ z = Var("z")
+ nil_case = Clause(PatternConstructor(nil, []), Fatal("cannot pass nil into head"))


this line is not used.

MarisaKirisame · 2019-09-18T22:53:02Z

@wweic @icemelon9 I had addressed the review comment, can you guys give another round of review or approve?

wweic

lgtm. could you fix the CI error?

broune · 2019-10-19T00:22:17Z

I was actually just now looking at support for Asserts, so this will be useful to me (Thanks to Wei for pointing me to this PR). I had been wondering what the TVM Relay policy is on side effects? I see that there is already Ref, but I didn't find the policy on it for Relay, and I wonder what it is. Side effects do change how a lot of optimizations can be done.

(Turns out, if you Google for "TVM side effects", the result is an official message "FDA Warns Public of TVM Side Effects", so I guess there's that, too.)

wweic · 2019-10-19T03:05:39Z

@broune Thanks for bringing it up! cc @tqchen @jroesch @zhiics @icemelon9

Regarding this PR, @MarisaKirisame please rebase so I can approve.

MarisaKirisame · 2019-10-19T03:38:33Z

@broune relay is a purposefully side-effect language - it has side effect, and it is the key ingridient for our automatic differentiation algorithm.
However, most pass assume the none existence of side effect, because it turn out having ocaml style Ref is very, very hard to reason about! I always planned to add an abstracting abstract machine to relay to deal with them, but it is hard and I have no time. So, they are only half-supported, use at your own risk.

lint lint lint do make completeness check an error lint drop changes address review comment lint address review add asf header fix

tqchen · 2019-10-21T21:47:04Z

I think it would be good to introduce a thread discussing side effect. My feeling is that we want to keep most of the code functional(which makes them easy to optimize) and only introduce side effect when necessary(e.g. update rules). Side effect was not necessary for first-order AD, and for higher order ADs, we will need to find out a way to reduce as many as possible, and has not yet becomes concern.

In a gist, a large component of optimizations could be focusing on optimizing side-effect free programs. @broune seems to have a lot of thoughts for relaxing the side effect semantics. I think that makes sense especially for asserts, mainly because we only want to receive errors in boundaries of functions(which could be an entire graph) and I think we could reorder things in these cases.

jroesch · 2019-10-21T21:58:22Z

@broune maybe we can have a longer form discussion and draft an RFC together? the current policy is if you don't use ref, the program should be side-effect free. I believe this may be violated by a few ops but my goal is to avoid side effects, and make it easy to do side-effect free analysis for many use cases.

broune · 2019-10-22T02:00:35Z

I'm happy to discuss further in whichever venue. (No side effects is ideal for optimization but hard to get 100%. E.g. with dynamic shapes, most ops can have shape errors at runtime, making them side effecting unless somehow the error becomes an output in the dataflow graph. A random number generator library like TVM's tvm.contrib.random has random numbers that are different each time you call the op, which is stateful and a side effect.)

MarisaKirisame · 2019-10-22T04:55:07Z

@tqchen that's the point of the PE pass.
However, side effect appear more then in AD.
Dropout introduce randomness, which is a form of side effect - referential transparency is broken. If you copy code that has dropout, they will return different answer when run.
Batchnorm also introduce side effect, as they maintain some global variable.
My take is that, people, sooner or later, has to embrace side effect, for training is full of them.
While that is painful, we can build analysis framework using abstract interpretation to still allow ppl to write basic, first order algorithm (the abstract interpreter will take the responsibility of dealing with control flow, closure, side effect).

on another note, can somebody get this merged? this had been sitting for a long time.

wweic · 2019-10-28T21:56:42Z

@jroesch @zhiics @icemelon9 @tqchen ping.

zhiics

LGTM. @jroesch @icemelon9 please take another look as well.

tqchen · 2020-03-30T20:55:27Z

close for now due to inactive status, feel free to bring another PR to the master

slyubomirsky reviewed Sep 10, 2019
View reviewed changes

Comment threadpython/tvm/relay/expr_functor.py Outdated

slyubomirsky reviewed Sep 10, 2019
View reviewed changes

Comment threadsrc/relay/pass/type_infer.cc Outdated

slyubomirsky approved these changes Sep 11, 2019
View reviewed changes

icemelon reviewed Sep 11, 2019
View reviewed changes

wweic requested changes Sep 18, 2019
View reviewed changes

MarisaKirisame force-pushed the fatal branch from 044d46b to b54b798Compare September 18, 2019 21:49

wweic reviewed Sep 26, 2019
View reviewed changes

MarisaKirisame force-pushed the fatal branch from 096a3ba to 116bdf7Compare September 26, 2019 23:08

MarisaKirisame added 2 commits October 19, 2019 03:43

add fatal
a5cd7d2
lint lint lint do make completeness check an error lint drop changes address review comment lint address review add asf header fix

save
66c9472

MarisaKirisame force-pushed the fatal branch from 116bdf7 to 66c9472Compare October 19, 2019 04:08

wweic approved these changes Oct 21, 2019
View reviewed changes

broune mentioned this pull request Oct 22, 2019
Add support for tf.assert (as no-op) and tf.no_op to TF Relay frontend. #4172
Merged

zhiics approved these changes Oct 28, 2019
View reviewed changes

tqchen assigned icemelon and unassigned icemelon Oct 30, 2019

tqchen added the status: inactive label Mar 30, 2020

tqchen closed this Mar 30, 2020


		RELAY_DEFINE_NODE_REF(RefWrite, RefWriteNode, Expr);

		/! \brief A fatal error has occured. Stop all execution and report with a message. /

	/! \brief A fatal error has occured. Stop all execution and report with a message. /
	/! \brief A fatal error has occurred. Stop all execution and report with a message. /

Uh oh!

Conversation

MarisaKirisame commented Sep 6, 2019

Uh oh!

Uh oh!

Uh oh!

slyubomirsky commented Sep 10, 2019

Uh oh!

slyubomirsky commented Sep 11, 2019

Uh oh!

MarisaKirisame commented Sep 11, 2019

Uh oh!

icemelonSep 11, 2019

Choose a reason for hiding this comment

Uh oh!

MarisaKirisameSep 12, 2019

Choose a reason for hiding this comment

Uh oh!

MarisaKirisame commented Sep 12, 2019

Uh oh!

slyubomirsky commented Sep 14, 2019

Uh oh!

wweic left a comment

Choose a reason for hiding this comment

Uh oh!

wweicSep 18, 2019

Choose a reason for hiding this comment

Uh oh!

wweicSep 18, 2019

Choose a reason for hiding this comment

Uh oh!

MarisaKirisame commented Sep 18, 2019

Uh oh!

wweic left a comment

Choose a reason for hiding this comment

Uh oh!

broune commented Oct 19, 2019

Uh oh!

wweic commented Oct 19, 2019

Uh oh!

MarisaKirisame commented Oct 19, 2019

Uh oh!

tqchen commented Oct 21, 2019

Uh oh!

jroesch commented Oct 21, 2019

Uh oh!

broune commented Oct 22, 2019

Uh oh!

MarisaKirisame commented Oct 22, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wweic commented Oct 28, 2019

Uh oh!

zhiics left a comment

Choose a reason for hiding this comment

Uh oh!

tqchen commented Mar 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

MarisaKirisame commented Oct 22, 2019•
edited
Loading