Use name for output variables instead of out in JIT #17386

cpuhrsch · 2019-02-22T01:41:55Z

This adds 88 matches.

facebook-github-bot

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito · 2019-02-22T03:06:19Z

I am pretty sure that renaming line was necessary to make the out keyword-only argument work. It was added in this PR: https://github.com/pytorch/pytorch/pull/13093/files which adds support for that. It is possible it is no longer necessary if all the _out variants have renamed their output variable to out.

cpuhrsch · 2019-02-22T17:40:25Z

@zdevito - ok, it looks like it's worth digging deeper into this.

cpuhrsch · 2019-02-22T18:51:46Z

The generated code changeset affects

        modified:   build/aten/src/ATen/Declarations.yaml
        modified:   torch/csrc/jit/generated/register_aten_ops_0.cpp
        modified:   torch/csrc/jit/generated/register_aten_ops_1.cpp
        modified:   torch/csrc/jit/generated/register_aten_ops_2.cpp

but doesn't impact any Python functions. Since the JIT doesn't care about the names of keyword arguments, it looks like these functions aren't exposed via Python anyway. All other functions for which it matters are thus presumably already using out as the output name as is required by the Python frontend.

facebook-github-bot

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

gchanan · 2019-02-25T23:55:41Z

aten/src/ATen/native/native_functions.yaml

gchanan · 2019-02-25T23:55:57Z

aten/src/ATen/native/native_functions.yaml

this looks weird -- output is an output in this case.

gchanan · 2019-02-25T23:56:02Z

aten/src/ATen/native/native_functions.yaml

gchanan · 2019-02-25T23:56:21Z

aten/src/ATen/native/native_functions.yaml

gchanan · 2019-02-25T23:57:12Z

tools/jit/gen_jit_dispatch.py

can you clean this up?

gchanan · 2019-02-25T23:58:14Z

tools/autograd/gen_python_functions.py

is this note correct? There are a bunch of cases with multiple output arguments that are Tensors AFAII.

It's determined above. typenames is of the same length as output_args and typename is set to be TensorList[len(typenames)] if that's longer than 1.

facebook-github-bot

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cpuhrsch · 2019-02-26T22:47:26Z

This is the codegen changeset.

facebook-github-bot

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

gchanan · 2019-02-27T20:18:42Z

aten/src/ATen/native/README.md

 change (e.g., you will probably need to update `tools/autograd/derivatives.yaml` at
 least). For more details please see the section on `variants`.

+As a convention we use 'out' to indicate an output argument. This aligns with the


nit: this message is a little strange. "This aligns with the Python bindings" is true, but the python bindings should be generated based on the schema here (and I think most would expect it is).

This also doesn't apply to operators with multiple output arguments.

"Check the generated code when making a change
to make sure you're not breaking the API when renaming an argument name of an
existing function."

this applies to any parameter you rename, not just output arguments.

facebook-github-bot

@cpuhrsch is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: This adds 88 matches. Pull Request resolved: pytorch/pytorch#17386 Differential Revision: D14179139 Pulled By: cpuhrsch fbshipit-source-id: 2c3263b8e4d084db84791e53290e8c8b1b7aecd5

Summary: Stacked on top of #17386 Brings us to 1014/1106 of writing. Pull Request resolved: #17385 Differential Revision: D14248008 Pulled By: cpuhrsch fbshipit-source-id: 033e00de91e3edf7ae01ca03ebe436c0446b3b5c

Summary: Stacked on top of pytorch/pytorch#17386 Brings us to 1014/1106 of writing. Pull Request resolved: pytorch/pytorch#17385 Differential Revision: D14248008 Pulled By: cpuhrsch fbshipit-source-id: 033e00de91e3edf7ae01ca03ebe436c0446b3b5c

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Feb 22, 2019

facebook-github-bot reviewed Feb 22, 2019

View reviewed changes

cpuhrsch changed the title ~~[DRAFT] Use name for output variables instead of out in JIT~~ Use name for output variables instead of out in JIT Feb 22, 2019

wanchaol approved these changes Feb 22, 2019

View reviewed changes

cpuhrsch changed the title ~~Use name for output variables instead of out in JIT~~ [DO NOT LAND] Use name for output variables instead of out in JIT Feb 22, 2019

cpuhrsch requested a review from zdevito February 22, 2019 19:07

facebook-github-bot reviewed Feb 22, 2019

View reviewed changes

cpuhrsch force-pushed the winwin branch from ca39c10 to 6ed0c4e Compare February 22, 2019 22:54

facebook-github-bot reviewed Feb 25, 2019

View reviewed changes

cpuhrsch force-pushed the winwin branch from 8498f91 to 2a3d443 Compare February 25, 2019 21:46

facebook-github-bot reviewed Feb 25, 2019

View reviewed changes

gchanan reviewed Feb 25, 2019

View reviewed changes

facebook-github-bot reviewed Feb 26, 2019

View reviewed changes

cpuhrsch force-pushed the winwin branch from 739020d to de80935 Compare February 26, 2019 02:22

facebook-github-bot reviewed Feb 26, 2019

View reviewed changes

cpuhrsch requested a review from gchanan February 27, 2019 01:37

cpuhrsch mentioned this pull request Feb 27, 2019

Remove (almost all) TensorOptions from native_functions.yaml #17385

Closed

cpuhrsch added 8 commits February 26, 2019 18:12

Use name for output variables instead of out in JIT

efc486b

deal with add

2783383

tmp

8c4c0ce

[wip] also consider python bindings

9ce99f5

update yamls

2b0dc2a

more yaml

d1875d5

activate assert

7a29609

nn python bindings

6fe0e38

cpuhrsch added 3 commits February 26, 2019 18:12

fix some arguments

1bd2a05

more python glue

35eac68

change some argument names

082ab37

cpuhrsch force-pushed the winwin branch from 3fc6470 to 082ab37 Compare February 27, 2019 02:13

facebook-github-bot reviewed Feb 27, 2019

View reviewed changes

gchanan reviewed Feb 27, 2019

View reviewed changes

gchanan approved these changes Feb 27, 2019

View reviewed changes

cpuhrsch changed the title ~~[DO NOT LAND] Use name for output variables instead of out in JIT~~ Use name for output variables instead of out in JIT Feb 27, 2019

facebook-github-bot reviewed Feb 27, 2019

View reviewed changes

facebook-github-bot closed this in e47aeed Feb 27, 2019

pytorchbot added the merged label Feb 27, 2019

Use name for output variables instead of out in JIT #17386

Use name for output variables instead of out in JIT #17386

Uh oh!

Conversation

cpuhrsch commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

zdevito commented Feb 22, 2019

Uh oh!

cpuhrsch commented Feb 22, 2019

Uh oh!

cpuhrsch commented Feb 22, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

cpuhrsch commented Feb 26, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cpuhrsch commented Feb 22, 2019 •

edited

Loading