[Relay] Algebraic data types #2442

slyubomirsky · 2019-01-16T02:51:23Z

This is a rather large set of changes that will implement algebraic data types for Relay, outlined in #2175.

Note that most of this code was first written and prototyped by @MarisaKirisame . However, besides reviewing the extensive changes, there also needs to be greater test coverage and integration of ADTs into other parts of Relay (we especially anticipate changes to type unification).

Still to be done:

Decide on overall design/naming for ADTs
Decide on what should be the contents of the prelude (default ADTs) and document it
Fix remaining empty cases (e.g., codegen) and bugs (e.g., interpreter test for add)
Add test cases for ADTs in various parts of code base, esp. for interpreter

python/tvm/relay/prelude.py

include/tvm/relay/adt.h

include/tvm/relay/type.h

include/tvm/relay/adt.h

slyubomirsky · 2019-01-16T22:14:28Z

@MarisaKirisame @tqchen What would be reasonable overrides for visit_match() and visit_constructor() in python/tvm/relay/backend/graph_runtime_codegen.py? I'm unfamiliar with that code and pylint is complaining about missing overrides there. There is probably functionality to fill in for generating code for ADTs in that case

src/relay/ir/type_functor.cc

src/relay/pass/type_infer.cc

src/relay/pass/kind_check.cc

slyubomirsky · 2019-01-18T01:11:39Z

src/relay/pass/type_infer.cc

@@ -548,10 +622,23 @@ Expr TypeInferencer::Infer(Expr expr) {
  return resolved_expr;
 }

