Skip to content

Commit 8cde4c4

Browse files
Will Fengfacebook-github-bot
Will Feng
authored and
facebook-github-bot
committed
Remove Variable::Impl and DifferentiableViewImpl (#17072)
Summary: As part of the Variable/Tensor merge work: #13638, we make the following changes in this PR: 1. Remove the `Variable::Impl` class and the `DifferentiableViewImpl` class 2. Change all `Variable.data()` call sites to either use `Variable` directly, or use `Variable.tensor_data()` 3. Remove `Variable.data()` API 3. Add `Variable.variable_data()` that matches `tensor.data` in Python API, which creates a new `Variable` that shares the same storage and tensor metadata with the original `Variable`, but with a completely new autograd history. After this PR, Variable doesn't wrap a Tensor internally anymore, and both Variable and Tensor use the same TensorImpl class as its `impl_`. The only difference is that Variable always has AutogradMeta in its TensorImpl, but Tensor doesn't. **Note that this PR is BC-breaking in the following use cases:** **Use Case 1:** Previously, `x.data = y` works even if `x` and `y` are of different TensorImpl type (e.g. `x` is a CPU dense tensor whose impl is of type TensorImpl, while `y` is a CPU sparse tensor whose impl is of type SparseTensorImpl). However, after this PR, `x.data = y` doesn't work anymore if `x` and `y` are of different TensorImpl type, because the underlying implementation `variable.set_data(tensor)` no longer works if `variable` and `tensor` have different TensorImpl type. **Use Case 2:** If a tensor `x`'s `grad` is sparse, accumulating dense gradients to `x` will change the tensor that `x.grad` is pointing to. This is better illustrated with the following example: ```python params = torch.tensor([1.5, 1.5]).requires_grad_() with torch.no_grad(): # Change gradient to a sparse tensor params.grad = torch.sparse_coo_tensor(torch.tensor([[1, 1]]).long(), torch.tensor([1., 1.])) grad_saved = params.grad params.backward(torch.tensor([1.5, 1.5])) assert id(grad_saved) == id(params.grad) # This will fail after this PR ``` The assertion in the last line will fail after this PR, because adding dense gradients to sparse gradients will change the `params.grad` tensor reference. Pull Request resolved: #17072 Differential Revision: D14075257 Pulled By: yf225 fbshipit-source-id: 0e681df641270dea586042dd26db59f2e76b5957
1 parent f93e061 commit 8cde4c4

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

49 files changed

+555
-484
lines changed

‎aten/src/ATen/Context.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,12 @@ bool Context::setFlushDenormal(bool on) {
112112
return at::cpu::set_flush_denormal(on);
113113
}
114114

115+
// NOTE: We also check `at::NonVariableTypeMode`, and if it's enabled we always
116+
// return non-Variable type in this function.
117+
// See NOTE [ Treating Variables as non-Variables in type dispatch ]
115118
TypeExtendedInterface& getType(TensorOptions options) {
116119
return globalContext().getType(
117-
options.backend(), typeMetaToScalarType(options.dtype()), options.is_variable());
120+
options.backend(), typeMetaToScalarType(options.dtype()), options.is_variable() && !at::NonVariableTypeMode::is_enabled());
118121
}
119122

120123
// NOTE: We also check `at::NonVariableTypeMode`, and if it's enabled we always

‎aten/src/ATen/OpaqueTensorImpl.h‎

Lines changed: 54 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -77,41 +77,66 @@ struct CAFFE2_API OpaqueTensorImpl : public TensorImpl {
7777
AT_ERROR("opaque tensors do not have storage");
7878
}
7979

