Memory format support for contiguous and is_contiguous #20455
Conversation
pytorchbot
added
caffe2
oncall: jit
VitalyFedyunin
requested review from
ezyang,
gchanan,
smessmer and
zdevito
and removed request for
zdevito
VitalyFedyunin
changed the title
There was a problem hiding this comment.
Minor nit: is this resulting in a double-dispatch, or does it call straight to the native implementation? I think it's the native implementation (that's good.)
There was a problem hiding this comment.
it calls native
There was a problem hiding this comment.
nit: Maybe it would be OK to suggest to users that we might relax this restriction in the future
There was a problem hiding this comment.
I want to 'relax' even before we officially put support into documentation. We just need to decide about transformations between NHWC -> NSC -> NHWC
torch/onnx/symbolic_opset9.py
Outdated
There was a problem hiding this comment.
Hmm, does it have to be an integer comparison here? (I don't actually know, just wondering)
There was a problem hiding this comment.
Unfortunately is it jit traced int constant at this point already. That is why I additionally commented it.
There was a problem hiding this comment.
Shouldn't the other format types be supported in this switch?
There was a problem hiding this comment.
Same as
} else if ("Mean" == text) {
return static_cast<int64_t>(Reduction::Mean);
we can introduce all options right now, or add them when we need them in native_functions.yaml
torch/csrc/MemoryFormat.h
Outdated
There was a problem hiding this comment.
Hmm, I'm guessing this was cargo culted from the other enum code. Do we really need this? I did a quick grep and I only see it being passed to THPUtils_packString which seems modestly wasteful.
There was a problem hiding this comment.
It is nice to have, if user decide to print(torch.channels_last) or for example print all arguments he is sending to the function.
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
I don't see any fatal problems, in my opinion, this is good to land. If you want to address the comments in a follow up PR, this is fine by me. You might want to give @dzhulgakov a chance to review this patch.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
zdevito
left a comment
zdevito
left a comment
There was a problem hiding this comment.
JIT changes look fine, did not look at rest of PR
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
VitalyFedyunin
force-pushed
the
memory_format_part1
branch
from
2e115bf to
95ad3db
Compare
VitalyFedyunin
force-pushed
the
memory_format_part1
branch
from
95ad3db to
e796e6c
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@VitalyFedyunin merged this pull request in 5b78a5e. |
Summary:
#19975 was separated by 2 PRs.
This one:
Introduce MemoryFormat argument to the `x.is_contiguous(memory_format=torch.channels_last)` and to the `y = x.contiguous(memory_format=torch.channels_last)` functions.
At this moment both functions just operate with strides and doesn't store any tensor state.
(Original RFC #19092)
-----
Expands functionality of two tensor functions `.is_contiguous` and `.contiguous` (both python and c++ api).
Note: We had several complaints about `.to(memory_format)` function, and decided not to support it.
1. `.contiguous` now support optional keyword-only argument - `memory_format`, which can be either `torch.contiguous_format` or `torch.channels_last`.
- Using `torch.contiguous_format` will preserve existing `.contiguous()` behavior.
- Calling `x.contiguous(memory_format=torch.channels_last)` returns new tensor which maintain same semantical layout (NCHW), but have different memory allocation pattern.
`x.contiguous(memory_format=torch.channels_last)` expects input tensor to be 3d, 4d or 5d; and fails otherwise.
2. `.is_contiguous` now support optional keyword-only argument - `memory_format`, which can be either `torch.contiguous_format` or `torch.channels_last`.
- `x.is_contiguous(memory_format=torch.contiguous_format)` preserves same functionality as `x.is_contiguous()` and remains unchanged.
- `x.is_contiguous(memory_format=torch.channels_last)` returns true if A) input tensor is contiguous in memory AND B) allocated in the memory in NWHC (or similar for 3d,5d) format.
Note: By the end of the phase one `x.is_contiguous(memory_format=torch.channels_last)` will calculate state of the Tensor on every call. This functionality going to be updated later.
Pull Request resolved: pytorch/pytorch#20455
Differential Revision: D15341577
Pulled By: VitalyFedyunin
fbshipit-source-id: bbb6b4159a8a49149110ad321109a3742383185d
| switch (memory_format) { | ||
| case MemoryFormat::Any: | ||
| return stream << "Any"; | ||
| case MemoryFormat::Preserve: |
There was a problem hiding this comment.
nit: in general I think we should be overly conservative about what we expose to python.
I don't think you need Preserve at this point.
I don't really think you need Any -- that seems like it can just be done with an optional, and that fits in with moving things to TensorOptions, which are all optionals anyway. Maybe there's a case where we would want this for default construction reasons, but I don't see it yet.
"Contiguous" is reusing some of our existing terminology that doesn't have standard definitions. So it makes sense from a historical perspective, but if you are new to PyTorch, it's not clear to me it makes sense. Should we call this "C_Contiguous"?
There was a problem hiding this comment.
Thinking more about Preserve -- I'm a bit worried about mixing up specifications and states -- Preserve is a valid specification but not a valid state.
We have a similar issue with Devices -- it seems fine at the python level to have a single type (for simplicity of the API), but I find it pretty annoying in the C++ level to constantly have to worry about getting a specification that isn't actually a device (i.e. 'cuda').
So, at the C++ level here we could potentially have MemoryFormat and MemoryFormatSpec.
And actually, do we even need preserve? Here's a proposal:
The parameter specification of memory format for empty_like is:
MemoryFormat? memory_format=None.
This could be handled in 1 of 2 ways:
- we just logically use
Noneto meanPreserveand everything is aMemoryFormat? - we complicate the parsing a bit and make the parser for
MemoryFormat?return aMemoryFormatSpec.
| return THPUtils_packString(self->name); | ||
| } | ||
|
|
||
| PyTypeObject THPMemoryFormatType = { |
There was a problem hiding this comment.
btw, have we considered switching to pybind11 for enums like this? that's a lot of boilerplate
| } | ||
| bool is_contiguous() const { | ||
| return impl_->is_contiguous(); | ||
| bool is_contiguous(at::MemoryFormat memory_format=at::MemoryFormat::Any) const { |
There was a problem hiding this comment.
why default value here is Any and not Contiguous? doesn't really matter, but might help readability
There was a problem hiding this comment.
See my comment above -- I think @VitalyFedyunin is going to look into removing Any.
|
I suggest to discuss |
8 participants
#19975 was separated by 2 PRs.
This one:
Introduce MemoryFormat argument to the
x.is_contiguous(memory_format=torch.channels_last)and to they = x.contiguous(memory_format=torch.channels_last)functions.At this moment both functions just operate with strides and doesn't store any tensor state.
(Original RFC #19092)
Expands functionality of two tensor functions
.is_contiguousand.contiguous(both python and c++ api).Note: We had several complaints about
.to(memory_format)function, and decided not to support it..contiguousnow support optional keyword-only argument -memory_format, which can be eithertorch.contiguous_formatortorch.channels_last.Using
torch.contiguous_formatwill preserve existing.contiguous()behavior.Calling
x.contiguous(memory_format=torch.channels_last)returns new tensor which maintain same semantical layout (NCHW), but have different memory allocation pattern.x.contiguous(memory_format=torch.channels_last)expects input tensor to be 3d, 4d or 5d; and fails otherwise..is_contiguousnow support optional keyword-only argument -memory_format, which can be eithertorch.contiguous_formatortorch.channels_last.x.is_contiguous(memory_format=torch.contiguous_format)preserves same functionality asx.is_contiguous()and remains unchanged.x.is_contiguous(memory_format=torch.channels_last)returns true if A) input tensor is contiguous in memory AND B) allocated in the memory in NWHC (or similar for 3d,5d) format.Note: By the end of the phase one
x.is_contiguous(memory_format=torch.channels_last)will calculate state of the Tensor on every call. This functionality going to be updated later.