Move TensorOptions ops to c10 by smessmer · Pull Request #39492 · pytorch/pytorch

smessmer · 2020-06-03T23:12:43Z

Stack from ghstack:

Unify TensorOptions signatures #39611 Unify TensorOptions signatures
Move TensorOptions ops to c10 #39492 Move TensorOptions ops to c10
Upgrade msvc to 14.13 #40109 Upgrade msvc to 14.13

This PR adds use_c10_dispatcher: full to ops taking TensorOptions. To allow this, since the c10 operator library doesn't know about TensorOptions, we need to register the operator kernels as optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool> instead, and also call them this way.

Changes:

Add use_c10_dispatcher: full to those ops
Write hacky_wrapper_for_legacy_signatures which takes an old-style kernel (i.e. one written to take TensorOptions) an creates a wrapper kernel for it that takes the scattered optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool> instead.
Change codegen so that all op registrations are wrapped into hacky_wrapper_for_legacy_signatures. This is added to all ops but is a no-op if the op doesn't take TensorOptions. This allows us in the future to just change a kernel signature from TensorOptions to the scattered version and have it work without having to touch codegen.
Change codegen so that the frontend calls those operators with expanded arguments instead of with a TensorOptions object. This is required because now the kernels are written in this way.

This PR does not remove TensorOptions special cases from codegen, but instead it separates kernels from the codegen/frontend issues. After this, kernels can be worked on separately without having to touch codegen and codegen can be worked on without having to touch kernels.

Codegen diff: P133121032

Differential Revision: D21581908

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) ghstack-source-id: 105217041 Pull Request resolved: #39492

dr-ci · 2020-06-03T23:19:52Z

💊 CI failures summary and remediations

As of commit 58bd7ff (more details on the Dr. CI page):

1/1 failures introduced in this PR

XLA failure

Job pytorch_xla_linux_bionic_py3_6_clang9_build is failing. Please create an issue with title prefixed by [PT_BREAK] in pytorch/xla and link to to this PR. If you have questions, please reach out to @ailzhang / @dlibenzi / @JackCaoG.

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 215 times.

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Pull Request resolved: #39492 ghstack-source-id: 105363132 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/)

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Pull Request resolved: #39492 ghstack-source-id: 105374944 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/)

ezyang · 2020-06-08T14:15:35Z

This PR is pretty long, can we get a longer PR description?

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Pull Request resolved: #39492 ghstack-source-id: 105637896 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/)

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h

This PR adds `use_c10_dispatcher: full` to ops taking `TensorOptions`. To allow this, since the c10 operator library doesn't know about `TensorOptions`, we need to register the operator kernels as `optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool>` instead, and also call them this way. Changes: - Add `use_c10_dispatcher: full` to those ops - Write hacky_wrapper_for_legacy_signatures which takes an old-style kernel (i.e. one written to take `TensorOptions`) an creates a wrapper kernel for it that takes the scattered `optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool>` instead. - Change codegen so that all op registrations are wrapped into `hacky_wrapper_for_legacy_signatures`. This is added to all ops but is a no-op if the op doesn't take TensorOptions. This allows us in the future to just change a kernel signature from `TensorOptions` to the scattered version and have it work without having to touch codegen. - Change codegen so that the frontend calls those operators with expanded arguments instead of with a `TensorOptions` object. This is required because now the kernels are written in this way. This PR does not remove TensorOptions special cases from codegen, but instead it separates kernels from the codegen/frontend issues. After this, kernels can be worked on separately without having to touch codegen and codegen can be worked on without having to touch kernels. Codegen diff: P133121032 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

ailzhang

Stamping as we have a fix for the XLA failure. Thanks!

This PR adds `use_c10_dispatcher: full` to ops taking `TensorOptions`. To allow this, since the c10 operator library doesn't know about `TensorOptions`, we need to register the operator kernels as `optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool>` instead, and also call them this way. Changes: - Add `use_c10_dispatcher: full` to those ops - Write hacky_wrapper_for_legacy_signatures which takes an old-style kernel (i.e. one written to take `TensorOptions`) an creates a wrapper kernel for it that takes the scattered `optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool>` instead. - Change codegen so that all op registrations are wrapped into `hacky_wrapper_for_legacy_signatures`. This is added to all ops but is a no-op if the op doesn't take TensorOptions. This allows us in the future to just change a kernel signature from `TensorOptions` to the scattered version and have it work without having to touch codegen. - Change codegen so that the frontend calls those operators with expanded arguments instead of with a `TensorOptions` object. This is required because now the kernels are written in this way. This PR does not remove TensorOptions special cases from codegen, but instead it separates kernels from the codegen/frontend issues. After this, kernels can be worked on separately without having to touch codegen and codegen can be worked on without having to touch kernels. Codegen diff: P133121032 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

bhosmer

Hey, made it down to native_functions.yaml, will resume in a bit - meanwhile a few actionable suggestions in line that I'd pitch pretty enthusiastically. :)