80-
// NOTE: `shallow_copy_and_detach()` does not copy the following TensorImpl fields:
81-
// 1. the AutogradMeta pointer, because it is unique for each Variable.
82-
// 2. the version counter, because it is set to the passed in `version_counter`.
83-
// See NOTE [ Version Counter Sharing ] for details.
84-
//
85-
// NOTE: `allow_tensor_metadata_change` determines whether the TensorImpl shallow-copy
86-
// allows changes to its metadata (e.g. sizes / strides / storage / storage_offset).
87-
// See NOTE [ Metadata Change for a Detached Tensor ] for details.
88-
c10::intrusive_ptr<TensorImpl> shallow_copy_and_detach(
89-
const c10::VariableVersion& version_counter,
90-
bool allow_tensor_metadata_change) const override {
91-
//AT_ASSERT(false);
92-
auto impl = c10::make_intrusive<OpaqueTensorImpl<OpaqueHandle>>(
93-
type_id(), dtype(), device(), opaque_handle_, sizes_);
94-
// TensorImpl general fields
95-
// Note that some of these fields are not used in opaque tensor code,
96-
// and we copy them here only for completeness.
97-
impl->sizes_ = sizes_;
98-
impl->strides_ = strides_;
99-
impl->storage_offset_ = storage_offset_;
100-
impl->is_contiguous_ = is_contiguous_;
101-
impl->is_wrapped_number_ = is_wrapped_number_;
102-
impl->reserved_ = reserved_;
103-
impl->set_version_counter(version_counter);
104-
impl->set_allow_tensor_metadata_change(allow_tensor_metadata_change);
105-
106-
// OpaqueTensorImpl-specific fields (none currently).
107-
return impl;
108-
}
80+
/**
81+
* Return a TensorImpl that is a shallow-copy of this TensorImpl.
82+
*
83+
* For usage of `version_counter` and `allow_tensor_metadata_change`,
84+
* see NOTE [ TensorImpl Shallow-Copying ].
85+
*/
86+
c10::intrusive_ptr<TensorImpl> shallow_copy_and_detach(
87+
const c10::VariableVersion& version_counter,
88+
bool allow_tensor_metadata_change) const override {
89+
auto impl = c10::make_intrusive<OpaqueTensorImpl<OpaqueHandle>>(
90+
type_id(), dtype(), device(), opaque_handle_, sizes_);
91+
copy_tensor_data(
92+
/*src_impl=*/this,
93+
/*dest_impl=*/impl.get(),
94+
/*version_counter=*/version_counter,
95+
/*allow_tensor_metadata_change=*/allow_tensor_metadata_change);
96+
impl->refresh_numel();
97+
return impl;
98+
}
99+
100+
/**
101+
* Shallow-copies data from another TensorImpl into this TensorImpl.
102+
*
103+
* For why this function doesn't check this TensorImpl's `allow_tensor_metadata_change_`,
104+
* see NOTE [ TensorImpl Shallow-Copying ].
105+
*/
106+
void shallow_copy_from(const c10::intrusive_ptr<TensorImpl>& impl) override {
107+
AT_ASSERT(typeid(*(impl.get())) == typeid(OpaqueTensorImpl<OpaqueHandle>));
108+
auto opaque_impl = static_cast<const OpaqueTensorImpl<OpaqueHandle>*>(impl.get());
109+
copy_tensor_data(
110+
/*src_impl=*/opaque_impl,
111+
/*dest_impl=*/this,
112+
/*version_counter=*/version_counter(),
113+
/*allow_tensor_metadata_change=*/allow_tensor_metadata_change());
114+
refresh_numel();
115+
}
116+
109117
OpaqueHandle& unsafe_opaque_handle() {
110118
return opaque_handle_;
111119
}
112120

113121
private:
114122
OpaqueHandle opaque_handle_;
123+
124+
/**
125+
* Copy the storage pointer and the tensor metadata fields (e.g. sizes / strides / storage_offset)
126+
* from one TensorImpl to another TensorImpl.
127+
*
128+
* For usage of `version_counter` and `allow_tensor_metadata_change`, see NOTE [ TensorImpl Shallow-Copying ].
129+
*/
130+
static void copy_tensor_data(
131+
const OpaqueTensorImpl<OpaqueHandle>* src_opaque_impl,
132+
OpaqueTensorImpl<OpaqueHandle>* dest_opaque_impl,
133+
const c10::VariableVersion& version_counter,
134+
bool allow_tensor_metadata_change) {
135+
TensorImpl::copy_tensor_data(src_opaque_impl, dest_opaque_impl, version_counter, allow_tensor_metadata_change);
136+
137+
// OpaqueTensorImpl-specific fields.
138+
dest_opaque_impl->opaque_handle_ = src_opaque_impl->opaque_handle_;
139+
}
115140
};
116141

117142
} // namespace at

‎aten/src/ATen/SparseTensorImpl.h‎

