Skip to content

Fix -Werror=type-limits in UnarySignKernels.cu #78573

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix -Werror=type-limits in UnarySignKernels.cu #78573

wants to merge 1 commit into from

Conversation

@seemethere
Copy link
Member

@seemethere seemethere commented May 31, 2022 •
edited
Loading

@seemethere
Fix -Werror=type-limits in UnarySignKernels.cu
e488f30 
Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 31, 2022 •
edited
Loading

🔗 Helpful links

❌ 1 New Failures, 7 Base Failures

As of commit e488f30 (more details on the Dr. CI page):

Expand to see more

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages

See GitHub Actions build pull / linux-bionic-cuda11.3-py3.7-clang9 / build (1/1)

Step: "Pull docker image" (full log | diagnosis details | 🔁 rerun)

2022-05-31T20:42:28.2254940Z �[91m./configure: line 6928: /usr/bin/file: No such file or directory 
2022-05-31T20:42:28.1106495Z checking for dlltool... no
2022-05-31T20:42:28.1108400Z checking how to associate runtime and link libraries... printf %s\n
2022-05-31T20:42:28.1113627Z checking for ar... ar
2022-05-31T20:42:28.1350228Z checking for archiver @FILE support... @
2022-05-31T20:42:28.1354775Z checking for strip... strip
2022-05-31T20:42:28.1359862Z checking for ranlib... ranlib
2022-05-31T20:42:28.2038323Z checking command to parse /usr/bin/nm -B output from gcc object... ok
2022-05-31T20:42:28.2050776Z checking for sysroot... no
2022-05-31T20:42:28.2096462Z checking for a working dd... /bin/dd
2022-05-31T20:42:28.2134863Z checking how to truncate binary pipes... /bin/dd bs=4096 count=1
2022-05-31T20:42:28.2254940Z �[91m./configure: line 6928: /usr/bin/file: No such file or directory
2022-05-31T20:42:28.2269131Z �[0mchecking for mt... no
2022-05-31T20:42:28.2302579Z checking if : is a manifest tool... no
2022-05-31T20:42:28.2501257Z checking how to run the C preprocessor... gcc -E
2022-05-31T20:42:28.3441889Z checking for ANSI C header files... yes
2022-05-31T20:42:28.3649896Z checking for sys/types.h... yes
2022-05-31T20:42:28.3893284Z checking for sys/stat.h... yes
2022-05-31T20:42:28.4135267Z checking for stdlib.h... yes
2022-05-31T20:42:28.4384976Z checking for string.h... yes
2022-05-31T20:42:28.4638629Z checking for memory.h... yes
2022-05-31T20:42:28.4894257Z checking for strings.h... yes

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet.

🚧 6 fixed upstream failures:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD
This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

seemethere added a commit that referenced this pull request May 31, 2022
@seemethere
Fix -Werror=type-limits in UnarySignKernels.cu
bf93aaf 
Signed-off-by: Eli Uriegas <eliuriegasfb.com>

ghstack-source-id: d476dfa
Pull Request resolved: #78573
@seemethere seemethere requested review from a team and dagitses May 31, 2022 20:36
malfet
malfet approved these changes May 31, 2022
View reviewed changes
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.

LGTM, but for uniformity sake, shouldn't we do it for UnarySignKernels.cpp as well

@seemethere
Copy link
Member Author

LGTM, but for uniformity sake, shouldn't we do it for UnarySignKernels.cpp as well

I can't seem to find a file UnarySignKernels.cpp :(

@malfet
Copy link
Contributor

malfet commented May 31, 2022

@seemethere sorry, I've meant here (in UnaryOpsKernel.cpp (why the heck ops are plural but kernel is singular?)

pytorch/aten/src/ATen/native/cpu/UnaryOpsKernel.cpp

Lines 302 to 305 in 96c1348

if (at::isIntegralType(iter.input_dtype(), /*includeBool=*/false)) {
AT_DISPATCH_INTEGRAL_TYPES(iter.input_dtype(), "signbit_cpu", [&]() {
cpu_kernel(iter, [](scalar_t a) -> bool { return a < 0; }); });
} else {

@seemethere
Copy link
Member Author

@pytorchbot merge -f

@seemethere
Copy link
Member Author

@seemethere sorry, I've meant here (in UnaryOpsKernel.cpp (why the heck ops are plural but kernel is singular?)

pytorch/aten/src/ATen/native/cpu/UnaryOpsKernel.cpp

Lines 302 to 305 in 96c1348

if (at::isIntegralType(iter.input_dtype(), /*includeBool=*/false)) {
AT_DISPATCH_INTEGRAL_TYPES(iter.input_dtype(), "signbit_cpu", [&]() {
cpu_kernel(iter, [](scalar_t a) -> bool { return a < 0; }); });
} else {

I see, we could potentially do this in a follow up, want to do the minimum so that we can get diff train unblocked for today.

@github-actions
Copy link
Contributor

Hey @seemethere.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@facebook-github-bot facebook-github-bot deleted the gh/seemethere/241/head branch June 4, 2022 14:17
@dagitses dagitses mentioned this pull request Jun 14, 2022
Closed
dagitses pushed a commit that referenced this pull request Jun 14, 2022
test that OSS flags type-limits warning as error
b338468 
Summary:
#78573 required a forward fix
in the landing train. We want this PR to have the right OSS CI build
failure.

Test Plan: DO NOT SUBMIT

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
dagitses pushed a commit that referenced this pull request Jun 14, 2022
test that OSS flags type-limits warning as error
2f6e91b 
Summary:
#78573 required a forward fix
in the landing train. We want this PR to have the right OSS CI build
failure.

Test Plan: DO NOT SUBMIT

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: 240cada
Pull Request resolved: #79536
@dagitses
Copy link
Collaborator

Good news everybody, this forward fix would no longer be necessary in the future because it would have been caught by OSS CI. Example: #79536

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malfet malfet malfet approved these changes

@dagitses dagitses Awaiting requested review from dagitses

Assignees

No one assigned

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@seemethere @facebook-github-bot @malfet @dagitses @pytorchmergebot