Skip to content
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

[BC] Allow only bool tensors as mask in masked_select #96112

Closed
wants to merge 4 commits into from

Conversation

malfet
Copy link
Contributor

@malfet malfet commented Mar 6, 2023

byte support was marked as deprecated in 1.8, so it's fine to remove this in 2.1 (or even 2.0)
Deprecation warning was added by #22261

Also, fix bunch of syntactic errors in comments

cc @albanD

`byte` support was marked as deprecated in 1.13, so it's fine to remove
it in 2.1
@pytorch-bot
Copy link

pytorch-bot bot commented Mar 6, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/96112

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f5152ec:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@malfet malfet added release notes: python_frontend release notes category topic: bc breaking topic category module: python frontend For issues relating to PyTorch's Python frontend and removed module: python frontend For issues relating to PyTorch's Python frontend labels Mar 6, 2023
@malfet
Copy link
Contributor Author

malfet commented Mar 6, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Mar 6, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

@@ -1899,12 +1894,12 @@ static Tensor & masked_select_out_impl_cpu(Tensor & result, const Tensor & self,
auto mask_long_data = mask_long.data_ptr<int64_t>();
auto mask_prefix_sum_data = mask_prefix_sum.data_ptr<int64_t>();
// TODO: Here can only use std::partial_sum for C++14,
// use std::exclusive_scan when PyTorch upgrades to C++17, which have better peformance.
// use std::exclusive_scan when PyTorch upgrades to C++17, which have better performance.
// std::exclusive_scan(mask_long_data, mask_long_data + mask_long.numel(), mask_prefix_sum_data, 0);
Copy link
Collaborator

@Skylion007 Skylion007 Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@malfet can you swap out the comments to std::exclusive_scan now that we are in C++17?

Copy link
Contributor Author

@malfet malfet Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do it (and few other changes) in followup PR (don't want to muddy this one with unrelated functional changes, which also can cause reverts due to ancient internal compilers or something of the natrue)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / macos-12-py3-arm64 / test (default, 1, 2, macos-m1-12)

Details for Dev Infra team Raised by workflow job

Do not test for `torch.uint8` and expect `torch.masked_select` to raise
an assert
@malfet
Copy link
Contributor Author

malfet commented Mar 6, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@malfet malfet requested a review from kulinseth as a code owner March 6, 2023 22:11
@malfet
Copy link
Contributor Author

malfet commented Mar 7, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

ydwu4 pushed a commit to ydwu4/pytorch that referenced this pull request Mar 10, 2023
)

`byte` support was marked as deprecated in 1.8, so it's fine to remove this in 2.1 (or even 2.0)
Deprecation warning was added by pytorch#22261

Also, fix bunch of syntactic errors in comments

Pull Request resolved: pytorch#96112
Approved by: https://github.com/ezyang
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 12, 2023
`byte` support was marked as deprecated in 1.8, so it's fine to remove this in 2.1 (or even 2.0)
Deprecation warning was added by pytorch/pytorch#22261

Also, fix bunch of syntactic errors in comments

Pull Request resolved: pytorch/pytorch#96112
Approved by: https://github.com/ezyang
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 12, 2023
`byte` support was marked as deprecated in 1.8, so it's fine to remove this in 2.1 (or even 2.0)
Deprecation warning was added by pytorch/pytorch#22261

Also, fix bunch of syntactic errors in comments

Pull Request resolved: pytorch/pytorch#96112
Approved by: https://github.com/ezyang
pytorchmergebot pushed a commit that referenced this pull request Mar 13, 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: #96594
Approved by: https://github.com/malfet, https://github.com/ngimel
ydwu4 added a commit to ydwu4/pytorch that referenced this pull request Mar 13, 2023
)

`byte` support was marked as deprecated in 1.8, so it's fine to remove this in 2.1 (or even 2.0)
Deprecation warning was added by pytorch#22261

Also, fix bunch of syntactic errors in comments

Pull Request resolved: pytorch#96112
Approved by: https://github.com/ezyang
@malfet malfet deleted the malfet/disable-bool-tensor-in-masked-select branch June 7, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: python_frontend release notes category topic: bc breaking topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants