fix: bump the upper bound of deepmd version #1691
Conversation
Fix deepmodeling#1688. Though I am not sure whether we should have an upper version.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #1691 +/- ##
=======================================
Coverage 49.51% 49.51%
=======================================
Files 83 83
Lines 14863 14863
=======================================
Hits 7359 7359
Misses 7504 7504 ☔ View full report in Codecov by Sentry. |
📝 WalkthroughWalkthroughThe pull request introduces modifications to two files: Changes
Assessment against linked issues
The changes directly address the bug report by updating the version handling process for the DeePMD library in the 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
🧹 Nitpick comments (5)
dpgen/generator/run.py (5)
Line range hint
477-481: Consider a more forward-compatible version check.
You are explicitly limiting the support to versions < 4. In the future, if DeePMD-kit v4 or above is compatible, you may need to revise this condition again. A more flexible check (e.g., checking for known-breaking changes) would reduce maintenance overhead.As a follow-up, you could define a helper function to handle version checks rather than scattering multiple conditionals throughout the code.
500-502: Centralize the error message.
Repeatedly using the message "DP-GEN currently only supports for DeePMD-kit 1.x to 3.x version!" in multiple locations can lead to redundancy. Consider storing it in a constant or dedicated method to avoid duplication.- raise RuntimeError( - "DP-GEN currently only supports for DeePMD-kit 1.x to 3.x version!" - ) + ERROR_UNSUPPORTED_VERSION = "DP-GEN only supports DeePMD-kit 1.x to 3.x versions." + raise RuntimeError(ERROR_UNSUPPORTED_VERSION)
574-574: Consider consolidating version constraints.
This version check is identical to others you perform in the file. Maintaining version checks in one place (e.g., a helper function or a single set of conditionals) can simplify upgrades to future DeePMD-kit versions.
598-600: Avoid repetitive runtime errors.
You raise the same “DP-GEN currently only supports for DeePMD-kit 1.x to 3.x version!” exception multiple times. This can be streamlined to ensure any future messaging or logic changes are updated in one place.
821-823: Clarify version support.
If new major versions of DeePMD-kit appear, consider strictly testing them before excluding them here. Maintaining a single point of truth for the supported version range can make it clearer to users how to proceed if they want to try a higher version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpgen/generator/arginfo.py(2 hunks)dpgen/generator/run.py(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- dpgen/generator/arginfo.py
🔇 Additional comments (2)
dpgen/generator/run.py (2)
1698-1698: Extend or validate version support for Gromacs engine.
This check enforces deepmd_version ≥ 2. Clarify (in documentation or error messages) whether partial functionality or a fallback might still be possible for older versions, to give users more context on why the restriction is necessary.
Would you like to run additional checks to ensure older versions cannot be safely supported?
525-526: Maintain consistent version-comparison logic.
Here, you check if Version("2") ≤ deepmd_version < Version("4"). Ensure that other version checks across the code apply these bounds consistently to prevent accidental mismatches and reduce the chance of confusing behavior for borderline cases.
✅ Verification successful
After analyzing the version comparison logic across the codebase, I can generate the final response:
Version comparison logic is consistent across the codebase
The codebase maintains consistent version-comparison patterns:
- Version checks for deepmd versions follow clear boundaries (1.x, 2.x, 3.x) with explicit lower and upper bounds
- API version checks consistently verify minimum version requirements (>= 1.0)
- Legacy version handling (< 1.0) is consistently rejected with appropriate error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence of consistent version-check logic across the code.
rg 'Version\(.*\)|<\s*Version\("|>\s*Version\("' -A 2
Length of output: 6076
Fix #1688. Though I am not sure whether we should have an upper version.
Summary by CodeRabbit
New Features
deepmdlibrary versions 1.x to 3.x.Bug Fixes
Documentation
Refactor