[Relay][Testing] Relay-to-Python compilation #3156

slyubomirsky · 2019-05-09T02:50:13Z

For a simple testing utility, @jroesch and I decided that it would be useful to have a translation of Relay programs into equivalent Python code that we could use as an oracle for fuzzing.

Here I have written an attempt at such a translation. The Python code it produces is meant to match the intended semantics of Relay very closely rather than be performant, so there are some ineffeciencies that a smarter implementation could eliminate through analysis (translating let bindings as functions, for example). Again, the point is for the Python code to be evidently correct rather than an implementation that we would use for anything other than testing purposes.

There are a couple of open issues though:

Tests for the translation
Handling ASTs

slyubomirsky · 2019-05-09T02:59:08Z

There's a rather annoying issue in handling ADTs: ConstructorValue uses a relay.Constructor object for representing ADT values -- if we want to use this translation for differential testing, that would require those constructor objects to match by pointer equality!

This makes it really hard to build ADT values in this translation. Granted, I could pass the Relay module as a variable directly to exec() and get the constructors from the module that way*. However, it seems better in the long run for us to take this opportunity to decide on a better representation for ConstructorValue that does not rely on having objects from the module in the value representation.

A simple approach would be a string tag (e.g., '{name of ADT}_{name of constructor}_{index in ADT definition}'), but there are other choices. I think simplifying the representation would be better for future interoperability (like with @MarisaKirisame's ahead-of-time compiler).

*Now that I've thought of it, I'll hack in this variant but I would prefer that we change the representation

slyubomirsky · 2019-05-09T03:00:05Z

conda/tvm/meta.yaml

@@ -41,6 +41,7 @@ requirements:
    - python
    - {{ pin_compatible('numpy') }}
    - decorator
+    - astor


Could someone more experienced with managing TVM's dependencies tell me if this is the right manner to indicate a new Python dependency? astor is only needed for testing (I could possibly exclude it entirely, though it would be convenient to have it)

Nevermind, @tqchen suggested it's better to exclude the new dependency if it's not used directly in tests (it shouldn't be; astor was only meant for debugging)

slyubomirsky · 2019-05-09T03:14:14Z

I would appreciate reviews, if possible: @jroesch, @MarisaKirisame , @zhiics, @wweic

MarisaKirisame · 2019-05-09T06:02:14Z

I still dont get the problem. Cant you just take that constructor value? I thought they are only constructed when adt is decclard.

MarisaKirisame · 2019-05-09T06:04:32Z

LGTM

slyubomirsky · 2019-05-09T06:43:49Z

Well it's not a "problem" in that I can pass the module into the generated Python program (as I describe in my above comment), but I would prefer not to have to do that and I think it would make it easier in the future to write code that has to produce or accept values produced by Relay.

wweic · 2019-05-09T16:56:46Z

I'll post a review today.

zhiics

Looks good to me so far. I will do a more careful review later. Please add some tests. Curious, it looks that ADT is missing here. How would you plan to support them as well?

zhiics · 2019-05-09T21:14:40Z

python/tvm/relay/testing/py_converter.py

+        return name
+
+
+    # returns an variable AST node for the given Relay var depending on


Suggested change

# returns an variable AST node for the given Relay var depending on

# returns a variable AST node for the given Relay var depending on

wweic · 2019-05-09T18:46:38Z

python/tvm/relay/testing/py_converter.py

+
+
+    # converts the given Relay function into a Python function
+    def convert_func_node(self, name_hint: str, func: Expr):


Suggested change

def convert_func_node(self, name_hint: str, func: Expr):

def convert_func_node(self, name_hint: str, func: Func):

?

It is probably a good idea to add finer-grained type checks. I will address those soon.

wweic · 2019-05-09T18:56:22Z

python/tvm/relay/op/op_attrs.py

+    """Attributes for nn.global_pool"""
+
+
+@register_relay_attr_node


Should these new attributes node change in a separate PR?

I needed them for this one because of the way op calls are handled: If the attr node is not registered, then it is treated as just a tvm.NodeBase object and there is thus no keys() method or any way to iterate through the attribute values.

I no longer depend on these changes so I will move them to another PR as requested. Included here: #3175

wweic · 2019-05-09T22:10:42Z

python/tvm/relay/testing/py_converter.py

+def run_as_python(expr: Expr, mod=relay.Module()):
+    '''Converts the given Relay expression into a Python script and
+    executes it.'''
+    py_ast = to_python_ast(expr, mod)


Where is to_python_ast defined?

That was a mistake, it should be calling to_python above

wweic · 2019-05-09T22:13:00Z

python/tvm/relay/testing/py_converter.py

+        return (ast.Name(id=gvar.name_hint, ctx=ast.Load()), [])
+
+
+    def visit_let(self, letexp: Expr):


add a unit test

wweic · 2019-05-09T22:31:44Z

python/tvm/relay/testing/py_converter.py

+        return (ast.Attribute(ref, attr='value', ctx=ast.Load()), defs)
+
+
+    def visit_ref_write(self, write: Expr):


Add a unit test

slyubomirsky · 2019-05-09T22:35:07Z

Yes, I will be adding ADTs today, along with tests. Thanks for the reviews

slyubomirsky · 2019-05-10T00:34:24Z

Unfortunately it looks like Python's ast library is really insistent that all expr and statement nodes include a lineno and col_offset attribute, which means I will either have to manually construct the whole translated program as AST nodes or construct it as a string and parse it, but not mix parsing and manually assembled ASTs (the line numbers and column offsets won't be consistent). I will see which reworking will be easier (probably the manually constructed ASTs)

slyubomirsky · 2019-05-10T02:49:31Z

Next on the agenda: adding tests, as I have resolved the issue of lineno and col_offset and have all of Relay's features represented.

slyubomirsky · 2019-05-10T22:31:02Z

@MarisaKirisame I followed the approach of your AoT compiler for lowering op calls, I would appreciate if you could look at that code. This has let me eliminate the call to Relay's interpreter from inside the generated code, so I will now be able to move the changes to attr nodes to their own PR because I don't use them here.

MarisaKirisame · 2019-05-10T23:46:37Z

@slyubomirsky I will looked later today/tmr morning.
Also, what is the reason behind calling this a translation instead of compilation? This is clearly a compilation and I think we should call it as such.

slyubomirsky · 2019-05-11T08:28:39Z

Yeah, fair enough on calling it a compilation. I called it a translation because I expected it to be much more modest in scope than it ultimately was. I can rename the file accordingly

slyubomirsky · 2019-05-15T00:33:37Z

Now I have tests running for just about every feature of the translation; I will continue to add them but think it's worth looking over in greater detail. The conversion has ended up being substantially more complicated than I was hoping it would be, so I hope it is not self-defeating in that regard.

slyubomirsky · 2019-05-15T00:57:22Z

@MarisaKirisame do you know if there's any way I could avoid running fusion but still be able to lower operators? I'd prefer this compiler not to have to depend on complicated Relay passes because its purpose is to test Relay

MarisaKirisame · 2019-05-15T19:20:43Z

@slyubomirsky fuse with opt-level = 0

slyubomirsky · 2019-05-16T00:09:33Z

Good idea, I'll do that too.

python/tvm/relay/module.py

tqchen · 2019-05-30T16:31:02Z

@zhiics @wweic please do another round of review and https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

zhiics · 2019-05-30T20:22:12Z

Sorry for missing this. I will do another review tonight.

wweic

lgtm. some minor comments.

wweic · 2019-05-31T01:07:01Z

python/tvm/relay/testing/py_converter.py

+        return name
+
+
+    # returns n variable AST node for the given Relay var depending on


Suggested change

# returns n variable AST node for the given Relay var depending on

# returns a variable AST node for the given Relay var depending on

wweic · 2019-05-31T05:26:51Z

python/tvm/relay/testing/py_converter.py

+    return converter.convert(expr)
+
+
+def run_as_python(expr: Expr, mod=relay.Module(), target=tvm.target.create('llvm')):


set mod to None and initialize it to relay..Module() in function body so every expression is evaluated in a fresh module. https://stackoverflow.com/a/33573262

Ah, very good to know! I will change it

zhiics

Only some nitpicks, otherwise ltgm.

zhiics · 2019-05-31T06:40:37Z

python/tvm/relay/testing/py_converter.py

+        return ast.fix_missing_locations(ast.Module(body=body))
+
+
+    def optimize(self, prog: Expr):


Should we add a todo here to move these passes to the pass manager?

Ah, I haven't been in the loop on that. If the pass manager is the right way to do these, then we should use it

It's not merged yet... The TODO just helps us to track it.

zhiics · 2019-05-31T06:42:30Z

python/tvm/relay/testing/py_converter.py

+        return re.sub(r'\W', '', name)
+
+
+    # generates a unique variable name starting from the hint


Should we keep the doc format consistent, i.e. moving them under def?
Same to the functions below.

For things that were meant as purely internal functions, I just put the comments on top but I will change it to be consistent, thanks for the feedback.

slyubomirsky · 2019-05-31T22:30:35Z

Thank you for the feedback!

wweic

thanks.

…module)

…ccommodate necessary demands

@MarisaKirisame

* First pass at Relay-to-Python converter testing utility * Indicate astor as a dependency * Add astor dep to host as well * Typos and small bugs * Handle ADTs and matching in Python conversion * Remove any dependency on ast.parse * Eliminate unnecessary type var field in Python version of ConstructorValue (already gone on C++ side) * Update constructor value, fix syntax errors * Don't forget keywords arg on Call nodes * Fix some incorrect calls to ast nodes * Fix more calls, a little more cleaning up * Missing cases in attr conversion * Lower op calls instead of running them through interpreter, as in @MarisaKirisame's AoT compiler * We do still need the module * Remove changes to op attrs: Will PR separately * Smoke test and corrections * More tests and fixes * Ensure imports are properly global in generated Python code * Add unit tests for refs * Add unit test for tuple indexing * Add unit test for if expression * Remove astor dependency * Remove astor from meta.yaml too * Fix if test and add basic local function test * Add global function test, refactor earlier tests * Correct 'clause' field in ADT so Python and C++ field names match * More fixes and tests for matching and constructors * Dramatically simplify matching: no need for a thunk * Improve ref writing test * Ensure local recursion works * cleanup * Add test for global recursion * Add test for higher-order calls * Get ops working, add basic tests * Remove accidentally duplicated test * More docstrings to appease pylint * Forgot to fix a test using constructor values * Reduce optimization level in fusion and fix tuple input to operators * Test op with tuple output, fix tuple output code * Add unit test for batch norm * Add a couple more tricky test cases * Correct nat constructor to drop unnecessary field * Fix the op attrs file (accidentally reduced it) * Address review comments * Adapt to new ConstructorValue representation (no more runtime dep on module) * Use pass manager and updated interfaces. Extend module.from_expr to accommodate necessary demands * Use sequential return value * Lift out nested conditionals * Replace triple single quotes with triple double quotes * Use main variable instead of entry_func

