Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip][pt1][tensor] BlobGetMutableTensor returns Tensor #14136

Closed
wants to merge 2 commits into from

Conversation

jerryzh168
Copy link
Contributor

@jerryzh168 jerryzh168 commented Nov 17, 2018

Stack:
    :black_circle:  #14136 [wip][pt1][tensor] BlobGetMutableTensor returns Tensor  💛

Since now Tensor is a shared_ptr, it doesn't make sense to have Tensor* around anymore,
this diff is changing Tensor* to Tensor in the interface.
We'll store pointer to intrusive_ptr to TensorImpl in Tensor and create Tensor object on the fly
when returning from BlobGetMutableTensor.
Another motivation is we want Blob to store intrusive_ptr of TensorImpl because IValue can store intrusive ptr. Without this change, IValue will have to store Blob which is an extra level of indirection.

This diff should be rebased on the diffs that removes Resize/ResizeLike+mutable_data

To remove Tensor*, we'll do following

auto* Y = Ouptut(0);
Y->mutable_data...

-->

auto Y = Output(0);
Y.mutable_data...

But to run clangr codemod, we'll keep both APIs in different names, e.g. Output and XOutput, and do the refactor and then delete the old method and rename the new method into the old one.
For example for Output, we'll first codemod the callsites from Output to XOutput, then delete the old Output and rename XOutput to Output in the end.

Differential Revision: D12934074

Differential Revision: D12934074
Differential Version: 63869630

CAFFE_DEFINE_PREALLOCATED_KNOWN_TYPE(27, _CaffeHighestPreallocatedTypeId)
// 27 is TensorImplPtr see tensor.cc
CAFFE_DEFINE_PREALLOCATED_KNOWN_TYPE(28, _CaffeHighestPreallocatedTypeId)

This comment was marked as off-topic.

caffe2/core/tensor.h Outdated Show resolved Hide resolved
caffe2/core/operator.h Outdated Show resolved Hide resolved
@ezyang
Copy link
Contributor

ezyang commented Nov 20, 2018

This diff doesn't actually help. You've traded new Tensor for new IntrusiveTensorPtr, which is basically the same thing as Tensor. In particular, you've still got two indirections in blob.

@jerryzh168
Copy link
Contributor Author

@ezyang yeah, in terms of Blob->Tensor it's the same, this diff is useful for changing Blob to IValue, because right now IValue have to store a Blob -> Tensor and with this diff IValue can store IValue -> intrusive_ptr to TensorImpl. cc @smessmer for more details.

Differential Revision: D12934074
Differential Version: 64161092
@ezyang
Copy link
Contributor

ezyang commented Nov 21, 2018

So, is the point to change the signatures of the functions in question? That seems fine, but you don't need to actually change the internal implementation to change the signatures, no?

@jerryzh168
Copy link
Contributor Author

jerryzh168 commented Nov 21, 2018 via email

@dzhulgakov
Copy link
Collaborator

Hm, I agree with Ed - the change of what you store in Blob is independent from what the API of BlobGetMutableTensor is.

You can store the caffe2::Tensor and still extract intrusivePtr from it if necessary

@jerryzh168
Copy link
Contributor Author

@dzhulgakov I think we want to change both API and underlying implementation. not just API.

Copy link
Contributor

@smessmer smessmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff doesn't change anything in the indirections, before it was Blob stores Tensor*, Tensor stores intrusive_ptr<TensorImpl>. After this diff, it is Blob stores intrusive_ptr<TensorImpl>*, both are two indirections. This is, however, not the goal of this diff, and will be fixed once we move to IValue, where we would directly store TensorImpl*.

The goal of this diff is to unblock the Blob -> IValue merge. When IValue stores Tensors, it already does the right thing and stores them as TensorImpl*. I don't want to add the ability to store Tensor* to IValue. That means, however, that if we had an accessor function returning Tensor*, it would be a pointer to a temporary object and stuff would break. So we need all accessor functions to return Tensor by value. And this diff does that for Blob accessors, so we can then replace Blob with IValue. That's the API part.

Another thing is that this diff does is change the implementation. We want to be able to easily convert Blob into IValue and the other way round. While that is possible when Blob stores Tensor*, it's easier if they have the same way of storing Tensors. If there's a strong argument against changing the implementation, we don't have to, but I don't see why not.

@@ -5,6 +5,8 @@
namespace caffe2 {

CAFFE_DEFINE_PREALLOCATED_KNOWN_TYPE(12, Tensor);
using TensorImplPtr = c10::intrusive_ptr<at::TensorImpl, at::UndefinedTensorImpl>;

This comment was marked as off-topic.

This comment was marked as off-topic.

@jerryzh168
Copy link
Contributor Author

My plan is include the codemod in this diff as well, since we'll be able to remove the temporary function that starts with X after the codemod, or should I land this first?

@jerryzh168
Copy link
Contributor Author

Oh, actually I think we can't land the implementation change without also changing API.

@smessmer
Copy link
Contributor

Land this first, it's easier to review if the codemod is separate.

@ezyang
Copy link
Contributor

ezyang commented Nov 23, 2018

@smessmer The API change seems fine. But I'm still confused about the implementation change.

Another thing is that this diff does is change the implementation. We want to be able to easily convert Blob into IValue and the other way round. While that is possible when Blob stores Tensor*, it's easier if they have the same way of storing Tensors. If there's a strong argument against changing the implementation, we don't have to, but I don't see why not.

But even after this patch they don't have the same way of storing tensors. ivalue stores:

    c10::intrusive_ptr_target* as_intrusive_ptr;

That's not what this patch is changing the implementation to.

@jerryzh168
Copy link
Contributor Author

just talked with @smessmer, we'll keep this diff API change only, and we can postpone the implementation change until we are replacing Blob with IValue.

jerryzh168 added a commit to jerryzh168/pytorch that referenced this pull request Nov 28, 2018
Summary:
Pull Request resolved: pytorch#14424

Pull Request resolved: pytorch#14136

Since now Tensor is a shared_ptr, it doesn't make sense to have Tensor* around anymore,
so we want to change Tensor* to Tensor in the interface.
We added functions that work with `Tensor` instead of `Tensor*` in this diff.

To remove Tensor*, we'll do following
```
auto* Y = Ouptut(0);
Y->mutable_data...
```
-->
```
auto Y = Output(0);
Y.mutable_data...
```

But to run clangr codemod, we'll keep both APIs in different names, e.g. `Output` and `XOutput`, and do the refactor and then delete the old method and rename the new method into the old one.
For example for `Output`, we'll first codemod the callsites from `Output` to `XOutput`, then delete the old `Output` and rename `XOutput` to `Output` in the end.

Reviewed By: smessmer

Differential Revision: D12934074

fbshipit-source-id: 120778830835fc4d90286cf2ed00b4994cf32737
@soumith soumith deleted the export-D12934074 branch February 21, 2019 23:27
@ezyang ezyang added the merged label Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants