[Relay][Parser] Improve Relay parser and pretty printing, including CMAKE #2377

jroesch · 2019-01-07T01:04:36Z

As per a conversation between @tqchen from last week we decided to split up my new work on error reporting into a couple different pieces. This is the first pieces which includes some fixes and improvements to the parser and text format.

I also fixed the CMAKE formula to correctly build locally on Macbooks which have set up ANTLR via Homebrew.

cc @tqchen @wweic @joshpoll

joshpoll · 2019-01-07T01:31:25Z

cmake/modules/ANTLR.cmake

@@ -1,7 +1,9 @@
 if(USE_ANTLR)
-  if(EXISTS /usr/local/lib/antlr-4.7.1-complete.jar)
-    set(ANTLR4 "/usr/local/lib/antlr-4.7.1-complete.jar")
+  file(GLOB_RECURSE ANTLR4


Won't this find a list of all matching files? What if there are multiple jars?

fixed. see below

joshpoll · 2019-01-07T01:32:20Z

cmake/modules/ANTLR.cmake

      DEPENDS ${RELAY_PARSER_DIR}/Relay.g4
      WORKING_DIRECTORY ${RELAY_PARSER_DIR})

    add_custom_target(relay_parser ALL DEPENDS ${RELAY_PARSER})
  else()
-    message(FATAL_ERROR "Can't find ANTLR4!")
+    message(FATAL_ERROR "Can't find ANTLR4: ANTLR4=" ${ANTLR4})


What could ANTLR4 be set to if it's not defined?

When we were using exists the variable would be set, but then not work, good to just print it out.

joshpoll · 2019-01-07T01:33:06Z

python/tvm/relay/_parser.py

@@ -76,21 +78,35 @@ def lookup(scopes, name):
                return val
    return None

+def spanify(f):


Could you add a docstring?

joshpoll · 2019-01-07T01:35:47Z

python/tvm/relay/_parser.py

@@ -410,6 +440,14 @@ def visitFuncType(self, ctx):

        return ty.FuncType(arg_types, ret_type, [], None)

+    def visitGraphExpr(self, ctx):
+        graph_nid = int(ctx.LOCAL_VAR().getText()[1:])


this will fail with an internal python message if the user accidentally uses a CNAME

joshpoll · 2019-01-07T01:38:12Z

python/tvm/relay/grammar/Relay.g4

@@ -20,7 +20,7 @@ NE: '!=' ;

 opIdent: CNAME ;
 GLOBAL_VAR: '@' CNAME ;
-LOCAL_VAR: '%' CNAME ;
+LOCAL_VAR: '%' (CNAME | INT);


imo LOCAL_VAR and GRAPH_VAR should be separated at the lexing stage. This will simplify code paths in _parser.py.

+1 to have a GRAPH_VAR definition. It would be clearer to write and read the parser code.

For sure, I was just hacking to try and get errors working.

joshpoll · 2019-01-07T01:39:50Z

python/tvm/relay/grammar/Relay.g4

@@ -83,8 +83,9 @@ expr

  | ident                                     # identExpr
  | scalar                                    # scalarExpr
-  // | expr '.' INT                              # project
-  // | 'debug'                                   # debug
+  | LOCAL_VAR '=' expr ';' expr               # graphExpr


Suggested change

| LOCAL_VAR '=' expr ';' expr # graphExpr

| var '=' expr ';' expr # seq

LOCAL_VAR/GRAPH_VAR should be enforced in _parser.py. Also I think this case should be added to visitSeq, since it shares a lot of code with the other sequencing cases.

joshpoll · 2019-01-07T01:45:31Z

python/tvm/relay/grammar/Relay.g4

@@ -131,7 +132,7 @@ identType: CNAME ;
 // Float16, Float32, Float64
 // Bool

-body: '{' expr '}' ;
+body: '{' expr ';' '}' ;


This syntax change my deserve a bit of discussion. We could add it, have it be optional, or not add it at all. There original justification for not having it is that ; was a logical replacement for in for let expressions, separating it from the imperative use of semicolons. To me, imperative uses of semicolons serve to separate individual statements, whereas the functional use here indicates chaining. It also opens up the question of whether to require/allow/forbid semicolons between global definitions.

I'm a fan of using semi-colons everywhere because it makes parsing simpler. The text format should be relatively straight forward to parse.

