Making ops c10 full: optional out arguments by smessmer · Pull Request #49083 · pytorch/pytorch

smessmer · 2020-12-09T12:03:13Z

Stack from ghstack:

Remove use_c10_dispatcher: full lines #49259 Remove use_c10_dispatcher: full lines
Remove generated_unboxing_wrappers and setManuallyBoxedKernel #49251 Remove generated_unboxing_wrappers and setManuallyBoxedKernel
Remove .impl_UNBOXED() and functionalities associated with it #49220 Remove .impl_UNBOXED() and functionalities associated with it
Remove codegen logic to support non-c10-full ops #49164 Remove codegen logic to support non-c10-full ops
Making ops c10-full: list of optional tensors #49138 [wip] Making ops c10-full: list of optional tensors
Making ops c10-full: ConstQuantizerPtr #49150 Making ops c10-full: ConstQuantizerPtr
Making ops c10-full: Storage arguments #49146 Making ops c10-full: Storage arguments
Making ops c10-full: optional lists #49088 Making ops c10-full: optional lists
Making ops c10 full: optional out arguments #49083 Making ops c10 full: optional out arguments

We have some (but very few) ops that take optional out arguments Tensor(a!)? out.
This PR makes them non-optional mandatory arguments and enables c10-fullness for them.
There is only a very small number of ops affected by this.

Putting this up for discussion.

Alternatives considered:
If we keep them optional, we run into lots of issues in the dispatcher. We have to decide what the dispatcher calling convention for this argument type should be.

If we keep passing them in as Tensor& arguments and return them as tuple<Tensor&, Tensor&, Tensor&>, so basically same as currently, then the schema inference check will say "Your kernel function got inferred to have a Tensor argument but your native_functions.yaml declaration says Tensor?. This is a mismatch, you made an error". We could potentially disable that check, but that would open the door for real mistakes to not be reported anymore in the future. This sounds bad.
If we change them to a type that schema inference could differentiate from Tensor, say we pass them in as const optional<Tensor>& and return them as tuple<const optional<Tensor>&, const optional<Tensor>&, const optional<Tensor>&>, then our boxing logic fails because it can't recognize those as out overloads anymore and shortcut the return value as it is doing right now. We might be able to rewrite the boxing logic, but that could be difficult and could easily develop into a rabbit hole of having to clean up Tensor& references throughout the system where we use them.

Furthermore, having optional out arguments in C++ doesn't really make sense. the C++ API puts them to the front of the argument list, so you can't omit them anyways when calling an op.
You would be able to omit them when calling from Python with out kwargs, but not sure if we want that discrepancy between the c++ and python API.

Differential Revision: D25422197

We have some (but very few) ops that take optional out arguments `Tensor(a!)? out`. This PR makes them non-optional mandatory arguments and enables c10-fullness for them. There is only a very small number of ops affected by this. Putting this up for discussion. Alternatives considered: If we keep them optional, we run into lots of issues in the dispatcher. We have to decide what the dispatcher calling convention for this argument type should be. 1) If we keep passing them in as `Tensor&` arguments and return them as `tuple<Tensor&, Tensor&, Tensor&>`, so basically same as currently, then the schema inference check will say "Your kernel function got inferred to have a `Tensor` argument but your native_functions.yaml declaration says `Tensor?`. This is a mismatch, you made an error". We could potentially disable that check, but that would open the door for real mistakes to not be reported anymore in the future. This sounds bad. 2) If we change them to a type that schema inference could differentiate from `Tensor`, say we pass them in as `const optional<Tensor>&` and return them as `tuple<const optional<Tensor>&, const optional<Tensor>&, const optional<Tensor>&>`, then our boxing logic fails because it can't recognize those as out overloads anymore and shortcut the return value as it is doing right now. We might be able to rewrite the boxing logic, but that could be difficult and could easily develop into a rabbit hole of having to clean up `Tensor&` references throughout the system where we use them. Furthermore, having optional out arguments in C++ doesn't really make sense. the C++ API puts them to the front of the argument list, so you can't omit them anyways when calling an op. You would be able to omit them when calling from Python with out kwargs, but not sure if we want that discrepancy between the c++ and python API. Differential Revision: [D25422197](https://our.internmc.facebook.com/intern/diff/D25422197/) [ghstack-poisoned]

