Adding function to convert Module to channels last #28991

3/3 broken upstream at merge base 56de885 (see grid view)
- You may want to rebase on the viable/strict branch (see its recency history):
  - If your commit is newer than viable/strict, you can try basing on an older, stable commit:
```
git fetch viable/strict
git rebase --onto viable/strict $(git merge-base origin/master HEAD)
```
  - If your commit is older than viable/strict:
```
git fetch viable/strict
git rebase viable/strict
```
0/3 failures introduced in this PR

Detailed failure analysis

One may explore the probable reasons each build failed interactively on the Dr. CI website.

3 upstream failures recognized by patterns:

These builds matched patterns, but were probably caused by upstream breakages:

This comment was automatically generated by Dr. CI.
Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 5 times.

gchanan · 2019-12-12T18:30:04Z

torch/nn/modules/module.py


        def convert(t):
+            if convert_to_format is not None and t.dim() == 4:
+                return t.to(device, dtype if t.is_floating_point() else None, non_blocking, memory_format=convert_to_format)


this doesn't match the documentation (which says the only case for to with memory-format is the 1-arg case).

Pardon, but I read 'This can be called as' as: here examples of calls, but they are not limited to this options.

that's not my reading of it, although I can see why you read it that way.

In particular, before memory_format was introduced, it corresponded exactly to the function signatures:

pytorch/torch/csrc/autograd/utils/python_arg_parsing.h

Lines 15 to 17 in 66f2bba

"to(Device device=None, ScalarType dtype=None, bool non_blocking=False, bool copy=False, *, MemoryFormat? memory_format=None)",

"to(ScalarType dtype, bool non_blocking=False, bool copy=False, *, MemoryFormat? memory_format=None)",

"to(Tensor tensor, bool non_blocking=False, bool copy=False, *, MemoryFormat? memory_format=None)",

(remove copy because it's not supported and remove memory_format because we are considering the case before memory_format was introduced).

So, the introduction of memory_format changed this from "these are the supported signatures" to "these are some examples of supported signatures". I think the former is more useful and we should change it back.

And actually, the example you added (.. function:: to(memory_format=torch.channels_last)) doesn't work because the parsing code hasn't been updated, right?

IMO we should do the following:

Add a memory_format only-overload to the python parsing.

List the valid calls, i.e.:

.. function:: to(device=None, dtype=None, non_blocking=False, memory_format=None) .. function:: to(dtype, non_blocking=False, memory_format=None) .. function:: to(tensor, non_blocking=False, memory_format=None) .. function:: to(memory_format)

(or similar).

I had parsing updated https://github.com/pytorch/pytorch/pull/28991/files#diff-
7a05cfd8eb442889dffd6c3d2e4d0ddcR24

Will update inline and html docs as follow-up PR

gchanan · 2019-12-12T18:31:07Z

torch/nn/modules/module.py

                the floating point parameters and buffers in this module
            tensor (torch.Tensor): Tensor whose dtype and device are the desired
                dtype and device for all parameters and buffers in this module
+            memory_format (:class:`torch.memory_format`): the desired memory


are you planning to have a reference section for memory_format that you can point to? This description isn't full enough for the long term. (e.g. why only 4D parameters/buffers? -- it's not clear)

Yes as soon as we land new defaults for .clone .to *_like ops I will work on updating docs.

facebook-github-bot · 2019-12-13T01:50:58Z

@VitalyFedyunin merged this pull request in 66f2bba.

ghstack-source-id: f650f3b Pull Request resolved: pytorch/pytorch#28991

Summary: Pull Request resolved: pytorch#28991 Test Plan: Imported from OSS Differential Revision: D18430810 Pulled By: VitalyFedyunin fbshipit-source-id: 0693d4e31fc6f9831722c29fc83517f16ddfc028

ppwwyyxx · 2020-05-27T06:06:53Z

This PR adds an API model.to(memory_layout=torch.channels_last), which recursively applies NHWC layout to all 4D parameters in the model.

