[LT] Support Tensor.is_alias_of #75246

alanwaketan · 2022-04-05T05:50:10Z

Stack from ghstack (oldest at bottom):

Summary:
Tensor.is_alias_of relies on Storage to perform. However, LTCTensorImpl was
not implemented with that in mind. This commit adds a fake storage to LazyTensor
as a marker to mark LazyTensors that point to the same storage. The reason
why it's not done at LTCTensorImpl is that LazyTensor maintains the view ops/alias
logic in LazyTensor class instead of relying on TensorImpl to do the check.

Test Plan:
./build/bin/test_lazy --gtest_filter=LazyOpsTest.IsAliasOf

Differential Revision: D35389123

Summary: Tensor.is_alias_of relies on Storage to perform. However, LTCTensorImpl was not implemented with that in mind. This commit adds a fake storage to LazyTensor as a marker to mark LazyTensors that point to the same storage. The reason why it's not done at LTCTensorImpl is that LazyTensor maintains the view ops/alias logic in LazyTensor class instead of relying on TensorImpl to do the check. Test Plan: ./build/bin/test_lazy --gtest_filter=LazyOpsTest.IsAliasOf [ghstack-poisoned]

facebook-github-bot · 2022-04-05T05:50:16Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/75246
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

💊 CI failures summary and remediations