dr-ci · 2020-12-09T12:34:07Z

💊 CI failures summary and remediations

As of commit 114aa3c (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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 to the (internal) Dr. CI Users group.

This comment has been revised 93 times.

We have some (but very few) ops that take optional out arguments `Tensor(a!)? out`. This PR makes them non-optional mandatory arguments and enables c10-fullness for them. There is only a very small number of ops affected by this. Putting this up for discussion. Alternatives considered: If we keep them optional, we run into lots of issues in the dispatcher. We have to decide what the dispatcher calling convention for this argument type should be. 1) If we keep passing them in as `Tensor&` arguments and return them as `tuple<Tensor&, Tensor&, Tensor&>`, so basically same as currently, then the schema inference check will say "Your kernel function got inferred to have a `Tensor` argument but your native_functions.yaml declaration says `Tensor?`. This is a mismatch, you made an error". We could potentially disable that check, but that would open the door for real mistakes to not be reported anymore in the future. This sounds bad. 2) If we change them to a type that schema inference could differentiate from `Tensor`, say we pass them in as `const optional<Tensor>&` and return them as `tuple<const optional<Tensor>&, const optional<Tensor>&, const optional<Tensor>&>`, then our boxing logic fails because it can't recognize those as out overloads anymore and shortcut the return value as it is doing right now. We might be able to rewrite the boxing logic, but that could be difficult and could easily develop into a rabbit hole of having to clean up `Tensor&` references throughout the system where we use them. Furthermore, having optional out arguments in C++ doesn't really make sense. the C++ API puts them to the front of the argument list, so you can't omit them anyways when calling an op. You would be able to omit them when calling from Python with out kwargs, but not sure if we want that discrepancy between the c++ and python API. Differential Revision: [D25422197](https://our.internmc.facebook.com/intern/diff/D25422197/) [ghstack-poisoned]

Pull Request resolved: #49083 We have some (but very few) ops that take optional out arguments `Tensor(a!)? out`. This PR makes them non-optional mandatory arguments and enables c10-fullness for them. There is only a very small number of ops affected by this. Putting this up for discussion. Alternatives considered: If we keep them optional, we run into lots of issues in the dispatcher. We have to decide what the dispatcher calling convention for this argument type should be. 1) If we keep passing them in as `Tensor&` arguments and return them as `tuple<Tensor&, Tensor&, Tensor&>`, so basically same as currently, then the schema inference check will say "Your kernel function got inferred to have a `Tensor` argument but your native_functions.yaml declaration says `Tensor?`. This is a mismatch, you made an error". We could potentially disable that check, but that would open the door for real mistakes to not be reported anymore in the future. This sounds bad. 2) If we change them to a type that schema inference could differentiate from `Tensor`, say we pass them in as `const optional<Tensor>&` and return them as `tuple<const optional<Tensor>&, const optional<Tensor>&, const optional<Tensor>&>`, then our boxing logic fails because it can't recognize those as out overloads anymore and shortcut the return value as it is doing right now. We might be able to rewrite the boxing logic, but that could be difficult and could easily develop into a rabbit hole of having to clean up `Tensor&` references throughout the system where we use them. Furthermore, having optional out arguments in C++ doesn't really make sense. the C++ API puts them to the front of the argument list, so you can't omit them anyways when calling an op. You would be able to omit them when calling from Python with out kwargs, but not sure if we want that discrepancy between the c++ and python API. ghstack-source-id: 118181599 Differential Revision: [D25422197](https://our.internmc.facebook.com/intern/diff/D25422197/)

