-
Notifications
You must be signed in to change notification settings - Fork 39
Replacing throw with TORCH_CHECK #725
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
base: main
Are you sure you want to change the base?
Conversation
TORCH_CHECK( | ||
!(seekMode_ == SeekMode::approximate && | ||
!streamMetadata.averageFps.has_value()), | ||
"Seek mode is approximate, but stream ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this boolean expression much harder to reason about than the previous one. It's wordier, but let's instead do:
if (seekMode_ == SeekMode::approximate) {
TORCH_CHECK(
streamMetadata.averageFpsFromHeader.has_value(),
"Same message as before");
}
I prefer that, as it reads closer to what we would say out loud: if the seek mode is approximate, assert that it has a value for fps.
TORCH_CHECK( | ||
!(reachedEOF || status == AVERROR_EOF), | ||
"Requested next frame while there are no more frames left to decode."); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to revert this check back to what it was previously. We turn the EndOfFileException
into a Python IndexError
in the Python-to-C++ layer:
torchcodec/src/torchcodec/_core/custom_ops.cpp
Lines 303 to 307 in f4a351c
try { | |
result = videoDecoder->getNextFrame(); | |
} catch (const SingleStreamDecoder::EndOfFileException& e) { | |
C10_THROW_ERROR(IndexError, e.what()); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change the custom_ops.cpp file instead to handle it using TORCH_CHECK runtime errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we should keep what's going on in custom_ops.cpp as-is. It has the behavior we want, and we're already using the most relevant PyTorch APIs to achieve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it!
@Krishn1412, thanks for working on this! You're getting test failures because you'll need to pull changes from main - we changed a bunch of variable names in the past few days. There's also likely going to be some test failures related to not throwing the |
Replaced all the occurrences of throw in the cpp code with TORCH_CHECK. TORCH_CHECK throws a runtime error, so the cpp test suite had to be updated as well.
3774eee
to
b585280
Compare
@Krishn1412, it looks like we're still getting some errors related to the exceptions we're now throwing versus the errors we expect to see. If you follow the contributing guide, you can actually run all of these tests locally, too. |
Replaced all the occurrences of throw in the cpp code with TORCH_CHECK.
TORCH_CHECK throws a runtime error, so the cpp test suite had to be updated as well.