[Parser] Parser 2.0 part 2 #6162

jroesch · 2020-07-28T22:05:17Z

This PR implements both metadata parsing and converts everything to the new diagnostic machinery.

I initially believed that the right way to parse the metadata table is to generate delayed references to the metadata table and then expand them after parsing is complete. In order to do this we need a piece of syntax i.e Expr, Type, Attrs, and so on for each meta reference. Currently this only works for Expr as I tried to define MetaRef as op right now in order to minimize changes.

This brings up a bigger long term question of how much we should rely on the meta table outside of things such as the constants or other "unprintable" values. I realize with my tokenization approach we could eagerly expand the meta references but this requires that we have the full program on hand in order to successfully parse any program fragment.

Would love to get some more thoughts and feedback cc @MarisaKirisame @joshpoll @tqchen @zhiics @electriclilies @mbrookhart @junrushao1994.

FYI cc @tmoreau89

src/parser/meta_ref.cc

src/ir/span.cc

src/parser/meta_ref.cc

tests/python/relay/test_ir_parser2.py

ANSHUMAN87

Thanks @jroesch for the PR.
As i missed your part-1 PR, i think i have lot to catch up :)
Please find below some high level comments, hope it helps!

src/parser/meta_ref.h

tests/python/relay/test_ir_parser.py

ANSHUMAN87 · 2020-07-29T08:10:52Z

include/tvm/ir/span.h