Lines changed: 49 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -183,41 +183,64 @@ struct CAFFE2_API SparseTensorImpl : public TensorImpl {
183183
// make it happen
184184
void set_indices_and_values_unsafe(const Tensor& indices, const Tensor& values);
185185

186-
// NOTE: `shallow_copy_and_detach()` does not copy the following TensorImpl fields:
187-
// 1. the AutogradMeta pointer, because it is unique for each Variable.
188-
// 2. the version counter, because it is set to the passed in `version_counter`.
189-
// See NOTE [ Version Counter Sharing ] for details.
190-
//
191-
// NOTE: `allow_tensor_metadata_change` determines whether the TensorImpl shallow-copy
192-
// allows changes to its metadata (e.g. sizes / strides / storage / storage_offset).
193-
// See NOTE [ Metadata Change for a Detached Tensor ] for details.
186+
/**
187+
* Return a TensorImpl that is a shallow-copy of this TensorImpl.
188+
*
189+
* For usage of `version_counter` and `allow_tensor_metadata_change`,
190+
* see NOTE [ TensorImpl Shallow-Copying ].
191+
*/
194192
c10::intrusive_ptr<TensorImpl> shallow_copy_and_detach(
195193
const c10::VariableVersion& version_counter,
196194
bool allow_tensor_metadata_change) const override {
197195
auto impl = c10::make_intrusive<SparseTensorImpl>(type_id(), dtype());
198-
// TensorImpl general fields
199-
// Note that these fields are not used in sparse tensor code, and we copy them here only for completeness.
200-
impl->sizes_ = sizes_;
201-
impl->strides_ = strides_;
202-
impl->storage_offset_ = storage_offset_;
203-
impl->is_contiguous_ = is_contiguous_;
204-
impl->is_wrapped_number_ = is_wrapped_number_;
205-
impl->reserved_ = reserved_;
206-
impl->set_version_counter(version_counter);
207-
impl->set_allow_tensor_metadata_change(allow_tensor_metadata_change);
208-
209-
// Sparse-specific fields
210-
impl->sparse_dim_ = sparse_dim();
211-
impl->dense_dim_ = dense_dim();
212-
impl->indices_ = indices();
213-
impl->values_ = values();
214-
impl->device_opt_ = device();
215-
impl->coalesced_ = coalesced();
196+
copy_tensor_data(
197+
/*src_impl=*/this,
198+
/*dest_impl=*/impl.get(),
199+
/*version_counter=*/version_counter,
200+
/*allow_tensor_metadata_change=*/allow_tensor_metadata_change);
216201
impl->refresh_numel();
217202
return impl;
218203
}
204+
205+
/**
206+
* Shallow-copies data from another TensorImpl into this TensorImpl.
207+
*
208+
* For why this function doesn't check this TensorImpl's `allow_tensor_metadata_change_`,
209+
* see NOTE [ TensorImpl Shallow-Copying ].
210+
*/
211+
void shallow_copy_from(const c10::intrusive_ptr<TensorImpl>& impl) override {
212+
AT_ASSERT(typeid(*(impl.get())) == typeid(SparseTensorImpl));
213+
auto sparse_impl = static_cast<const SparseTensorImpl*>(impl.get());
214+
copy_tensor_data(
215+
/*src_impl=*/sparse_impl,
216+
/*dest_impl=*/this,
217+
/*version_counter=*/version_counter(),
218+
/*allow_tensor_metadata_change=*/allow_tensor_metadata_change());
219+
refresh_numel();
220+
}
219221
private:
220222
explicit SparseTensorImpl(at::TensorTypeId, const caffe2::TypeMeta&, at::Tensor indices, at::Tensor values);
223+
224+
/**
225+
* Copy the storage pointer and the tensor metadata fields (e.g. sizes / strides / storage_offset)
226+
* from one TensorImpl to another TensorImpl.
227+
*
228+
* For usage of `version_counter` and `allow_tensor_metadata_change`, see NOTE [ TensorImpl Shallow-Copying ].
229+
*/
230+
static void copy_tensor_data(
231+
const SparseTensorImpl* src_sparse_impl,
232+
SparseTensorImpl* dest_sparse_impl,
233+
const c10::VariableVersion& version_counter,
234+
bool allow_tensor_metadata_change) {
235+
TensorImpl::copy_tensor_data(src_sparse_impl, dest_sparse_impl, version_counter, allow_tensor_metadata_change);
236+
237+
// Sparse-specific fields
238+
dest_sparse_impl->sparse_dim_ = src_sparse_impl->sparse_dim();
239+
dest_sparse_impl->dense_dim_ = src_sparse_impl->dense_dim();
240+
dest_sparse_impl->indices_ = src_sparse_impl->indices();
241+
dest_sparse_impl->values_ = src_sparse_impl->values();
242+
dest_sparse_impl->coalesced_ = src_sparse_impl->coalesced();
243+
}
221244
};
222245

223246
} // namespace at

‎aten/src/ATen/native/Activation.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,11 @@ Tensor & celu_(Tensor & self, Scalar alpha) {
4242
}
4343

4444
Tensor rrelu(const Tensor & self, Scalar lower, Scalar upper, bool training, Generator* generator) {
45-
return at::rrelu_with_noise(self, at::empty({0}, self.options()), lower, upper, training, generator);
45+
return at::rrelu_with_noise(self, at::empty_like(self), lower, upper, training, generator);
4646
}
4747

4848
Tensor & rrelu_(Tensor & self, Scalar lower, Scalar upper, bool training, Generator* generator) {
49-
return at::rrelu_with_noise_(self, at::empty({0}, self.options()), lower, upper, training, generator);
49+
return at::rrelu_with_noise_(self, at::empty_like(self), lower, upper, training, generator);
5050
}
5151

5252
// computes `result = self <= threshold ? value : other`

