[pytorch] Add return type for list of tensors by larryliu0820 · Pull Request #76049 · pytorch/pytorch

larryliu0820 · 2022-04-19T16:54:52Z

Summary:
Adding codegen logic to return at::TensorList as a return type.

Although we don't really have a return type of -> Tensor(a)[] in native_functions.yaml, there are use cases for a TensorList return type.

If the type is a list and the element type is tensor, return a at::TensorList. Since the copy cost of an ArrayRef is very small, we don't need to return a reference.

Test Plan: Rely on unit test.

Reviewed By: iseeyuan

Differential Revision: D35737310

facebook-github-bot · 2022-04-19T16:54:58Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/76049
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

💊 CI failures summary and remediations

As of commit 952d148 (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).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

facebook-github-bot · 2022-04-19T16:55:15Z

This pull request was exported from Phabricator. Differential Revision: D35737310

facebook-github-bot · 2022-04-19T18:01:17Z

This pull request was exported from Phabricator. Differential Revision: D35737310

facebook-github-bot · 2022-04-19T18:40:44Z

This pull request was exported from Phabricator. Differential Revision: D35737310

ezyang · 2022-04-20T01:18:54Z

I kind of want to understand if this is just because this is the path of least resistance, or there really is an important reason to want to return a mutable list of tensors.

To explain, we have to first understand why inplace operations like neg_ have the signature neg_(Tensor(a!) self) -> Tensor(a!). The return argument is guaranteed to be exactly the same as the input argument. So technically, the signature could also be neg_(Tensor(a!) self) -> (). It is not expressed in this way for two reasons:

In the user facing API, inplace operations return themselves to make it easier to do method chaining, e.g., neg_(...).add_(...)
In TorchScript interpreter, we are operating a stack machine, and whether or not we "return" the argument says whether or not we pop the input argument off the stack, or keep it on the stack (so method chaining could compile to marginally more efficient code). This is very very marginal and I don't think it matters.

With lists of tensors, the method chaining justification doesn't apply at all. I don't think you have any interpreter-y things. So as far as I'm concerned, these functions can just return void. In fact, returning a list of tensors is problematic. If the input tensor list is not contiguous (as is the case for IValue tensor list), you have to reallocate it for output (incurring refcount bumps).

So yes, there is a bit of inconsistency if we return void. But I believe that we can make it work out without too much effort.

Summary: Pull Request resolved: pytorch#76049 Adding codegen logic to return `at::TensorList` as a return type. Although we don't really have a return type of `-> Tensor(a)[]` in `native_functions.yaml`, there are use cases for a `TensorList` return type. If the type is a list and the element type is tensor, return a `at::TensorList`. Since the copy cost of an `ArrayRef` is very small, we don't need to return a reference. Test Plan: Rely on unit test. Reviewed By: iseeyuan Differential Revision: D35737310 fbshipit-source-id: 94c7d36c7a52697d0f3b2736232b0fe8cdfd258e

facebook-github-bot · 2022-04-20T16:16:50Z

This pull request was exported from Phabricator. Differential Revision: D35737310

…r of returns (#76049) Summary: Pull Request resolved: #76049 ## Context We are trying to add an out variant for an existing operator, e.g.,: ``` chunk.out(Tensor self, int chunks, int dim=0, *, Tensor(a!)[] out) -> Tensor(a!)[] ``` Notice the out argument is a mutable list of tensors. The existing guideline defined in [model.py](https://fburl.com/nn299ifx) requires the same argument type to be returned from this operator. Given the fact that we don't support mutable tensor list as a return type and it seems not useful to add such a return type. The solution I'm proposing is to relax the constraint that the number of outs needs to be the same as the number of returns, so we can return a `void`. ``` chunk.out(Tensor self, int chunks, int dim=0, *, Tensor(a!)[] out) -> () ``` Test Plan: Rely on existing CI Reviewed By: ezyang, iseeyuan Differential Revision: D35737310 fbshipit-source-id: 66b5738cc1dcd13d532a6c97fea979bd58f381df

github-actions · 2022-04-20T23:11:15Z

Hey @larryliu0820.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

…s, clean up mutable return type checks" This PR cleans up some of the codegen type checks / special casing for foreach ops. In particular, as described in #76049, schemas that mutate their inputs and return the mutated input directly as an output are only really useful for method chaining. If your schema involves a mutable input that you can't easily method chain on (like a `Tensor?` or a `Tensor[]`), then we expect the return type to be void. We actually also have to allow return types of tuples of mutable tensors, since we have a bunch of ops that fit that description today. [ghstack-poisoned]

…ble return type checks" This PR cleans up some of the codegen type checks / special casing for foreach ops. In particular, as described in #76049, schemas that mutate their inputs and return the mutated input directly as an output are only really useful for method chaining. If your schema involves a mutable input that you can't easily method chain on (like a `Tensor?` or a `Tensor[]`), then we expect the return type to be void. We actually also have to allow return types of tuples of mutable tensors, since we have a bunch of ops that fit that description today. [ghstack-poisoned]

…s, clean up mutable return type checks" This PR cleans up some of the codegen type checks / special casing for foreach ops. In particular, as described in #76049, schemas that mutate their inputs and return the mutated input directly as an output are only really useful for method chaining. If your schema involves a mutable input that you can't easily method chain on (like a `Tensor?` or a `Tensor[]`), then we expect the return type to be void. We actually also have to allow return types of tuples of mutable tensors, since we have a bunch of ops that fit that description today. [ghstack-poisoned]

…ble return type checks" This PR cleans up some of the codegen type checks / special casing for foreach ops. In particular, as described in #76049, schemas that mutate their inputs and return the mutated input directly as an output are only really useful for method chaining. If your schema involves a mutable input that you can't easily method chain on (like a `Tensor?` or a `Tensor[]`), then we expect the return type to be void. We actually also have to allow return types of tuples of mutable tensors, since we have a bunch of ops that fit that description today. [ghstack-poisoned]

…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]

…n about tensorlist out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]