@@ -85,16 +85,26 @@ class SpanNode : public Object {
  int line;


I believe what you are trying to achieve here is the range for line[begin, end], column[begin, end].
nit: May be we can rename the variables to reflect the same.

Yeah I think I agree with @ANSHUMAN87. Maybe its better to use "begin_line", "begin_column", "end_line", "end_column"?

I think it should be begin_line, end_line, begin_column, end_column, as both values are related. Maybe even make a small range struct.

This is kind of painful to refactor due some of the Python code. I still need to update the Python bindings how about I add to tracking issue and we can do in follow up work?

include/tvm/parser/source_map.h

python/tvm/parser/__init__.py

src/parser/diagnostic.h

include/tvm/relay/adt.h

src/ir/span.cc

src/parser/diagnostic.h

src/parser/meta_ref.cc

ANSHUMAN87 · 2020-07-30T15:56:46Z

src/parser/meta_ref.cc

+  return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")


nit: As "parser.MetaRef" carry special meaning in the context, i think we can define this key at one place, and reuse it in all places. This helps if we want to change it at later point in time.

src/parser/meta_ref.cc

ANSHUMAN87 · 2020-07-30T16:04:42Z

src/parser/meta_ref.h

+ */
+Expr MetaRef(std::string type_key, uint64_t node_index);
+
+relay::Function ExpandMetaRefs(const MetaTable& meta_table, const relay::Function& mod);


mod -> func ?

yeah I will do this.

src/parser/parser.cc

jroesch · 2020-07-31T21:40:52Z

This PR is now ready for full review, if possible can people please chime in about meta-table and related decisions. I know there are still some incomplete aspects but this is already more robust then the previous parser and I would like to incrementalize instead of sending many 4kloc PRs.

junrushao · 2020-07-31T23:11:54Z

include/tvm/ir/span.h

@@ -85,16 +85,26 @@ class SpanNode : public Object {
  int line;


Yeah I think I agree with @ANSHUMAN87. Maybe its better to use "begin_line", "begin_column", "end_line", "end_column"?

junrushao · 2020-07-31T23:13:19Z

include/tvm/parser/source_map.h

+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {


if it is not like POD aggregation, I personally prefer to use class instead

Suggested change

struct Source {

class Source {

public:

junrushao · 2020-07-31T23:16:46Z

include/tvm/relay/adt.h

   */
-  TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true);
+  TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true, Span span = Span());


Maybe we should think a bit about Span(nullptr) vs Optional<Span>?

I actually don't want spans to be optional if possible, but currently we need to migrate to setting them everywhere.

include/tvm/parser/source_map.h

junrushao · 2020-07-31T23:19:38Z

include/tvm/parser/source_map.h

+  /*! \brief Construct a source from a string. */
+  TVM_DLL explicit Source(const std::string& source);
+
+  TVM_DLL Source(const Source& source) : source(source.source), line_map(source.line_map) {}


Shall we just use = default?

junrushao · 2020-07-31T23:20:55Z

include/tvm/parser/source_map.h

+class SourceMapNode : public Object {
+ public:
+  /*! \brief The source mapping. */
+  Map<SourceName, tvm::String> source_map;


nit: not much difference tho

Suggested change

Map<SourceName, tvm::String> source_map;

Map<SourceName, tvm::runtime::String> source_map;

junrushao · 2020-07-31T23:27:47Z

src/parser/diagnostic.h

-    out << std::endl;
-  }
-};
-
 /*! \brief The diagnostic level, controls the printing of the message. */
 enum DiagnosticLevel {


Use enum class if possible

src/parser/diagnostic.h

junrushao · 2020-08-01T00:41:38Z

src/parser/tokenizer.h

 #include "./token.h"

 namespace tvm {
 namespace parser {

 using namespace runtime;

+// trim from start (in place)
+static inline void ltrim(std::string& s) {  // NOLINT(*)
+  s.erase(s.begin(), std::find_if(s.begin(), s.end(), [](int ch) { return !std::isspace(ch); }));


Do you prefer to use IsWhitespace we defined below?

src/parser/token.h

mbrookhart

Some general thoughts after a first pass through the PR, I'll do a more detailed review tonight or tomorrow:

Yay for getting rid of Antlr! This looks much easier to maintain.
You've added diagnostic information via the Span object, and attempted to propagate it through the ExprMutator and ExprVisitor classes. There are a lot of passes that extend those classes without keeping span information, or directly use ExprFunctopr, as soon as we hit one of those passes, the diagnostic information will disappear, correct? Do we need to enforce valid spans at compile time to catch all of the passes?
Since the diagnostic machinery is all based on source, I guess we need a pass to convert python-constructed graphs into relay source code for diagnostic information? Does this already exist somewhere in the compilation flow?

zhiics

lgtm. only a few nitpicks

src/parser/meta_ref.cc

src/printer/text_printer.h

src/parser/parser.cc

mbrookhart

A few nitpicks. I'm happy to leave the completion of the diagnostic system discussed in my last comment for another PR, let's make sure those issues make it into the tracking issue.

python/tvm/error.py

src/parser/parser.cc

src/parser/token.h

… metadata section references.

tqchen · 2020-08-10T15:55:26Z

@jroesch please followup to address the CI error

jroesch · 2020-08-11T08:50:12Z

@tqchen this should be gtg, looks like I got passed the last few CPU tests.

zhiics

lgtm

* Add code from livestream with JK * Fix errors parsing ResNet * Parse metadata section efficiently and do most of plumbing to resolve metadata section references. * WIP * Change meta reference to an operator * Meta references now work * MetaReference expansion now works * Start working on source map and move diagnostic context * Convert tokenizer and parser to use new machinery * Kill to_json * Fix comment in type_infer.cc * Remove old parser * Rename parser tests and remove old ones * Record span end information * Convert to using spans everywhere * Add span fields back to all Relay constructors * Start passing spans * Pass spans around visitors * Format * Fix * Fix * disable reference lint from string helpers * Fix tokenizer * Fix issue with empty metadata section * Document new span fields and small tweaks * Formatting * Add span doc fields * Add format tweak * Improve errors and fix the semantic version tags in Prelude * Update gradient.rly * Clean up broken spans * Clean up parser tests and turn on previously skipped tests * Update errors to handle skipped cases * Tweak * Tweak * Format * Fix some minor issues with ADT tests * Format * Fix path * WIP * WIP * Fix ir_text_printer * format * Formatted * More formatting * Repair test cases * Fix CI * Retrigger CI

tkonolige reviewed Jul 28, 2020

View reviewed changes

src/parser/meta_ref.cc Outdated Show resolved Hide resolved

tqchen added the status: need review label Jul 28, 2020

MarisaKirisame reviewed Jul 29, 2020

View reviewed changes

src/ir/span.cc Outdated Show resolved Hide resolved

src/parser/meta_ref.cc Outdated Show resolved Hide resolved

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

ANSHUMAN87 reviewed Jul 29, 2020

View reviewed changes

ANSHUMAN87 reviewed Jul 30, 2020

View reviewed changes

jroesch force-pushed the parser-2.0-pt2 branch from 998875d to 581970a Compare July 30, 2020 23:42

jroesch marked this pull request as ready for review July 31, 2020 05:10

junrushao reviewed Aug 1, 2020

View reviewed changes

jroesch force-pushed the parser-2.0-pt2 branch from 0635f7d to ebb3aa7 Compare August 3, 2020 18:09

mbrookhart reviewed Aug 3, 2020

View reviewed changes

zhiics reviewed Aug 3, 2020

View reviewed changes

src/parser/meta_ref.cc Outdated Show resolved Hide resolved

src/printer/text_printer.h Outdated Show resolved Hide resolved

src/parser/parser.cc Outdated Show resolved Hide resolved

mbrookhart approved these changes Aug 5, 2020

View reviewed changes

python/tvm/error.py Show resolved Hide resolved

src/parser/parser.cc Outdated Show resolved Hide resolved

src/parser/parser.cc Show resolved Hide resolved

src/parser/token.h Outdated Show resolved Hide resolved

jroesch added 17 commits August 7, 2020 15:56

Add code from livestream with JK

fbe3a5b

Fix errors parsing ResNet

4c61844

Parse metadata section efficiently and do most of plumbing to resolve…

432e0d9

… metadata section references.

WIP

71dc90c

Change meta reference to an operator

b940e8b

Meta references now work

089b342

MetaReference expansion now works

a4cfb3f

Start working on source map and move diagnostic context

0cb6c85

Convert tokenizer and parser to use new machinery

4502adc

Kill to_json

39df873

Fix comment in type_infer.cc

b7dd570

Remove old parser

9968fdf

Rename parser tests and remove old ones

1113953

Record span end information

669aafc

Convert to using spans everywhere

9be4e7b

Add span fields back to all Relay constructors

ab782e6

Start passing spans

b7e7fc5

jroesch added 6 commits August 7, 2020 15:56

Fix some minor issues with ADT tests

4510899

Format

c991440

Fix path

50b517c

WIP

6d1a38d

WIP

2e7bdbd

Fix ir_text_printer

e3e5ef8

jroesch force-pushed the parser-2.0-pt2 branch from 7cb2d1e to e3e5ef8 Compare August 8, 2020 00:46

jroesch added 3 commits August 7, 2020 17:47

format

2a8d67a

Formatted

470bd86

More formatting

43dfe42

jroesch added 2 commits August 10, 2020 23:26

Repair test cases

7fa4c3f

Fix CI

dd61083

Retrigger CI

77397ba

zhiics approved these changes Aug 11, 2020

View reviewed changes

jroesch merged commit fa2213f into apache:master Aug 11, 2020

tqchen added status: accepted and removed status: need review labels Aug 12, 2020

tqchen mentioned this pull request Aug 12, 2020

Improve ANTLR Language Dependency #4495

Closed

cbalint13 mentioned this pull request Aug 12, 2020

Fix newer GCC compiler warnings. #6257

Merged

zhiics mentioned this pull request Aug 17, 2020

[BYOC][ETHOSN] Introduce the Ethos-N BYOC integration #6222

Merged

ZihengJiang mentioned this pull request Sep 25, 2020

TVM v0.7 Release Note Candidate #6486

Closed

jroesch deleted the parser-2.0-pt2 branch February 4, 2021 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Parser] Parser 2.0 part 2 #6162

[Parser] Parser 2.0 part 2 #6162

jroesch commented Jul 28, 2020

ANSHUMAN87 left a comment

ANSHUMAN87 Jul 29, 2020

junrushao Jul 31, 2020

MarisaKirisame Aug 1, 2020

jroesch Aug 4, 2020

ANSHUMAN87 Jul 30, 2020

ANSHUMAN87 Jul 30, 2020

jroesch Jul 31, 2020

jroesch commented Jul 31, 2020

junrushao Jul 31, 2020

junrushao Jul 31, 2020

junrushao Jul 31, 2020

jroesch Aug 4, 2020

junrushao Jul 31, 2020

junrushao Jul 31, 2020

junrushao Jul 31, 2020

junrushao Aug 1, 2020

mbrookhart left a comment

zhiics left a comment •

edited

Loading

mbrookhart left a comment

tqchen commented Aug 10, 2020

jroesch commented Aug 11, 2020

zhiics left a comment

		@@ -85,16 +85,26 @@ class SpanNode : public Object {
		int line;

	Map<SourceName, tvm::String> source_map;
	Map<SourceName, tvm::runtime::String> source_map;

[Parser] Parser 2.0 part 2 #6162

[Parser] Parser 2.0 part 2 #6162

Conversation

jroesch commented Jul 28, 2020

ANSHUMAN87 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

jroesch commented Jul 31, 2020

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 left a comment

Choose a reason for hiding this comment

zhiics left a comment • edited Loading

Choose a reason for hiding this comment

mbrookhart left a comment

Choose a reason for hiding this comment

tqchen commented Aug 10, 2020

jroesch commented Aug 11, 2020

zhiics left a comment

Choose a reason for hiding this comment

zhiics left a comment •

edited

Loading