We have some (but very few) ops that take optional out arguments `Tensor(a!)? out`. This PR makes them non-optional mandatory arguments and enables c10-fullness for them. There is only a very small number of ops affected by this. Putting this up for discussion. Alternatives considered: If we keep them optional, we run into lots of issues in the dispatcher. We have to decide what the dispatcher calling convention for this argument type should be. 1) If we keep passing them in as `Tensor&` arguments and return them as `tuple<Tensor&, Tensor&, Tensor&>`, so basically same as currently, then the schema inference check will say "Your kernel function got inferred to have a `Tensor` argument but your native_functions.yaml declaration says `Tensor?`. This is a mismatch, you made an error". We could potentially disable that check, but that would open the door for real mistakes to not be reported anymore in the future. This sounds bad. 2) If we change them to a type that schema inference could differentiate from `Tensor`, say we pass them in as `const optional<Tensor>&` and return them as `tuple<const optional<Tensor>&, const optional<Tensor>&, const optional<Tensor>&>`, then our boxing logic fails because it can't recognize those as out overloads anymore and shortcut the return value as it is doing right now. We might be able to rewrite the boxing logic, but that could be difficult and could easily develop into a rabbit hole of having to clean up `Tensor&` references throughout the system where we use them. Furthermore, having optional out arguments in C++ doesn't really make sense. the C++ API puts them to the front of the argument list, so you can't omit them anyways when calling an op. You would be able to omit them when calling from Python with out kwargs, but not sure if we want that discrepancy between the c++ and python API. Differential Revision: [D25422197](https://our.internmc.facebook.com/intern/diff/D25422197/) [ghstack-poisoned]

ezyang · 2020-12-10T15:53:47Z

Furthermore, having optional out arguments in C++ doesn't really make sense. the C++ API puts them to the front of the argument list, so you can't omit them anyways when calling an op.

This reasoning isn't valid. Look at the implementation of one of these functions:

std::tuple<Tensor&, Tensor&, Tensor&> slow_conv_transpose2d_backward_out_cpu(
    Tensor& grad_input,
    Tensor& grad_weight,
    Tensor& grad_bias,
    const Tensor& grad_output,
    const Tensor& input,
    const Tensor& weight,
    IntArrayRef kernel_size,
    IntArrayRef stride,
    IntArrayRef padding,
    IntArrayRef output_padding,
    IntArrayRef dilation,
    const Tensor& columns,
    const Tensor& ones) {
  if (grad_input.defined()) {
    slow_conv_transpose2d_backward_out_cpu_template(
        input,
        grad_output,
        grad_input,
        weight,
        columns,
        kernel_size,
        stride,
        padding,
        output_padding,
        dilation);
  }

  if (grad_weight.defined()) {
    grad_weight.resize_(weight.sizes());
    grad_weight.zero_();
  }

What's going on here is that when you have a function with multiple out arguments that can be optional, you can pass None for some of the out arguments to say, "I don't care about this output", and the function can use this information to short circuit actually doing work to compute those outputs.

Now, you might say it is not worth supporting this feature, but that's very different from saying that the feature is completely useless.

ezyang

I audited for direct call sites to slow_conv_transpose2d_backward.grad_output and didn't find any, it seems we only use the functional version, and the function calls native directly. So looks safe to drop the functionality.

We have some (but very few) ops that take optional out arguments `Tensor(a!)? out`. This PR makes them non-optional mandatory arguments and enables c10-fullness for them. There is only a very small number of ops affected by this. Putting this up for discussion. Alternatives considered: If we keep them optional, we run into lots of issues in the dispatcher. We have to decide what the dispatcher calling convention for this argument type should be. 1) If we keep passing them in as `Tensor&` arguments and return them as `tuple<Tensor&, Tensor&, Tensor&>`, so basically same as currently, then the schema inference check will say "Your kernel function got inferred to have a `Tensor` argument but your native_functions.yaml declaration says `Tensor?`. This is a mismatch, you made an error". We could potentially disable that check, but that would open the door for real mistakes to not be reported anymore in the future. This sounds bad. 2) If we change them to a type that schema inference could differentiate from `Tensor`, say we pass them in as `const optional<Tensor>&` and return them as `tuple<const optional<Tensor>&, const optional<Tensor>&, const optional<Tensor>&>`, then our boxing logic fails because it can't recognize those as out overloads anymore and shortcut the return value as it is doing right now. We might be able to rewrite the boxing logic, but that could be difficult and could easily develop into a rabbit hole of having to clean up `Tensor&` references throughout the system where we use them. Furthermore, having optional out arguments in C++ doesn't really make sense. the C++ API puts them to the front of the argument list, so you can't omit them anyways when calling an op. You would be able to omit them when calling from Python with out kwargs, but not sure if we want that discrepancy between the c++ and python API. Differential Revision: [D25422197](https://our.internmc.facebook.com/intern/diff/D25422197/) [ghstack-poisoned]

facebook-github-bot · 2020-12-16T11:12:42Z