+struct AllCheckTypePopulated : ExprVisitor {


Why do we need this check, by the way? Was it a problem before?

I forgot whether problem exist before, but this is a good postcondition we should check neverthless.

include/tvm/relay/module.h

slyubomirsky · 2019-01-22T00:35:06Z

python/tvm/relay/expr.py

@@ -172,6 +172,20 @@ def name_hint(self):
        name = self.vid.name_hint
        return name

+    def __call__(self, *args):


Much of the ADT code assumed that the call operator for vars was overridden, so I added this to make those work. Is there a general position on operator overriding in the Python bindings? Is an override like this alright to have?

slyubomirsky · 2019-01-22T03:05:17Z

python/tvm/relay/backend/graph_runtime_codegen.py

@@ -283,6 +283,12 @@ def visit_call(self, call):
    def visit_op(self, _):
        raise Exception("can not compile op in non-eta expanded form")

+    def visit_constructor(self, _):


@MarisaKirisame @tqchen I am not familiar with this section of the codebase so I would need some pointers on what code should be generated for the ADT cases (tagging you because git blame showed you wrote most of this file)

we do not support codegen for adt. this just add more descriptive error.

Alright, I guess

It is impossible to run ADT using code on graph runtime. We should use the same error in all these cases making it clear we can not execute them.

Understood, I'll change the error message.

slyubomirsky · 2019-01-23T20:39:02Z

I think I've changed my mind about changing the names of GlobalTypeVar and TypeCall: it's probably better to keep the design general should we choose to add more features to our type system. For now, kind-checking can be used to ensure that these are only used for ADTs.

slyubomirsky · 2019-01-23T21:46:51Z

I've reorganized and documented the prelude. Right now it only has some very basic list functions (granted, very many common functions can be implemented from folds). We should decide on a fuller picture of what to have in the prelude. Anything besides lists? What other list functions? Etc.

MarisaKirisame · 2019-01-23T21:59:50Z

@slyubomirsky can you keep both nat and tree in extra.py, which basically fill them in prelude?
I need them for other testing as well.

slyubomirsky · 2019-01-23T22:11:13Z

Alright

e: Now they're back

slyubomirsky · 2019-01-28T21:54:05Z

@jroesch should there be an ADT counterpart to ModuleNode::FromExpr()? I'm not sure what the use case for that would be and would appreciate any clarification.

jroesch · 2019-02-01T06:06:10Z

@slyubomirsky I don't think so. My current feeling is we should get consensus on this code, merge and follow up.

jroesch · 2019-02-01T06:06:33Z

@zhiics @wweic @icemelon9 could some subset of you guys review this?

jroesch

Overall looks good to me, just a few small things and then I think we should push to merge.

jroesch · 2019-01-28T02:28:33Z

python/tvm/relay/ty.py

@@ -106,6 +122,51 @@ def __init__(self, var, kind=Kind.Type):
        self.__init_handle_by_constructor__(_make.TypeVar, var, kind)


+@register_relay_node
+class GlobalTypeVar(Type):


I think we should probably use a different name then GlobalTypeVar because it sounds like its a type variable and not a name, maybe we could just use TypeName? I'll mull it over.

I wasn't sure about the name either but think global type var is possibly a good choice because we can use it for other things down the line potentially.

jroesch · 2019-02-01T06:07:30Z

include/tvm/relay/pass.h

 *
 * \return List of free vars, in the PostDFS order visited by expr.
 */
-tvm::Array<TypeVar> FreeTypeVars(const Expr& expr);
+tvm::Array<TypeVar> FreeTypeVars(const Expr& expr, const Module& mod);


Why do we now need the module on all these functions?

For ADTs, we need to know if there are type vars in the definitions

Why is that? can you illustrate an example for me?

A constructor can contain type vars that are bound only in the type data. If someone uses the wrong type var in a constructor (i.e., one that is not bound in the type data), the FreeVars pass can catch that.

Might be a pain to have to pass the module for just this marginal case, though.

python/tvm/relay/ty.py

include/tvm/relay/adt.h

wweic

LGTM. Some cosmetic comments.

include/tvm/relay/adt.h

python/tvm/relay/adt.py

wweic · 2019-02-02T04:08:51Z

python/tvm/relay/module.py

+                    k = _ty.GlobalTypeVar(k)
+                if not isinstance(k, _ty.GlobalTypeVar):
+                    raise TypeError("Expect type_definitions to be Dict[GlobalTypeVar, Type]")
+            type_definitions = mapped_type_defs


type_definitions = {}, no need to define mapped_type_defs

Why would we not want to define it?

sorry I missed this reply. If I read correctly, lint 40 to line 44 does not change mapped_type_defs, what's the effect of the block actually?

Oh, now I understand what you meant. I will change it, the block is supposed to set the map (that was an accidental omission).

include/tvm/relay/adt.h

src/relay/pass/kind_check.cc

src/relay/pass/type_infer.cc

* First pass on ADTs * Add doc string for tag field * Visit constructors in TypeVisitor for TypeData * Add to description of type call * Add type call to type solving and unification * Make type mutator for typecall consistent with others (only create new node if there's a change) * Ensure kindchecking can handle type calls and typedata * Fix bad nesting in module constructor * Correctly construct call in typecall test * Add call override for ordinary vars (do we want this?) * Remove generalization hack from type inference because it was breaking ADT constructors * Check that there are no free type vars in exprs after inferring type * Free var checks need module because of ADT constructors * Typecall test can't have unbound type var, make it global * Uncomment tmap test and remove comments about failing to infer ret type; those work now * Put in dummy visits for ADTs in graph runtime codegen to placate pylint * Fix Relay type infer test module constructor * Mark override for TypeCallNode in type solver * Ensure free vars check treats patern vars as bound * Run interpreter in more ADT test cases * Refactor kind check to return the kind, like typechecking * Fix invalid typecall in test * Add kind check to type inference, do not use nulls in func_type_annotation()! * Redundant whitespace * Make TypeData a separate kind * Make ADT handles a separate kind too, document calling convention better * Remove nats and tree from prelude, move to test, document prelude * Restore and document nat and tree to prelude, add more tree tests * Add alpha equality tests for match cases, fix variable binding bug * Add more kind check tests for ADTs * Add more tests for finding free or bound vars in match exprs * Add unification tests for type call * Update main() for alpha equality tests * Add simple type inference test cases for match exprs and ADT constructors * Add more ADT interpreter tests * Allow incomplete types when typechecking match cases * Type inference for pattern vars should use the type annotation if it's there * Two more specific test cases for ADT matching * Add option ADT to prelude * Fix broken reference to kind enum * Fix rebase snags * Do not attach checked types to constructors * More docstrings for module fields * Use proper wrapper for indexing into module type data * checked_type for constructors is not populated * Expand type call docstring * Rename PatternConstructor con field * Use error reporter for pattern constructor case * Condense error reporting in kind check, use error reporter * Expand docstrings and rename ADT fields * Rename 'option' ADT to 'optional' for consistency with Python * Add various list iterators and utility functions to prelude * Add smoke tests for new iterators in prelude * Add concat to prelude * Add smoke test for concat * Correct docstrings in prelude * Ensure that type defs are written in module initialization * Various requested renamings * Correct rebase snags * Add kind check tests for ref types * Update the main() for kind checking tests

slyubomirsky commented Jan 16, 2019

View reviewed changes

python/tvm/relay/prelude.py Show resolved Hide resolved

slyubomirsky commented Jan 16, 2019

View reviewed changes

include/tvm/relay/adt.h Show resolved Hide resolved

slyubomirsky commented Jan 16, 2019

View reviewed changes

include/tvm/relay/type.h Show resolved Hide resolved

slyubomirsky commented Jan 16, 2019

View reviewed changes

include/tvm/relay/adt.h Show resolved Hide resolved

slyubomirsky force-pushed the relay-adt branch from 6b49fa9 to a6b4cad Compare January 16, 2019 22:00

slyubomirsky commented Jan 17, 2019

View reviewed changes

src/relay/ir/type_functor.cc Show resolved Hide resolved

slyubomirsky force-pushed the relay-adt branch from e47fa11 to e6929a2 Compare January 17, 2019 00:11

slyubomirsky commented Jan 17, 2019

View reviewed changes

src/relay/pass/type_infer.cc Show resolved Hide resolved

slyubomirsky commented Jan 17, 2019

View reviewed changes

src/relay/pass/kind_check.cc Show resolved Hide resolved

slyubomirsky closed this Jan 17, 2019

slyubomirsky reopened this Jan 17, 2019

slyubomirsky commented Jan 18, 2019

View reviewed changes

slyubomirsky commented Jan 21, 2019

View reviewed changes

include/tvm/relay/module.h Show resolved Hide resolved

slyubomirsky commented Jan 22, 2019

View reviewed changes

slyubomirsky changed the title ~~[Relay][WIP] Algebraic data types~~ [Relay] Algebraic data types Jan 23, 2019

slyubomirsky force-pushed the relay-adt branch 2 times, most recently from 86b1ea8 to 430b27d Compare January 28, 2019 21:51

jroesch requested changes Feb 1, 2019

View reviewed changes

slyubomirsky force-pushed the relay-adt branch from a4c1907 to 0e015a4 Compare February 1, 2019 23:43

wweic approved these changes Feb 2, 2019

View reviewed changes

zhiics reviewed Feb 3, 2019

View reviewed changes

include/tvm/relay/adt.h Outdated Show resolved Hide resolved

src/relay/pass/kind_check.cc Outdated Show resolved Hide resolved

src/relay/pass/kind_check.cc Outdated Show resolved Hide resolved

src/relay/pass/type_infer.cc Outdated Show resolved Hide resolved

slyubomirsky added 18 commits February 15, 2019 13:03

Do not attach checked types to constructors

0c660aa

More docstrings for module fields

f5cec3e

Use proper wrapper for indexing into module type data

b56bc36

checked_type for constructors is not populated

6b8dbb8

Expand type call docstring

07ea915

Rename PatternConstructor con field

b7cfc59

Use error reporter for pattern constructor case

8cd15f2

Condense error reporting in kind check, use error reporter

1d9ae48

Expand docstrings and rename ADT fields

acc2ec0

Rename 'option' ADT to 'optional' for consistency with Python

737514b

Add various list iterators and utility functions to prelude

1a6e48a

Add smoke tests for new iterators in prelude

511a931

Add concat to prelude

3eeca4c

Add smoke test for concat

868f76e

Correct docstrings in prelude

5a85aa8

Ensure that type defs are written in module initialization

bd9bfc7

Various requested renamings

de7ea6e

Correct rebase snags

8909e18

slyubomirsky force-pushed the relay-adt branch from d969cc6 to 8909e18 Compare February 15, 2019 21:24

slyubomirsky added 2 commits February 15, 2019 13:29

Add kind check tests for ref types

ffd5c80

Update the main() for kind checking tests

7a6eff0

yzhliu approved these changes Feb 16, 2019

View reviewed changes

wweic approved these changes Feb 16, 2019

View reviewed changes

ZihengJiang merged commit 2ae3124 into apache:master Feb 16, 2019

tqchen mentioned this pull request Feb 18, 2019

[Relay][RFC] Algebraic Data Type #2175

Closed

yzhliu mentioned this pull request Mar 2, 2019

[DEV] TVM v0.6 Roadmap #2623

Closed

28 tasks

tqchen mentioned this pull request Nov 8, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay] Algebraic data types #2442

[Relay] Algebraic data types #2442

slyubomirsky commented Jan 16, 2019 •

edited

Loading

slyubomirsky commented Jan 16, 2019 •

edited

Loading

slyubomirsky Jan 18, 2019

MarisaKirisame Jan 18, 2019

slyubomirsky Jan 22, 2019

slyubomirsky Jan 22, 2019

MarisaKirisame Jan 23, 2019

slyubomirsky Jan 23, 2019

jroesch Jan 28, 2019

slyubomirsky Jan 28, 2019

slyubomirsky commented Jan 23, 2019

slyubomirsky commented Jan 23, 2019 •

edited

Loading

MarisaKirisame commented Jan 23, 2019

slyubomirsky commented Jan 23, 2019 •

edited

Loading

slyubomirsky commented Jan 28, 2019

jroesch commented Feb 1, 2019

jroesch commented Feb 1, 2019

jroesch left a comment

jroesch Jan 28, 2019

slyubomirsky Feb 1, 2019

jroesch Feb 1, 2019

slyubomirsky Feb 1, 2019

jroesch Feb 4, 2019

slyubomirsky Feb 4, 2019

wweic left a comment

wweic Feb 2, 2019

slyubomirsky Feb 4, 2019

wweic Feb 15, 2019

slyubomirsky Feb 15, 2019

[Relay] Algebraic data types #2442

[Relay] Algebraic data types #2442

Conversation

slyubomirsky commented Jan 16, 2019 • edited Loading

slyubomirsky commented Jan 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky commented Jan 23, 2019

slyubomirsky commented Jan 23, 2019 • edited Loading

MarisaKirisame commented Jan 23, 2019

slyubomirsky commented Jan 23, 2019 • edited Loading

slyubomirsky commented Jan 28, 2019

jroesch commented Feb 1, 2019

jroesch commented Feb 1, 2019

jroesch 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyubomirsky commented Jan 16, 2019 •

edited

Loading

slyubomirsky commented Jan 16, 2019 •

edited

Loading

slyubomirsky commented Jan 23, 2019 •

edited

Loading

slyubomirsky commented Jan 23, 2019 •

edited

Loading