aten/src/ATen/common_with_cwrap.py

aten/src/ATen/core/jit_type.h

bhosmer · 2020-06-18T20:39:37Z

aten/src/ATen/function_wrapper.py

 DEFAULT_FUNCTION_REGISTRATION = CodeTemplate("""\
-m.impl("${unqual_operator_name_with_overload}", TORCH_FN(TypeDefault::${type_wrapper_name}));
+m.impl("${unqual_operator_name_with_overload}",
+       c10::impl::hacky_wrapper_for_legacy_signatures(TORCH_FN(TypeDefault::${type_wrapper_name})));


It's a bit of a bummer that our nice clean API has so quickly gotten encrusted with this shim, in particular to the extent these calls will serve as example API usage. Ergonomically it's kind of rough (especially since it might do nothing). Two ideas:

teach the codegen to only include the wrapper when necessary. I realize this intrudes on the complete decoupling you were going for here, but I think it would be worth it - the API usage will be better exemplified both for legacy and non-legacy cases. I.e., I think the value of complete decoupling is outweighed by the extent to which it obscures the (evolving) relationship between registered and implemented signatures, and by the readability hit.

To pitch the change another way: we want registrations for kernels that are already in our desired final state to look like it.

hide the wrapper behind TORCH_FN, or a macro that wraps TORCH_FN. I think this is less desirable since it simply hides everything, rather than clarifying when the wrapper is needed, but it would fix the readability issue.

We can teach the codegen when to emit the wrapper just by looking for tensor options in the signature, correct? If so, I'd suggest having a function needs_legacy_wrapper where any future triggering conditions would also go (per the comment header in above the metaprogramming for this)

aten/src/ATen/function_wrapper.py

aten/src/ATen/gen_backend_select_register.py

bhosmer · 2020-06-18T20:54:33Z

aten/src/ATen/gen_backend_select_register.py

  m.impl_UNBOXED("aten::${op_name_with_overload_name}", ${function_name});
 """)