@MarisaKirisame

* First pass at Relay-to-Python converter testing utility * Indicate astor as a dependency * Add astor dep to host as well * Typos and small bugs * Handle ADTs and matching in Python conversion * Remove any dependency on ast.parse * Eliminate unnecessary type var field in Python version of ConstructorValue (already gone on C++ side) * Update constructor value, fix syntax errors * Don't forget keywords arg on Call nodes * Fix some incorrect calls to ast nodes * Fix more calls, a little more cleaning up * Missing cases in attr conversion * Lower op calls instead of running them through interpreter, as in @MarisaKirisame's AoT compiler * We do still need the module * Remove changes to op attrs: Will PR separately * Smoke test and corrections * More tests and fixes * Ensure imports are properly global in generated Python code * Add unit tests for refs * Add unit test for tuple indexing * Add unit test for if expression * Remove astor dependency * Remove astor from meta.yaml too * Fix if test and add basic local function test * Add global function test, refactor earlier tests * Correct 'clause' field in ADT so Python and C++ field names match * More fixes and tests for matching and constructors * Dramatically simplify matching: no need for a thunk * Improve ref writing test * Ensure local recursion works * cleanup * Add test for global recursion * Add test for higher-order calls * Get ops working, add basic tests * Remove accidentally duplicated test * More docstrings to appease pylint * Forgot to fix a test using constructor values * Reduce optimization level in fusion and fix tuple input to operators * Test op with tuple output, fix tuple output code * Add unit test for batch norm * Add a couple more tricky test cases * Correct nat constructor to drop unnecessary field * Fix the op attrs file (accidentally reduced it) * Address review comments * Adapt to new ConstructorValue representation (no more runtime dep on module) * Use pass manager and updated interfaces. Extend module.from_expr to accommodate necessary demands * Use sequential return value * Lift out nested conditionals * Replace triple single quotes with triple double quotes * Use main variable instead of entry_func

