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


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


test/test_torch.py

        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

 {
  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

            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 ezyang and gchanan 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 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

Move autograd metadata from VariableImpl to TensorImpl #13827

Move autograd metadata from VariableImpl to TensorImpl #13827

Uh oh!

Conversation

yf225 commented Nov 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Dec 18, 2018

Uh oh!

yf225 commented Dec 18, 2018

Uh oh!

gchanan Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

yf225 Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gchanan left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 commented Dec 22, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yf225 commented Nov 11, 2018 •

edited

Loading