I don't think it should go in this PR. It needs to be added in several places and test cases need to be updated. Could you open an issue?

tqchen · 2019-01-07T04:20:26Z

lint http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/PR-2377/1/pipeline

wweic · 2019-01-07T16:51:32Z

python/tvm/relay/_parser.py

@@ -410,6 +440,14 @@ def visitFuncType(self, ctx):

        return ty.FuncType(arg_types, ret_type, [], None)

+    def visitGraphExpr(self, ctx):


To be consistent with other visit* functions, add the function's type as a comment.

wweic · 2019-01-07T17:06:29Z

python/tvm/relay/_parser.py

+    if isinstance(source_name, str):
+        source_name = SourceName(source_name)
+
+    import pdb; pdb.set_trace()


remove debug statement?

wweic · 2019-01-07T17:17:40Z

python/tvm/relay/grammar/Relay.g4

@@ -20,7 +20,7 @@ NE: '!=' ;

 opIdent: CNAME ;
 GLOBAL_VAR: '@' CNAME ;
-LOCAL_VAR: '%' CNAME ;
+LOCAL_VAR: '%' (CNAME | INT);


+1 to have a GRAPH_VAR definition. It would be clearer to write and read the parser code.

jroesch · 2019-01-11T19:18:05Z

@joshpoll what else is required for this?

tqchen · 2019-01-12T20:11:49Z

Please do fix the lint errors, ping reviewers to do another round of review and when things get approved, let us merge it in

joshpoll · 2019-01-12T20:44:39Z

@jroesch you should add a doc comment for spanify and we need to fix some erroring tests I think. that's it.

joshpoll · 2019-01-12T20:46:17Z

also the changes to text_printer.cc should be reverted since ; changes are being punted

jroesch · 2019-01-12T23:57:46Z

don't we need semi-colon to parse? the whole point of this is to move towards round-tripping

joshpoll · 2019-01-13T01:37:50Z

I'm not super familiar with how the text printer works now, but it should only be necessary to print semicolons between the two expressions in a let node and after an interstitial temp var.

tqchen · 2019-01-15T18:08:32Z

@joshpoll can we remove the semicolon for now and try to make CI green?

jroesch · 2019-01-15T23:19:32Z

@joshpoll Can you open an issue about getting round-tripping working? maybe you, @wweic and I can work on a solution.

wweic · 2019-01-16T13:30:31Z

python/tvm/relay/_parser.py

@@ -172,6 +205,7 @@ def visitTerminal(self, node):
                raise ParseError("Unrecognized BOOL_LIT: `{}`".format(node_text))

        else:
+            print(node_text)


wweic · 2019-01-16T13:33:10Z

tests/python/relay/test_ir_parser.py

@@ -48,6 +49,11 @@
    "float16x4",
 }

+def parses_as(code, expr):
+    # type: (str, relay.Expr) -> bool
+    print(relay.fromtext(SEMVER + "\n" + code))


wweic · 2019-01-16T13:39:11Z

python/tvm/relay/_parser.py

@@ -420,6 +499,14 @@ def visitFuncType(self, ctx):

        return ty.FuncType(arg_types, ret_type, [], None)

+    def visitGraphExpr(self, ctx):


it doesn't seem like antlr will generate this function.

tqchen · 2019-01-16T23:13:06Z

@joshpoll @wweic, please take another look and https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

wweic

LGTM. Contingent on successful integration tests pass.

tqchen · 2019-01-17T16:57:32Z

Thanks to @wweic @jroesch @joshpoll , this is now merged

