Skip to content

Enable creation of boxing wrappers for some aten operators #26273

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

Closed
wants to merge 18 commits into from

Conversation

smessmer
Copy link
Contributor

@smessmer smessmer commented Sep 16, 2019

Stack from ghstack:

[namedtensor ci]

Differential Revision: D17393318

smessmer added a commit that referenced this pull request Sep 16, 2019
[namedtensor ci]

Differential Revision: [D17393318](https://our.internmc.facebook.com/intern/diff/D17393318/)

ghstack-source-id: 90146417
Pull Request resolved: #26273
@ezyang
Copy link
Contributor

ezyang commented Sep 16, 2019

For the record: I oppose adding a second partial state here. We should have 100% coverage for boxing wrappers in c10 if we add it. If you can't handle an op, disable it from c10 instead.

@smessmer
Copy link
Contributor Author

smessmer commented Sep 16, 2019

The reason for the two-step process is that it allows us to delete globalATenDispatch earlier. Once all ops are in stage 1, we can already delete it and making them boxed is a problem inside the c10 dispatcher. If we force all ops to immediately be box-able, it will significantly delay the deletion of globalATenDispatch and there will be a longer time we have to deal with multiple dispatchers.

My original plan was what you're describing - keep the c10 dispatcher pure and only move what can be moved, add features and operators step-by-step. But you and @dzhulgakov convinced me that having multiple dispatchers is a major issue for other development flows (like multi dispatch or lazy) and that it's important to get down to only one dispatcher earlier than that and that it's ok if the c10 dispatcher has both unboxed_only and regular ops for a while.

smessmer added a commit that referenced this pull request Sep 17, 2019
Pull Request resolved: #26273

[namedtensor ci]
ghstack-source-id: 90222273

Differential Revision: [D17393318](https://our.internmc.facebook.com/intern/diff/D17393318/)
@ezyang
Copy link
Contributor

ezyang commented Sep 17, 2019

OK

@smessmer smessmer mentioned this pull request Sep 18, 2019
smessmer added a commit that referenced this pull request Sep 18, 2019
Pull Request resolved: #26273

[namedtensor ci]
ghstack-source-id: 90310841

Differential Revision: [D17393318](https://our.internmc.facebook.com/intern/diff/D17393318/)
@pytorchbot pytorchbot added the module: internals Related to internal abstractions in c10 and ATen label Sep 18, 2019
smessmer added a commit that referenced this pull request Sep 18, 2019
Pull Request resolved: #26273

[namedtensor ci]
ghstack-source-id: 90315164

Differential Revision: [D17393318](https://our.internmc.facebook.com/intern/diff/D17393318/)
smessmer added a commit that referenced this pull request Sep 18, 2019
Pull Request resolved: #26273

[namedtensor ci]
ghstack-source-id: 90315398

Differential Revision: [D17393318](https://our.internmc.facebook.com/intern/diff/D17393318/)
@smessmer smessmer changed the title [wip] Enable creation of boxing wrappers for some aten operators Enable creation of boxing wrappers for some aten operators Sep 19, 2019
zdevito pushed a commit to zdevito/ATen that referenced this pull request Sep 21, 2019
Summary:
Pull Request resolved: pytorch/pytorch#26273

[namedtensor ci]
ghstack-source-id: 90459908

Test Plan: unit tests

Differential Revision: D17393318

fbshipit-source-id: 1831da121ca7c64d1148b34c88a57bdc16c9fddf
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 5016796.

mingbowan pushed a commit to mingbowan/pytorch that referenced this pull request Sep 23, 2019
…6273)

Summary:
Pull Request resolved: pytorch#26273

[namedtensor ci]
ghstack-source-id: 90459908

Test Plan: unit tests

Differential Revision: D17393318

fbshipit-source-id: 1831da121ca7c64d1148b34c88a57bdc16c9fddf
@facebook-github-bot facebook-github-bot deleted the gh/smessmer/45/head branch October 28, 2019 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged module: internals Related to internal abstractions in c10 and ATen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants