Skip to content

Replace std::runtime_error with TORCH_CHECK #159344

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
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Replace std::runtime_error with TORCH_CHECK #159344

wants to merge 3 commits into from

Conversation

zeshengzong
Copy link
Contributor

Fixes part of #148114

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Jul 29, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 07c7d51 with merge base 3a56237 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@zeshengzong
Copy link
Contributor Author

@pytorchbot label "topic: not user facing"

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Jul 29, 2025
cyyever
cyyever approved these changes Jul 29, 2025
View reviewed changes
Copy link
Collaborator

@cyyever cyyever left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work, but I think it's better to replace TORCH_CHECK(false with the unsatisfied condition (if possible), which can remove more if branches.

@zeshengzong zeshengzong marked this pull request as ready for review July 30, 2025 08:03
Skylion007
Skylion007 reviewed Jul 31, 2025
View reviewed changes
torch/csrc/DataLoader.cpp Outdated
@@ -149,7 +149,7 @@ static PyObject* THPModule_errorIfAnyWorkerFails(
// This is necessary. Otherwise, the runtime error will kill the other
// workers, and trigger this again.
pid_set.clear();
throw std::runtime_error(oss.str());
TORCH_CHECK(false, oss.str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slight nit, would be more efficient using fmtlib. Since this string isn't used a lot in the normal passing case, you could also just use c10::LazyStr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed, but not find c10::LazyStr, is there an example of using it? That it would be helpful. Thanks! :D

torch/csrc/utils/generated_serialization_types.h
@@ -187,15 +188,15 @@ inline std::string_view printEnum(const ArgumentKind& e) {
case ArgumentKind::POSITIONAL: return "POSITIONAL";
case ArgumentKind::KEYWORD: return "KEYWORD";
default:
throw std::runtime_error("Unknown enum value");
TORCH_CHECK(false, "Unknown enum value");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file isn't generated by some other file or schema right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a generated file, the content is tracked by git. Thanks!

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

@Skylion007 Skylion007 Skylion007 left review comments

@cyyever cyyever cyyever approved these changes

Assignees
No one assigned
Labels
open source topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zeshengzong @Skylion007 @cyyever @pytorchbot