test(api): add universal DeepPot C API coverage#5538
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a shared header ( ChangesUniversal DeepPot Test Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/api_cc/tests/test_deeppot_universal.cc (1)
17-99: ⚡ Quick winConsider extracting shared test infrastructure to a common header. The
Backendenum,ModelCasestruct,path_exists,backend_enabled,backend_name, andmodel_casesfunctions are duplicated verbatim across both test files. Factoring these into a shared header (e.g.,deeppot_universal_test_common.h) would eliminate ~80 lines of duplication and ensure backend configuration and tolerance values remain synchronized.
source/api_cc/tests/test_deeppot_universal.cc#L17-L99: Move the duplicated definitions to a shared header and include it.source/api_c/tests/test_deeppot_universal.cc#L17-L99: Include the shared header instead of duplicating the definitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/api_cc/tests/test_deeppot_universal.cc` around lines 17 - 99, The Backend enum, ModelCase struct, and utility functions (path_exists, backend_enabled, backend_name, model_cases) are duplicated across two test files. Create a new shared header file named deeppot_universal_test_common.h in the source/api_cc/tests directory and move all these duplicated definitions into it, then update source/api_cc/tests/test_deeppot_universal.cc to include this shared header instead of defining these symbols locally, and update source/api_c/tests/test_deeppot_universal.cc to also include the shared header and remove its own duplicate definitions of these symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@source/api_cc/tests/test_deeppot_universal.cc`:
- Around line 17-99: The Backend enum, ModelCase struct, and utility functions
(path_exists, backend_enabled, backend_name, model_cases) are duplicated across
two test files. Create a new shared header file named
deeppot_universal_test_common.h in the source/api_cc/tests directory and move
all these duplicated definitions into it, then update
source/api_cc/tests/test_deeppot_universal.cc to include this shared header
instead of defining these symbols locally, and update
source/api_c/tests/test_deeppot_universal.cc to also include the shared header
and remove its own duplicate definitions of these symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d6a0011-cb3c-4d21-821c-3b52011a4b67
📒 Files selected for processing (4)
source/api_c/tests/CMakeLists.txtsource/api_c/tests/test_deeppot_universal.ccsource/api_cc/tests/test_deeppot_universal.ccsource/tests/infer/deeppot_universal_data.h
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5538 +/- ##
==========================================
+ Coverage 82.21% 82.23% +0.01%
==========================================
Files 892 895 +3
Lines 101506 101770 +264
Branches 4242 4264 +22
==========================================
+ Hits 83456 83690 +234
- Misses 16749 16770 +21
- Partials 1301 1310 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add focused parameterized C and C++ API smoke tests that exercise DeepPot metadata and atomic inference over the shared TensorFlow, PyTorch, PT-Expt, JAX, and Paddle fixture matrix when those backends and generated artifacts are available. Link torch into the C API test target so PT-Expt guards are detected consistently with the C++ API tests. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
source/api_c/tests/test_deeppot_universal.cc (1)
119-119: ⚡ Quick winAdd error handling for pbtxt-to-pb conversion.
The conversion call does not check for errors. If
DP_ConvertPbtxtToPbfails silently, the subsequent model loading on line 122 will fail with an error message that does not indicate the root cause was a conversion failure, making debugging more difficult.🛡️ Suggested approach to add error checking
Check the signature of
DP_ConvertPbtxtToPbto determine its error-reporting mechanism (return value, error string, or exception), then add appropriate error handling. For example, if it returns an error string:if (param.convert_pbtxt) { converted_model = "deeppot_c_universal_" + param.name + ".pb"; - DP_ConvertPbtxtToPb(param.model_path.c_str(), converted_model.c_str()); + const char* conv_error = DP_ConvertPbtxtToPb(param.model_path.c_str(), converted_model.c_str()); + if (conv_error && strlen(conv_error) > 0) { + std::string error_msg(conv_error); + DP_DeleteChar(conv_error); + GTEST_SKIP() << "Conversion failed: " << error_msg; + } model_path = converted_model; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/api_c/tests/test_deeppot_universal.cc` at line 119, The DP_ConvertPbtxtToPb function call on line 119 lacks error handling, which means if the conversion fails, the subsequent model loading error will mask the root cause. Check the signature and return type of DP_ConvertPbtxtToPb to determine its error-reporting mechanism (return value, error string output parameter, or exception), then add appropriate error checking and handling immediately after the conversion call that either logs the error details and returns early or throws an exception with a clear message indicating the pbtxt-to-pb conversion failed.source/api_cc/tests/test_deeppot_universal.cc (1)
119-119: ⚡ Quick winAdd error handling for pbtxt-to-pb conversion.
The conversion call does not check for errors. If
deepmd::convert_pbtxt_to_pbfails (either by throwing an exception or silently), the subsequentdp.init()call on line 122 may fail with an error message that does not clearly indicate the root cause was a conversion failure.🛡️ Suggested approach to add error checking
Wrap the conversion in a try-catch block or verify its error-reporting mechanism:
if (param.convert_pbtxt) { converted_model = "deeppot_universal_" + param.name + ".pb"; - deepmd::convert_pbtxt_to_pb(param.model_path, converted_model); + try { + deepmd::convert_pbtxt_to_pb(param.model_path, converted_model); + } catch (const std::exception& e) { + GTEST_SKIP() << "Conversion failed: " << e.what(); + } model_path = converted_model; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/api_cc/tests/test_deeppot_universal.cc` at line 119, The deepmd::convert_pbtxt_to_pb function call does not have error handling, which means if the conversion fails (either by throwing an exception or returning an error state), the subsequent dp.init() call will fail with an unclear error message that does not indicate the conversion was the root cause. Add error handling around the deepmd::convert_pbtxt_to_pb call by either wrapping it in a try-catch block to catch exceptions or checking its return value/error state if it provides one, and ensure that any conversion failure is logged or handled appropriately before attempting the dp.init() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@source/api_c/tests/test_deeppot_universal.cc`:
- Line 119: The DP_ConvertPbtxtToPb function call on line 119 lacks error
handling, which means if the conversion fails, the subsequent model loading
error will mask the root cause. Check the signature and return type of
DP_ConvertPbtxtToPb to determine its error-reporting mechanism (return value,
error string output parameter, or exception), then add appropriate error
checking and handling immediately after the conversion call that either logs the
error details and returns early or throws an exception with a clear message
indicating the pbtxt-to-pb conversion failed.
In `@source/api_cc/tests/test_deeppot_universal.cc`:
- Line 119: The deepmd::convert_pbtxt_to_pb function call does not have error
handling, which means if the conversion fails (either by throwing an exception
or returning an error state), the subsequent dp.init() call will fail with an
unclear error message that does not indicate the conversion was the root cause.
Add error handling around the deepmd::convert_pbtxt_to_pb call by either
wrapping it in a try-catch block to catch exceptions or checking its return
value/error state if it provides one, and ensure that any conversion failure is
logged or handled appropriately before attempting the dp.init() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc176e68-293c-42ff-897d-53e8e9bd3679
📒 Files selected for processing (4)
source/api_c/tests/CMakeLists.txtsource/api_c/tests/test_deeppot_universal.ccsource/api_cc/tests/test_deeppot_universal.ccsource/tests/infer/deeppot_universal_data.h
🚧 Files skipped from review as they are similar to previous changes (2)
- source/api_c/tests/CMakeLists.txt
- source/tests/infer/deeppot_universal_data.h
PyTorch C API backends return the same type map with a trailing separator, so normalize whitespace before checking metadata in the universal DeepPot coverage. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Build PyTorch type maps without a trailing separator so the C and C++ APIs return the same metadata string as the TensorFlow, JAX, and Paddle backends. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Compare universal test type maps as tokens so this coverage PR does not carry the PyTorch trailing-separator fix, which is split into a dedicated PR. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
1 participant
Problem
Change
DP_DeepPotCompute2/DP_DeepPotComputef2and link torch in the C test target so PT-Expt guards are detected consistently.Verification
uvx pre-commit run --files source/api_c/tests/CMakeLists.txt source/api_c/tests/test_deeppot_universal.cc source/api_cc/tests/test_deeppot_universal.cc source/tests/infer/deeppot_universal_data.huvx pre-commit run --from-ref origin/master --to-ref HEADRefs #3905
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Summary by CodeRabbit