Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Relay] Remove Free Variables/Free Type Variables in Module #3476

Merged
merged 1 commit into from
Jul 10, 2019
Merged

[Relay] Remove Free Variables/Free Type Variables in Module #3476

merged 1 commit into from
Jul 10, 2019

Conversation

MarisaKirisame
Copy link
Contributor

Module will now CHECK that there aren't any free variables/free type variables.
It simplify all passes as they can assume the absence of free variables.
@zhiics @wweic @junrushao1994 can you guys review?

@jroesch
Copy link
Member

jroesch commented Jul 2, 2019

Functions in a module must be closed in both expression and type scope.

@MarisaKirisame
Copy link
Contributor Author

@eqy I change the output of some test in autotvm. Can you look and make sure it is ok?

@zhiics
Copy link
Member

zhiics commented Jul 3, 2019

@MarisaKirisame This change sounds good to me. I took a look into the error. It seems that there is some thing wrong with binding.

@zhiics
Copy link
Member

zhiics commented Jul 3, 2019 •
edited
Loading

@MarisaKirisame thanks, I see you are correcting it. Otherwise, creating a new Function in FromExpr when expr is Function would also work.

@MarisaKirisame
Copy link
Contributor Author

@zhiics I did both.

@MarisaKirisame
Copy link
Contributor Author

I think the newly added warning in the module should be an error, but there is too much instance and I need this fix to fix PE, so I think we can do it in another PR.

zhiics
zhiics approved these changes Jul 3, 2019
View reviewed changes
Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please fix the conflict.

vinx13
vinx13 requested changes Jul 3, 2019
View reviewed changes
src/relay/ir/module.cc Outdated
auto fv = FreeVars(func);
auto ftv = FreeTypeVars(func, mod);
if (fv.size() != 0) {
std::cout
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use LOG(WARNING)

src/relay/ir/module.cc Outdated
<< "warning: there are free variables: "
<< fv
<< " in function: "
<< AsText(func)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AsText(func, false) to omit meta data

src/relay/ir/expr_functor.cc Show resolved Hide resolved
@MarisaKirisame
Copy link
Contributor Author

@jroesch @zhiics @eqy This break partial type inference as every type inference will now duplicate all the variables. Is this a reasonable design, and do we need partial TI?

@wweic
Copy link
Contributor

wweic commented Jul 3, 2019

@MarisaKirisame How about making it a pass so pass can depend on it if needed? Would it fix the partial type inference failure?

@MarisaKirisame
Copy link
Contributor Author

@wweic reexposing the function -> function pass will also work.
The idea is that all thing in Module is only suppose to have no free variables, it is incorrect if it has free variable.

@zhiics
Copy link
Member

zhiics commented Jul 3, 2019

@MarisaKirisame @wweic It think making it into a pass probably will not solve the problem. It looks to me that partial TI is not really quite needed if very thing is baked into a module. How do you guys think? @jroesch

@MarisaKirisame
Copy link
Contributor Author

@zhiics I mean not pass, but the function, and deprecate it slowly.
I remember @eqy was relying on partial type inference, so I want his opinion.

@jroesch
Copy link
Member

jroesch commented Jul 5, 2019

I think we should have a special API for partial type inference, so people know when they are opting in. We just make all the free vars dummy parameters, run inference then remove them.

eqy
eqy reviewed Jul 5, 2019
View reviewed changes
Copy link
Contributor

@eqy eqy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarisaKirisame @kevinthesun added the graph tuner tests I believe

@eqy
Copy link
Contributor

eqy commented Jul 5, 2019

I am also not sure if this breaks things because the quantization PR is not merged yet. There are still a few calls to InferType in the pass but I am not sure they do PI

@MarisaKirisame
Copy link
Contributor Author

sounds good to me, so I think we can add this, and add a special type infer function as @jroesch say.

@MarisaKirisame
init
55b52bc 
lint

update

address comment

comment out breaking test
@jroesch jroesch merged commit 273c028 into apache:master Jul 10, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Jul 11, 2019
@MarisaKirisame
init (apache#3476)
d3f694d 
lint

update

address comment

comment out breaking test
wweic pushed a commit to wweic/tvm that referenced this pull request Jul 11, 2019
@MarisaKirisame
init (apache#3476)
fb0ae5f 
lint

update

address comment

comment out breaking test
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jul 11, 2019
@MarisaKirisame @wweic
init (apache#3476)
d9aedcc 
lint

update

address comment

comment out breaking test
@MarisaKirisame MarisaKirisame deleted the no-freevars-in-mod branch July 13, 2019 20:58
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eqy eqy eqy left review comments

@vinx13 vinx13 vinx13 approved these changes

@zhiics zhiics zhiics approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MarisaKirisame @jroesch @zhiics @wweic @eqy @vinx13