Pad: don't error when unused fill value is zero#76307
Pad: don't error when unused fill value is zero#76307peterbell10 wants to merge 2 commits intogh/peterbell10/307/basefrom
Conversation
Fixes pytorch/vision#5873 In the python version of `F.pad`, checking that the fill value was left as default was done by comparing against zero: https://github.com/pytorch/pytorch/blob/bc2c6edaf163b1a1330e37a6e34caf8c553e4755/torch/nn/functional.py#L4366 So if someone does explicitly pass in a zero-value, then this `TORCH_CHECK` was an accidental BC-break. Instead, we should just warn in that case. [ghstack-poisoned]
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit aaf2eba (more details on the Dr. CI page): Expand to see more
🕵️♀️ 1 failure not recognized by patterns:The following CI failures may be due to changes from the PR
This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
datumbox
left a comment
There was a problem hiding this comment.
Thanks for the change @peterbell10. I confirm that this should resolve the BC problem and fix the downstream issues at TorchVision.
The only question I have is on whether we should be raising a deprecation warning. Hence, I'll leave it to @albanD to approve and merge if the current approach aligns with Core's deprecation policy.
aten/src/ATen/native/PadNd.cpp
Outdated
| "\" doesn't take in value argument"); | ||
| // Emit error if value != 0, otherwise just warn | ||
| TORCH_CHECK(*value == 0, err_msg); | ||
| TORCH_WARN(err_msg); |
There was a problem hiding this comment.
Do we really want to deprecate this? Or are we just happy with allowing 0. here?
Given that people use this already and this is not very harmful. I would vote for not changing the current behavior. At least for now so that we can quickly land this. We can discuss (with Joel) if we want to deprecate that later.
There was a problem hiding this comment.
True, it isn't really harmful. Might not be worth the pain of deprecation
There was a problem hiding this comment.
I agree that maintaining the current behaviour until we figure out what to do is probably preferable.
Note that we already have a PR at pytorch/vision#5875 which mitigates the issue by not passing any parameter in specific modes but maintaining BC is worth it for other users.
There was a problem hiding this comment.
Okay, I've removed the warning. I would argue that a passing parameter that gets ignored is somewhat confusing, but if it's already baked into torchvision then I guess it's too late to fix.
There was a problem hiding this comment.
Thanks for the quick forward fix!
re: Vasilis' question: Our policy in core is to throw deprecation warnings using TORCH_WARN_ONCE that are actionable to silence the warning. The warning here can be silenced by passing None instead of 0 to torch's pad.
That said, it seems like torchvision's public API for pad would have to change to pass None as the default value to torch's pad. Otherwise, this warning will be thrown under normal usage with no way to remove it - is this true?
If so, maybe we should just avoid the deprecation for now to minimize pain. As @albanD mentioned, it isn't a particularly harmful thing to pass an unused fill value.
…e is zero" Fixes pytorch/vision#5873 In the python version of `F.pad`, checking that the fill value was left as default was done by comparing against zero: https://github.com/pytorch/pytorch/blob/bc2c6edaf163b1a1330e37a6e34caf8c553e4755/torch/nn/functional.py#L4366 So if someone does explicitly pass in a zero-value, then this `TORCH_CHECK` was an accidental BC-break. [ghstack-poisoned]
datumbox
left a comment
There was a problem hiding this comment.
LGTM, thanks!
FYI TorchVision is going to switch to passing the value only when needed (see pytorch/vision#5875), so that we can fix ASAP our CI.
|
@pytorchbot merge this please |
|
Hey @peterbell10. |
Summary: Fixes pytorch/vision#5873 In the python version of `F.pad`, checking that the fill value was left as default was done by comparing against zero: https://github.com/pytorch/pytorch/blob/bc2c6edaf163b1a1330e37a6e34caf8c553e4755/torch/nn/functional.py#L4366 So if someone does explicitly pass in a zero-value, then this `TORCH_CHECK` was an accidental BC-break. Pull Request resolved: #76307 Approved by: https://github.com/albanD, https://github.com/jbschlosser, https://github.com/datumbox Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/f02b7a9c36dd6182da694bc47a5c345285dfd951 Reviewed By: osalpekar Differential Revision: D35938194 fbshipit-source-id: dabefdded870182ddc198d0e6473009270b895d5
Stack from ghstack (oldest at bottom):
Fixes pytorch/vision#5873
In the python version of
F.pad, checking that the fill value wasleft as default was done by comparing against zero:
pytorch/torch/nn/functional.py
Line 4366 in bc2c6ed
So if someone does explicitly pass in a zero-value, then this
TORCH_CHECKwas an accidental BC-break.