Move autograd metadata from VariableImpl to TensorImpl #13827

yf225 · 2018-11-11T16:49:29Z

Changes originally in this PR:

Move Variable::Impl data members into TensorImpl as AutogradMeta struct
Change Variable::Impl functions to use data members in AutogradMeta struct
Add shallow_copy_and_detach() function to each subclass of TensorImpl
Do shallow copy when the user calls make_variable(tensor) / make_variable_view(tensor) / variable.set_data(tensor) / variable.detach()

Changes moved from #13645:

Add a flag to Variable to disallow size/stride/storage_ptr changes from in-place operations such as resize_ / resize_as_ / set_ / transpose_, and set this flag to true when people call tensor.data in Python.
Write text in the docs to actively discourage changing the shape or storage of tensor_detached and expecting tensor to also be updated.

This is the 1st+2nd PR mentioned in #13638.

torch/csrc/autograd/variable.cpp

torch/csrc/autograd/variable.h

torch/csrc/autograd/variable.cpp

torch/tensor.py

aten/src/ATen/core/TensorImpl.h

aten/src/TH/THTensor.cpp

test/test_sparse.py

@@ -1710,6 +1710,16 @@ def test_is_nonzero(self):
        with self.assertRaisesRegex(RuntimeError, "bool value of Tensor with no values is ambiguous"):
            torch.sparse_coo_tensor(([0, 1],), self.ValueTensor(2, 0), (4, 0)).is_nonzero()

+    def test_allow_size_or_storage_change(self):
+        def do_test(t):
+            a = self.SparseTensor(3, 3)


test/test_torch.py

@@ -9321,6 +9321,26 @@ def test_reverse_binary_ops_multiple_device(self):
        with self.assertRaisesRegex(RuntimeError, "expected both inputs to be on same device"):
            torch.tensor(2).to("cuda:1") // torch.tensor(3).to("cuda:0")

+    def test_allow_size_or_storage_change(self):


torch/csrc/autograd/python_variable.cpp