…MAKE (apache#2377)

joshpoll suggested changes Jan 7, 2019

View reviewed changes

wweic reviewed Jan 7, 2019

View reviewed changes

jroesch changed the title ~~Improve Relay parser and pretty printing, including CMAKE~~ [Relay][Parser] Improve Relay parser and pretty printing, including CMAKE Jan 12, 2019

joshpoll force-pushed the relay-parser-pp branch from f0f25d6 to 9213897 Compare January 15, 2019 21:14

jroesch and others added 9 commits January 15, 2019 15:17

Improve parser and pretty printing, including build

bd55406

fix cmake file for multiple antlr jars

73015bc

split up graph and local vars

c07eba1

int -> nat

c7e09b9

function attributes for parser

8d4445f

add semver to all programs. temporarily disable currying

a92a668

Fix linting

3cdfd8a

Address lint

cfde2f7

Add doc comment

a6e0b96

jroesch force-pushed the relay-parser-pp branch from 9213897 to a6e0b96 Compare January 15, 2019 23:18

jroesch and others added 3 commits January 15, 2019 15:21

Remove semi-colons

ea61b8b

fix parser tests. fix _parser type comments and misc changes

5dbc2af

multiline type annotation to make linter happy

9eb5861

wweic reviewed Jan 16, 2019

View reviewed changes

joshpoll added 2 commits January 16, 2019 15:46

remove dead code

a1c30a7

missed one

5b1b016

joshpoll approved these changes Jan 16, 2019

View reviewed changes

wweic approved these changes Jan 16, 2019

View reviewed changes

joshpoll added 2 commits January 16, 2019 19:18

fix broken parser test and improve error messages

2eb17ba

fix line length

616aa65

tqchen merged commit d274e4b into apache:master Jan 17, 2019

tqchen added the status: accepted label Jan 17, 2019

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

[Relay][Parser] Improve Relay parser and pretty printing, including C…

0acd9ae

…MAKE (apache#2377)

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

[Relay][Parser] Improve Relay parser and pretty printing, including C…

0680b68

…MAKE (apache#2377)

jroesch deleted the relay-parser-pp branch February 11, 2019 00:01

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

[Relay][Parser] Improve Relay parser and pretty printing, including C…

20425ad

…MAKE (apache#2377)

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

[Relay][Parser] Improve Relay parser and pretty printing, including C…

ba089d7

…MAKE (apache#2377)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay][Parser] Improve Relay parser and pretty printing, including CMAKE #2377

[Relay][Parser] Improve Relay parser and pretty printing, including CMAKE #2377

jroesch commented Jan 7, 2019

joshpoll Jan 7, 2019

joshpoll Jan 9, 2019

joshpoll Jan 7, 2019

jroesch Jan 11, 2019

joshpoll Jan 7, 2019

joshpoll Jan 7, 2019

joshpoll Jan 7, 2019

wweic Jan 7, 2019

jroesch Jan 8, 2019

joshpoll Jan 7, 2019

joshpoll Jan 7, 2019

jroesch Jan 8, 2019

joshpoll Jan 9, 2019

tqchen commented Jan 7, 2019

wweic Jan 7, 2019

wweic Jan 7, 2019

wweic Jan 7, 2019

jroesch commented Jan 11, 2019

tqchen commented Jan 12, 2019

joshpoll commented Jan 12, 2019

joshpoll commented Jan 12, 2019

jroesch commented Jan 12, 2019

joshpoll commented Jan 13, 2019 •

edited

Loading

tqchen commented Jan 15, 2019

jroesch commented Jan 15, 2019

wweic Jan 16, 2019

wweic Jan 16, 2019

wweic Jan 16, 2019

tqchen commented Jan 16, 2019 •

edited

Loading

wweic left a comment

tqchen commented Jan 17, 2019

	\| LOCAL_VAR '=' expr ';' expr # graphExpr
	\| var '=' expr ';' expr # seq

		@@ -410,6 +440,14 @@ def visitFuncType(self, ctx):

		return ty.FuncType(arg_types, ret_type, [], None)

		def visitGraphExpr(self, ctx):

		@@ -420,6 +499,14 @@ def visitFuncType(self, ctx):

		return ty.FuncType(arg_types, ret_type, [], None)

		def visitGraphExpr(self, ctx):

[Relay][Parser] Improve Relay parser and pretty printing, including CMAKE #2377

[Relay][Parser] Improve Relay parser and pretty printing, including CMAKE #2377

Conversation

jroesch commented Jan 7, 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

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 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jroesch commented Jan 11, 2019

tqchen commented Jan 12, 2019

joshpoll commented Jan 12, 2019

joshpoll commented Jan 12, 2019

jroesch commented Jan 12, 2019

joshpoll commented Jan 13, 2019 • edited Loading

tqchen commented Jan 15, 2019

jroesch commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jan 16, 2019 • edited Loading

wweic left a comment

Choose a reason for hiding this comment

tqchen commented Jan 17, 2019

joshpoll commented Jan 13, 2019 •

edited

Loading

tqchen commented Jan 16, 2019 •

edited

Loading