Replaced CHECK_ by TORCH_CHECK_ #6322

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

Merged

vfdev-5 merged 1 commit into pytorch:main from vfdev-5:fix-csrc-check-issue

Jul 27, 2022

Collaborator

vfdev-5 commented Jul 27, 2022 •

edited

Loading

Try to fix failing CI

Related pytorch PR: pytorch/pytorch#82032

CUDA CI jobs are failing as they still install 1.13.0.dev20220726 but CPU CI jobs are already running on 1.13.0.dev20220727
-> Check https://anaconda.org/pytorch-nightly/pytorch/files
-> Restart failing CI jobs only


          Replaced CHECK_ by TORCH_CHECK_

facebook-github-bot added the cla signed label

vfdev-5 marked this pull request as ready for review

July 27, 2022 09:37

datumbox previously approved these changes

View reviewed changes

Contributor

datumbox left a comment

LGTM, thanks!

datumbox added module: ops other labels

Contributor

datumbox commented Jul 27, 2022 •

edited

Loading

@vfdev-5 Some test are failing, seem related.

datumbox dismissed their stale review

July 27, 2022 10:44

failing CI

Collaborator Author

vfdev-5 commented Jul 27, 2022 •

edited

Loading

Yes, it is related as these jobs are still installing yesterday's pytorch nightly where CHECK_ is still there. Let's wait for uploaded binaries and restart failing CI jobs

datumbox approved these changes

View reviewed changes

vfdev-5 merged commit ec0e9e1 into pytorch:main

vfdev-5 deleted the fix-csrc-check-issue branch

July 27, 2022 12:48

seemethere mentioned this pull request

Replaced CHECK_ by TORCH_CHECK_ pytorch/audio#2582

Closed

facebook-github-bot pushed a commit to pytorch/audio that referenced this pull request


          Replaced CHECK_ by TORCH_CHECK_ (#2582)

04057fa

Summary:
Pull Request resolved: #2582

CHECK_ were deprecated in upstream so we should replace them here as
well

Similar to pytorch/vision#6322, relates to pytorch/pytorch#82032

Signed-off-by: Eli Uriegas <[email protected]>

Test Plan: Imported from OSS

Reviewed By: malfet, mthrok

Differential Revision: D38208356

Pulled By: seemethere

fbshipit-source-id: 6f42d517362f415e0775803514eee2628402918f

skim0514 pushed a commit to skim0514/audio that referenced this pull request


          Replaced CHECK_ by TORCH_CHECK_ (pytorch#2582)

a2f58a2

Summary:
Pull Request resolved: pytorch#2582

CHECK_ were deprecated in upstream so we should replace them here as
well

Similar to pytorch/vision#6322, relates to pytorch/pytorch#82032

Signed-off-by: Eli Uriegas <[email protected]>

Test Plan: Imported from OSS

Reviewed By: malfet, mthrok

Differential Revision: D38208356

Pulled By: seemethere

fbshipit-source-id: 6f42d517362f415e0775803514eee2628402918f

facebook-github-bot pushed a commit that referenced this pull request


          [fbsync] Replaced CHECK_ by TORCH_CHECK_ (#6322)

Reviewed By: NicolasHug

Differential Revision: D38351759

fbshipit-source-id: 33baf079eb49590752d73bd37d0d37edb1ec87f4

datumbox reviewed

View reviewed changes

Contributor

datumbox left a comment •

edited

Loading

Here is how TORCH_CHECK_* is defined:

#include <glog/logging.h>

// Additional macros on top of glog
#ifndef NDEBUG
#define TORCH_CHECK_EQ(val1, val2) CHECK_EQ(val1, val2)
#else // !NDEBUG
// These versions generate no code in optimized mode.
#define TORCH_CHECK_EQ(val1, val2) \
  while (false)                    \
  CHECK_EQ(val1, val2)
#endif // NDEBUG

This PR broke FB internals. It seems that the real cause of the problem is the change on the semantics/behaviour of *CHECK* on Core. See pytorch/pytorch#82032 (comment).

Regardless, to resolve the situation I'm issuing a forward fix at D38383266 on Phabricator and at #6357 on Github.

torchvision/csrc/io/decoder/sync_decoder_test.cpp

@@ @@ -50,7 +50,7 @@ void gotFilesStats(std::vector<VideoFileStats>& stats) { @@
                   fseek(f, 0, SEEK_END);
                   std::vector<uint8_t> buffer(ftell(f));
                   rewind(f);
-                  CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));
+                  TORCH_CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));