I think this is making an assumption that "any 4D parameters in the module needs a conversion to NHWC layout, if user wants to use nvidia's NHWC kernels". From what I can see this assumption is limiting in many ways:

Not all 4D parameters in a user's custom model means "NCHW". The dimensions could have arbitrary meanings, and changing their memory layout may hurt performance.
Not all parameters that are in "NCHW" format needs to be changed to a different format. They need to be changed only in certain layers where Nvidia provides an implementation, e.g. convolution. Imagine a user's custom variant of convolution kernel that may or may not benefit from this layout change.
Converting the layout of filters may not be the best strategy. Converting filters from NCHW to NHWC may happen to be what Nvidia now asks for when using NHWC inputs. However it seems possible that the next version of GPUs, or some other runtime library have different requirements. They may for example decide that NHWC inputs works best together with NCHW filters. Aligning the layout of filters together with the layout of inputs, under the same API, doesn't seem like a good idea.

It may be better if such an API:

only change layout of weights from certain fixed set of layers that can benefit from this change, e.g. convolution
has a more backend-specific name, such as model = optimize_for_cudnn_channels_last(model):
- It seems to me that having to use NHWC filter layout is merely an optimization specifically made for cudnn, instead of a generally applied strategy.
- what filter layout is best for NHWC input layout is a detail of cudnn, and should not be exposed to users. The API model.to(memory_layout=) implies that it is changing the layout of weights to NHWC, which is a cudnn requirement that might change in future hardwares.
- Calling this function should be optional (not sure if it is now). Using NCHW filter layout together with NHWC input should be allowed with a perf penalty on users. With model.to(memory_layout=) it gives an impression that it is required (just like to(dtype) or to(device) has to match)

dreiss · 2020-09-01T23:42:34Z

@ppwwyyxx , I totally agree.

Adding function to convert Module to channels last

7d82089

[ghstack-poisoned]

VitalyFedyunin requested a review from apaszke as a code owner October 31, 2019 20:43

VitalyFedyunin added a commit that referenced this pull request Oct 31, 2019

Adding function to convert Module to channels last

e68e634

ghstack-source-id: f4b9ba9 Pull Request resolved: #28991

soumith suggested changes Nov 1, 2019

View reviewed changes

torch/nn/modules/module.py Outdated Show resolved Hide resolved

add TensroIterator tests on "Adding function to convert Module to cha…

931adfd

…nnels last" [ghstack-poisoned]

VitalyFedyunin added a commit that referenced this pull request Nov 4, 2019

Adding function to convert Module to channels last

c13ede1

ghstack-source-id: 896744c Pull Request resolved: #28991

Add some tests on "Adding function to convert Module to channels last"

b1342bf

[ghstack-poisoned]

This was referenced Nov 8, 2019

[WIP] Prepare copy op to switch memory format preservation defaults #29459

Closed

[WIP] Switch default of clone, to, *_like ops #29460

Closed

[WIP] Experiments #29461

Closed

VitalyFedyunin added 2 commits November 8, 2019 10:23

Addressed PR feedback on "Adding function to convert Module to channe…

4cf0038

…ls last" [ghstack-poisoned]

Changed model conversion code on "Adding function to convert Module t…

41cccb9

…o channels last" [ghstack-poisoned]

VitalyFedyunin requested a review from soumith November 8, 2019 21:33

VitalyFedyunin added 3 commits November 8, 2019 13:43

Linter fix on "Adding function to convert Module to channels last"

95b0788

[ghstack-poisoned]

Fix resize_ on "Adding function to convert Module to channels last"

976c3ff

[ghstack-poisoned]

Fix TensorIterator on "Adding function to convert Module to channels …

1f0ddea

…last" [ghstack-poisoned]

VitalyFedyunin requested review from gchanan and ifedan November 11, 2019 19:00

Fix Autograd on "Adding function to convert Module to channels last"

3bab119

Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

gchanan reviewed Nov 11, 2019

