[BC-breaking] Shallow-copy indices and values in sparse tensor ctor #20330

yf225 · 2019-05-09T18:48:30Z

After the Variable/Tensor merge, there is no guarantee that indices and values passed into the sparse tensor constructor don't contain AutogradMeta. However, we want to maintain the existing invariant that indices_ and values_ of a sparse tensor don't contain AutogradMeta, and to achieve this we need do shallow-copy in the sparse tensor constructor.

Note that this is BC-breaking for code that changes the sizes / strides of the indices or values tensor after it's used to create a sparse tensor. In current master, such changes will be reflected in the sparse tensor and break sparse tensor invariants. After this PR, those changes will not be reflected in the sparse tensor, and thus the sparse tensor invariants are always preserved. Specifically, running in-place size/stride-changing ops such as resize_ / resize_as_ / as_strided_ / set_ / transpose_ on the original values tensor will not update the sparse tensor's values_. For example:

# Calling resize_ on non-requires-grad value tensor
i2 = torch.zeros([1, 1])
v2 = torch.ones([1, 2, 3])
t2 = torch.sparse_coo_tensor(i2, v2, torch.Size([2, 2, 3]))
v2.resize_(4, 5)
t2.coalesce().values().size()
# On current master, this throws "indices and values must have same nnz, but got nnz from indices: 1, nnz from values: 4", because resizing the original value tensor affects `values_` of the sparse tensor.
# After this PR, this prints "torch.Size([1, 2, 3])", which means resizing the original value tensor doesn't affect `values_` of the sparse tensor.

yf225 · 2019-05-09T20:25:55Z

@pytorchbot rebase this please

gchanan · 2019-05-10T19:33:57Z

This is BC breaking and it's never mentioned!

yf225 · 2019-05-11T04:02:40Z

@pytorchbot rebase this please

yf225 · 2019-05-11T16:56:59Z

@pytorchbot rebase this please

gchanan · 2019-05-10T19:34:28Z

aten/src/ATen/native/sparse/SparseTensor.cpp

please comment.

gchanan · 2019-05-10T19:42:08Z

aten/src/ATen/native/sparse/SparseTensor.cpp

what about the version counter? Can we please take a non-default argument to shallow_copy_and_detach for what to do with the version counter? It's troublesome that this API doesn't expose a crucial thing that needs to be considered.

yf225 · 2019-05-16T13:53:05Z

aten/src/ATen/native/sparse/SparseTensor.cpp

+    /*allow_tensor_metadata_change=*/true));
+  auto values_shallow_copy = Tensor(values.unsafeGetTensorImpl()->shallow_copy_and_detach(
+    /*version_counter=*/values.unsafeGetTensorImpl()->version_counter(),
+    /*allow_tensor_metadata_change=*/true));


We don't set allow_tensor_metadata_change to false here, because we need to be able to resize the sparse tensor in subsequent operations, e.g. in https://github.com/pytorch/pytorch/blob/master/test/test_sparse.py#L1852.

facebook-github-bot

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

gchanan

can you add a test for the new behavior, please?

gchanan · 2019-05-16T16:22:03Z

aten/src/ATen/native/sparse/SparseTensor.cpp