Contributor

datumbox Aug 3, 2022 •

edited

Loading

This is not just a check. This is a function call that must be performed and then checked for its output. Unfortunately the new TORCH_CHECK_EQ mechanism can skip completely the method call if not in debug mode. This means that the function call will never be done causing issues.

torchvision/csrc/io/decoder/sync_decoder_test.cpp

@@ @@ -90,7 +90,7 @@ size_t measurePerformanceUs( @@
                   fseek(f, 0, SEEK_END);
                   std::vector<uint8_t> buffer(ftell(f));
                   rewind(f);
-                  CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));
+                  TORCH_CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));

Contributor

datumbox Aug 3, 2022

another one

torchvision/csrc/io/decoder/sync_decoder_test.cpp

@@ @@ -324,7 +324,7 @@ TEST(SyncDecoder, TestMemoryBuffer) { @@
                 fseek(f, 0, SEEK_END);
                 std::vector<uint8_t> buffer(ftell(f));
                 rewind(f);
-                CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));
+                TORCH_CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));

Contributor

datumbox Aug 3, 2022

this one too

torchvision/csrc/io/decoder/sync_decoder_test.cpp

@@ @@ -349,7 +349,7 @@ TEST(SyncDecoder, TestMemoryBufferNoSeekableWithFullRead) { @@
                 fseek(f, 0, SEEK_END);
                 std::vector<uint8_t> buffer(ftell(f));
                 rewind(f);
-                CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));
+                TORCH_CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));

Contributor

datumbox Aug 3, 2022

this one too

torchvision/csrc/io/decoder/sync_decoder_test.cpp

                 fseek(f, 0, SEEK_END);
                 std::vector<uint8_t> buffer(ftell(f));
                 rewind(f);
-                CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));
+                TORCH_CHECK_EQ(buffer.size(), fread(buffer.data(), 1, buffer.size(), f));

Contributor

datumbox Aug 3, 2022

this one too

datumbox mentioned this pull request

Replace all CHECK_ and DCHECK_ with TORCH_* macros pytorch/pytorch#82032

Closed

facebook-github-bot pushed a commit that referenced this pull request


          Move func calls outside of *CHECK_*

8b09cd7

Summary:
Resolves https://www.internalfb.com/tasks/?t=128004042

Caused by TorchVision's PR at #6322 which was in response to a change on PyTorch Core at pytorch/pytorch#82032

Reviewed By: fmassa

Differential Revision: D38383266

fbshipit-source-id: 3f0ebd04a610031c4720123d1869a851f76455cd

datumbox added a commit to datumbox/vision that referenced this pull request


          Move func calls outside of *CHECK_*

a4260ca

Summary:
Resolves https://www.internalfb.com/tasks/?t=128004042

Caused by TorchVision's PR at pytorch#6322 which was in response to a change on PyTorch Core at pytorch/pytorch#82032

Reviewed By: fmassa

Differential Revision: D38383266

fbshipit-source-id: 3f0ebd04a610031c4720123d1869a851f76455cd

datumbox mentioned this pull request

[FBcode->GH] Move func calls outside of *CHECK_* #6357

Merged

datumbox added a commit that referenced this pull request


          [FBcode->GH] Move func calls outside of *CHECK_* (#6357)

a5f0f79

Summary:
Resolves https://www.internalfb.com/tasks/?t=128004042

Caused by TorchVision's PR at #6322 which was in response to a change on PyTorch Core at pytorch/pytorch#82032

Reviewed By: fmassa

Differential Revision: D38383266

fbshipit-source-id: 3f0ebd04a610031c4720123d1869a851f76455cd

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

Labels

cla signed module: ops other