feat: add input_extra_files for lmp-md and lmp-template #306
Conversation
📝 WalkthroughWalkthroughAdds optional support for attaching extra input files to LMP and NPT task groups. Extends set_lmp and set_md signatures to accept input_extra_files, stores basenames and contents, and appends them to tasks during creation. Updates config argument builders to expose input_extra_files and fixes a trj_freq doc string. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User/Config
participant CFG as TaskGroup Args Builder
participant TG as TaskGroup (LMP/NPT)
participant TK as ExplorationTask
U->>CFG: Provide input_extra_files (optional)
CFG->>TG: set_* (… , input_extra_files)
alt extras provided
TG->>TG: Read files (basename + contents)
else no extras
TG->>TG: Initialize empty lists
end
TG->>TK: Create task with standard inputs
loop for each extra file
TG->>TK: add_file(name, content)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ 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/Issue comments)Type 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: 2
🧹 Nitpick comments (9)
dpgen2/exploration/task/lmp_template_task_group.py (4)
51-52: Add concise doc and type expectation for input_extra_filesBriefly document acceptable values (paths to readable text files) and note that only basenames are kept. This helps config users avoid unexpected overrides.
42-56: Avoid mutable default for revisionsUsing a mutable default dict is a latent footgun. Prefer None and coalesce.
Apply this diff:
def set_lmp( self, numb_models: int, lmp_template_fname: str, plm_template_fname: Optional[str] = None, - revisions: dict = {}, + revisions: Optional[dict] = None, traj_freq: int = 10, extra_pair_style_args: str = "", pimd_bead: Optional[str] = None, input_extra_files: Optional[List[str]] = None, ) -> None: - self.lmp_template = Path(lmp_template_fname).read_text().split("\n") - self.revisions = revisions + self.lmp_template = Path(lmp_template_fname).read_text().split("\n") + self.revisions = revisions or {}
147-152: Add a defensive assertion before zipping filenames and contentsGuards against future changes that might desynchronize the two lists.
Apply this diff:
- # Add extra files to the task - for file_name, file_content in zip( + # Add extra files to the task + assert len(self.input_extra_files) == len(self.input_extra_file_contents), \ + "Mismatched extra file names and contents" + for file_name, file_content in zip( self.input_extra_files, self.input_extra_file_contents ): task.add_file(file_name, file_content)
58-66: DRY: Factor out “load-and-validate extra files” into a utilityThe same logic appears here and in NPTTaskGroup. Consider a shared helper (e.g., dpgen2/exploration/task/utils.py: load_extra_files(paths, reserved)) to centralize validation and encoding behavior.
If you want, I can draft a small utils module and the two call sites.
Also applies to: 147-152
dpgen2/exploration/task/npt_task_group.py (3)
56-57: Document the new parameter and align semantics with config defaultsAdd a short note in the set_md docstring about input_extra_files (list of file paths; basenames kept). Given make_task_group_from_config supplies default [] (not None), ensure current logic handles both None and [] consistently. It does, but documenting avoids confusion.
Would you like me to add a one-liner docstring update here?
152-157: Add a defensive assertion before zipping filenames and contentsSmall safety net against future changes that could desync the lists.
Apply this diff:
- # Add extra files to the task - for file_name, file_content in zip( + # Add extra files to the task + assert len(self.input_extra_files) == len(self.input_extra_file_contents), \ + "Mismatched extra file names and contents" + for file_name, file_content in zip( self.input_extra_files, self.input_extra_file_contents ): task.add_file(file_name, file_content)
79-87: DRY with LmpTemplateTaskGroupThis logic duplicates the LMP-template path. A shared helper would improve consistency and testability.
I can factor a reusable helper and update both call sites in a follow-up commit.
Also applies to: 152-157
dpgen2/exploration/task/make_task_group_from_config.py (2)
51-53: Docs + config plumbing for input_extra_files look good; consider early validationPlumbing and docs are clear. Optionally, validate that each entry is a string path and exists to fail fast at config time.
Example (outside these ranges, in make_lmp_task_group_from_config before set_md/set_lmp):
# add near the start of make_lmp_task_group_from_config from pathlib import Path # at file top # ... files = config.get("input_extra_files") or [] if not all(isinstance(p, str) for p in files): raise TypeError("input_extra_files must be a list of str paths") missing = [p for p in files if not Path(p).is_file()] if missing: raise FileNotFoundError(f"input_extra_files not found: {missing}")Also applies to: 122-128
139-141: LMP-template docs and args: mirror the NPT behaviorLooks consistent with the NPT variant. Consider explicitly mentioning that only basenames are used and collisions with core inputs are disallowed (once checks are added at the call site).
If you add the collision checks suggested in the task groups, I can update these doc strings in the same PR for full alignment.
Also applies to: 190-196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
dpgen2/exploration/task/lmp_template_task_group.py(2 hunks)dpgen2/exploration/task/make_task_group_from_config.py(5 hunks)dpgen2/exploration/task/npt_task_group.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dpgen2/exploration/task/npt_task_group.py (1)
dpgen2/exploration/task/task.py (1)
add_file(30-46)
dpgen2/exploration/task/lmp_template_task_group.py (1)
dpgen2/exploration/task/task.py (1)
add_file(30-46)
🔇 Additional comments (2)
dpgen2/exploration/task/npt_task_group.py (1)
3-5: LGTM: Path importNecessary for reading extra files; no concerns.
dpgen2/exploration/task/make_task_group_from_config.py (1)
76-77: Good fix: trj_freq now references the right doc stringThis resolves the previous doc_nsteps mismatch.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #306 +/- ##
==========================================
+ Coverage 84.22% 84.23% +0.01%
==========================================
Files 104 104
Lines 6112 6129 +17
==========================================
+ Hits 5148 5163 +15
- Misses 964 966 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
fix issue #305
Now we can add input_extra_files for lmp-md and lmp-template in json configurations. The docs were updated too.
Summary by CodeRabbit
New Features
Documentation