[Relay][Prelude] Use the Relay parser to define the Relay prelude by jroesch · Pull Request #3043 · apache/tvm

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

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

+    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

+            return type_param

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


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

        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?

joshpoll

Please bump the version number.

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


-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

-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

  // 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

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

 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

+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

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


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

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

    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

+    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

+                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

+__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

@@ -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

+
 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

Comment thread tests/python/relay/test_prelude.py Outdated

Comment thread python/tvm/relay/prelude.rly Outdated

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

	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

Uh oh!

Conversation

jroesch commented Apr 18, 2019

Uh oh!

MarisaKirisame commented Apr 18, 2019

Uh oh!

slyubomirsky commented Apr 18, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshpoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

slyubomirsky commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slyubomirsky commented May 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slyubomirsky commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slyubomirsky commented May 30, 2019

slyubomirsky commented May 30, 2019 •

edited

Loading

slyubomirsky commented May 30, 2019 •

edited

Loading

slyubomirsky commented May 30, 2019 •

edited

Loading