@MarisaKirisame

* First pass at Relay-to-Python converter testing utility * Indicate astor as a dependency * Add astor dep to host as well * Typos and small bugs * Handle ADTs and matching in Python conversion * Remove any dependency on ast.parse * Eliminate unnecessary type var field in Python version of ConstructorValue (already gone on C++ side) * Update constructor value, fix syntax errors * Don't forget keywords arg on Call nodes * Fix some incorrect calls to ast nodes * Fix more calls, a little more cleaning up * Missing cases in attr conversion * Lower op calls instead of running them through interpreter, as in @MarisaKirisame's AoT compiler * We do still need the module * Remove changes to op attrs: Will PR separately * Smoke test and corrections * More tests and fixes * Ensure imports are properly global in generated Python code * Add unit tests for refs * Add unit test for tuple indexing * Add unit test for if expression * Remove astor dependency * Remove astor from meta.yaml too * Fix if test and add basic local function test * Add global function test, refactor earlier tests * Correct 'clause' field in ADT so Python and C++ field names match * More fixes and tests for matching and constructors * Dramatically simplify matching: no need for a thunk * Improve ref writing test * Ensure local recursion works * cleanup * Add test for global recursion * Add test for higher-order calls * Get ops working, add basic tests * Remove accidentally duplicated test * More docstrings to appease pylint * Forgot to fix a test using constructor values * Reduce optimization level in fusion and fix tuple input to operators * Test op with tuple output, fix tuple output code * Add unit test for batch norm * Add a couple more tricky test cases * Correct nat constructor to drop unnecessary field * Fix the op attrs file (accidentally reduced it) * Address review comments * Adapt to new ConstructorValue representation (no more runtime dep on module) * Use pass manager and updated interfaces. Extend module.from_expr to accommodate necessary demands * Use sequential return value * Lift out nested conditionals * Replace triple single quotes with triple double quotes * Use main variable instead of entry_func

slyubomirsky commented May 9, 2019

View reviewed changes

MarisaKirisame approved these changes May 9, 2019

View reviewed changes

zhiics reviewed May 9, 2019

View reviewed changes

wweic requested changes May 9, 2019

View reviewed changes

slyubomirsky changed the title ~~[TVM][WIP] Relay-to-Python translation~~ [Relay][Testing] Relay-to-Python compilation May 15, 2019

tqchen added the status: need review label May 16, 2019

MarisaKirisame reviewed May 21, 2019

View reviewed changes

python/tvm/relay/module.py Outdated Show resolved Hide resolved

slyubomirsky force-pushed the python-translation branch from b6e502a to 5ffbacd Compare May 23, 2019 02:44

wweic requested changes May 31, 2019

View reviewed changes

zhiics reviewed May 31, 2019

View reviewed changes

wweic approved these changes May 31, 2019

View reviewed changes

slyubomirsky added 24 commits July 6, 2019 22:52

More fixes and tests for matching and constructors

e0284b0

Dramatically simplify matching: no need for a thunk

cae1f51

Improve ref writing test

42363e4

Ensure local recursion works

d0f0537

cleanup

c4b37a0

Add test for global recursion

