fix(tf): fix argcheck when compressing a model converted from other backends#4331
Conversation
…backends (cherry picked from commit 232f7cd)
📝 WalkthroughWalkthroughThe changes in this pull request modify the Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
deepmd/tf/entrypoints/compress.py (1)
150-151: LGTM with suggestions for improvement.The fix correctly addresses the issue with converted models by providing default values for required normalization arguments. The placement and default values are appropriate.
Consider these improvements:
- Add a comment explaining why these specific defaults were chosen
- Add validation to warn users when these defaults are used, as it might indicate unexpected model configuration
+ # Default values for models converted from other backends that only include 'model' section + if "training" not in jdata: + log.warning("No training configuration found. Using defaults for converted model.") jdata.setdefault("training", {"numb_steps": 0}) + if "learning_rate" not in jdata: + log.warning("No learning rate configuration found. Using defaults for converted model.") jdata.setdefault("learning_rate", {})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
deepmd/tf/entrypoints/compress.py(1 hunks)
🔇 Additional comments (1)
deepmd/tf/entrypoints/compress.py (1)
150-154: Verify normalization behavior with default values.
The code adds default values before normalization. Let's ensure the normalize function properly handles these defaults.
✅ Verification successful
Based on the search results, I found that the normalize function is implemented in deepmd/utils/argcheck.py and is used to validate and normalize input arguments. The code shows that the normalize function properly handles empty dictionaries through the base Argument class. Additionally, looking at the compression code in deepmd/tf/entrypoints/compress.py, we can see that:
- Empty dictionaries are explicitly set as defaults for both "training" and "learning_rate"
- The normalize function is called after setting these defaults
- The normalize function is designed to handle these default values through argument validation
Default values are properly handled by the normalization step.
The code follows a pattern of setting defaults first, then normalizing the input through a robust argument validation system. This is a safe approach that ensures proper initialization of configuration parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for normalize function implementation to verify handling of empty learning_rate
ast-grep --pattern 'def normalize($$$)'
# Search for any validation of training or learning_rate fields
rg -A 5 'training.*numb_steps|learning_rate' --type python
Length of output: 133
Script:
#!/bin/bash
# Search for normalize function implementation without file type restriction
rg -A 10 "def normalize"
# Search for validation patterns without file type restriction
rg -A 5 "training.*numb_steps|learning_rate"
# Find all Python files that might contain normalize function
fd "\.py$" --exec grep -l "normalize"
Length of output: 161722
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4331 +/- ##
==========================================
- Coverage 84.58% 84.54% -0.05%
==========================================
Files 571 571
Lines 53180 53182 +2
Branches 3059 3059
==========================================
- Hits 44981 44961 -20
- Misses 7237 7258 +21
- Partials 962 963 +1 ☔ View full report in Codecov by Sentry. |
njzjz
changed the title
2 participants
When the model is converted from other backends, the input script only contains the
modelsection. This PR sets the default for any necessary argument.Summary by CodeRabbit
New Features
Bug Fixes