Implemented non-named version of unflatten #42563

heitorschueroff · 2020-08-04T21:15:31Z

Stack from ghstack:

Implemented torch::nn::Unflatten in libtorch #42613 Implemented torch::nn::Unflatten in libtorch
Implemented non-named version of unflatten #42563 Implemented non-named version of unflatten

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic.

Differential Revision: D23030301

[ghstack-poisoned]

ghstack-source-id: 7b176ea Pull Request resolved: #42563

dr-ci · 2020-08-04T21:23:51Z

💊 CI failures summary and remediations

As of commit 011eb2b (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-xenial-rocm3.5.1-py3.6

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

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 47 times.

aten/src/ATen/native/native_functions.yaml

aten/src/ATen/native/TensorShape.cpp

torch/tensor.py

test/test_torch.py

glaringlee

Other than reviews from @zou3519
Two things want to confirm here @zou3519 @heitorschueroff :

This is not really a bc breaking change, but it breaks bc CI test, I think this is one time breaking after checking the check_backward_compatibility.py, but need to confirm.
I think unflatten should be added into _overrides.py here like flatten:

pytorch/torch/_overrides.py

Line 314 in 206db5c

torch.flatten: lambda input, start_dim=0, end_dim=-1: -1,

zou3519 · 2020-08-05T15:46:35Z

Aug 04 23:06:48 Broken ops: [
Aug 04 23:06:48 aten::unflatten.Dimname(Tensor self, str dim, int[] sizes, str[] names) -> (Tensor)
Aug 04 23:06:48 aten::unflatten.int(Tensor self, int dim, int[] sizes, str[] names) -> (Tensor)
Aug 04 23:06:48 ]

I'm pretty confident no one has serialized the above operators because they literally don't work in TorchScript (we've banned named tensors in TorchScript). So I wouldn't worry about the BC-breakage.

I remember there was a way to "approve" a change to the backwards compatibility test so the PR doesn't break anything when it gets merged. cc @houseroad -- how does one do that again?

glaringlee · 2020-08-05T16:57:17Z

Other than reviews from @zou3519
Two things want to confirm here @zou3519 @heitorschueroff :

This is not really a bc breaking change, but it breaks bc CI test, I think this is one time breaking after checking the check_backward_compatibility.py, but need to confirm.

I think unflatten should be added into _overrides.py here like flatten:

pytorch/torch/_overrides.py

Line 314 in 206db5c

torch.flatten: lambda input, start_dim=0, end_dim=-1: -1,

Update: No need to add unflatten into _overrides.py, since we don't support torch.unflatten.

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. [ghstack-poisoned]

glaringlee · 2020-08-06T17:50:51Z

Aug 04 23:06:48 Broken ops: [
Aug 04 23:06:48 aten::unflatten.Dimname(Tensor self, str dim, int[] sizes, str[] names) -> (Tensor)
Aug 04 23:06:48 aten::unflatten.int(Tensor self, int dim, int[] sizes, str[] names) -> (Tensor)
Aug 04 23:06:48 ]

I'm pretty confident no one has serialized the above operators because they literally don't work in TorchScript (we've banned named tensors in TorchScript). So I wouldn't worry about the BC-breakage.

I remember there was a way to "approve" a change to the backwards compatibility test so the PR doesn't break anything when it gets merged. cc @houseroad -- how does one do that again?

@heitorschueroff Per @houseroad , please add unflatten function here https://fburl.com/lsb7ah1y
The date should be 1 week after expected landing time.

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. [ghstack-poisoned]

glaringlee · 2020-08-07T00:59:50Z

@heitorschueroff
All looks good to me now. One more thing, please fix flake8-py3 lint error

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. [ghstack-poisoned]

glaringlee

This LGTM now

torch/tensor.py

glaringlee

@zou3519
Agree unflatten(Tensor, int, namedshape) should be handled.
Just curious, how we handle unflatten(Tensor, str, namedshape)? if input tensor is not named, will user pass str?

@heitorschueroff I think we should add back unflatten(Tensor, int, namedshape)` support

zou3519 · 2020-08-10T21:43:28Z

How about this one? unflatten(Tensor, str, namedshape), if input tensor is not named, will user pass str?

On master that errors out, so I'd expect it to give a nice error after this PR

glaringlee

per above comments

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. Differential Revision: [D23030301](https://our.internmc.facebook.com/intern/diff/D23030301) [ghstack-poisoned]

zou3519

LGTM, thanks for addressing all the comments!

I had some minor nits left, feel free to address (or not)

test/test_namedtensor.py

test/test_torch.py

zou3519 · 2020-08-11T20:26:08Z

aten/src/ATen/native/TensorShape.cpp

+      dim, " (", self.size(dim), ") in the input tensor");
+  }
+
+  auto shape = self.sizes().vec();


nit: Using a DimVector gives better performance because it avoids dynamic allocations on the heap when the tensor has <= 5 dimensions:

pytorch/aten/src/ATen/core/DimVector.h

Line 11 in 4afbf39

using DimVector = SmallVector<int64_t, kDimVectorStaticSize>;

IIRC the usage would be: DimVector shape(self.sizes().begin(), self.sizes().end());

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. Differential Revision: [D23030301](https://our.internmc.facebook.com/intern/diff/D23030301) [ghstack-poisoned]

facebook-github-bot · 2020-08-13T00:16:03Z

@heitorschueroff merged this pull request in 62bd2dd.

Implemented non-named version of unflatten

cdebc0c

[ghstack-poisoned]

heitorschueroff requested a review from apaszke as a code owner August 4, 2020 21:15

heitorschueroff requested a review from glaringlee August 4, 2020 21:16

Update on "Implemented non-named version of unflatten"

7773ada

[ghstack-poisoned]

heitorschueroff added a commit that referenced this pull request Aug 4, 2020

Implemented non-named version of unflatten

b8bfa5c

ghstack-source-id: 7b176ea Pull Request resolved: #42563

glaringlee requested review from albanD and zou3519 August 4, 2020 21:29

zou3519 reviewed Aug 5, 2020

View reviewed changes

glaringlee reviewed Aug 5, 2020

View reviewed changes

Update on "Implemented non-named version of unflatten"

4adb6bc

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. [ghstack-poisoned]

heitorschueroff mentioned this pull request Aug 5, 2020

Implemented torch::nn::Unflatten in libtorch #42613

Closed

glaringlee requested a review from houseroad August 6, 2020 16:48

Update on "Implemented non-named version of unflatten"

42aa80d

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. [ghstack-poisoned]

heitorschueroff added 2 commits August 7, 2020 12:10

Update on "Implemented non-named version of unflatten"

513c5d2

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. [ghstack-poisoned]

Update on "Implemented non-named version of unflatten"

8baf7a2

Moved logic for non-named unflatten from python nn module to aten/native to be reused by the nn module later. Fixed some inconsistencies with doc and code logic. [ghstack-poisoned]

glaringlee approved these changes Aug 7, 2020

View reviewed changes

zou3519 reviewed Aug 10, 2020

View reviewed changes

torch/tensor.py Outdated Show resolved Hide resolved

zou3519 reviewed Aug 10, 2020

View reviewed changes

torch/tensor.py Show resolved Hide resolved

glaringlee reviewed Aug 10, 2020

View reviewed changes

glaringlee requested changes Aug 10, 2020

View reviewed changes

heitorschueroff requested review from glaringlee and zou3519 August 11, 2020 16:15

heitorschueroff added 2 commits August 11, 2020 15:56

zou3519 approved these changes Aug 11, 2020

View reviewed changes

heitorschueroff added 2 commits August 12, 2020 07:27

zou3519 approved these changes Aug 12, 2020

View reviewed changes

facebook-github-bot closed this in 62bd2dd Aug 12, 2020

facebook-github-bot added the merged label Aug 13, 2020

facebook-github-bot deleted the gh/heitorschueroff/1/head branch August 16, 2020 14:16

mruberry added the Merged label Oct 28, 2020

Implemented non-named version of unflatten #42563

Implemented non-named version of unflatten #42563

Uh oh!

Conversation

heitorschueroff commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glaringlee left a comment • 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!

zou3519 commented Aug 5, 2020

Uh oh!

glaringlee commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glaringlee commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glaringlee commented Aug 7, 2020

Uh oh!

glaringlee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glaringlee left a comment • 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!

zou3519 commented Aug 10, 2020

Uh oh!

glaringlee left a comment

Choose a reason for hiding this comment

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zou3519 Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

zou3519 Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

heitorschueroff commented Aug 4, 2020 •

edited

Loading

dr-ci bot commented Aug 4, 2020 •

edited

Loading

glaringlee left a comment •

edited

Loading

glaringlee commented Aug 5, 2020 •

edited

Loading

glaringlee commented Aug 6, 2020 •

edited

Loading

glaringlee left a comment •

edited

Loading