@@ -105,7 +105,7 @@ static int THPVariable_traverse(THPVariable *self, visitproc visit, void *arg)
 static int THPVariable_clear(THPVariable *self)
 {
  Py_CLEAR(self->backward_hooks);
-  if (self->cdata.defined()) {
+  if (self->cdata.defined() && self->cdata.get_autograd_meta()) {


torch/csrc/autograd/python_variable.cpp

aten/src/ATen/core/TensorImpl.h

facebook-github-bot

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

ezyang · 2018-12-18T21:03:28Z

Hi Will, do you want a rereview? If so, what's new?

yf225 · 2018-12-18T21:08:36Z

@ezyang the new change is mostly 859aa38, where we pass an argument to make_variable()/make_variable_view() to control the value of allow_tensor_metadata_change. Also wanted to wait for Greg's another pass at the PR. cc @gchanan

gchanan · 2018-12-20T05:35:17Z

test/test_jit.py

@@ -4859,7 +4859,7 @@ class DerivedStateModule(torch.jit.ScriptModule):
        def __init__(self):
            super(TestScript.DerivedStateModule, self).__init__()
            self.param = torch.nn.Parameter(torch.ones(3, 4, dtype=torch.float))
-            self.register_buffer('derived', torch.neg(self.param).detach())
+            self.register_buffer('derived', torch.neg(self.param).detach().clone())


what's going on here? Is the detached thing being inplace modified later?

@gchanan yes self.derived is being in-place modified in https://github.com/pytorch/pytorch/pull/13827/files#diff-78aab5b4217aeaed920efab0c5f5f5c8R4904 and https://github.com/pytorch/pytorch/pull/13827/files#diff-78aab5b4217aeaed920efab0c5f5f5c8R4909

torch/tensor.py

gchanan

it would be nice if you could track down more places where we could set allow_tensor_metadata_change (is that even possible?). But that could be a follow-up (again, assuming it's possible).

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

yf225 · 2018-12-22T03:26:52Z

Ran into a weird use-after-free error in internal tests. Currently investigating.

facebook-github-bot

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

facebook-github-bot

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

Summary: Changes originally in this PR: 1. Move Variable::Impl data members into TensorImpl as `AutogradMeta` struct 2. Change Variable::Impl functions to use data members in `AutogradMeta` struct 3. Add `shallow_copy_and_detach()` function to each subclass of TensorImpl 4. Do shallow copy when the user calls `make_variable(tensor)` / `make_variable_view(tensor)` / `variable.set_data(tensor)` / `variable.detach()` Changes moved from pytorch/pytorch#13645: 1. Add a flag to Variable to disallow size/stride/storage_ptr changes from in-place operations such as `resize_` / `resize_as_` / `set_` / `transpose_`, and set this flag to true when people call `tensor.data` in Python. 2. Write text in the docs to actively discourage changing the shape or storage of `tensor_detached` and expecting `tensor` to also be updated. This is the 1st+2nd PR mentioned in pytorch/pytorch#13638. Pull Request resolved: pytorch/pytorch#13827 Differential Revision: D13507173 Pulled By: yf225 fbshipit-source-id: b177b08438d534a8197e34e1ad4a837e2db0ed6a

yf225 force-pushed the tensorimpl_autogradmeta branch from 40e3575 to 18e0875 Compare November 11, 2018 16:50

yf225 closed this Nov 11, 2018

yf225 reopened this Nov 11, 2018

yf225 force-pushed the tensorimpl_autogradmeta branch from eb509d9 to 8c2d05b Compare November 12, 2018 19:23

yf225 changed the title ~~[WIP] Move autograd metadata from VariableImpl to TensorImpl~~ Move autograd metadata from VariableImpl to TensorImpl Nov 12, 2018

yf225 requested review from gchanan and ezyang November 12, 2018 19:28

yf225 mentioned this pull request Nov 13, 2018

Variable/Tensor Merge Proposal #13638

Closed

22 tasks

yf225 force-pushed the tensorimpl_autogradmeta branch 2 times, most recently from 82d723d to 4e4a9f7 Compare November 13, 2018 18:21

yf225 mentioned this pull request Nov 13, 2018

Add flag for disallowing shape or storage change for tensor from .data or .detach() #13645

Closed

yf225 force-pushed the tensorimpl_autogradmeta branch 6 times, most recently from ac524cd to 6d74b6a Compare November 13, 2018 20:48

apaszke reviewed Nov 13, 2018

View reviewed changes

torch/csrc/autograd/variable.cpp Show resolved Hide resolved

torch/csrc/autograd/variable.h Outdated Show resolved Hide resolved

torch/csrc/autograd/variable.h Outdated Show resolved Hide resolved

torch/csrc/autograd/variable.cpp Outdated Show resolved Hide resolved

yf225 force-pushed the tensorimpl_autogradmeta branch from a911dfa to 666bd74 Compare November 13, 2018 22:18

yf225 mentioned this pull request Nov 13, 2018

[REVIEW ONLY] Move autograd functions from VariableImpl to Variable #13932

Closed

yf225 force-pushed the tensorimpl_autogradmeta branch from 666bd74 to 918b052 Compare November 14, 2018 16:51

gchanan reviewed Nov 14, 2018

View reviewed changes

yf225 force-pushed the tensorimpl_autogradmeta branch 3 times, most recently from aa46050 to 46e7780 Compare November 15, 2018 17:29

yf225 commented Nov 16, 2018

View reviewed changes

aten/src/ATen/core/TensorImpl.h Outdated Show resolved Hide resolved

yf225 force-pushed the tensorimpl_autogradmeta branch 4 times, most recently from 52b58f7 to ba1abc4 Compare November 20, 2018 05:09

fix merge

8a43cf6

yf225 force-pushed the tensorimpl_autogradmeta branch from 8159235 to 8a43cf6 Compare December 17, 2018 19:24

Will Feng added 2 commits December 17, 2018 21:08

Fix make_variable_view flag passing

42d98b9

fix TestScript.test_pack_unpack_state

22618fb

facebook-github-bot reviewed Dec 18, 2018

View reviewed changes

gchanan reviewed Dec 20, 2018

View reviewed changes

torch/tensor.py Outdated Show resolved Hide resolved

gchanan approved these changes Dec 20, 2018

View reviewed changes

Merge branch 'master' into tensorimpl_autogradmeta

debac5f

facebook-github-bot reviewed Dec 20, 2018

View reviewed changes

Will Feng added 3 commits December 20, 2018 13:45

improve comment

fbb1f70

add test cases for sparse zero_ and copy_

e67cde8

Merge branch 'master' into tensorimpl_autogradmeta

7e5e205

facebook-github-bot reviewed Dec 20, 2018

View reviewed changes

facebook-github-bot reviewed Dec 21, 2018

View reviewed changes

This was referenced Dec 21, 2018

[PLEASE IGNORE] Move VariableImpl functions to AutogradMeta #15486

Closed

Move VariableImpl functions to AutogradMeta and Variable #15487

Closed

Merge branch 'master' into tensorimpl_autogradmeta

faabb1d

facebook-github-bot reviewed Dec 26, 2018

View reviewed changes

Merge branch 'master' into tensorimpl_autogradmeta

4753702

facebook-github-bot reviewed Dec 26, 2018

View reviewed changes

facebook-github-bot closed this in 7b87eca Dec 27, 2018

kmatzen mentioned this pull request Jan 3, 2019

fix DistributedDataParallel for VariableImpl to TensorImpl redesign #15703

Closed

ezyang added the merged label Jun 25, 2019

sigeisler mentioned this pull request Mar 30, 2020

Error when loading a sparse tensor parameter from a state_dict in pytorch 1.1.0 #22616

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move autograd metadata from VariableImpl to TensorImpl #13827

Move autograd metadata from VariableImpl to TensorImpl #13827

yf225 commented Nov 11, 2018 •

edited

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

facebook-github-bot left a comment

ezyang commented Dec 18, 2018

yf225 commented Dec 18, 2018

gchanan Dec 20, 2018

yf225 Dec 20, 2018

gchanan left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

yf225 commented Dec 22, 2018

facebook-github-bot left a comment

facebook-github-bot left a comment

Move autograd metadata from VariableImpl to TensorImpl #13827

Move autograd metadata from VariableImpl to TensorImpl #13827

Conversation

yf225 commented Nov 11, 2018 • edited Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

facebook-github-bot left a comment

Choose a reason for hiding this comment

ezyang commented Dec 18, 2018

yf225 commented Dec 18, 2018

gchanan Dec 20, 2018

Choose a reason for hiding this comment

yf225 Dec 20, 2018

Choose a reason for hiding this comment

gchanan left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

yf225 commented Dec 22, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

yf225 commented Nov 11, 2018 •

edited

Loading