Skip to content

Conversation

FFFrog
Copy link
Collaborator

@FFFrog FFFrog commented Apr 11, 2024

Fixes #123715

As the title stated.

But, maybe we should pay attention to this #33206, which removed the half support for cpu about 4 years ago.

@FFFrog FFFrog requested a review from mruberry as a code owner April 11, 2024 07:18
Copy link

pytorch-bot bot commented Apr 11, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 8044c54 with merge base 236b0d1 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@FFFrog FFFrog marked this pull request as draft April 11, 2024 07:35
@FFFrog FFFrog marked this pull request as ready for review April 11, 2024 09:54
@albanD albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 12, 2024
@FFFrog
Copy link
Collaborator Author

FFFrog commented Apr 16, 2024

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 16, 2024
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@FFFrog FFFrog requested review from kulinseth and malfet as code owners April 16, 2024 08:50
@pytorch-bot pytorch-bot bot added the ciflow/mps Run MPS tests (subset of trunk) label Apr 16, 2024
@malfet malfet added release notes: python_frontend python frontend release notes category topic: improvements topic category labels Apr 16, 2024
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Should we create some sort of a new issues, as a lot of things has happened since 4 years ago, for example CPU no longer equals x86_64 only, and on ARM half support makes a lot of sense. But there are also precision consideration

@malfet
Copy link
Contributor

malfet commented Apr 16, 2024

@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: 3 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
Copy link
Contributor

malfet commented Apr 16, 2024

@FFFrog looks you've uncovered a bug in MPS binary cross-entropy implementation

test_mps.py::TestConsistencyCPU::test_output_match_nn_functional_binary_cross_entropy_cpu_float16 loc("mps_subtract"("(mpsFileLoc): /AppleInternal/Library/BuildRoots/9e200cfa-7d96-11ed-886f-a23c4f261b56/Library/Caches/com.apple.xbs/Sources/MetalPerformanceShadersGraph/mpsgraph/MetalPerformanceShadersGraph/Core/Files/MPSGraphUtilities.mm":228:0)): error: input types 'tensor<f32>' and 'tensor<1xf16>' are not broadcast compatible

Please file an issue and disable the test for this dtype (there is a skip list)

@FFFrog
Copy link
Collaborator Author

FFFrog commented Apr 17, 2024

@FFFrog looks you've uncovered a bug in MPS binary cross-entropy implementation

test_mps.py::TestConsistencyCPU::test_output_match_nn_functional_binary_cross_entropy_cpu_float16 loc("mps_subtract"("(mpsFileLoc): /AppleInternal/Library/BuildRoots/9e200cfa-7d96-11ed-886f-a23c4f261b56/Library/Caches/com.apple.xbs/Sources/MetalPerformanceShadersGraph/mpsgraph/MetalPerformanceShadersGraph/Core/Files/MPSGraphUtilities.mm":228:0)): error: input types 'tensor<f32>' and 'tensor<1xf16>' are not broadcast compatible

Please file an issue and disable the test for this dtype (there is a skip list)

Thank you for your reply. I will file an issue about this.

@FFFrog
Copy link
Collaborator Author

FFFrog commented Apr 17, 2024

@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

@FFFrog FFFrog deleted the mrl_fix_bce branch April 17, 2024 10:08
sanketpurandare pushed a commit to sanketpurandare/pytorch that referenced this pull request Apr 22, 2024
Fixes pytorch#123715

As the title stated.

But, maybe we should pay attention to this pytorch#33206, which removed the half support for cpu about 4 years ago.

Pull Request resolved: pytorch#123823
Approved by: https://github.com/Skylion007, https://github.com/malfet
petrex pushed a commit to petrex/pytorch that referenced this pull request May 3, 2024
Fixes pytorch#123715

As the title stated.

But, maybe we should pay attention to this pytorch#33206, which removed the half support for cpu about 4 years ago.

Pull Request resolved: pytorch#123823
Approved by: https://github.com/Skylion007, https://github.com/malfet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/mps Run MPS tests (subset of trunk) ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: python_frontend python frontend release notes category topic: improvements topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support to bf16 element type for binary_cross_entropy.

6 participants