`empty_like`, `to`, `resize_as_` and `clone` now preserve memory format #23899

VitalyFedyunin · 2019-08-06T21:13:49Z

This PR adds ability to switch behavior of empty_like, to, cuda, resize_as_ and clone operators.
If set_memory_format_propagation set to True. The output of these operators will be channel last tensor if input tensor is channel last.

torch.get_memory_format_propagation() == False # Default value

torch.set_memory_format_propagation(True)
nhwc = torch.randn(20,3,32,16).contiguous(memory_format=torch.channels_last)
clone = nhwc.clone()
clone.is_contiguous(memory_format=torch.channels_last) == True

# And without switch
torch.set_memory_format_propagation(False)
nhwc = torch.randn(20,3,32,16).contiguous(memory_format=torch.channels_last)
clone = nhwc.clone()
clone.is_contiguous(memory_format=torch.channels_last) == False

It also changes suggest_memory_format function, and makes is sensitive to memory_format_proparation switch.

CC #23403

No noticeable performance impact.

aten/src/ATen/core/Tensor.h

…e_empty_supports_memory_format

ezyang · 2019-08-13T17:52:01Z

This is the first time I've seen set_memory_format_propagation. Is there prior discussion of this flag that I haven't seen yet?

gchanan · 2019-08-13T18:00:53Z

what's the plan for the flag? Is it just for temporary transitioning and it will be removed for the next release? I don't believe we can actually correctly write code that works for both states of the flag.

Also, is there any writeup on the plan for resize / resize_as? The behavior is currently pretty unsafe and crazy, and I don't really understand if we'll end up in a good state eventually.

VitalyFedyunin · 2019-08-13T18:28:21Z

This is the first time I've seen set_memory_format_propagation. Is there prior discussion of this flag that I haven't seen yet?

We discussed ability to enable/disable this feature. But not in details, that is why I want you and @gchanan to see this PR first.

what's the plan for the flag? Is it just for temporary transitioning and it will be removed for the next release? I don't believe we can actually correctly write code that works for both states of the flag.

Also, is there any writeup on the plan for resize / resize_as? The behavior is currently pretty unsafe and crazy, and I don't really understand if we'll end up in a good state eventually.

Let me write detailed plan, but hight level is:

Have is False by default (existing behavior).
Every operator that supports True must be test covered with both values.
Officially support switching to True.
Convert viable set of operators ( used in nn).
Set default to True.
Deprecate flag.

ezyang · 2019-08-13T18:33:29Z

OK, so it is going to be a "this is how we migrate people to the new behavior" flag. That's fine, but the intent needs to much more clearly communicated, both in the PR description, and also where it lives in the code. I'll give some suggestions inline.

gchanan · 2019-08-13T18:33:39Z

just to be explicit: I don't believe we should do a release with this flag available. It's too burdensome to write code when you have to think about both states of this flag. It's fine while in-development though.

ezyang · 2019-08-13T18:38:51Z

torch/__init__.py

+    _C._set_memory_format_propagation(b)
+
+def get_memory_format_propagation():
+    return _C._get_memory_format_propagation()


I want to bikeshed the API here. I'll give justification for the bikeshedding I suggest.

First, let's move this out of torch top-level namespace, and into a module like torch.__future__. Because the intent of this is to NOT be a permanent feature toggle, but rather a way to opt into what will (eventually) be the default behavior, it needs to be extremely clear to end users that this is NOT a supported way to toggle between one mode or another, it is purely a BC knob. In Python, there is an existing convention for this: from __future__ import absolute_import, for example. I believe that using a name similar to __future__ will inform people about the intent appropriately.

Second, let's not have a getter/setter function, but instead have this be a regular variable living in the namespace, e.g., torch.__future__.propagate_memory_format = True. This brings this toggle in line with other similar global toggles we have, such as torch.backends.cudnn.enabled = True.

I also have some deeper points about the semantics of this flag. More on this shortly.

ezyang · 2019-08-13T18:51:39Z

c10/core/MemoryFormat.cpp

+#include <c10/core/MemoryFormat.h>
+namespace c10 {
+namespace {
+thread_local bool memory_format_propagation_enabled = false;


@gchanan has stated that it is too difficult to write code that works under both settings of this toggle, and thus we should not release with this toggle available to users. I have an alternate proposal, which I have written up at #24261 for ensuring this toggle only affects user code, and not internal code.

ezyang · 2019-08-13T18:53:57Z

aten/src/ATen/core/Tensor.h

@@ -183,7 +183,7 @@ class CAFFE2_API Tensor {
  }

  at::MemoryFormat suggest_memory_format() const {
-    if (impl_->is_strides_like_channels_last()) {
+    if (get_memory_format_propagation() && impl_->is_strides_like_channels_last()) {


So... this is reverting a change that you made previously?

Will likely be solved through a merge conflict later, but just to point out the potential issue that was addressed in my pending PR: https://github.com/pytorch/pytorch/pull/23861/files#diff-4632522f237f1e4e728cb824300403acR188

ezyang · 2019-08-13T18:55:48Z

aten/src/ATen/core/Tensor.h

@@ -183,7 +183,7 @@ class CAFFE2_API Tensor {
  }

  at::MemoryFormat suggest_memory_format() const {


So, what are the new semantics of suggest_memory_format? Can we get a docstring explaining what exactly this function does, and when you should call it?

…e_empty_supports_memory_format

VitalyFedyunin added 4 commits August 5, 2019 14:20

Simplify tests that should cover all possible devices

e2c3380

More examples

4eb6dbf

Better messages

1fbad8b

empty_like, to and clone now preserve memory format if necessary

8e09355

pytorchbot added module: internals Related to internal abstractions in c10 and ATen module: operators module: tests Issues related to tests (not the torch.testing module) labels Aug 6, 2019

VitalyFedyunin added the module: memory format Memory format/layout related issues/changes (channels_last, nhwc) label Aug 6, 2019

WIP

6fb84e4

jjsjann123 reviewed Aug 8, 2019

View reviewed changes

aten/src/ATen/core/Tensor.h Outdated Show resolved Hide resolved

VitalyFedyunin mentioned this pull request Aug 8, 2019

Memory Format support for Resnet models #23403

Closed

10 tasks

resize_as_ memory format preservation

70dc62e

pytorchbot added the module: cuda Related to torch.cuda, and CUDA support in general label Aug 8, 2019

VitalyFedyunin changed the title ~~[WIP] empty_like, to and clone now preserve memory format~~ [WIP] empty_like, to, resize_as_ and clone now preserve memory format Aug 8, 2019

jjsjann123 mentioned this pull request Aug 8, 2019

[cudnn nhwc support] #23861

Closed

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

b6044a5

…e_empty_supports_memory_format

VitalyFedyunin changed the title ~~[WIP] empty_like, to, resize_as_ and clone now preserve memory format~~ empty_like, to, resize_as_ and clone now preserve memory format Aug 13, 2019

VitalyFedyunin requested review from gchanan, fmassa and ezyang August 13, 2019 17:38

ezyang reviewed Aug 13, 2019

View reviewed changes

ezyang mentioned this pull request Aug 13, 2019

"PyTorch core" thread local flag #24261

Open

ezyang reviewed Aug 13, 2019

View reviewed changes

VitalyFedyunin added 2 commits August 13, 2019 11:57

Fix tests

0499117

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

3025114

…e_empty_supports_memory_format

VitalyFedyunin closed this Oct 30, 2019

VitalyFedyunin deleted the clone_empty_supports_memory_format branch October 30, 2019 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`empty_like`, `to`, `resize_as_` and `clone` now preserve memory format #23899

`empty_like`, `to`, `resize_as_` and `clone` now preserve memory format #23899

VitalyFedyunin commented Aug 6, 2019 •

edited

ezyang commented Aug 13, 2019

gchanan commented Aug 13, 2019

VitalyFedyunin commented Aug 13, 2019

ezyang commented Aug 13, 2019

gchanan commented Aug 13, 2019

ezyang Aug 13, 2019

ezyang Aug 13, 2019

ezyang Aug 13, 2019

jjsjann123 Aug 14, 2019

ezyang Aug 13, 2019

		@@ -183,7 +183,7 @@ class CAFFE2_API Tensor {
		}

		at::MemoryFormat suggest_memory_format() const {

empty_like, to, resize_as_ and clone now preserve memory format #23899

empty_like, to, resize_as_ and clone now preserve memory format #23899

Conversation

VitalyFedyunin commented Aug 6, 2019 • edited

ezyang commented Aug 13, 2019

gchanan commented Aug 13, 2019

VitalyFedyunin commented Aug 13, 2019

ezyang commented Aug 13, 2019

gchanan commented Aug 13, 2019

ezyang Aug 13, 2019

Choose a reason for hiding this comment

ezyang Aug 13, 2019

Choose a reason for hiding this comment

ezyang Aug 13, 2019

Choose a reason for hiding this comment

jjsjann123 Aug 14, 2019

Choose a reason for hiding this comment

ezyang Aug 13, 2019

Choose a reason for hiding this comment

`empty_like`, `to`, `resize_as_` and `clone` now preserve memory format #23899

`empty_like`, `to`, `resize_as_` and `clone` now preserve memory format #23899

VitalyFedyunin commented Aug 6, 2019 •

edited