+  // NOTE: There is no guarantee that `indices` and `values` don't contain AutogradMeta. However,
+  // we want to maintain the invariant that `indices_` and `values_` of a sparse tensor don't
+  // contain AutogradMeta, and to achieve that we shallow-copy `indices` and `values` here.
+  auto indices_shallow_copy = LongTensor(indices.unsafeGetTensorImpl()->shallow_copy_and_detach(


I'm a little confused about what is going on with the version counter.

Given this change, I'd expect that when I call SparseTensor._values() I get a tensor that shares the version counter with the values that were originally passed in. But that doesn't seem to be the case, either in this PR or previously -- do you know why this is?

Example:

>>> ind=torch.tensor([[0],[1]]).add_(1).sub_(1) >>> values = torch.tensor([1.]).add_(1).add_(1).sub_(1).sub_(1) >>> c=torch.sparse_coo_tensor(ind, values) >>> c._values()._version 0 >>> c._indices()._version 0 >>> c=torch.sparse_coo_tensor(ind, values).coalesce() >>> c.values()._version 0 >>> c.indices()._version 0

The current implementation of VariableType::_values() makes it so that c._values() shares the same version counter as c. I think this is the right semantics because updating c._values() should also bump the version counter of c.

A related issue is we need to make sure in-place update on the original value tensor values also updates the version counter of the sparse tensor's c's values_ tensor, and it should throw version mismatch error in the backward() operation if the original value tensor is changed (this requires saving the value tensor for backward in the sparse constructor). I added the task in #13638.

…_shallow_copy

gchanan · 2019-05-16T19:34:20Z

test/test_sparse.py

+        v = torch.ones([1, 2, 3])
+        t = torch.sparse_coo_tensor(i, v, torch.Size([2, 2, 3]))
+        v.transpose_(0, 1)
+        self.assertEqual(list(t.coalesce().values().size()), [1, 2, 3])


what about tests for changing indices?

Added tests for changing indices.

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.

facebook-github-bot

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

Summary: (Reopens #20330 and fixes test error.) After the Variable/Tensor merge, there is no guarantee that `indices` and `values` passed into the sparse tensor constructor don't contain AutogradMeta. However, we want to maintain the existing invariant that `indices_` and `values_` of a sparse tensor don't contain AutogradMeta, and to achieve this we need do shallow-copy in the sparse tensor constructor. Note that this is BC-breaking for code that changes the sizes / strides of the indices or values tensor after it's used to create a sparse tensor. In current master, such changes will be reflected in the sparse tensor and break sparse tensor invariants. After this PR, those changes will not be reflected in the sparse tensor, and thus the sparse tensor invariants are always preserved. Specifically, running in-place size/stride-changing ops such as `resize_` / `resize_as_` / `as_strided_` / `set_` / `transpose_` on the original values tensor will not update the sparse tensor's `values_`. For example: ```python # Calling resize_ on non-requires-grad value tensor i2 = torch.zeros([1, 1]) v2 = torch.ones([1, 2, 3]) t2 = torch.sparse_coo_tensor(i2, v2, torch.Size([2, 2, 3])) v2.resize_(4, 5) t2.coalesce().values().size() # On current master, this throws "indices and values must have same nnz, but got nnz from indices: 1, nnz from values: 4", because resizing the original value tensor affects `values_` of the sparse tensor. # After this PR, this prints "torch.Size([1, 2, 3])", which means resizing the original value tensor doesn't affect `values_` of the sparse tensor. ``` Pull Request resolved: #20614 Differential Revision: D15385811 Pulled By: yf225 fbshipit-source-id: e963fcf5e4097f8c881b56145f408565d97cf5c1

Summary: (Reopens pytorch/pytorch#20330 and fixes test error.) After the Variable/Tensor merge, there is no guarantee that `indices` and `values` passed into the sparse tensor constructor don't contain AutogradMeta. However, we want to maintain the existing invariant that `indices_` and `values_` of a sparse tensor don't contain AutogradMeta, and to achieve this we need do shallow-copy in the sparse tensor constructor. Note that this is BC-breaking for code that changes the sizes / strides of the indices or values tensor after it's used to create a sparse tensor. In current master, such changes will be reflected in the sparse tensor and break sparse tensor invariants. After this PR, those changes will not be reflected in the sparse tensor, and thus the sparse tensor invariants are always preserved. Specifically, running in-place size/stride-changing ops such as `resize_` / `resize_as_` / `as_strided_` / `set_` / `transpose_` on the original values tensor will not update the sparse tensor's `values_`. For example: ```python # Calling resize_ on non-requires-grad value tensor i2 = torch.zeros([1, 1]) v2 = torch.ones([1, 2, 3]) t2 = torch.sparse_coo_tensor(i2, v2, torch.Size([2, 2, 3])) v2.resize_(4, 5) t2.coalesce().values().size() # On current master, this throws "indices and values must have same nnz, but got nnz from indices: 1, nnz from values: 4", because resizing the original value tensor affects `values_` of the sparse tensor. # After this PR, this prints "torch.Size([1, 2, 3])", which means resizing the original value tensor doesn't affect `values_` of the sparse tensor. ``` Pull Request resolved: pytorch/pytorch#20614 Differential Revision: D15385811 Pulled By: yf225 fbshipit-source-id: e963fcf5e4097f8c881b56145f408565d97cf5c1

pytorchbot added module: operators module: sparse Related to torch.sparse labels May 9, 2019

yf225 removed module: operators module: sparse Related to torch.sparse labels May 9, 2019

yf225 force-pushed the sparse_shallow_copy branch from 397985c to d7a0f9f Compare May 9, 2019 18:49

yf225 changed the title ~~[DO NOT MERGE] Test if we can shallow-copy indices and values in sparse tensor ctor~~ Shallow-copy indices and values in sparse tensor ctor May 10, 2019

yf225 requested a review from gchanan May 10, 2019 18:07

yf225 changed the title ~~Shallow-copy indices and values in sparse tensor ctor~~ [BC-breaking] Shallow-copy indices and values in sparse tensor ctor May 10, 2019

yf225 changed the title ~~[BC-breaking] Shallow-copy indices and values in sparse tensor ctor~~ Shallow-copy indices and values in sparse tensor ctor May 10, 2019

yf225 changed the title ~~Shallow-copy indices and values in sparse tensor ctor~~ [BC-breaking] Shallow-copy indices and values in sparse tensor ctor May 10, 2019

yf225 mentioned this pull request May 11, 2019

Variable/Tensor Merge Proposal #13638

Closed

22 tasks

pytorchbot added module: operators module: sparse Related to torch.sparse labels May 11, 2019

gchanan requested changes May 13, 2019

View reviewed changes

yf225 added the module: bc-breaking Related to a BC-breaking change label May 14, 2019

Will Feng added 2 commits May 16, 2019 00:07

[WIP]

634863f

better comments

084cfe2

yf225 force-pushed the sparse_shallow_copy branch from 811d7fa to 084cfe2 Compare May 16, 2019 04:07

fix shallow_copy_and_detach

5b193ec

yf225 commented May 16, 2019

View reviewed changes

facebook-github-bot reviewed May 16, 2019

View reviewed changes

gchanan requested changes May 16, 2019

View reviewed changes

gchanan reviewed May 16, 2019

View reviewed changes

Will Feng added 2 commits May 16, 2019 15:18

add test

d658b34

Merge branch 'master' of https://github.com/yf225/pytorch into sparse…

f69015b

…_shallow_copy

gchanan reviewed May 16, 2019

View reviewed changes

Will Feng added 4 commits May 16, 2019 16:07

test indices

c2df590

Fix test

43b88d1

Fix test

797725a

Fix test

2f78477

gchanan approved these changes May 16, 2019

View reviewed changes

facebook-github-bot reviewed May 16, 2019

View reviewed changes

facebook-github-bot closed this in 4f02321 May 16, 2019

fix cuda test

4cacc72

yf225 reopened this May 16, 2019

facebook-github-bot reviewed May 16, 2019

View reviewed changes

more fix

1e8f503

facebook-github-bot reviewed May 16, 2019

View reviewed changes

facebook-github-bot added the merged label May 16, 2019

more fix

7994243

yf225 removed the merged label May 16, 2019

pytorch deleted a comment from facebook-github-bot May 16, 2019

facebook-github-bot reviewed May 16, 2019

View reviewed changes

zdevito closed this in zdevito/ATen@c64d6c8 May 16, 2019

yf225 reopened this May 16, 2019

yf225 closed this May 16, 2019

yf225 mentioned this pull request May 16, 2019

[Second Try] [BC-breaking] Shallow-copy indices and values in sparse tensor ctor #20614

Closed

facebook-github-bot added the merged label May 17, 2019

[BC-breaking] Shallow-copy indices and values in sparse tensor ctor #20330

[BC-breaking] Shallow-copy indices and values in sparse tensor ctor #20330

Uh oh!

Conversation

yf225 commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 commented May 9, 2019

Uh oh!

gchanan commented May 10, 2019

Uh oh!

yf225 commented May 11, 2019

Uh oh!

yf225 commented May 11, 2019

Uh oh!

gchanan May 10, 2019

Choose a reason for hiding this comment

Uh oh!

gchanan May 10, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 May 16, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gchanan left a comment

Choose a reason for hiding this comment

Uh oh!

gchanan May 16, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 May 16, 2019

Choose a reason for hiding this comment

Uh oh!

gchanan May 16, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 May 16, 2019

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!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yf225 commented May 9, 2019 •

edited

Loading

yf225 May 16, 2019 •

edited

Loading