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.

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

Add mod supoort in relay.build #3424

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Conversation

apivovarov
Copy link
Contributor

@apivovarov apivovarov commented Jun 23, 2019

# in order to compile models we usually call
func, params = relay.frontend.from_<framework>(model_artifact, shape, ...)

# and then we call
graph, lib, params = relay.build(func, target, params=params)

Recently it was a change which changes from_<framework> output to return mod instead of func #3353
But relay.build still supports func as an input. This is why all existing users' scripts to compile models should be updated. All people need to add extra line to their scripts before relay.build

func = mod[mod.entry_func]

In order to avoid all people fixing their compile scripts I think we need to add support for mod input to relay.build.
This PR adds mod input support to relay.build and updates TFLite tutorial.
I can update other tutorials if we agree with this change.

@apivovarov
Copy link
Contributor Author

@tqchen @jroesch Could you have a look?

@tqchen
Copy link
Member

tqchen commented Jun 24, 2019

cc @zhiics This seems to be an artifact of incomplete migration to module, I agree with the change but would suggest we move to only require module and add deprecation warnings to function as input

@@ -192,6 +192,9 @@ def build(func, target=None, target_host=None, params=None):

with tophub_context:
bld_mod = BuildModule()
if "tvm.relay.module.Module" in str(type(func)):
Copy link
Member

Choose a reason for hiding this comment

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

this is not the best way to check a type in python use

isinstance

Copy link
Contributor Author

@apivovarov apivovarov Jun 24, 2019

Choose a reason for hiding this comment

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

Ok, I changed it to use isinstance here.

@zhiics
Copy link
Member

zhiics commented Jun 24, 2019

yeah, I was actually aware of this. But I think this build function is mostly for graphruntime which doesn't actually accept module. Therefore, I don't change it to accept mod because it may confuse people.

@apivovarov
Copy link
Contributor Author

yeah, I was actually aware of this. But I think this build function is mostly for graphruntime which doesn't actually accept module. Therefore, I don't change it to accept mod because it may confuse people.

build_module has class BuildModule which has method build
Also, build_module itself has helper method build which calls BuildModule.build

In all tutorials to compile models we use helper method build.
What we can do is to change helper method build to accept mod instead of func.
But at the same time keep class method BuildModule.build to still accept func.

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

@apivovarov
Copy link
Contributor Author

@tqchen Can you review it one more time?

@tqchen tqchen merged commit e182717 into apache:master Jun 27, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Jun 28, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants