Implement Dataclasses as Input #330
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds two public dataclass input schemas, ChangesTyped Input Schemas & Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #330 +/- ##
==========================================
+ Coverage 88.78% 89.06% +0.28%
==========================================
Files 11 12 +1
Lines 1159 1198 +39
==========================================
+ Hits 1029 1067 +38
- Misses 130 131 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
![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 (1)
src/lammpsparser/compatibility/data.py (1)
23-23: ⚡ Quick win
unitsfield in both dataclasses is silently overridden and effectively deadIn
file.py, the MD path (line 175) and minimize path (line 185) both executecalc_kwargs["units"] = units, whereunitsis the function-level parameter (defaulting to"metal"). This unconditionally overwrites whatever the dataclass field contains. A caller who setsCalcMDInput(units="lj")will have that value silently discarded.The
unitsfield serves no purpose in its current form. The cleanest fix is to remove it from both dataclasses and rely solely on the function-levelunitsparameter, or to readcalc_kwargs["units"]into the outerunitsvariable when a dataclass is provided.♻️ Proposed fix — remove redundant `units` from both dataclasses
`@dataclass` class CalcMDInput: ... - units: str = "metal"`@dataclass` class CalcMinimizeInput: ... - units: str = "metal"Also applies to: 36-36
🤖 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 `@src/lammpsparser/compatibility/data.py` at line 23, The dataclass field units is redundant and gets overwritten by the function-level units parameter; remove the units attribute from the CalcMDInput and CalcMinimizeInput dataclasses in src/lammpsparser/compatibility/data.py and rely solely on the function-level units parameter (and calc_kwargs["units"] assignment) in the MD/minimize paths, or alternatively ensure the function reads any units provided by a dataclass by setting units = calc_kwargs.get("units", units) before assigning calc_kwargs["units"] = units; update either CalcMDInput/CalcMinimizeInput to drop units or implement the get-from-calc_kwargs read in the MD/minimize code paths (refer to CalcMDInput, CalcMinimizeInput, calc_kwargs and the function-level parameter units).
🤖 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.
Inline comments:
In `@src/lammpsparser/compatibility/data.py`:
- Around line 8-23: CalcMDInput is missing an n_ionic_steps field so asdict()
never emits it and MD jobs fall back to 1; add an integer field named
n_ionic_steps to the CalcMDInput dataclass (e.g., n_ionic_steps: int = 1) so the
key is present when asdict() is used and existing dispatch code that
int(calc_kwargs.pop("n_ionic_steps")) continues to work.
In `@src/lammpsparser/compatibility/file.py`:
- Around line 96-107: The CalcMinimizeInput branch converts the dataclass to
calc_kwargs with asdict(), which includes a "structure" key and causes a
duplicate keyword when later calling calc_minimize(structure=structure,
**calc_kwargs); fix by removing the structure entry from calc_kwargs before the
call (e.g., pop("structure", None)) in the block that handles CalcMinimizeInput
so calc_minimize receives the explicit structure param only; ensure this change
is applied only for the CalcMinimizeInput path (symbols: calc_dataclass,
CalcMinimizeInput, calc_kwargs, calc_minimize, CalcMDInput).
---
Nitpick comments:
In `@src/lammpsparser/compatibility/data.py`:
- Line 23: The dataclass field units is redundant and gets overwritten by the
function-level units parameter; remove the units attribute from the CalcMDInput
and CalcMinimizeInput dataclasses in src/lammpsparser/compatibility/data.py and
rely solely on the function-level units parameter (and calc_kwargs["units"]
assignment) in the MD/minimize paths, or alternatively ensure the function reads
any units provided by a dataclass by setting units = calc_kwargs.get("units",
units) before assigning calc_kwargs["units"] = units; update either
CalcMDInput/CalcMinimizeInput to drop units or implement the
get-from-calc_kwargs read in the MD/minimize code paths (refer to CalcMDInput,
CalcMinimizeInput, calc_kwargs and the function-level parameter units).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e60c6e9f-6cf6-4a7c-9e00-aabcf13adf4f
📒 Files selected for processing (2)
src/lammpsparser/compatibility/data.pysrc/lammpsparser/compatibility/file.py
| if calc_dataclass is not None: | ||
| if isinstance(calc_dataclass, CalcMDInput): | ||
| calc_mode = "md" | ||
| calc_kwargs = asdict(calc_dataclass) | ||
|
|
||
| elif isinstance(calc_dataclass, CalcMinimizeInput): | ||
| calc_mode = "minimize" | ||
| calc_kwargs = asdict(calc_dataclass) | ||
| else: | ||
| raise TypeError( | ||
| "calc_dataclass must be an instance of either CalcMDInput or CalcMinimizeInput" | ||
| ) |
There was a problem hiding this comment.
CalcMinimizeInput.structure causes TypeError in calc_minimize call
CalcMinimizeInput includes a structure field. asdict() converts every dataclass field into a dict entry, recursing into nested dataclasses, dicts, lists, and tuples — and other objects (like Atoms) are copied with copy.deepcopy(). So calc_kwargs = asdict(calc_dataclass) will include a "structure" key.
At line 192 this dict is then spread into:
calc_minimize(structure=structure, **calc_kwargs)which is equivalent to passing structure= twice → TypeError: got multiple values for keyword argument 'structure'. The CalcMinimizeInput path is entirely broken at runtime.
🐛 Proposed fix — pop `structure` from `calc_kwargs`
elif isinstance(calc_dataclass, CalcMinimizeInput):
calc_mode = "minimize"
calc_kwargs = asdict(calc_dataclass)
+ calc_kwargs.pop("structure", None) # structure is passed separately to the function🤖 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 `@src/lammpsparser/compatibility/file.py` around lines 96 - 107, The
CalcMinimizeInput branch converts the dataclass to calc_kwargs with asdict(),
which includes a "structure" key and causes a duplicate keyword when later
calling calc_minimize(structure=structure, **calc_kwargs); fix by removing the
structure entry from calc_kwargs before the call (e.g., pop("structure", None))
in the block that handles CalcMinimizeInput so calc_minimize receives the
explicit structure param only; ensure this change is applied only for the
CalcMinimizeInput path (symbols: calc_dataclass, CalcMinimizeInput, calc_kwargs,
calc_minimize, CalcMDInput).
|
2 participants
The dataclasses for the output are already available at:
https://github.com/pyiron/pyiron_dataclasses/blob/main/pyiron_dataclasses/v1/jobs/lammps.py
Summary by CodeRabbit