Faithful out arguments by smessmer · Pull Request #47712 · pytorch/pytorch

smessmer · 2020-11-10T23:40:15Z

Stack from ghstack:

Making ops c10 full: out overloads with default arguments #49012 Making ops c10 full: out overloads with default arguments
Making ops c10-full: Dimname arguments #49008 Making ops c10-full: Dimname arguments
Making ops c10-full: ops blocked by manual registrations #49007 Making ops c10-full: ops blocked by manual registrations
Making ops c10 full: ops returning multiple out arguments #49006 Making ops c10 full: ops returning multiple out arguments
Make out ops c10-full (with hacky-wrapper) #48912 Make out ops c10-full (with hacky-wrapper)
hacky_wrapper_for_legacy_signatures reorders out arguments #48911 hacky_wrapper_for_legacy_signatures reorders out arguments
Metaprogramming facilities to support hacky_wrapper to reorder function arguments #48910 Metaprogramming facilities to support hacky_wrapper to reorder function arguments
Make aten::div.out c10-full #47793 Make aten::div.out c10-full
VariableType calls faithful C++ API for c10-full out ops #47792 VariableType calls faithful C++ API for c10-full out ops
Faithful out arguments #47712 Faithful out arguments
Enable faithful API for all ops #47711 Enable faithful API for all ops

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# .

After this, an op will generate the following overloads for the C++ API:

// Generated from the aten::abs operator (NOT from aten::abs.out)
Tensor at::abs(Tensor& self)

// Generated from the aten::abs.out operator
Tensor& at::abs(Tensor& self, Tensor& out)
Tensor& at::abs_out(Tensor& out, Tensor& self)

This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet.
It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes.

The old API is still around and valid, so backward compatibility is not broken there.
However, this does break bc in an a bit more unexpected way (in the same way as adding a new operator overload always breaks bc). Concretely, the Tensor::m(at::my_op, args...) only works if at::my_op doesn't have multiple overloads. Previously, out overloads were excluded from that rule and an operator that had one regular and one out overload still worked. This does now not work anymore because out overloads behave more like regular overloads. This also breaks torch::nn::Functional(at::sigmoid) for those ops. Furthermore, this will break any third party code that uses at::my_op as a symbol to get its function pointer without specifying the full argument list.

Those broken APIs, however, only worked for ops without overloads previously and adding any overload would have broken them. A workaround for users to still use those APIs is to explicitly specify the argument list like torch::nn::Functional(static_cast<Tensor(*)(const Tensor&)>(at::sigmoid)). But this is super ugly.
We probably should reconsider those APIs and reimplement them differently.

edit: To avoid breaking the APIs, we decided to introduce at::myop_outf overloads instead that represent the faithful version of out overloads. This means we don't do the API cleanup yet that this was planning, but there's plans to do that down the road the right way.

Differential Revision: D24835252

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# . After this, an op will generate the following overloads for the C++ API: ```cpp // Generated from the aten::abs operator (NOT from aten::abs.out) Tensor at::abs(Tensor& self) // Generated from the aten::abs.out operator Tensor& at::abs(Tensor& self, Tensor& out) Tensor& at::abs_out(Tensor& out, Tensor& self) ``` This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet. It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes. Differential Revision: [D24835252](https://our.internmc.facebook.com/intern/diff/D24835252/) [ghstack-poisoned]

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# . After this, an op will generate the following overloads for the C++ API: ```cpp // Generated from the aten::abs operator (NOT from aten::abs.out) Tensor at::abs(Tensor& self) // Generated from the aten::abs.out operator Tensor& at::abs(Tensor& self, Tensor& out) Tensor& at::abs_out(Tensor& out, Tensor& self) ``` This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet. It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes. Differential Revision: [D24835252](https://our.internmc.facebook.com/intern/diff/D24835252/) ghstack-source-id: 116359049 Pull Request resolved: #47712

dr-ci · 2020-11-10T23:57:00Z

💊 CI failures summary and remediations

As of commit 24d4c01 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

4/4 broken upstream at merge base 88ebf6f on Dec 07 from 9:23am to 4:26pm PDT (11 commits; 8bc6023 - f67259f)

🚧 4 fixed upstream failures:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

