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

PyTorch frontend: make type inference incremental #6900

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

t-vi
Copy link
Contributor

@t-vi t-vi commented Nov 11, 2020

Currently, the PyTorch frontend will use the vanilla TVM type inference pass to get the types.
Combined with the incremental nature of translating the graph, this makes for quadratic (in the graph size) runtime.
This patch runs type inference on a subgraph (starting from the things that has types) instead. It is a bit hacky though because it essentially tries to implement in-place modification where TVM does not foresee it.

For converting Huggingface BERT, this patch gives a ~10x increase in the speed of conversion (from 31 seconds to just below 3), so it is solving a very real and quite bad problem.

@t-vi
Copy link
Contributor Author

t-vi commented Nov 11, 2020

The other question is whether we would want to make the translator a use a class interface so that the type inference can directly access the outputs' types.

@t-vi
Copy link
Contributor Author

t-vi commented Nov 11, 2020

@masahi
Copy link
Member

masahi commented Nov 12, 2020

The other question is whether we would want to make the translator a use a class interface so that the type inference can directly access the outputs' types.

Yes we can consider this approach. Initially I thought function-only approach would be cleaner (e.g. for recursively converting blocks in If and Loop) but this resulted in passing various constants such as convert_map, prelude, default_dtype etc around each function.

We can introduce a global object (like a class) to hold these constants.

t-vi added a commit to t-vi/tvm that referenced this pull request Dec 3, 2020
While the functional approach is pretty neat, we ended up having
global state (default frontend, dtype) and it'll be more soon
(caching of inferred types, see apache#6900). To not have to pass around
the state, this moves the op conversion into a class with instances
having the state.
t-vi added a commit to t-vi/tvm that referenced this pull request Dec 4, 2020
While the functional approach is pretty neat, we ended up having
global state (default frontend, dtype) and it'll be more soon
(caching of inferred types, see apache#6900). To not have to pass around
the state, this moves the op conversion into a class with instances
having the state.
t-vi added a commit to t-vi/tvm that referenced this pull request Dec 4, 2020
While the functional approach is pretty neat, we ended up having
global state (default frontend, dtype) and it'll be more soon
(caching of inferred types, see apache#6900). To not have to pass around
the state, this moves the op conversion into a class with instances
having the state.
t-vi added a commit to t-vi/tvm that referenced this pull request Dec 4, 2020
While the functional approach is pretty neat, we ended up having
global state (default frontend, dtype) and it'll be more soon
(caching of inferred types, see apache#6900). To not have to pass around
the state, this moves the op conversion into a class with instances
having the state.
masahi pushed a commit that referenced this pull request Dec 4, 2020
While the functional approach is pretty neat, we ended up having
global state (default frontend, dtype) and it'll be more soon
(caching of inferred types, see #6900). To not have to pass around
the state, this moves the op conversion into a class with instances
having the state.
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
While the functional approach is pretty neat, we ended up having
global state (default frontend, dtype) and it'll be more soon
(caching of inferred types, see apache#6900). To not have to pass around
the state, this moves the op conversion into a class with instances
having the state.
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
While the functional approach is pretty neat, we ended up having
global state (default frontend, dtype) and it'll be more soon
(caching of inferred types, see apache#6900). To not have to pass around
the state, this moves the op conversion into a class with instances
having the state.
@t-vi t-vi force-pushed the incremental_type_inference branch from a9badc1 to 5e07318 Compare December 8, 2020 15:38
@t-vi
Copy link
Contributor Author

t-vi commented Dec 8, 2020

@masahi @siju-samuel I think this is ready for review now. In BERT conversion, I get a 10x speedup for from_python. In addition to removing the N² problem, it prunes the module before type inference, which seems essential for prelude.

@masahi
Copy link
Member

masahi commented Dec 9, 2020

@t-vi Please have a look at the CI issue. It is due to the recent change I made.

@t-vi t-vi force-pushed the incremental_type_inference branch from 5e07318 to 1046b65 Compare December 9, 2020 12:15
@t-vi
Copy link
Contributor Author

t-vi commented Dec 9, 2020

Oh, right, an undetected merge confict. Fixed. Thank you @masahi.

@masahi masahi merged commit db0215e into apache:main Dec 9, 2020
@masahi
Copy link
Member

masahi commented Dec 9, 2020

Thanks @t-vi

@t-vi
Copy link
Contributor Author

t-vi commented Dec 9, 2020

Thank you @masahi, for the guidance and discussion, and review!

TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Jan 20, 2021
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jan 21, 2021
electriclilies pushed a commit to electriclilies/tvm that referenced this pull request Feb 18, 2021
While the functional approach is pretty neat, we ended up having
global state (default frontend, dtype) and it'll be more soon
(caching of inferred types, see apache#6900). To not have to pass around
the state, this moves the op conversion into a class with instances
having the state.
electriclilies pushed a commit to electriclilies/tvm that referenced this pull request Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants