Deprecate non-bool masks in masked_fill #96594
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/96594
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit b80429e: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
ciflow/mps
github-actions
bot
added
module: cpu
kvathupo
requested review from
BowenBao,
abock,
digantdesai,
jerryzh168,
jianyuh,
kimishpatel,
kulinseth and
salilsdesai
as code owners
Contributor
Author
malfet
approved these changes
Mar 12, 2023
Contributor
malfet
left a comment
malfet
left a comment
There was a problem hiding this comment.
Thank you very much for the PR. Looks good to me.
Please cleanup mask_t template parameter (left a few suggestions, tough technically its optional)
aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp
Outdated
Show resolved
Hide resolved
aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp
Outdated
Show resolved
Hide resolved
aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp
Outdated
Show resolved
Hide resolved
malfet
added
topic: bc breaking
Contributor
Author
Applied, looks cleaner without the redundant bool params and checks ! |
ngimel
approved these changes
Mar 12, 2023
Collaborator
ngimel
left a comment
ngimel
left a comment
There was a problem hiding this comment.
Let's hope nothing breaks
Contributor
Author
|
@pytorchbot merge |
Collaborator
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Mar 23, 2023
__What?__ Per discussion at #94634, deprecate `masked_fill` with non-bool masks. Deprecation warnings were previously added by #22261, but not for Apple MPS. I can revert the MPS changes if deprecation warnings are wanted first tho. See also #96112. Fixes #85063 and #89320. __Further Development?__ - Fixed the mask dtype checking for the cuda dispatch for `masked_fill` in `aten/src/ATen/native/cuda/Indexing.cu` Pull Request resolved: pytorch/pytorch#96594 Approved by: https://github.com/malfet, https://github.com/ngimel
cyyever
pushed a commit
to cyyever/pytorch_private
that referenced
this pull request
Mar 27, 2023
__What?__ Per discussion at #94634, deprecate `masked_fill` with non-bool masks. Deprecation warnings were previously added by #22261, but not for Apple MPS. I can revert the MPS changes if deprecation warnings are wanted first tho. See also #96112. Fixes #85063 and #89320. __Further Development?__ - Fixed the mask dtype checking for the cuda dispatch for `masked_fill` in `aten/src/ATen/native/cuda/Indexing.cu` Pull Request resolved: pytorch/pytorch#96594 Approved by: https://github.com/malfet, https://github.com/ngimel
pytorchmergebot
pushed a commit
that referenced
this pull request
Apr 1, 2023
Modify test_torch to check that assert is raised in this case torch.uint8 usage has been deprecated for a few releases, and errors has been raised for other dtypes on CUDA device, but not on CPU. This PR finally restricts mask to just `torch.bool` See #96594 as an example doing it for `torch.masked_fill` Fixes #94634 Pull Request resolved: #97999 Approved by: https://github.com/ngimel
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Per discussion at #94634, deprecate
masked_fillwith non-bool masks. Deprecation warnings were previously added by #22261, but not for Apple MPS. I can revert the MPS changes if deprecation warnings are wanted first tho. See also #96112.Fixes #85063 and #89320.
Further Development?
masked_fillinaten/src/ATen/native/cuda/Indexing.cucc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10 @EikanWang @soumith @voznesenskym @penguinwu @anijain2305 @Guobing-Chen @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @desertfire