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

Fix size_t vs int logic in CHECK_OP #55730

Merged
merged 3 commits into from Apr 28, 2022

Conversation

awf
Copy link

@awf awf commented Apr 25, 2022

Previously, there was custom logic for size_t vs int instances of CHECK_OP.

However the logic was correct only for equality ops, not != or greater/less than.

Example: Before

bazel  test --jobs=40  --config=dbg --verbose_failures //tensorflow/core:__tensorflow_core_lib_math_math_util_test
...
-----------------------------------------------------------------------------
[==========] Running 5 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 5 tests from MathUtil
[ RUN      ] MathUtil.CeilOfRatio
2022-04-25 13:53:45.485294: F ./tensorflow/core/lib/math/math_util.h:123] Check failed: 0 != denominator (0 vs. 18446744073709551615)Division by zero is not supported.
*** Received signal 6 ***

After: test passes.

Fixes #55530

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Apr 25, 2022
@google-cla
Copy link

google-cla bot commented Apr 25, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@mihaimaruseac mihaimaruseac added the kokoro:force-run Tests on submitted change label Apr 25, 2022
@mihaimaruseac
Copy link
Collaborator

Please sign CLA. We cannot review this before that.

Since this is in macros, we might need to make sure all builds are passing before this lands.

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 25, 2022
@awf
Copy link
Author

awf commented Apr 25, 2022

Please sign CLA. We cannot review this before that.

Oops, apologies, I thought that had refreshed. Now done.

Since this is in macros, we might need to make sure all builds are passing before this lands.

Makes sense.

@mihaimaruseac mihaimaruseac added the kokoro:force-run Tests on submitted change label Apr 25, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 25, 2022
@awf
Copy link
Author

awf commented Apr 26, 2022

Windows Bazel GPU failing, but I can't see the log ... is it possible to view it?

image

@gbaned gbaned added the comp:core issues related to core part of tensorflow label Apr 26, 2022
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Apr 26, 2022
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Apr 26, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 26, 2022
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Apr 26, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 26, 2022
@mihaimaruseac mihaimaruseac changed the title Fix size_t vs int logic in CHECK_OP Fix size_t vs int logic in CHECK_OP Apr 26, 2022
@mihaimaruseac
Copy link
Collaborator

So, windows GPU failure:

T:\src\github\tensorflow>CALL tensorflow\tools\ci_build\windows\gpu\pip\run.bat --enable_remote_cache "T:\src\gfile\bazel_wrapper.py"  1>tensorflow/tools/ci_build/builds/win.out 2>&1 

ERROR: Aborting VM command due to timeout of 10800 seconds

This can be ignored, I see the other one passed, so we should be good.

@awf
Copy link
Author

awf commented Apr 26, 2022

Cool, thanks.

I guess I don't need to/can't do anything from this point?

@mihaimaruseac
Copy link
Collaborator

No, nothing left to do on your side. I think the pylint check has been deprecated, waiting for some context on that.

We're now waiting for 'import/copybara' step to import this PR into an internal changelist (CL). Then this one will be reviewed again, tests will run with this CL and the internal google code and if all is green the PR will be automerged (or at least have a commit message update here). If something breaks, we'll come back. here is the full process:

7XNWZfbTauSmBXn

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Apr 28, 2022
@copybara-service copybara-service bot merged commit b917181 into tensorflow:master Apr 28, 2022
13 of 15 checks passed
mihaimaruseac pushed a commit that referenced this pull request May 8, 2022
mihaimaruseac pushed a commit that referenced this pull request May 8, 2022
mihaimaruseac pushed a commit that referenced this pull request May 8, 2022
mihaimaruseac pushed a commit that referenced this pull request May 8, 2022
mihaimaruseac added a commit that referenced this pull request May 8, 2022
…a41f4d938dc369109a1-on-r2.9

Merge pull request #55730 from graphcore:awf/issue-55530
mihaimaruseac added a commit that referenced this pull request May 8, 2022
…a41f4d938dc369109a1-on-r2.8

Merge pull request #55730 from graphcore:awf/issue-55530
mihaimaruseac added a commit that referenced this pull request May 8, 2022
…a41f4d938dc369109a1-on-r2.7

Merge pull request #55730 from graphcore:awf/issue-55530
mihaimaruseac added a commit that referenced this pull request May 8, 2022
…a41f4d938dc369109a1-on-r2.6

Merge pull request #55730 from graphcore:awf/issue-55530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:core issues related to core part of tensorflow ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

Test fail on r2.8: core:__tensorflow_core_lib_math_math_util_test
5 participants