-
Notifications
You must be signed in to change notification settings - Fork 687
Make determinism of channels_last more conservative #14862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary: When in doubt because both is_contiguous and is_contiguous(channels_last) return true, assume it is channels first Differential Revision: D83998877
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14862
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit f0abe93 with merge base 8ac6300 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@kimishpatel has exported this pull request. If you are a Meta employee, you can view the originating Diff in D83998877. |
This PR needs a
|
def _is_nhwc_tensor(tensor: torch.Tensor) -> bool: | ||
nhwc = tensor.is_contiguous(memory_format=torch.channels_last) | ||
nchw = tensor.is_contiguous() | ||
# if both are true false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just use Tensor.dim_order(ambiguity_check=True)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it raises runtime error when ambiguity_check=True. Thats why i had to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC this is about ambiguous cases right? If yes, when we have ambiguous shape [1,1,1,1] this may cause issues when the shape changes at runtime and our assumptions about the memory format (one way or the other) breaks down and results in silently calculating incorrect results.
The right solution might be to get rid of torch.memory_format
from XNNPACK switch to unambiguous (at least with the new API with check) and deal with it properly.
How would you determine the unambiguous cases? What I found for one of the models, which was not using channels in the conventional sense, it was just a 4D tensor, it resulted in failures to lower.
Yes, but that challenge is what is unambiguous. Problem is we dont have a language in pytorch to unambiguously say what is the dim order. I think the right solution would be that. Although when I think about it, it feels fairly non-trivial. User would have to be explicit in their intent within the model to tell you what is the dim order. I think safe thing to do would be start out wit making everything contiguous/channels first and then introduce any dim order changes explicitly and this would have to happen within XNNPACK backend passes. |
Differential Revision: D83998877 Pull Request resolved: pytorch#14862
Are there any specific examples of this that you're aware of that yield silent correctness issues? |
if a tensor of a given shape can be mapped to multiple dim_order ==> it is ambiguous. |
Summary:
When in doubt because both is_contiguous and is_contiguous(channels_last)
return true, assume it is channels first
Differential Revision: D83998877