[Relay][Prelude] Use the Relay parser to define the Relay prelude #3043

jroesch · 2019-04-18T06:12:19Z

The current Relay prelude is huge pain to modify and doesn't always result in correct ASTs requiring painful hand modifications each time we update it.

It would be far more useful to make use of the Relay parser to load the Prelude from disk.

This PR makes it possible to partially load a Relay Prelude from disk. The PR also includes some modifications to the Relay parser to support defining more code in Relay.

This should help motivate one piece of the final push on the Relay Text format (see #3016).

My goal is to make it possible to define a limited form of TensorArray using this in the coming days.

cc @wweic @joshpoll @MarisaKirisame

MarisaKirisame · 2019-04-18T16:01:29Z

AFAICT relay dont has a way of defining adt.
@joshpoll maybe we can add that?

slyubomirsky · 2019-04-18T22:45:09Z

Finalizing a Relay ADT syntax would be of great help to docs too

tests/python/relay/test_prelude.py

weberlo · 2019-04-19T03:53:29Z

python/tvm/relay/prelude.py

+    if mod is None:
+        mod = Module({})
+
+    file = os.path.join(__PRELUDE_PATH__, "prelude.rly")


file is a builtin. rename

weberlo · 2019-04-19T03:53:51Z

python/tvm/relay/_parser.py


-        raise ParseError("Unknown builtin type: {}".format(ident_type))
+        print(type_ident)


weberlo · 2019-04-19T03:54:16Z

python/tvm/relay/_parser.py

@@ -454,15 +467,21 @@ def visitIncompleteType(self, ctx):
        return None

    def visitIdentType(self, ctx):
+        # TODO(@jroesch) Identifier Type doesn't make sense should be Type Identifier.


Can we resolve this TODO in this PR?

python/tvm/relay/_parser.py

joshpoll

Please bump the version number.

joshpoll · 2019-04-19T05:30:14Z

python/tvm/relay/grammar/Relay.g4

@@ -111,8 +111,8 @@ expr
  // | 'debug'                                # debug
  ;

-func: 'fn'        '(' argList ')' ('->' type_)? body ;
-defn: 'def' ident '(' argList ')' ('->' type_)? body ;
+func: 'fn'        (typeParamSeq)? '(' argList ')' ('->' type_)? body ;


Suggested change

func: 'fn' (typeParamSeq)? '(' argList ')' ('->' type_)? body ;

func: 'fn' typeParamSeq? '(' argList ')' ('->' type_)? body ;

joshpoll · 2019-04-19T05:30:25Z

python/tvm/relay/grammar/Relay.g4

-func: 'fn'        '(' argList ')' ('->' type_)? body ;
-defn: 'def' ident '(' argList ')' ('->' type_)? body ;
+func: 'fn'        (typeParamSeq)? '(' argList ')' ('->' type_)? body ;
+defn: 'def' ident (typeParamSeq)? '(' argList ')' ('->' type_)? body ;


Suggested change

defn: 'def' ident (typeParamSeq)? '(' argList ')' ('->' type_)? body ;

defn: 'def' ident typeParamSeq? '(' argList ')' ('->' type_)? body ;

joshpoll · 2019-04-19T05:30:39Z

python/tvm/relay/grammar/Relay.g4

@@ -140,7 +146,7 @@ type_
  | 'Tensor' '[' shapeSeq ',' type_ ']'             # tensorType
  // currently unused
  // | identType '[' (type_ (',' type_)*)? ']'         # callType
-  | 'fn' '(' (type_ (',' type_)*)? ')' '->' type_   # funcType
+  | 'fn' (typeParamSeq)? '(' (type_ (',' type_)*)? ')' '->' type_   # funcType


Suggested change

| 'fn' (typeParamSeq)? '(' (type_ (',' type_)*)? ')' '->' type_ # funcType

| 'fn' typeParamSeq? '(' (type_ (',' type_)*)? ')' '->' type_ # funcType

tests/python/relay/test_prelude.py

python/tvm/relay/prelude.rly

slyubomirsky · 2019-05-30T01:18:33Z

@jroesch has asked me to take over this PR so I will work on addressing feedback and fixing linting errors to get this to a mergeable state soon. The next step would be to add ADTs and matching to the text format so the entire prelude can be written in the text format (I think that should be a separate PR)

slyubomirsky · 2019-05-30T01:36:10Z

Pinging reviewers: @wweic @joshpoll @MarisaKirisame

joshpoll · 2019-05-30T02:39:26Z

python/tvm/relay/grammar/Relay.g4

@@ -19,7 +19,7 @@

 grammar Relay;

-SEMVER: 'v0.0.1' ;
+SEMVER: 'v0.0.1.1' ;


Suggested change

SEMVER: 'v0.0.1.1' ;

SEMVER: 'v0.0.2' ;

This is the convention for pre-release semver.

Thanks, done

joshpoll · 2019-05-30T02:40:45Z

python/tvm/relay/grammar/Relay.g4

+typeParamSeq
+  : '[' ']'
+  | '[' ident ']'
+  | '[' ident (',' ident)+ ']'


Suggested change

| '[' ident (',' ident)+ ']'

| '[' ident (',' ident)* ']'

to merge this case and the previous one.

tuples need something different b/c they require a trailing comma

Good idea. Done

joshpoll · 2019-05-30T02:41:38Z

python/tvm/relay/grammar/Relay.g4

+typeParamSeq
+  : '[' ']'
+  | '[' ident ']'
+  | '[' ident (',' ident)+ ']'


tuples need something different b/c they require a trailing comma

joshpoll · 2019-05-30T02:42:03Z

tests/python/relay/test_ir_parser.py

@@ -28,7 +28,7 @@
 else:
    raises_parse_error = lambda x: x

-SEMVER = "v0.0.1"
+SEMVER = "v0.0.1.1"


Suggested change

SEMVER = "v0.0.1.1"

SEMVER = "v0.0.2"

joshpoll · 2019-05-30T02:44:55Z

python/tvm/relay/_parser.py

+    def visitTypeIdent(self, ctx):
+        '''
+        Handle type identifier.
+        type: (RelayParser.TypeIdentContext) -> Union[ty.TensorType, str]


this type annotation should be # type: (RelayParser.IdentTypeContext) -> Union[ty.TensorType, str] and go above and outside the doc string. cf: https://mypy.readthedocs.io/en/latest/cheat_sheet.html

Pylint complained when I had it that way

Ah, it complained because I had the annotation after the docstring, not before

joshpoll · 2019-05-30T02:46:16Z

python/tvm/relay/_parser.py

+                return ty.scalar_type(type_ident)
+
+        type_param = lookup(self.type_param_scopes, type_ident)
+        if type_param:


Suggested change

if type_param:

if type_param is not None:

Thanks, done

joshpoll · 2019-05-30T02:48:27Z

python/tvm/relay/prelude.py

+__PRELUDE_PATH__ = os.path.dirname(os.path.realpath(__file__))
+
+def load_prelude(mod=None):
+    '''Parses the portions of the Prelude written in Relay's text format and adds


newline after '''. Consider using either ''' or """ for all docstrings in this file

slyubomirsky · 2019-05-30T04:14:11Z

Thanks for the feedback. I also removed test_prelude because it is redundant, since test_adt tests the prelude functions, which would require loading the file

slyubomirsky · 2019-05-30T05:33:53Z

Fixing the previous test failure led me to realize that we have an issue with the parser flag: If we want to have the Prelude and other important Relay functionality written using the text format, should the parser be an optional flag? I changed the prelude to check whether the parser is enabled and default to the old manually written ASTs, but this does not seem like a particularly good practice, since it requires us to maintain two definitions, one of which is indeed a manually assembled AST. What do you all believe is a good approach moving forward? (addressed to the reviewers, also @jroesch and @tqchen)

jroesch · 2019-05-30T08:04:16Z

We should conditionally support generating the parser, when we check in we should generate a copy, commit it, and mark the files as binaries in my opinion, it satisfies both design tensions.

slyubomirsky · 2019-05-30T23:02:50Z

I agree that there should be a pre-generated parser available for users who do not intend to modify the grammar or parser. It would be good not to have to introduce ANTLR as a mandatory dependency for users who want to use Relay's text format. Does everyone agree that we should have a pre-generated parser committed and marked as a binary?

slyubomirsky · 2019-05-31T00:44:57Z

I've now added the parser to the repo so users can always run the parser, even if they don't have ANTLR enabled in their build. It will be easy to revert that commit if this choice is objectionable

wweic · 2019-05-31T00:47:46Z

I think having a pre-generated parser is a good idea, it will also enable running text format unit test for every tvm user without ANTLR installed.

slyubomirsky · 2019-05-31T01:11:01Z

Would appreciate input from anyone knowledgeable whether the generated parser files should have the Apache headers on them (we'd need to script for them to be appended during the build if so)

jroesch · 2019-06-03T18:25:50Z

python/tvm/relay/grammar/py2/RelayLexer.py

@@ -0,0 +1,209 @@
+# Generated from /home/sslyu/tvm/python/tvm/relay/grammar/Relay.g4 by ANTLR 4.7.2


Can you mark these all as binary files?

I did in .gitattributes, but it didn't change how they display on Github. I will try this as well: https://help.github.com/en/articles/customizing-how-changed-files-appear-on-github

weberlo · 2019-06-04T16:53:27Z

tests/python/relay/test_ir_parser.py

@@ -113,7 +98,7 @@ def test_comments():
        UNIT
    )

-@if_parser_enabled
+
 def test_int_literal():
    assert isinstance(relay.fromtext(SEMVER+"1"), relay.Constant)
    assert isinstance(relay.fromtext(SEMVER+"1").data, tvm.ndarray.NDArray)


I know this isn't the point of this PR, but can we have relay.fromtext(SEMVER + _) as a helper function for this test file? It's a bit of an eye sore.

slyubomirsky · 2019-06-04T20:50:28Z

No idea what's with the test failure but it definitely appears unrelated

…naries, and have parser tests always fire

…ache#3043) * Add ability to load Prelude from disk * Port over id * Define compose * Linting errors and style changes * Eliminate unnecessary parens * Rename identType to typeIdent (makes more sense) * Another unnecessary paren * Bump the version number for the text format * Ensure .rly (Relay text files) are permitted * Correct release number and simplify grammar rule * Correct load_prelude docstring * Corrections to _parser * Add Apache headers to prelude source file * Remove test_prelude (redundant) * Correct misleading error message * Add check that parser is enabled in Prelude * Commit pre-generated parser, ensure generated files are treated as binaries, and have parser tests always fire * Permit parser files and git attributes files * Exclude gitattributes and parser files from apache check * Another attempt at appeasing Apache audit checker * Corrections to rat-excludes * Apache should be truly appeased now * Ignore Relay parser files by name * Mark parser files as generated so they don't show up on Github * Add parsing helper function for tests * Mark parser files as not detectable

jroesch self-assigned this Apr 18, 2019

weberlo reviewed Apr 19, 2019

View reviewed changes

joshpoll suggested changes Apr 19, 2019

View reviewed changes

wweic requested changes Apr 19, 2019

View reviewed changes

tests/python/relay/test_prelude.py Outdated Show resolved Hide resolved

python/tvm/relay/prelude.rly Outdated Show resolved Hide resolved

joshpoll reviewed May 30, 2019

View reviewed changes

joshpoll suggested changes May 30, 2019

View reviewed changes

slyubomirsky force-pushed the text-prelude branch from 91fa672 to 57557ae Compare May 30, 2019 03:58

jroesch commented Jun 3, 2019

View reviewed changes

wweic approved these changes Jun 4, 2019

View reviewed changes

jroesch mentioned this pull request Jun 4, 2019

[Frontend][Onnx] Bump ONNX version #3286

Merged

weberlo reviewed Jun 4, 2019

View reviewed changes

joshpoll approved these changes Jun 4, 2019

View reviewed changes

weberlo approved these changes Jun 4, 2019

View reviewed changes

slyubomirsky force-pushed the text-prelude branch from cfc05e6 to 76e8006 Compare June 5, 2019 22:30

jroesch added 3 commits June 6, 2019 13:40

Add ability to load Prelude from disk

ef42cc2

Port over id

6ddc598

Define compose

612b7a2

slyubomirsky added 23 commits June 6, 2019 13:40

Linting errors and style changes

0a3df46

Eliminate unnecessary parens

c282ac7

Rename identType to typeIdent (makes more sense)

5598aa8

Another unnecessary paren

b6a1241

Bump the version number for the text format

fa1be6c

Ensure .rly (Relay text files) are permitted

47fe2d1

Correct release number and simplify grammar rule

093c614

Correct load_prelude docstring

e8b6f6c

Corrections to _parser

d567278

Add Apache headers to prelude source file

57d93f6

Remove test_prelude (redundant)

e7a9796

Correct misleading error message

a56f2f2

Add check that parser is enabled in Prelude

137df1a

Commit pre-generated parser, ensure generated files are treated as bi…

24d89a5

…naries, and have parser tests always fire

Permit parser files and git attributes files

b504501

Exclude gitattributes and parser files from apache check

72a98b4

Another attempt at appeasing Apache audit checker

67e7237

Corrections to rat-excludes

e2799ed

Apache should be truly appeased now

d56349d

Ignore Relay parser files by name

790d5f7

Mark parser files as generated so they don't show up on Github

e417e3e

Add parsing helper function for tests

e67b1b5

Mark parser files as not detectable

6e08ad5

slyubomirsky force-pushed the text-prelude branch from 76e8006 to 6e08ad5 Compare June 6, 2019 20:40

eqy approved these changes Jun 11, 2019

View reviewed changes

eqy merged commit c4245e3 into apache:master Jun 11, 2019

tqchen mentioned this pull request Nov 8, 2019

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

Closed

jroesch deleted the text-prelude branch February 4, 2021 04:40


		raise ParseError("Unknown builtin type: {}".format(ident_type))
		print(type_ident)

	func: 'fn' (typeParamSeq)? '(' argList ')' ('->' type_)? body ;
	func: 'fn' typeParamSeq? '(' argList ')' ('->' type_)? body ;

	defn: 'def' ident (typeParamSeq)? '(' argList ')' ('->' type_)? body ;
	defn: 'def' ident typeParamSeq? '(' argList ')' ('->' type_)? body ;

	\| 'fn' (typeParamSeq)? '(' (type_ (',' type_)*)? ')' '->' type_ # funcType
	\| 'fn' typeParamSeq? '(' (type_ (',' type_)*)? ')' '->' type_ # funcType

		@@ -0,0 +1,209 @@
		# Generated from /home/sslyu/tvm/python/tvm/relay/grammar/Relay.g4 by ANTLR 4.7.2

[Relay][Prelude] Use the Relay parser to define the Relay prelude #3043

[Relay][Prelude] Use the Relay parser to define the Relay prelude #3043

Conversation

jroesch commented Apr 18, 2019

MarisaKirisame commented Apr 18, 2019

slyubomirsky commented Apr 18, 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

joshpoll 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

slyubomirsky commented May 30, 2019 • edited Loading

slyubomirsky commented May 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky commented May 30, 2019 • edited Loading

slyubomirsky commented May 30, 2019

jroesch commented May 30, 2019

slyubomirsky commented May 30, 2019 • edited Loading

slyubomirsky commented May 31, 2019

wweic commented May 31, 2019

slyubomirsky commented May 31, 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

slyubomirsky commented Jun 4, 2019

slyubomirsky commented May 30, 2019 •

edited

Loading

slyubomirsky commented May 30, 2019 •

edited

Loading

slyubomirsky commented May 30, 2019 •

edited

Loading