‎aten/src/ATen/native/LegacyDefinitions.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ namespace at { namespace native {
99
// Methods
1010

1111
void* data_ptr(const Tensor & self) {
12-
return self.unsafeGetTensorImpl()->slow_data();
12+
return self.unsafeGetTensorImpl()->data();
1313
}
1414

1515
Tensor & set_(Tensor& self, Storage source) {

‎aten/src/ATen/quantized/QTensorImpl.h‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,24 +25,64 @@ struct CAFFE2_API QTensorImpl : public c10::TensorImpl {
2525
return quantizer_;
2626
}
2727

28+
/**
29+
* Return a TensorImpl that is a shallow-copy of this TensorImpl.
30+
*
31+
* For usage of `version_counter` and `allow_tensor_metadata_change`,
32+
* see NOTE [ TensorImpl Shallow-Copying ].
33+
*/
2834
c10::intrusive_ptr<TensorImpl> shallow_copy_and_detach(
2935
const c10::VariableVersion& version_counter,
3036
bool allow_tensor_metadata_change) const override {
3137
auto impl = c10::make_intrusive<QTensorImpl>(
3238
Storage(storage()), type_id(), quantizer_);
33-
impl->set_sizes_and_strides(sizes(), strides());
34-
impl->storage_offset_ = storage_offset_;
35-
impl->is_wrapped_number_ = is_wrapped_number_;
36-
impl->reserved_ = reserved_;
39+
copy_tensor_data(
40+
/*src_impl=*/this,
41+
/*dest_impl=*/impl.get(),
42+
/*version_counter=*/version_counter,
43+
/*allow_tensor_metadata_change=*/allow_tensor_metadata_change);
3744
impl->refresh_numel();
3845
impl->refresh_contiguous();
39-
impl->set_version_counter(version_counter);
40-
impl->set_allow_tensor_metadata_change(allow_tensor_metadata_change);
4146
return impl;
4247
}
4348

49+
/**
50+
* Shallow-copies data from another TensorImpl into this TensorImpl.
51+
*
52+
* For why this function doesn't check this TensorImpl's `allow_tensor_metadata_change_`,
53+
* see NOTE [ TensorImpl Shallow-Copying ].
54+
*/
55+
void shallow_copy_from(const c10::intrusive_ptr<TensorImpl>& impl) override {
56+
AT_ASSERT(typeid(*(impl.get())) == typeid(QTensorImpl));
57+
auto q_impl = static_cast<const QTensorImpl*>(impl.get());
58+
copy_tensor_data(
59+
/*src_impl=*/q_impl,
60+
/*dest_impl=*/this,
61+
/*version_counter=*/version_counter(),
62+
/*allow_tensor_metadata_change=*/allow_tensor_metadata_change());
63+
refresh_numel();
64+
refresh_contiguous();
65+
}
66+
4467
private:
4568
QuantizerPtr quantizer_;
69+
70+
/**
71+
* Copy the storage pointer and the tensor metadata fields (e.g. sizes / strides / storage_offset)
72+
* from one TensorImpl to another TensorImpl.
73+
*
74+
* For usage of `version_counter` and `allow_tensor_metadata_change`, see NOTE [ TensorImpl Shallow-Copying ].
75+
*/
76+
static void copy_tensor_data(
77+
const QTensorImpl* src_q_impl,
78+
QTensorImpl* dest_q_impl,
79+
const c10::VariableVersion& version_counter,
80+
bool allow_tensor_metadata_change) {
81+
TensorImpl::copy_tensor_data(src_q_impl, dest_q_impl, version_counter, allow_tensor_metadata_change);
82+
83+
// OpaqueTensorImpl-specific fields.
84+
dest_q_impl->quantizer_ = src_q_impl->quantizer_;
85+
}
4686
};
4787

4888
} // namespace at

‎aten/src/TH/THTensor.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,7 @@ void THTensor_stealAndSetStoragePtr(THTensor* tensor, THStorage* storage) {
164164
// Caffe2 also has uninitialized dtype states, which we disallow here
165165
AT_ASSERT(tensor->storage().dtype() == storage->dtype());
166166

167-
// We used to allow this, but this breaks device caching,
168-
// see Note [We regret making Variable hold a Tensor]
167+
// We used to allow this, but this breaks device caching.
169168
// Let's put an actual error message for this one.
170169
TORCH_CHECK(tensor->storage().device() == storage->device(),
171170
"Attempted to set the storage of a tensor on device \"", tensor->storage().device(),

‎c10/core/TensorImpl.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ bool TensorImpl::compute_contiguous() const {
8282
}
8383

8484
void TensorImpl::release_resources() {
85+
autograd_meta_.reset();
8586
if (storage_) {
8687
storage_ = {};
8788
}

0 commit comments

Comments
 (0)