Skip to content

[BE] Enforce sign-compare#96723

Closed
malfet wants to merge 13 commits intomasterfrom
malfet/be-enforce-sign-compare
Closed

[BE] Enforce sign-compare#96723
malfet wants to merge 13 commits intomasterfrom
malfet/be-enforce-sign-compare

Conversation

@malfet
Copy link
Copy Markdown
Contributor

@malfet malfet commented Mar 14, 2023

Number of OSS PR were reverted, because new signed-unsigned comparison warnings, which are treated as errors in some internal builds.
Not sure how those selective rules are applied, but this PR removes -Wno-sign-compare from PyTorch codebase.

The only tricky part in this PR, as making sure that non-ASCII character detection works for both signed and unsigned chars here:

Exclude several files from sign-compare if flash attention is used, due to the violation in cutlass, to be fixed by NVIDIA/cutlass#869
Do not try to fix sign compare violations in caffe2 codebase

cc @gujinghui @PenghuiCheng @XiaobingSuper @jianyuh @jgong5 @mingfeima @sanchitintel @ashokei @jingxu10 @min-jean-cho @yanbing-j @Guobing-Chen @Xia-Weiwen @EikanWang

@pytorch-bot
Copy link
Copy Markdown

pytorch-bot bot commented Mar 14, 2023

🔗 Helpful Links

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

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

✅ No Failures

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

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

@pytorch-bot pytorch-bot bot added ciflow/mps Run MPS tests (subset of trunk) release notes: mps Release notes category labels Mar 14, 2023
@github-actions github-actions bot added the NNC label Mar 14, 2023
@malfet malfet added topic: not user facing topic category and removed release notes: mps Release notes category labels Mar 14, 2023
@malfet malfet force-pushed the malfet/be-enforce-sign-compare branch from 34002d2 to 34b70c8 Compare March 14, 2023 14:18
Copy link
Copy Markdown
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Current change look pretty good! :)

@github-actions github-actions bot added the module: mkldnn Related to Intel IDEEP or oneDNN (a.k.a. mkldnn) integration label Mar 14, 2023
Copy link
Copy Markdown
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of that!

@malfet malfet added the ciflow/trunk Trigger trunk jobs on your pull request label Mar 14, 2023
malfet added 5 commits March 15, 2023 02:10
Number of OSS PR were reverted, because new signed-unsigned comparison
types

Not sure how those selective build rules are applied, but this PR
removes `-Wno-sign-compare` from PyTorch codebase
@malfet malfet force-pushed the malfet/be-enforce-sign-compare branch from 168861e to ae0cd4c Compare March 15, 2023 02:10
@malfet
Copy link
Copy Markdown
Contributor Author

malfet commented Mar 15, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / win-vs2019-cuda11.7-py3 / build

Details for Dev Infra team Raised by workflow job

@malfet
Copy link
Copy Markdown
Contributor Author

malfet commented Mar 15, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Copy Markdown
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

@malfet malfet deleted the malfet/be-enforce-sign-compare branch March 15, 2023 14:03
@xwang233
Copy link
Copy Markdown
Collaborator

cc @jjsjann123

cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 23, 2023
Number of OSS PR were reverted, because new signed-unsigned comparison warnings, which are treated as errors in some internal builds.
Not sure how those selective rules are applied, but this PR removes `-Wno-sign-compare` from PyTorch codebase.

The only tricky part in this PR, as making sure that non-ASCII character detection works for both signed and unsigned chars  here:
https://github.com/pytorch/pytorch/blob/6e3d51b08ac108b27d892ab1be85eeb593ec1f0c/torch/csrc/jit/serialization/python_print.cpp#L926

Exclude several files from sign-compare if flash attention is used, due to the violation in cutlass, to be fixed by NVIDIA/cutlass#869
Do not try to fix sign compare violations in caffe2 codebase
Pull Request resolved: pytorch/pytorch#96723
Approved by: https://github.com/albanD
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 27, 2023
Number of OSS PR were reverted, because new signed-unsigned comparison warnings, which are treated as errors in some internal builds.
Not sure how those selective rules are applied, but this PR removes `-Wno-sign-compare` from PyTorch codebase.

The only tricky part in this PR, as making sure that non-ASCII character detection works for both signed and unsigned chars  here:
https://github.com/pytorch/pytorch/blob/6e3d51b08ac108b27d892ab1be85eeb593ec1f0c/torch/csrc/jit/serialization/python_print.cpp#L926

Exclude several files from sign-compare if flash attention is used, due to the violation in cutlass, to be fixed by NVIDIA/cutlass#869
Do not try to fix sign compare violations in caffe2 codebase
Pull Request resolved: pytorch/pytorch#96723
Approved by: https://github.com/albanD
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 module: mkldnn Related to Intel IDEEP or oneDNN (a.k.a. mkldnn) integration NNC topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants