Better reshape with autograd support by YifanShenSZ · Pull Request #82754 · pytorch/pytorch

YifanShenSZ · 2022-08-03T21:07:26Z

Previous reshape #80981 is ok for forward, but needs improvement for backward: need to handle "sometimes view sometimes copy" behavior.

This pull request fixes it by:

add a new alias dispatch key CompositeImplicitAutogradNestedTensor, which ideally would work as nested-tensor version of CompositeImplicitAutograd
register reshape_nested to reshape by CompositeImplicitAutogradNestedTensor

Side changes:

add contiguous memory format support to clone_nested
add view_nested
add reshape_as_nested

Fix issue #83041

facebook-github-bot · 2022-08-03T21:07:32Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/82754
✖️ Python docs build was skipped
✖️ C++ docs build was skipped
❓Need help or want to give feedback on the CI? Visit our office hours

❌ 1 New Failures, 7 Pending

As of commit 7c93475 (more details on the Dr. CI page):

Expand to see more

1/1 failures introduced in this PR

🕵️‍♀️ 1 failure not recognized by patterns:

The following CI failures may be due to changes from the PR

Job	Step
^{macos-12-py3-arm64 / test (default, 2, 2, macos-m1-12)}	^Unknown

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.

YifanShenSZ · 2022-08-04T21:23:15Z

@pytorchbot rebase

pytorchmergebot · 2022-08-04T21:24:41Z

@pytorchbot successfully started a rebase job. Check the current status here

pytorchmergebot · 2022-08-04T21:24:46Z

Successfully rebased nt-reshape onto refs/remotes/origin/master, please pull locally before adding more changes (for example, via git checkout nt-reshape && git pull --rebase)

drisspg · 2022-08-15T19:46:40Z

+    //       efficient implementation of nested_size_tensor_ and nested_stride_tensor.
+    return wrap_buffer(buffer, sizemat.clone(), stridemat.clone(), offsets);
+  }
+  else if (memory_format == c10::MemoryFormat::Contiguous) {


I would maybe add a comment that is the case when the memory format is contiguous but self is a non_contiguous nested tensor.

One thing I am trying to hash out now. I think all current nested tensor instances satisfy the bottom two are equal.

nested_tensor.numel() -> this is calculated from nested_sizes to have less elements than its underlying
storage -> nested_tensor.storage.nbytes()//dtype.

In theory one can easily create this tensor with wrap_buffer(), however I am not sure if that is done anywhere.

reshape, transpose, view / all current view ops appear to maintain this invariant but need to confirm.
@jbschlosser

slice is an example to have nested_tensor.numel() < nested_tensor.storage.nbytes()//dtype

nt = torch.nested_tensor([x, y, z]) slice = nt[0 : 3 : 2]

where the memory of y is skipped

Speaking of slice, we can support those advanced indexing in dimension 0 now by extracting the rows of nested_size and nested_stride + the elements of offsets. @jbschlosser wdyt?

albanD

separating adding the new alias key would allow you to land it without waiting for Driss's change.
I am not sure why view calls into reshape but didn't dive into this in details yet, maybe it is just a function name issue.

albanD · 2022-08-15T20:15:30Z

  //      to any of its backends.
  //      See Note [Undefined in dispatchTable_] for the special handling for Undefined.
+
+  if (dispatch_key != DispatchKey::Undefined && isIncludedInAlias(dispatch_key, DispatchKey::CompositeImplicitAutogradNestedTensor)) {


You most likely want to update the comment above.

bdhirsh · 2022-08-24T19:08:30Z

Left some final comments on the codegen but otherwise I think this PR looks good!

albanD

SGTM
@drisspg I think we want to import this before landing it to see if internal changes are necessary. Can you take care of that?

albanD · 2022-08-25T14:19:34Z

+    return output;
+  }
+  else {
+    AT_ERROR("nested tensor clone supports Preserve and Contiguous memory foramts");


Always use TORCH_CHECK*

drisspg · 2022-08-25T16:13:03Z

SGTM @drisspg I think we want to import this before landing it to see if internal changes are necessary. Can you take care of that?

Will work through that now

facebook-github-bot · 2022-08-25T16:13:31Z

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

facebook-github-bot · 2022-08-25T16:25:49Z

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

facebook-github-bot · 2022-08-25T17:13:11Z

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

YifanShenSZ · 2022-08-26T01:14:02Z

I'll leave it to you guys 😜

bdhirsh

nice work!

@drisspg should probably be the one to merge, since he's working on appeasing the internal build

Summary: Pull Request resolved: pytorch#84154 Previous reshape [https://github.com/pytorch/pytorch/issues/80981](https://github.com/pytorch/pytorch/pull/80981) is ok for forward, but needs improvement for backward: need to handle "sometimes view sometimes copy" behavior. This pull request fixes it by: 1. add a new alias dispatch key `CompositeImplicitAutogradNestedTensor`, which ideally would work as nested-tensor version of `CompositeImplicitAutograd` 2. register `reshape_nested` to `reshape` by `CompositeImplicitAutogradNestedTensor` Side changes: * add contiguous memory format support to `clone_nested` * add `view_nested` * add `reshape_as_nested` Fix issue [https://github.com/pytorch/pytorch/issues/83041](https://github.com/pytorch/pytorch/issues/83041) Pull Request resolved: pytorch#82754 Test Plan: Imported from GitHub, without a `Test Plan:` line. **Static Docs Preview: executorch** |[Full Site](https://our.intern.facebook.com/intern/staticdocs/eph/D39023822/V26/executorch/)| |**Modified Pages**| **Static Docs Preview: executorch** |[Full Site](https://our.intern.facebook.com/intern/staticdocs/eph/D39023822/V17/executorch/)| |**Modified Pages**| Reviewed By: albanD, bdhirsh Differential Revision: D39023822 Pulled By: drisspg fbshipit-source-id: 87acd4f9fb61cd094fccad7801c25e2a1bfed88b

github-actions · 2022-09-01T20:02:18Z

Hey @YifanShenSZ.
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: #84154 Previous reshape [https://github.com/pytorch/pytorch/issues/80981](https://github.com/pytorch/pytorch/pull/80981) is ok for forward, but needs improvement for backward: need to handle "sometimes view sometimes copy" behavior. This pull request fixes it by: 1. add a new alias dispatch key `CompositeImplicitAutogradNestedTensor`, which ideally would work as nested-tensor version of `CompositeImplicitAutograd` 2. register `reshape_nested` to `reshape` by `CompositeImplicitAutogradNestedTensor` Side changes: * add contiguous memory format support to `clone_nested` * add `view_nested` * add `reshape_as_nested` Fix issue [https://github.com/pytorch/pytorch/issues/83041](https://github.com/pytorch/pytorch/issues/83041) Pull Request resolved: #82754 Test Plan: Imported from GitHub, without a `Test Plan:` line. **Static Docs Preview: executorch** |[Full Site](https://our.intern.facebook.com/intern/staticdocs/eph/D39023822/V27/executorch/)| |**Modified Pages**| **Static Docs Preview: executorch** |[Full Site](https://our.intern.facebook.com/intern/staticdocs/eph/D39023822/V17/executorch/)| |**Modified Pages**| Reviewed By: albanD, bdhirsh Differential Revision: D39023822 Pulled By: drisspg fbshipit-source-id: 872c81dc847d280366ef9f187f9b9bcb06aac73f

@YifanShenSZ

The original author is @YifanShenSZ and the original PR is: pytorch#82754 # Summary: Previous reshape [https://github.com/pytorch/pytorch/issues/80981](https://github.com/pytorch/pytorch/pull/80981) is ok for forward, but needs improvement for backward: need to handle "sometimes view sometimes copy" behavior. This pull request fixes it by: 1. add a new alias dispatch key `CompositeImplicitAutogradNestedTensor`, which ideally would work as nested-tensor version of `CompositeImplicitAutograd` 2. register `reshape_nested` to `reshape` by `CompositeImplicitAutogradNestedTensor` Side changes: * add contiguous memory format support to `clone_nested` * add `view_nested` * add `reshape_as_nested` Fix issue [https://github.com/pytorch/pytorch/issues/83041](https://github.com/pytorch/pytorch/issues/83041) Pull Request resolved: pytorch#82754 Test Plan: Imported from GitHub, without a `Test Plan:` line. **Static Docs Preview: executorch** |[Full Site](https://our.intern.facebook.com/intern/staticdocs/eph/D39023822/V13/executorch/)| |**Modified Pages**| Reviewed By: albanD Differential Revision: D39023822 Pulled By: drisspg Pull Request resolved: pytorch#84154 Approved by: https://github.com/bdhirsh, https://github.com/albanD

facebook-github-bot added the cla signed label Aug 3, 2022

Yifan Shen added 2 commits August 11, 2022 13:30

better reshape with autograd support

c15eef8

comply with linter

c861d19

YifanShenSZ changed the title ~~Better reshape with autograd support~~ [Waiting for [#82658](https://github.com/pytorch/pytorch/pull/82658)] Better reshape with autograd support Aug 11, 2022

YifanShenSZ changed the title ~~[Waiting for [#82658](https://github.com/pytorch/pytorch/pull/82658)] Better reshape with autograd support~~ [Waiting for https://github.com/pytorch/pytorch/pull/82658] Better reshape with autograd support Aug 11, 2022

YifanShenSZ changed the title ~~[Waiting for https://github.com/pytorch/pytorch/pull/82658] Better reshape with autograd support~~ [Waiting for dependency] Better reshape with autograd support Aug 11, 2022

YifanShenSZ marked this pull request as ready for review August 15, 2022 15:07

YifanShenSZ requested review from albanD and soulitzer as code owners August 15, 2022 15:07

YifanShenSZ requested review from bdhirsh, drisspg and jbschlosser August 15, 2022 15:08

drisspg reviewed Aug 15, 2022

View reviewed changes

Comment thread aten/src/ATen/native/nested/NestedTensorMath.cpp Outdated

drisspg reviewed Aug 15, 2022

View reviewed changes

Comment thread aten/src/ATen/native/nested/NestedTensorMath.cpp Outdated

albanD reviewed Aug 15, 2022

View reviewed changes

Yifan Shen added 2 commits August 15, 2022 17:07

update comments

66acbdb

merge upstream viable/strict

0df24a4

YifanShenSZ changed the title ~~[Waiting for dependency] Better reshape with autograd support~~ Better reshape with autograd support Aug 18, 2022

update according to #82658

ab77629

YifanShenSZ marked this pull request as draft August 18, 2022 23:26

YifanShenSZ marked this pull request as ready for review August 19, 2022 02:51

YifanShenSZ requested review from albanD and drisspg August 19, 2022 02:52

jbschlosser mentioned this pull request Aug 19, 2022

Introduce SDP op to BT fastpath #83550

Closed

drisspg reviewed Aug 19, 2022

View reviewed changes

Comment thread aten/src/ATen/native/nested/NestedTensorMath.cpp Outdated

bdhirsh reviewed Aug 24, 2022

View reviewed changes

Comment thread torchgen/native_function_generation.py Outdated

ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 24, 2022

revise CompositeImplicitAutogradNestedTensor codegen

ac0e57f

albanD reviewed Aug 25, 2022

View reviewed changes

Merge branch 'master' into nt-reshape

f57fe4a

drisspg and others added 2 commits August 25, 2022 09:52

Update constructor call

c847d15

lint

7c93475

bdhirsh approved these changes Aug 26, 2022

View reviewed changes

albanD approved these changes Aug 26, 2022

View reviewed changes

drisspg mentioned this pull request Aug 26, 2022

Better reshape with autograd support (#82754) #84154

Closed

pytorchmergebot closed this in 673b35c Sep 1, 2022

YifanShenSZ mentioned this pull request Sep 2, 2022

Codegen for AutogradNestedTensor #83041

Closed

Conversation

YifanShenSZ commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

🔗 Helpful links

❌ 1 New Failures, 7 Pending

🕵️‍♀️ 1 failure not recognized by patterns:

Uh oh!

YifanShenSZ commented Aug 4, 2022

Uh oh!

pytorchmergebot commented Aug 4, 2022

Uh oh!

pytorchmergebot commented Aug 4, 2022

Uh oh!

Uh oh!

drisspg Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

YifanShenSZ Aug 15, 2022 • 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!

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

albanD Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bdhirsh commented Aug 24, 2022

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

albanD Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

drisspg commented Aug 25, 2022

Uh oh!

facebook-github-bot commented Aug 25, 2022

Uh oh!

facebook-github-bot commented Aug 25, 2022

Uh oh!

facebook-github-bot commented Aug 25, 2022

Uh oh!

YifanShenSZ commented Aug 26, 2022

Uh oh!

bdhirsh left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

YifanShenSZ commented Aug 3, 2022 •

edited

Loading

facebook-github-bot commented Aug 3, 2022 •

edited

Loading

YifanShenSZ Aug 15, 2022 •

edited

Loading