bc89695

Add test for higher-order calls

ce45d77

Get ops working, add basic tests

7660332

Remove accidentally duplicated test

92f8e59

More docstrings to appease pylint

2c564a4

Forgot to fix a test using constructor values

9e3e001

Reduce optimization level in fusion and fix tuple input to operators

5949d18

Test op with tuple output, fix tuple output code

6200ceb

Add unit test for batch norm

1071050

Add a couple more tricky test cases

ea241e9

Correct nat constructor to drop unnecessary field

4130cf8

Fix the op attrs file (accidentally reduced it)

3940e0d

Address review comments

0929988

Adapt to new ConstructorValue representation (no more runtime dep on …

893d0f1

…module)

Use pass manager and updated interfaces. Extend module.from_expr to a…

7e383dc

…ccommodate necessary demands

Use sequential return value

f3d5cdb

Lift out nested conditionals

6f1c7e9

Replace triple single quotes with triple double quotes

ee4d514

Use main variable instead of entry_func

3be301d

slyubomirsky force-pushed the python-translation branch from b9d6d77 to 3be301d Compare July 7, 2019 05:54

jroesch merged commit db841c2 into apache:master Jul 10, 2019

yzhliu mentioned this pull request Nov 11, 2019

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

Closed

		return name


		# returns an variable AST node for the given Relay var depending on

	# returns an variable AST node for the given Relay var depending on
	# returns a variable AST node for the given Relay var depending on



		# converts the given Relay function into a Python function
		def convert_func_node(self, name_hint: str, func: Expr):

		"""Attributes for nn.global_pool"""


		@register_relay_attr_node

		return (ast.Name(id=gvar.name_hint, ctx=ast.Load()), [])


		def visit_let(self, letexp: Expr):

		return (ast.Attribute(ref, attr='value', ctx=ast.Load()), defs)


		def visit_ref_write(self, write: Expr):

		return name


		# returns n variable AST node for the given Relay var depending on

		return converter.convert(expr)


		def run_as_python(expr: Expr, mod=relay.Module(), target=tvm.target.create('llvm')):

		return ast.fix_missing_locations(ast.Module(body=body))


		def optimize(self, prog: Expr):

		return re.sub(r'\W', '', name)


		# generates a unique variable name starting from the hint

[Relay][Testing] Relay-to-Python compilation #3156

[Relay][Testing] Relay-to-Python compilation #3156

Conversation

slyubomirsky commented May 9, 2019 • edited Loading

slyubomirsky commented May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky commented May 9, 2019

MarisaKirisame commented May 9, 2019

MarisaKirisame commented May 9, 2019

slyubomirsky commented May 9, 2019 • edited Loading

wweic commented May 9, 2019

zhiics left a comment • edited Loading

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 May 10, 2019 • edited Loading

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 9, 2019 • edited Loading

slyubomirsky commented May 10, 2019

slyubomirsky commented May 10, 2019

slyubomirsky commented May 10, 2019

MarisaKirisame commented May 10, 2019

slyubomirsky commented May 11, 2019

slyubomirsky commented May 15, 2019

slyubomirsky commented May 15, 2019

MarisaKirisame commented May 15, 2019

slyubomirsky commented May 16, 2019

tqchen commented May 30, 2019

zhiics commented May 30, 2019

wweic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky May 31, 2019 • edited Loading

Choose a reason for hiding this comment

zhiics 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

slyubomirsky May 31, 2019 • edited Loading

Choose a reason for hiding this comment

slyubomirsky commented May 31, 2019

wweic left a comment

Choose a reason for hiding this comment

slyubomirsky commented May 9, 2019 •

edited

Loading

slyubomirsky commented May 9, 2019 •

edited

Loading

slyubomirsky commented May 9, 2019 •

edited

Loading

zhiics left a comment •

edited

Loading

slyubomirsky May 10, 2019 •

edited

Loading

slyubomirsky commented May 9, 2019 •

edited

Loading

slyubomirsky May 31, 2019 •

edited

Loading

slyubomirsky May 31, 2019 •

edited

Loading