[Relay] Add generic & informative Relay error reporting #2408

jroesch · 2019-01-09T22:16:05Z

This PR adds the ability to generate informative error messages for Relay programs. This works irrespective of spans, and source programs.

The new error reporting works by collecting the set of errors generated by the program, then reporting them in two separate steps.

This design enables us to collect more than a single error when processing a program and report many at the same time. We can report errors on a module using the function mod->ReportError(...).

When can then choose to render the errors to the user as an error message using mod->RenderErrors(...). This will use the reported errors to annotated the program using the text printer.
The error messages will be marked in red on the line they occurred.

You can see an example of the error reporting in action below:

Currently I have only enabled this behavior in the type inferencer, but the machinery is generic and exposed by relay::Module ideally future passes can take advantage of it.

I plan to propose and implement a generic pass & pass manager interface soon. The pass manager should make it easy to opt-in to error reporting without much work from pass authors.

I just got a basic version of this working and am still in the process of polishing the PR.

cc @ZihengJiang @MarisaKirisame @joshpoll @weberlo @tqchen @zhiics @icemelon9

src/relay/ir/module.cc

include/tvm/relay/error.h

src/relay/ir/module.cc

jroesch · 2019-01-10T00:20:32Z

This is ready for review, could everyone check it out?

include/tvm/relay/error.h

include/tvm/relay/module.h

src/relay/pass/type_infer.cc

src/relay/ir/module.cc

tqchen · 2019-01-10T04:18:14Z

I will do it tomorrow evening

include/tvm/relay/module.h

src/relay/ir/module.cc

zhiics · 2019-01-10T07:22:22Z

@jroesch I briefly went through. I will do another pass later.

tests/python/relay/test_error_reporting.py

include/tvm/relay/module.h

src/relay/ir/module.cc

tqchen · 2019-01-11T03:10:06Z

src/relay/ir/module.cc

+  // it may be good to instead report it to std::cout.
+  LOG(FATAL) << annotated_prog.str() << std::endl;
+
+  exit(1);


not necessary.

src/relay/util/rang.h

include/tvm/relay/module.h

weberlo · 2019-01-11T18:57:14Z

include/tvm/relay/error.h

+ * expression, we will default to throwing an exception with a textual representation
+ * of the error and no indication of where it occured in the original program.
+ *
+ * The latter mode is not ideal, and the goal of the new error reporting machinery is


I misread this doc comment initially, as only describing two modes. It should be "The final mode" here. My bad.

src/relay/ir/module.cc

include/tvm/relay/module.h

tqchen · 2019-01-18T01:17:02Z

src/relay/util/CPPLINT.cfg

@@ -0,0 +1 @@
+exclude_files=rang.h


not necessary anymore

tqchen · 2019-01-18T01:18:29Z

include/tvm/relay/module.h

+   *
+   * \returns The entry point function, (i.e. main).
+   */
+  Expr EntryPoint();


This name could still use more discussions.

Given that this is an one-liner module[module->entry_func], maybe we don't need this function

What about Main?

remove this

tqchen · 2019-01-18T01:19:13Z

include/tvm/relay/error_reporter.h