Since your merge base is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test2 on Dec 07 from 9:23am to 4:26pm PDT (11 commits; 8bc6023 - f67259f)
- 🔁 rerun
pytorch_linux_xenial_py3_clang5_asan_test2 on Dec 07 from 9:23am to 4:26pm PDT (11 commits; 8bc6023 - f67259f)
- 🔁 rerun
pytorch_linux_bionic_py3_6_clang9_test on Dec 07 from 9:23am to 4:26pm PDT (11 commits; 8bc6023 - f67259f)
- 🔁 rerun
pytorch_linux_bionic_py3_8_gcc9_coverage_test2 on Dec 07 from 9:23am to 4:26pm PDT (11 commits; 8bc6023 - f67259f)
- 🔁 rerun

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 62 times.

bhosmer

A few things I think we need to address here - interestingly they all have the same overall shape, moving new information into the data model and using it upstream, rather than adding free floating flag variables and using them downstream to tweak output :) happy to talk through any of these suggestions, not sure how much overlap there is between what I'm saying here and what you and @ezyang have talked about.

tools/codegen/api/cpp.py

tools/codegen/api/dispatcher.py

tools/codegen/api/python.py

tools/codegen/gen.py

ezyang · 2020-11-12T15:47:41Z

@smessmer Google docs are not visible to external people (because the FB gdocs org doesn't allow public sharing). You'll have to move it to your personal Google, or republish to https://github.com/pytorch/rfcs/ for accessibility

tools/codegen/api/types.py

ezyang · 2020-11-18T20:24:40Z

Here's my new high level thinking about where we should be headed with the code generation here.

The shared model has to be structured enough to cover the cross-product of all use-cases for each of the downstream APIs. Previously, this wasn't true, because we didn't do grouping in the model; but post Move argument grouping into FunctionSchema #48195 it's now true. So now you can basically read out CppArguments straight from FunctionSchema, as long as you know if you are faithful and if you know you're a method/function.
Given this, I suggest we STOP doing intermediate translations to _argument_packs and _returns_type in CppSignature; instead, the idea is that CppSignature only stores "is faithful? is method?" and we compute CppArguments on the fly when necessary. We can get rid of the CppArgumentPacks now since they're no longer necessary, just read out the structure from the model.
And finally, quoting you from our personal chat, we can now do "schema -> exprs" directly. If you need to query some relevant information about how exactly cpp API was encoded, you on the fly compute the CppArgument, and then read out its name there. This solves my other constraint, "It should be possible to rename the name of a local variable (e.g., an argument binding) by modifying only one location in the code, and no others".

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# . After this, an op will generate the following overloads for the C++ API: ```cpp // Generated from the aten::abs operator (NOT from aten::abs.out) Tensor at::abs(Tensor& self) // Generated from the aten::abs.out operator Tensor& at::abs(Tensor& self, Tensor& out) Tensor& at::abs_out(Tensor& out, Tensor& self) ``` This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet. It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes. Differential Revision: [D24835252](https://our.internmc.facebook.com/intern/diff/D24835252/) [ghstack-poisoned]

smessmer · 2020-12-04T23:21:40Z

I reimplemented this PR on top of #48195. This makes it much better. PTAL

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# . After this, an op will generate the following overloads for the C++ API: ```cpp // Generated from the aten::abs operator (NOT from aten::abs.out) Tensor at::abs(Tensor& self) // Generated from the aten::abs.out operator Tensor& at::abs(Tensor& self, Tensor& out) Tensor& at::abs_out(Tensor& out, Tensor& self) ``` This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet. It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes. Differential Revision: [D24835252](https://our.internmc.facebook.com/intern/diff/D24835252/) [ghstack-poisoned]

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# . After this, an op will generate the following overloads for the C++ API: ```cpp // Generated from the aten::abs operator (NOT from aten::abs.out) Tensor at::abs(Tensor& self) // Generated from the aten::abs.out operator Tensor& at::abs(Tensor& self, Tensor& out) Tensor& at::abs_out(Tensor& out, Tensor& self) ``` This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet. It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes. The old API is still around and valid, so backward compatibility is not broken there. However, this does break bc in an a bit more unexpected way (in the same way as adding a new operator overload always breaks bc). Concretely, the `Tensor::m(at::my_op, args...)` only works if `at::my_op` doesn't have multiple overloads. Previously, out overloads were excluded from that rule and an operator that had one regular and one out overload still worked. This does now not work anymore because out overloads behave more like regular overloads. Differential Revision: [D24835252](https://our.internmc.facebook.com/intern/diff/D24835252/) [ghstack-poisoned]

aten/src/ATen/test/basic.cpp

tools/codegen/api/cpp.py

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# . After this, an op will generate the following overloads for the C++ API: ```cpp // Generated from the aten::abs operator (NOT from aten::abs.out) Tensor at::abs(Tensor& self) // Generated from the aten::abs.out operator Tensor& at::abs(Tensor& self, Tensor& out) Tensor& at::abs_out(Tensor& out, Tensor& self) ``` This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet. It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes. The old API is still around and valid, so backward compatibility is not broken there. However, this does break bc in an a bit more unexpected way (in the same way as adding a new operator overload always breaks bc). Concretely, the `Tensor::m(at::my_op, args...)` only works if `at::my_op` doesn't have multiple overloads. Previously, out overloads were excluded from that rule and an operator that had one regular and one out overload still worked. This does now not work anymore because out overloads behave more like regular overloads. This also breaks `torch::nn::Functional(at::sigmoid)` for those ops. Furthermore, this will break any third party code that uses `at::my_op` as a symbol to get its function pointer without specifying the full argument list. Those broken APIs, however, only worked for ops without overloads previously and adding any overload would have broken them. A workaround for users to still use those APIs is to explicitly specify the argument list like `torch::nn::Functional(static_cast<Tensor(*)(const Tensor&)>(at::sigmoid))`. But this is super ugly. We probably should reconsider those APIs and reimplement them differently. Differential Revision: [D24835252](https://our.internmc.facebook.com/intern/diff/D24835252/) [ghstack-poisoned]

codecov · 2020-12-07T00:49:08Z

Codecov Report

Merging #47712 (cfb9173) into gh/smessmer/267/base (1448178) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@                   Coverage Diff                    @@
##           gh/smessmer/267/base   #47712      +/-   ##
========================================================
- Coverage                 80.79%   80.79%   -0.01%     
========================================================
  Files                      1865     1865              
  Lines                    201099   201099              
========================================================
- Hits                     162473   162469       -4     
- Misses                    38626    38630       +4

bhosmer

Some comments inline, I'll follow up 1-1 :)

tools/codegen/api/cpp.py

test/cpp/api/modules.cpp

tools/codegen/api/types.py

tools/codegen/gen.py

test/cpp/api/modules.cpp

tools/codegen/api/dispatcher.py

tools/codegen/api/types.py

tools/codegen/api/dispatcher.py

ezyang

Still a bit nervous about m() and Functional(), but this seems good enough to go in.

bhosmer

Putting a changes requested to make sure we discuss the overloading issue (Functional() etc.), its possible downstream consequences and the cost of alternatives. Let's schedule a chat once you're settled.

The less consequential stuff (per earlier inline comments) we can potentially sort out post-PR, in conjunction with @ezyang's refactoring ideas.

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# . After this, an op will generate the following overloads for the C++ API: ```cpp // Generated from the aten::abs operator (NOT from aten::abs.out) Tensor at::abs(Tensor& self) // Generated from the aten::abs.out operator Tensor& at::abs(Tensor& self, Tensor& out) Tensor& at::abs_out(Tensor& out, Tensor& self) ``` This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet. It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes. The old API is still around and valid, so backward compatibility is not broken there. However, this does break bc in an a bit more unexpected way (in the same way as adding a new operator overload always breaks bc). Concretely, the `Tensor::m(at::my_op, args...)` only works if `at::my_op` doesn't have multiple overloads. Previously, out overloads were excluded from that rule and an operator that had one regular and one out overload still worked. This does now not work anymore because out overloads behave more like regular overloads. This also breaks `torch::nn::Functional(at::sigmoid)` for those ops. Furthermore, this will break any third party code that uses `at::my_op` as a symbol to get its function pointer without specifying the full argument list. Those broken APIs, however, only worked for ops without overloads previously and adding any overload would have broken them. A workaround for users to still use those APIs is to explicitly specify the argument list like `torch::nn::Functional(static_cast<Tensor(*)(const Tensor&)>(at::sigmoid))`. But this is super ugly. We probably should reconsider those APIs and reimplement them differently. Differential Revision: [D24835252](https://our.internmc.facebook.com/intern/diff/D24835252/) [ghstack-poisoned]

bhosmer

Cool - very happy to dodge the newly-overloaded ops risk.

couple nits inline
after this stack lands we should do a refactor based on Ed's "high level thinking" comment, which will ripple into here in a good way
I'll add an H1 item for the struct-overload-bundle idea

test/cpp/api/optim.cpp

test/cpp/api/serialize.cpp

tools/codegen/api/types.py

This adds a faithful API for ops with out arguments, as described in https://docs.google.com/document/d/1h7nBibRwkRLQ8rsPhfALlwWR0QbkdQm30u4ZBwmaps8/edit# . After this, an op will generate the following overloads for the C++ API: ```cpp // Generated from the aten::abs operator (NOT from aten::abs.out) Tensor at::abs(Tensor& self) // Generated from the aten::abs.out operator Tensor& at::abs(Tensor& self, Tensor& out) Tensor& at::abs_out(Tensor& out, Tensor& self) ``` This is an important step towards making those ops c10-full (it allows VariableType, XLA and other backends to ignore reordering and just call through with the same argument order), but this does not make any of those ops c10-full yet. It enables the faithful API independent from c10-fullness. That means the API is more consistent with the same API for all ops and making an op c10-full in the future will not trigger future C++ API changes. The old API is still around and valid, so backward compatibility is not broken there. However, this does break bc in an a bit more unexpected way (in the same way as adding a new operator overload always breaks bc). Concretely, the `Tensor::m(at::my_op, args...)` only works if `at::my_op` doesn't have multiple overloads. Previously, out overloads were excluded from that rule and an operator that had one regular and one out overload still worked. This does now not work anymore because out overloads behave more like regular overloads. This also breaks `torch::nn::Functional(at::sigmoid)` for those ops. Furthermore, this will break any third party code that uses `at::my_op` as a symbol to get its function pointer without specifying the full argument list. Those broken APIs, however, only worked for ops without overloads previously and adding any overload would have broken them. A workaround for users to still use those APIs is to explicitly specify the argument list like `torch::nn::Functional(static_cast<Tensor(*)(const Tensor&)>(at::sigmoid))`. But this is super ugly. We probably should reconsider those APIs and reimplement them differently. **edit**: To avoid breaking the APIs, we decided to introduce `at::myop_outf` overloads instead that represent the faithful version of out overloads. This means we don't do the API cleanup yet that this was planning, but there's plans to do that down the road the right way. Differential Revision: [D24835252](https://our.internmc.facebook.com/intern/diff/D24835252/) [ghstack-poisoned]

facebook-github-bot · 2020-12-08T13:14:34Z

This pull request has been merged in 3ef36dc.

facebook-github-bot added the cla signed label Nov 10, 2020

This was referenced Nov 10, 2020

Improve error messages for operator registration API #47636

Closed

Enable faithful API for all ops #47711

Closed

smessmer requested review from bhosmer and ezyang November 10, 2020 23:45

This was referenced Nov 11, 2020

VariableType calls faithful C++ API for c10-full out ops #47792

Closed

Make aten::div.out c10-full #47793

Closed

bhosmer reviewed Nov 11, 2020

View reviewed changes

ezyang reviewed Nov 18, 2020

View reviewed changes

tools/codegen/api/types.py Outdated Show resolved Hide resolved

ezyang mentioned this pull request Nov 18, 2020

Move argument grouping into FunctionSchema #48195

Closed

smessmer added 2 commits December 3, 2020 17:49

smessmer requested review from bhosmer and ezyang December 4, 2020 23:21

smessmer added the module: bc-breaking Related to a BC-breaking change label Dec 5, 2020

ezyang reviewed Dec 5, 2020

View reviewed changes

aten/src/ATen/test/basic.cpp Show resolved Hide resolved

ezyang reviewed Dec 5, 2020

View reviewed changes

tools/codegen/api/cpp.py Show resolved Hide resolved

smessmer requested a review from glaringlee as a code owner December 6, 2020 20:51

This was referenced Dec 7, 2020

Metaprogramming facilities to support hacky_wrapper to reorder function arguments #48910

Closed

hacky_wrapper_for_legacy_signatures reorders out arguments #48911

Closed

Make out ops c10-full (with hacky-wrapper) #48912

Closed

bhosmer reviewed Dec 7, 2020

View reviewed changes

tools/codegen/api/cpp.py Outdated Show resolved Hide resolved

test/cpp/api/modules.cpp Outdated Show resolved Hide resolved

tools/codegen/api/types.py Outdated Show resolved Hide resolved

tools/codegen/gen.py Show resolved Hide resolved