View reviewed changes

torch/nn/modules/module.py Outdated Show resolved Hide resolved

torch/nn/modules/module.py Outdated Show resolved Hide resolved

VitalyFedyunin added 4 commits November 11, 2019 16:38

Fix resize_as_ with new test on "Adding function to convert Module to…

c752a07

… channels last" Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

Fix resize_as_ on "Adding function to convert Module to channels last"

c2593f4

Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

Fix resize_ on "Adding function to convert Module to channels last"

9faeac3

Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

Fix model.to on "Adding function to convert Module to channels last"

973ade6

Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

VitalyFedyunin mentioned this pull request Nov 19, 2019

Switch default memory format of clone operator to Preserve #30089

Closed

VitalyFedyunin added 2 commits November 20, 2019 13:45

rebase to master + fix tests on "Adding function to convert Module to…

42e174b

… channels last" Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

fix tests code on "Adding function to convert Module to channels last"

0ca4bd9

Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

soumith approved these changes Nov 21, 2019

View reviewed changes

add experimental code to check model on "Adding function to convert M…

4574d96

…odule to channels last" Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

VitalyFedyunin mentioned this pull request Nov 22, 2019

[WIP] Copy of #30174 Fix suggest_memory_format() for ambiguous cases #30325

Closed

experiments with tag on "Adding function to convert Module to channel…

e03272c

…s last" Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

This was referenced Dec 4, 2019

WIP #30741

Closed

Revert "[WIP] Copy of #30174 Fix suggest_memory_format() for ambiguous cases" #30742

Closed

[WIP] Add channels last tag #30743

Closed

[WIP] Add channels last tag #30745

Closed

3d t support #30992

Closed

[WIP] Fixing resize_ #30993

Closed

This was referenced Dec 11, 2019

Add memory_format support to zeros, ones, full #31131

Closed

[WIP] Fix cudnn channels_last descriptio problem #31139

Closed

rebase to master on "Adding function to convert Module to channels last"

ea3ed3d

Differential Revision: [D18430810](https://our.internmc.facebook.com/intern/diff/D18430810) [ghstack-poisoned]

gchanan requested changes Dec 12, 2019

View reviewed changes

facebook-github-bot closed this in 66f2bba Dec 12, 2019

facebook-github-bot added the merged label Dec 13, 2019

facebook-github-bot deleted the gh/VitalyFedyunin/24/head branch December 16, 2019 15:17

xxtEchjovs44 pushed a commit to xxtEchjovs44/pytorch that referenced this pull request Jan 29, 2020

Adding function to convert Module to channels last

25acb95

ghstack-source-id: f650f3b Pull Request resolved: pytorch/pytorch#28991

mruberry added the Merged label Oct 28, 2020

ezyang mentioned this pull request Mar 5, 2021

[POC] Pass on Module kwargs to Parameter initialization #53144

Closed

ppwwyyxx mentioned this pull request Oct 22, 2022

x.to(memory_format=torch.contiguous_format) does not always return a contiguous tensor #62027

Open

	"to(Device device=None, ScalarType dtype=None, bool non_blocking=False, bool copy=False, *, MemoryFormat? memory_format=None)",
	"to(ScalarType dtype, bool non_blocking=False, bool copy=False, *, MemoryFormat? memory_format=None)",
	"to(Tensor tensor, bool non_blocking=False, bool copy=False, *, MemoryFormat? memory_format=None)",

Adding function to convert Module to channels last #28991

Adding function to convert Module to channels last #28991

Uh oh!

Conversation

VitalyFedyunin commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soumith left a comment

Choose a reason for hiding this comment

Uh oh!

kostmo commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CircleCI build failures summary

Detailed failure analysis

3 upstream failures recognized by patterns:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 13, 2019

Uh oh!

ppwwyyxx commented May 27, 2020

Uh oh!

dreiss commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

VitalyFedyunin commented Oct 31, 2019 •

edited

Loading

kostmo commented Dec 12, 2019 •

edited

Loading