Add the check of API names in check_nan_or_inf, so that the checks …#78816
Conversation
…for nan or inf can be skipped in certain APIs
|
你的PR提交成功，感谢你对开源项目的贡献! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #78816 +/- ##
==========================================
Coverage ? 93.75%
==========================================
Files ? 2
Lines ? 16
Branches ? 0
==========================================
Hits ? 15
Misses ? 1
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR extends eager-mode NaN/Inf debugging by adding an API-name-based skip mechanism to CheckTensorHasNanOrInf, controlled via a new public flag, to avoid checking tensors for specific APIs (including always skipping empty and empty_like).
Changes:
- Added
FLAGS_check_nan_inf_blacklist(comma-separated op/api names) as a public exported flag. - Updated
egr::CheckTensorHasNanOrInfto skip checks whenapi_nameisempty,empty_like, or appears in the blacklist. - Added a C++ unit test covering the new blacklist/skip behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
paddle/fluid/eager/nan_inf_utils.cc |
Adds API-name skip logic (hardcoded empty/empty_like + blacklist parsing) before performing NaN/Inf checks. |
paddle/common/flags.cc |
Introduces the new exported string flag check_nan_inf_blacklist with help text. |
test/cpp/eager/task_tests/nan_inf_utils_test.cc |
Adds test cases validating skip vs non-skip behavior based on API name and blacklist contents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Operator related FLAG | ||
| * Name: FLAGS_check_nan_inf_blacklist | ||
| * Since Version: | ||
| * Value Range: string, default="" | ||
| * Example: FLAGS_check_nan_inf_blacklist="op1,op2,op3" | ||
| * Note: Blacklist of ops to skip when checking NAN/INF | ||
| */ |
There was a problem hiding this comment.
The new exported flag documentation block leaves "Since Version" empty. Since this is a public/debug flag, please fill in the version it is introduced (or remove the field if the file’s convention allows omitting it) to keep the flag docs consistent and actionable.
| if (!FLAGS_check_nan_inf_blacklist.empty()) { | ||
| std::stringstream blacklist_ss(FLAGS_check_nan_inf_blacklist); | ||
| std::string blacklisted_op; | ||
| while (std::getline(blacklist_ss, blacklisted_op, ',')) { | ||
| if (api_name == blacklisted_op) { | ||
| VLOG(4) << "Current op is in blacklist, skip nan inf check: " | ||
| << api_name; | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Blacklist parsing compares api_name to raw comma-split tokens without trimming whitespace. As a result, a value like "op1, op2" will never match "op2" (leading space), which is a common configuration pattern. Consider trimming spaces around each token (and ignoring empty tokens) before comparing to api_name, or explicitly documenting that spaces are not allowed.
There was a problem hiding this comment.
这块也可以改，有几个选择：1. 把所有空格都移除掉（相当于无视空格，容易有意外情况） 2. 直接有空格就报错（最安全），可以单独豁免", "这种情况 3. 或者把任意数量的空格和, 都当成分隔符。
| TEST(NanInfUtils, BlacklistSkipCheck) { | ||
| auto nan_tensor = paddle::experimental::full( | ||
| {3, 4}, std::numeric_limits<double>::quiet_NaN(), phi::DataType::FLOAT64); | ||
|
|
||
| FLAGS_check_nan_inf_blacklist = ""; | ||
| CHECK_APINAME_SKIP("empty", nan_tensor); | ||
|
|
||
| // Test that "empty_like" always skips regardless of blacklist | ||
| FLAGS_check_nan_inf_blacklist = ""; | ||
| CHECK_APINAME_SKIP("empty_like", nan_tensor); | ||
|
|
||
| // Test with empty blacklist (default behavior) | ||
| FLAGS_check_nan_inf_blacklist = ""; | ||
| CHECK_APINAME_NO_SKIP("some_op", nan_tensor); | ||
|
|
||
| // Test with single op in blacklist | ||
| FLAGS_check_nan_inf_blacklist = "single_op"; | ||
| CHECK_APINAME_SKIP("single_op", nan_tensor); | ||
| CHECK_APINAME_NO_SKIP("other_op", nan_tensor); | ||
|
|
||
| // Even when blacklist is set, these should still skip | ||
| CHECK_APINAME_SKIP("empty", nan_tensor); | ||
| CHECK_APINAME_SKIP("empty_like", nan_tensor); | ||
|
|
||
| // blacklist="op1,op2,op3" and op is in blacklist | ||
| FLAGS_check_nan_inf_blacklist = "op1,op2,op3"; | ||
| CHECK_APINAME_SKIP("op1", nan_tensor); | ||
| CHECK_APINAME_SKIP("op2", nan_tensor); | ||
| CHECK_APINAME_SKIP("op3", nan_tensor); | ||
| // not in blacklist, should perform nan_or_inf check | ||
| CHECK_APINAME_NO_SKIP("op4", nan_tensor); | ||
|
|
||
| FLAGS_check_nan_inf_blacklist = ""; | ||
| } |
There was a problem hiding this comment.
This test mutates the global FLAGS_check_nan_inf_blacklist but only resets it to "" at the end. If the test binary is invoked with a non-default value (e.g., via command line/env) or if other tests in the same binary rely on its previous value, this introduces order-dependent behavior. Save the original flag value at the start of the test and restore it (RAII guard or finally-style reset) before returning.
| /** | ||
| * Operator related FLAG | ||
| * Name: FLAGS_check_nan_inf_blacklist | ||
| * Since Version: | ||
| * Value Range: string, default="" | ||
| * Example: FLAGS_check_nan_inf_blacklist="op1,op2,op3" | ||
| * Note: Blacklist of ops to skip when checking NAN/INF | ||
| */ |
|
✅ Cherry-pick successful! Created PR: #78841 |
|
✅ Cherry-pick successful! Created PR: #78842 |
PR Category
Execute Infrastructure
PR Types
Bug fixes
Description
新增了FLAGS_check_nan_inf_blacklist变量，在
eager/nan_inf_utils.cc的CheckTensorHasNanOrInf中在检查tensor前先对api_name进行判断，如果api_name满足(1)是”empty”(2)是“empty_like“(3)在FLAGS_check_nan_inf_blacklist黑名单中，就跳过对tensor的检查是否引起精度变化
否