+#include <vector>
+
+namespace tvm {
+namespace relay {


can we put this into error.h? assuming this is part of error handling mechanism

There was a cyclic dependency issue that I was trying to avoid by splitting the files, a few places use relay::Error which ErrorReporter depends on.

include/tvm/relay/error.h

tests/python/relay/test_error_reporting.py

tqchen · 2019-01-18T01:22:19Z

This is a very crucial feature for high-level IR, I would like to invite a few more reviewers, @zhiics @icemelon9 @nishi-t please comment, if you have time.

In particular, please see if the public error reporting API makes sense to you

zhiics · 2019-01-18T02:10:21Z

@tqchen No problem. I will do one tonight.

zhiics · 2019-01-18T02:17:25Z

include/tvm/relay/error.h

 struct Error : public dmlc::Error {
-  explicit Error(const std::string &msg) : dmlc::Error(msg) {}
+  Span sp;
+  explicit Error(const std::string &msg) : dmlc::Error(msg), sp() {}


Suggested change

explicit Error(const std::string &msg) : dmlc::Error(msg), sp() {}

explicit Error(const std::string& msg) : dmlc::Error(msg), sp() {}

zhiics · 2019-01-18T03:03:26Z

src/relay/ir/error_reporter.cc

+  // it may be good to instead report it to std::cout.
+  LOG(FATAL) << annotated_prog.str() << std::endl;
+
+  exit(1);


no need to have exit after LOG(FATAL)?

LOG(FATAL) doesn't tell the compiler that it is [noreturn] causing a warning to be triggered. We can choose to omit these but in general marking unreachable code and code that doesn't return allows the compiler to better optimize.

src/relay/ir/module.cc

src/relay/pass/type_infer.cc

tests/python/relay/test_error_reporting.py

tqchen · 2019-01-25T18:17:50Z

Thank you, @jroesch @joshpoll @zhiics @MarisaKirisame ! this is now merged

junrushao · 2019-03-28T19:13:42Z

src/relay/pass/type_infer.cc

+    } else {
+      return func->body;
+    }
+  } else {


@jroesch I guess we forgot to call TypeInferencer here...?

jroesch commented Jan 9, 2019

View reviewed changes

src/relay/ir/module.cc Outdated Show resolved Hide resolved

MarisaKirisame reviewed Jan 9, 2019

View reviewed changes

include/tvm/relay/error.h Outdated Show resolved Hide resolved

include/tvm/relay/error.h Show resolved Hide resolved

src/relay/ir/module.cc Outdated Show resolved Hide resolved

src/relay/ir/module.cc Outdated Show resolved Hide resolved

jroesch changed the title ~~[Relay][WIP] Add generic & informative Relay error reporting~~ [Relay] Add generic & informative Relay error reporting Jan 10, 2019

jroesch mentioned this pull request Jan 10, 2019

[RELAY][Errors][WIP] Add descriptive errors for Relay #2341

Closed

weberlo reviewed Jan 10, 2019

View reviewed changes

joshpoll reviewed Jan 10, 2019

View reviewed changes

src/relay/pass/type_infer.cc Show resolved Hide resolved

joshpoll reviewed Jan 10, 2019

View reviewed changes

src/relay/pass/type_infer.cc Outdated Show resolved Hide resolved

joshpoll suggested changes Jan 10, 2019

View reviewed changes

src/relay/pass/type_infer.cc Outdated Show resolved Hide resolved

src/relay/pass/type_infer.cc Outdated Show resolved Hide resolved

src/relay/pass/type_infer.cc Outdated Show resolved Hide resolved

src/relay/ir/module.cc Outdated Show resolved Hide resolved

tqchen added the status: need review label Jan 10, 2019

zhiics reviewed Jan 10, 2019

View reviewed changes

zhiics reviewed Jan 11, 2019

View reviewed changes

tests/python/relay/test_error_reporting.py Show resolved Hide resolved

tqchen requested changes Jan 11, 2019

View reviewed changes

tqchen self-assigned this Jan 11, 2019

weberlo reviewed Jan 11, 2019

View reviewed changes

tqchen requested changes Jan 11, 2019

View reviewed changes

include/tvm/relay/module.h Outdated Show resolved Hide resolved

include/tvm/relay/module.h Outdated Show resolved Hide resolved

jroesch force-pushed the relay-error-reporting branch 5 times, most recently from 2a3cc59 to 7a370ea Compare January 16, 2019 23:45

tqchen requested changes Jan 18, 2019

View reviewed changes

zhiics reviewed Jan 18, 2019

View reviewed changes

MarisaKirisame approved these changes Jan 18, 2019

View reviewed changes

weberlo approved these changes Jan 18, 2019

View reviewed changes

jroesch force-pushed the relay-error-reporting branch from 1add04c to e1ec636 Compare January 18, 2019 23:53

jroesch added 11 commits January 24, 2019 15:42

Fix warning

27c21cf

Address code review

91853fe

More code review feedback

fa6eb72

Add support for rendering type relation errors in new style

2110b98

Fix up error reporting

f178806

Refactor to allow type relations to throw exceptions

8d71718

Fix CI failure

d96316a

Clean up a few impl. details and add docs

8fcc15e

Fix circular dep issue

52616a5

Fix error reporting for dmlc::Error

07d1674

Address final comments

c98bb22

jroesch force-pushed the relay-error-reporting branch from 4e32721 to c98bb22 Compare January 25, 2019 00:07

tqchen added the status: need update need update based on feedbacks label Jan 25, 2019

Remove EntryPoint

1263614

tqchen approved these changes Jan 25, 2019

View reviewed changes

tqchen merged commit 02631f6 into apache:master Jan 25, 2019

tqchen added status: accepted and removed status: need review status: need update need update based on feedbacks labels Jan 25, 2019

Anthony-Mai pushed a commit to Anthony-Mai/tvm that referenced this pull request Jan 26, 2019

[Relay] Add generic & informative Relay error reporting (apache#2408)

8818faa

Anthony-Mai pushed a commit to Anthony-Mai/tvm that referenced this pull request Jan 28, 2019

[Relay] Add generic & informative Relay error reporting (apache#2408)

a980a33

Anthony-Mai pushed a commit to Anthony-Mai/tvm that referenced this pull request Jan 28, 2019

[Relay] Add generic & informative Relay error reporting (apache#2408)

24138d3

zhiics pushed a commit to zhiics/tvm that referenced this pull request Jan 30, 2019

[Relay] Add generic & informative Relay error reporting (apache#2408)

8cdad24

ZihengJiang mentioned this pull request Feb 1, 2019

TVM 0.5 Release Note #2448

Closed

jroesch deleted the relay-error-reporting branch February 11, 2019 00:01

merrymercy pushed a commit to merrymercy/tvm that referenced this pull request Feb 18, 2019

[Relay] Add generic & informative Relay error reporting (apache#2408)

c78a067

wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019

[Relay] Add generic & informative Relay error reporting (apache#2408)

cb3f3f5

wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019

[Relay] Add generic & informative Relay error reporting (apache#2408)

01ba738

junrushao reviewed Mar 28, 2019

View reviewed changes

src/relay/pass/type_infer.cc

} else {

return func->body;

}

} else {

Copy link

Member

junrushao Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jroesch I guess we forgot to call TypeInferencer here...?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay] Add generic & informative Relay error reporting #2408

[Relay] Add generic & informative Relay error reporting #2408

jroesch commented Jan 9, 2019

jroesch commented Jan 10, 2019

tqchen commented Jan 10, 2019

zhiics commented Jan 10, 2019

tqchen Jan 11, 2019

weberlo Jan 11, 2019

tqchen Jan 18, 2019

tqchen Jan 18, 2019

jroesch Jan 21, 2019

tqchen Jan 24, 2019

tqchen Jan 18, 2019

jroesch Jan 18, 2019

tqchen commented Jan 18, 2019

zhiics commented Jan 18, 2019

zhiics Jan 18, 2019

zhiics Jan 18, 2019

jroesch Jan 18, 2019 •

edited

Loading

tqchen commented Jan 25, 2019

junrushao Mar 28, 2019

	explicit Error(const std::string &msg) : dmlc::Error(msg), sp() {}
	explicit Error(const std::string& msg) : dmlc::Error(msg), sp() {}

		@@ -0,0 +1 @@
		exclude_files=rang.h

[Relay] Add generic & informative Relay error reporting #2408

[Relay] Add generic & informative Relay error reporting #2408

Conversation

jroesch commented Jan 9, 2019

jroesch commented Jan 10, 2019

tqchen commented Jan 10, 2019

zhiics commented Jan 10, 2019

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

tqchen commented Jan 18, 2019

zhiics commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jroesch Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

tqchen commented Jan 25, 2019

Choose a reason for hiding this comment

jroesch Jan 18, 2019 •

edited

Loading