feat: for user-input systems, it will recursively search for subsystems now. #4858
Conversation
📝 WalkthroughWalkthroughUpdated docstrings clarifying the "systems" argument formats for training/validation helpers and modified process_systems to expand string entries inside list inputs and validate unsupported input types (raising ValueError). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant process_systems
participant expand_sys_str
Caller->>process_systems: call(systems)
alt systems is str
process_systems->>expand_sys_str: expand_sys_str(systems)
expand_sys_str-->>process_systems: expanded list
process_systems-->>Caller: return expanded list
else systems is list
loop for each element
alt element is str
process_systems->>expand_sys_str: expand_sys_str(element)
expand_sys_str-->>process_systems: expanded list
else
process_systems-->>process_systems: append element
end
end
process_systems-->>Caller: return accumulated list
else
process_systems-->>Caller: raise ValueError
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
📝 WalkthroughWalkthroughThe documentation for the "systems" argument in two utility functions was clarified to better describe its expected formats and processing logic. Separately, the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant process_systems
participant expand_sys_str
Caller->>process_systems: Call with systems (str or list)
alt systems is str
process_systems->>expand_sys_str: Expand systems string
expand_sys_str-->>process_systems: List of system paths
process_systems-->>Caller: Return expanded list
else systems is list
loop For each element in systems
alt element is str
process_systems->>expand_sys_str: Expand element string
expand_sys_str-->>process_systems: List of system paths
else element is not str
process_systems-->>process_systems: Append element as-is
end
end
process_systems-->>Caller: Return flattened list
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~6 minutes Possibly related PRs
Suggested labels
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (2)
deepmd/utils/argcheck.py (2)
2924-2928: Docstring improvement looks good, minor clarity tweak suggestedGreat to see the recursive-search behavior documented.
For completeness, consider noting that, when the list contains non-string items (e.g. already-expandedPathobjects returned byprocess_systems), those elements are preserved as is. This matches the current implementation indata_system.pyand avoids future confusion.
3004-3007: Replicate the same clarification for validation systemsSame comment as above: explicitly mention that non-string elements in the list are passed through unchanged so that the training/validation docs stay perfectly in sync with
process_systems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepmd/utils/argcheck.py(2 hunks)deepmd/utils/data_system.py(2 hunks)
🔇 Additional comments (2)
deepmd/utils/data_system.py (2)
740-740: LGTM: Clear documentation updateThe documentation accurately reflects the new behavior where list elements can be either system paths or directories that will be recursively searched.
761-769: LGTM: Robust implementation of recursive system expansionThe implementation correctly handles the new recursive search functionality:
- Properly iterates through list elements and processes strings with
expand_sys_str- Flattens the results by extending rather than appending, avoiding nested lists
- Preserves non-string elements unchanged for backward compatibility
- Aligns perfectly with the PR objective to recursively search for subsystems
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4858 +/- ##
==========================================
- Coverage 84.76% 84.34% -0.43%
==========================================
Files 699 702 +3
Lines 68077 68592 +515
Branches 3541 3572 +31
==========================================
+ Hits 57708 57854 +146
- Misses 9235 9599 +364
- Partials 1134 1139 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deepmd/utils/data_system.py (1)
761-769: Answering prior review: clarify “other situations” and enforce policyRe: “What’s the other situations?” Non-string list elements are the only “other” case here. Current behavior appends them as-is, which is ambiguous. Prefer either:
- Reject non-string items (as in the fix above), or
- Officially support os.PathLike (normalize via
os.fspath), and document it.I recommend rejection for now to keep the API strict and consistent.
🧹 Nitpick comments (4)
deepmd/utils/data_system.py (2)
740-742: Doc update aligns with PR intent; ensure clarity on patterns applicationGood clarification on list handling. To avoid confusion, explicitly state that when
rglob_patternsis provided, it is applied per string item in the list (same as the single-string case).
769-769: Optional: de-duplicate discovered systems while preserving orderIf the same system can be discovered from multiple inputs, consider de-duplicating:
- systems = result_systems + systems = list(dict.fromkeys(result_systems))deepmd/utils/argcheck.py (2)
2926-2928: Doc nit: mention rglob_patterns are applied per list item tooTo mirror
process_systemssemantics and prevent ambiguity, append that whenrglob_patternsare set, they apply to each string item in the list, same as single-string input.
3004-3007: Doc nit: mirror training_data wording about rglob_patterns per itemSame suggestion as training: explicitly mention that
rglob_patternsare applied per-item whensystemsis a list of strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepmd/utils/argcheck.py(2 hunks)deepmd/utils/data_system.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test C++ (true)
- GitHub Check: Test C++ (false)
- GitHub Check: Build C library (2.14, >=2.5.0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
|
I don't find my comments were resolved by any commits |
2 participants
Previously, the systems in the input.json can either be a str containing a directory that has multiple systems to be searched or a list of direct path of systems.
Now it will also search for subsystems for each str in the list.
Summary by CodeRabbit
Documentation
Bug Fixes