As of commit 3f12fab (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Summary: Tensor.is_alias_of relies on Storage to perform. However, LTCTensorImpl was not implemented with that in mind. This commit adds a fake storage to LazyTensor as a marker to mark LazyTensors that point to the same storage. The reason why it's not done at LTCTensorImpl is that LazyTensor maintains the view ops/alias logic in LazyTensor class instead of relying on TensorImpl to do the check. Test Plan: ./build/bin/test_lazy --gtest_filter=LazyOpsTest.IsAliasOf ghstack-source-id: 2069b77 Pull Request resolved: #75246

alanwaketan · 2022-04-05T05:51:12Z

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

alanwaketan · 2022-04-06T19:33:44Z

Please refer to the #71378 for the discussion on why we support is_alias_of in this way instead of making the method virtual. cc @sujoysaraswati.

wconstab · 2022-04-06T19:51:29Z

torch/csrc/lazy/core/tensor.cpp

 LazyTensorPtr LazyTensor::CreateViewTensor(ViewInfo view_info) const {
-  return Create(CreateView(std::move(view_info)), GetDevice());
+  auto new_tensor = Create(CreateView(std::move(view_info)), GetDevice());
+  new_tensor->storage_ = Storage();


hmm, so we explicitly copy the Storage from one LT to another one if we know we are creating a view.

i was actually thinking we could tie it into the 'data()' object in some way, so if 2 LazyTensors happen to point to the same IR value then they also share the same storage. I'm not actually sure if that is a desirable property, and maybe your approach is good enough. It certainly seems to be simpler, which is nice.

if 2 LazyTensors happen to point to the same IR value then they also share the same storage

just curious - are you saying it's possible to be in a state where you have two LTC tensor's that aren't views of each other, but they actually point to the same IR under the hood?

wconstab · 2022-04-06T20:09:01Z

I think this change would be OK, but i'll wait for @bdhirsh to stamp

bdhirsh

Looks ok to me! I'm hoping to have a mini prototype of integration functionalization into LTC pretty soon (where a bunch more changes are needed / stuff breaks, but hopefully I can show what the integration would look like at a high level)

sujoysaraswati · 2022-04-07T05:25:50Z

I have a question on the LazyTensor data_ptr() access. Since the has_storage() and storage() methods on LazyTensor would now return true and the fake storage, how does it affect APIs like data_ptr() for LazyTensor, is it planned to be supported for lazy tensor?

sujoysaraswati · 2022-04-07T05:30:18Z

Please refer to the #71378 for the discussion on why we support is_alias_of in this way instead of making the method virtual. cc @sujoysaraswati.

Thanks @alanwaketan, we have followed the suggestion to use is_alias_of with a dummy storage and have closed #71378 now.

alanwaketan · 2022-04-09T07:02:54Z

I have a question on the LazyTensor data_ptr() access. Since the has_storage() and storage() methods on LazyTensor would now return true and the fake storage, how does it affect APIs like data_ptr() for LazyTensor, is it planned to be supported for lazy tensor?

It shouldn't affect any exist APIs in the LazyTensor class. Or do you mean data_ptr() APIs somewhere else?

sujoysaraswati · 2022-04-11T07:12:45Z

I have a question on the LazyTensor data_ptr() access. Since the has_storage() and storage() methods on LazyTensor would now return true and the fake storage, how does it affect APIs like data_ptr() for LazyTensor, is it planned to be supported for lazy tensor?

It shouldn't affect any exist APIs in the LazyTensor class. Or do you mean data_ptr() APIs somewhere else?

My question is If we call a tensor.data_ptr() on a LazyTensor, will it work and provide a valid pointer?

alanwaketan · 2022-04-11T23:17:13Z

I have a question on the LazyTensor data_ptr() access. Since the has_storage() and storage() methods on LazyTensor would now return true and the fake storage, how does it affect APIs like data_ptr() for LazyTensor, is it planned to be supported for lazy tensor?

It shouldn't affect any exist APIs in the LazyTensor class. Or do you mean data_ptr() APIs somewhere else?

My question is If we call a tensor.data_ptr() on a LazyTensor, will it work and provide a valid pointer?

It will return the empty DataPtr that is created in https://github.com/pytorch/pytorch/pull/75246/files#diff-824247c5519c382811fb78f1272745e2f1dfbce4fb85fbb5bb04f550ea836206R83.

Not sure about the implications on other functions built on top of the DataPtr API. Before we will throw an assertion which basically means LazyTensor won't touch any of these APIs. And the for the future, we will deprecate this and use functionalization. Maybe worth a TORCH_WARN, but I don't see any particular issues here. @bdhirsh What do you think?

sujoysaraswati · 2022-04-12T03:37:34Z

Yes, I was referring to the empty data pointer value that would be returned and if this would create issue for any caller code looking for a valid data pointer. Could you please also give some details on how this is planned to change with functionalization?

bdhirsh · 2022-04-12T13:32:16Z

That's true that accessing the data ptr won't give you a nice error message anymore.

With functionalization (integration with LTC is underway), the lazy tensors will be wrapped in a [FunctionalTensorWrapper](https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/FunctionalTensorWrapper.h) - it also has a null data ptr, but it can do more than just track aliases - it properly propagates mutations across aliases, and tracks sizes/stride information.

Summary: Tensor.is_alias_of relies on Storage to perform. However, LTCTensorImpl was not implemented with that in mind. This commit adds a fake storage to LazyTensor as a marker to mark LazyTensors that point to the same storage. The reason why it's not done at LTCTensorImpl is that LazyTensor maintains the view ops/alias logic in LazyTensor class instead of relying on TensorImpl to do the check. Test Plan: ./build/bin/test_lazy --gtest_filter=LazyOpsTest.IsAliasOf Differential Revision: [D35389123](https://our.internmc.facebook.com/intern/diff/D35389123) [ghstack-poisoned]

Summary: Tensor.is_alias_of relies on Storage to perform. However, LTCTensorImpl was not implemented with that in mind. This commit adds a fake storage to LazyTensor as a marker to mark LazyTensors that point to the same storage. The reason why it's not done at LTCTensorImpl is that LazyTensor maintains the view ops/alias logic in LazyTensor class instead of relying on TensorImpl to do the check. Test Plan: ./build/bin/test_lazy --gtest_filter=LazyOpsTest.IsAliasOf ghstack-source-id: b6d8a7c Pull Request resolved: #75246

alanwaketan · 2022-04-13T22:19:48Z

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

facebook-github-bot · 2022-04-14T07:26:50Z

@pytorchbot merge this

(Initiating merge automatically since Phabricator Diff has merged)

facebook-github-bot · 2022-04-14T07:27:11Z

@pytorchbot merge this

(Initiating merge automatically since Phabricator Diff has merged)

pytorchmergebot · 2022-04-14T07:28:17Z

Can't merge closed PR #75246

github-actions · 2022-04-14T07:28:43Z

Hey @alanwaketan.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Pull Request resolved: #75246 Tensor.is_alias_of relies on Storage to perform. However, LTCTensorImpl was not implemented with that in mind. This commit adds a fake storage to LazyTensor as a marker to mark LazyTensors that point to the same storage. The reason why it's not done at LTCTensorImpl is that LazyTensor maintains the view ops/alias logic in LazyTensor class instead of relying on TensorImpl to do the check. Test Plan: ./build/bin/test_lazy --gtest_filter=LazyOpsTest.IsAliasOf Reviewed By: bdhirsh Differential Revision: D35389123 Pulled By: alanwaketan fbshipit-source-id: 488fdba84660d8e57bbbe97a775780e1d312fd88

This was referenced Apr 5, 2022

[LT] Support diagonal op #75230

Closed

[LT] Turn expand into a view op #75245

Closed

facebook-github-bot added the cla signed label Apr 5, 2022

This was referenced Apr 5, 2022

[Distributed] Add a guard for non CPU/CUDA devices #75247

Closed

[LT] Support narrow #75344

Closed

[LT] Compose DDP with LazyTensor #75350

Closed

alanwaketan requested review from Krovatkin, albanD, bdhirsh, desertfire and wconstab April 6, 2022 19:35

wconstab reviewed Apr 6, 2022

View reviewed changes

bdhirsh approved these changes Apr 6, 2022

View reviewed changes

pytorchmergebot closed this in ab0d9b1 Apr 14, 2022

alanwaketan added release notes: lazy release notes category topic: new features topic category labels Apr 14, 2022

facebook-github-bot deleted the gh/alanwaketan/18/head branch April 17, 2022 14:17

alanwaketan mentioned this pull request Aug 30, 2022

[WIP] Reproduce nan loss with DDP and XLA backend pytorch/xla#3915

Closed

[LT] Support Tensor.is_alias_of #75246

[LT] Support Tensor.is_alias_of #75246

Uh oh!

Conversation

alanwaketan commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

Uh oh!

alanwaketan commented Apr 5, 2022

Uh oh!

alanwaketan commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wconstab Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

wconstab commented Apr 6, 2022

Uh oh!

bdhirsh left a comment

Choose a reason for hiding this comment

Uh oh!

sujoysaraswati commented Apr 7, 2022

Uh oh!

sujoysaraswati commented Apr 7, 2022

Uh oh!

alanwaketan commented Apr 9, 2022

Uh oh!

sujoysaraswati commented Apr 11, 2022

Uh oh!

alanwaketan commented Apr 11, 2022

Uh oh!

sujoysaraswati commented Apr 12, 2022

Uh oh!

bdhirsh commented Apr 12, 2022

Uh oh!

alanwaketan commented Apr 13, 2022

Uh oh!

facebook-github-bot commented Apr 14, 2022

Uh oh!

facebook-github-bot commented Apr 14, 2022

Uh oh!

pytorchmergebot commented Apr 14, 2022

Uh oh!

github-actions bot commented Apr 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alanwaketan commented Apr 5, 2022 •

edited

Loading

facebook-github-bot commented Apr 5, 2022 •

edited

Loading

alanwaketan commented Apr 6, 2022 •

edited

Loading