This pull request has been merged in d69d42d.

Summary: Pull Request resolved: pytorch#49083 We have some (but very few) ops that take optional out arguments `Tensor(a!)? out`. This PR makes them non-optional mandatory arguments and enables c10-fullness for them. There is only a very small number of ops affected by this. Putting this up for discussion. Alternatives considered: If we keep them optional, we run into lots of issues in the dispatcher. We have to decide what the dispatcher calling convention for this argument type should be. 1) If we keep passing them in as `Tensor&` arguments and return them as `tuple<Tensor&, Tensor&, Tensor&>`, so basically same as currently, then the schema inference check will say "Your kernel function got inferred to have a `Tensor` argument but your native_functions.yaml declaration says `Tensor?`. This is a mismatch, you made an error". We could potentially disable that check, but that would open the door for real mistakes to not be reported anymore in the future. This sounds bad. 2) If we change them to a type that schema inference could differentiate from `Tensor`, say we pass them in as `const optional<Tensor>&` and return them as `tuple<const optional<Tensor>&, const optional<Tensor>&, const optional<Tensor>&>`, then our boxing logic fails because it can't recognize those as out overloads anymore and shortcut the return value as it is doing right now. We might be able to rewrite the boxing logic, but that could be difficult and could easily develop into a rabbit hole of having to clean up `Tensor&` references throughout the system where we use them. Furthermore, having optional out arguments in C++ doesn't really make sense. the C++ API puts them to the front of the argument list, so you can't omit them anyways when calling an op. You would be able to omit them when calling from Python with out kwargs, but not sure if we want that discrepancy between the c++ and python API. ghstack-source-id: 118660075 Test Plan: waitforsandcastle Reviewed By: ezyang Differential Revision: D25422197 fbshipit-source-id: 3cb25c5a3d93f9eb960d70ca014bae485be9f058

facebook-github-bot added the cla signed label Dec 9, 2020

smessmer mentioned this pull request Dec 9, 2020

Making ops c10-full: optional lists #49088

Closed

smessmer requested review from bhosmer and ezyang December 9, 2020 15:00

smessmer added 3 commits December 9, 2020 19:04

smessmer mentioned this pull request Dec 10, 2020

Making ops c10-full: list of optional tensors #49138

Closed

This was referenced Dec 10, 2020

Making ops c10-full: Storage arguments #49146

Closed

Making ops c10-full: ConstQuantizerPtr #49150

Closed

smessmer added 2 commits December 9, 2020 23:17

smessmer mentioned this pull request Dec 10, 2020

Remove codegen logic to support non-c10-full ops #49164

Closed

ezyang approved these changes Dec 10, 2020

View reviewed changes

smessmer added 4 commits December 10, 2020 11:18

bhosmer approved these changes Dec 11, 2020

View reviewed changes

smessmer mentioned this pull request Dec 11, 2020

Remove .impl_UNBOXED() and functionalities associated with it #49220

Closed

smessmer added 5 commits December 10, 2020 23:51

This was referenced Dec 11, 2020

Remove generated_unboxing_wrappers and setManuallyBoxedKernel #49251

Closed

Remove use_c10_dispatcher: full lines #49259

Closed

smessmer added 4 commits December 12, 2020 02:24

facebook-github-bot closed this in d69d42d Dec 16, 2020

facebook-github-bot added the Merged label Dec 16, 2020

facebook-github-bot deleted the gh/smessmer/279/head branch December 19, 2020 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making ops c10 full: optional out arguments#49083

Making ops c10 full: optional out arguments#49083
smessmer wants to merge 21 commits intogh/smessmer/279/basefrom
gh/smessmer/279/head

smessmer commented Dec 9, 2020 •

edited

Loading

Uh oh!

dr-ci bot commented Dec 9, 2020 •

edited by facebook-github-bot

Loading

Uh oh!

ezyang commented Dec 10, 2020

Uh oh!

ezyang left a comment

Uh oh!

facebook-github-bot commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Uh oh!

dr-ci bot commented Dec 9, 2020 • edited by facebook-github-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

ezyang commented Dec 10, 2020

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smessmer commented Dec 9, 2020 •

edited

Loading

dr-ci bot commented Dec 9, 2020 •

edited by facebook-github-bot

Loading