+FUNCTION_REGISTRATION = CodeTemplate("""\


Same suggestion re making hacky_wrapper conditional. Could use the same central discriminant function to decide; this would actually reinforce the difference as a canonical part of codegen logic and clarify where/why it was necessary.

aten/src/ATen/gen_backend_select_register.py

aten/src/ATen/native/native_functions.yaml

This PR adds `use_c10_dispatcher: full` to ops taking `TensorOptions`. To allow this, since the c10 operator library doesn't know about `TensorOptions`, we need to register the operator kernels as `optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool>` instead, and also call them this way. Changes: - Add `use_c10_dispatcher: full` to those ops - Write hacky_wrapper_for_legacy_signatures which takes an old-style kernel (i.e. one written to take `TensorOptions`) an creates a wrapper kernel for it that takes the scattered `optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool>` instead. - Change codegen so that all op registrations are wrapped into `hacky_wrapper_for_legacy_signatures`. This is added to all ops but is a no-op if the op doesn't take TensorOptions. This allows us in the future to just change a kernel signature from `TensorOptions` to the scattered version and have it work without having to touch codegen. - Change codegen so that the frontend calls those operators with expanded arguments instead of with a `TensorOptions` object. This is required because now the kernels are written in this way. This PR does not remove TensorOptions special cases from codegen, but instead it separates kernels from the codegen/frontend issues. After this, kernels can be worked on separately without having to touch codegen and codegen can be worked on without having to touch kernels. Codegen diff: P133121032 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

bhosmer

LG, a few things inline, mostly copy pasting, but the only major question is whether to conditionalize the hacky wrapper use in this PR or a followup - up to you 😁

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h

aten/src/ATen/function_wrapper.py

aten/src/ATen/gen_backend_select_register.py

tools/autograd/gen_variable_type.py

This PR adds `use_c10_dispatcher: full` to ops taking `TensorOptions`. To allow this, since the c10 operator library doesn't know about `TensorOptions`, we need to register the operator kernels as `optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool>` instead, and also call them this way. Changes: - Add `use_c10_dispatcher: full` to those ops - Write hacky_wrapper_for_legacy_signatures which takes an old-style kernel (i.e. one written to take `TensorOptions`) an creates a wrapper kernel for it that takes the scattered `optional<ScalarType>, optional<Device>, optional<Layout>, optional<bool>` instead. - Change codegen so that all op registrations are wrapped into `hacky_wrapper_for_legacy_signatures`. This is added to all ops but is a no-op if the op doesn't take TensorOptions. This allows us in the future to just change a kernel signature from `TensorOptions` to the scattered version and have it work without having to touch codegen. - Change codegen so that the frontend calls those operators with expanded arguments instead of with a `TensorOptions` object. This is required because now the kernels are written in this way. This PR does not remove TensorOptions special cases from codegen, but instead it separates kernels from the codegen/frontend issues. After this, kernels can be worked on separately without having to touch codegen and codegen can be worked on without having to touch kernels. Codegen diff: P133121032 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

facebook-github-bot · 2020-06-23T21:42:59Z

This pull request has been merged in b623bde.

ezyang · 2020-07-20T19:34:32Z

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h

+    using parameters_before_tensoroptions =
+        guts::typelist::take_t<gathered_parameter_types, tensoroptions_arg_index>;
+    using parameters_after_tensoroptions =
+        guts::typelist::drop_t<gathered_parameter_types, tensoroptions_arg_index + 1>;


Not gonna lie: I'm impressed this all works at all! Do you remember if there were any hidden bombshells, e.g., compiler bugs, you had to work around to get this working? (What are the buried skeletons?)

Move TensorOptions ops to c10

1c0399b

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

This was referenced Jun 3, 2020

Show which type was the wrong one when a signature is invalid #39491

Closed

Assert that kernels are called with the right signature #38361

Closed

smessmer added a commit that referenced this pull request Jun 3, 2020

Move TensorOptions ops to c10

c0365df

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) ghstack-source-id: 105217041 Pull Request resolved: #39492

Update on "Move TensorOptions ops to c10"

51a5937

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

smessmer added a commit that referenced this pull request Jun 5, 2020

Move TensorOptions ops to c10

ceeb3de

Pull Request resolved: #39492 ghstack-source-id: 105363132 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/)

Update on "Move TensorOptions ops to c10"

3d56484

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

smessmer requested review from bhosmer and ezyang June 5, 2020 22:03

Update on "Move TensorOptions ops to c10"

e240c59

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

smessmer added a commit that referenced this pull request Jun 5, 2020

Move TensorOptions ops to c10

5d72edf

Pull Request resolved: #39492 ghstack-source-id: 105374944 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/)

This was referenced Jun 5, 2020

Unify TensorOptions signatures #39611

Closed

Gaps for making template-unboxing work for all operators #32366

Closed

smessmer added 5 commits June 8, 2020 11:36

Update on "Move TensorOptions ops to c10"

8942473

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Update on "Move TensorOptions ops to c10"

82267a1

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Update on "Move TensorOptions ops to c10"

da53306

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Update on "Move TensorOptions ops to c10"

2ea0ac5

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Update on "Move TensorOptions ops to c10"

d1bf0b3

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

ailzhang self-requested a review June 9, 2020 21:06

Update on "Move TensorOptions ops to c10"

50556a6

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Update on "Move TensorOptions ops to c10"

5349276

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

Update on "Move TensorOptions ops to c10"

ed0b194

Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/) [ghstack-poisoned]

smessmer added a commit that referenced this pull request Jun 10, 2020

Move TensorOptions ops to c10

3d76405

Pull Request resolved: #39492 ghstack-source-id: 105637896 Differential Revision: [D21581908](https://our.internmc.facebook.com/intern/diff/D21581908/)

bhosmer reviewed Jun 10, 2020

View reviewed changes

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h Outdated Show resolved Hide resolved

bhosmer reviewed Jun 10, 2020

View reviewed changes

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h Show resolved Hide resolved

ailzhang mentioned this pull request Jun 17, 2020

Cleanup TensorOptions. pytorch/xla#2235

Merged

smessmer added 2 commits June 17, 2020 15:00

smessmer requested review from bhosmer and ezyang June 18, 2020 01:17

ailzhang approved these changes Jun 18, 2020

View reviewed changes

bhosmer reviewed Jun 18, 2020

View reviewed changes

ailzhang mentioned this pull request Jun 18, 2020

Include expanded TensorOptions version of op in at:: namespace #40250

Open

smessmer mentioned this pull request Jun 18, 2020

Assert that kernels are called with the right signature #40251

Closed

smessmer requested a review from bhosmer June 19, 2020 22:33

bhosmer approved these changes Jun 20, 2020

View reviewed changes

smessmer mentioned this pull request Jun 20, 2020

Upgrade msvc to 14.13 #40109

Closed

smessmer added 5 commits June 22, 2020 17:16

facebook-github-bot closed this in b623bde Jun 23, 2020

facebook-github-bot added the merged label Jun 23, 2020

facebook-github-bot deleted the gh/smessmer/224/head branch June 27, 2020 14:16

ezyang reviewed Jul 20, 2020

View reviewed changes

bhosmer mentioned this pull request Aug 26, 2020

pull empty() out of use_c10_dispatcher: full #43572

Closed

mruberry added the Merged label Oct 28, 2020

Conversation

smessmer commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

XLA failure

Uh oh!

ezyang commented Jun 8, 2020

Uh oh!

Uh oh!

Uh oh!

ailzhang left a comment

Choose a reason for hiding this comment

Uh oh!

bhosmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bhosmer Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhosmer Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bhosmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot commented Jun 23, 2020

Uh oh!

ezyang Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

smessmer commented Jun 3, 2020 •

edited

Loading

dr-ci bot commented Jun 3, 2020 •

edited

Loading