Fix Simpson integration error endpoint handling#117
Open
Yi-FanLi wants to merge 22 commits into
Open
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThis PR introduces a JSON-driven workflow orchestration system for composing multi-step thermodynamic-integration campaigns with state persistence and concurrent execution, adds per-phase model support in DPDT setup, rewrites the Simpson integration error algorithm, and documents the new workflow feature. ChangesDPTI Workflow System and Thermodynamic Integration Enhancements
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 @@
## devel #117 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 25 26 +1
Lines 6305 6575 +270
======================================
- Misses 6305 6575 +270 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_gdi_make_task.py (1)
57-58: 💤 Low valueTest fixture naming is semantically confusing.
Using
beta.lmp(a LAMMPS config) as a stand-in forgraph.1.pb(a DeepMD model) works for MD5 comparison but may confuse readers. Consider adding a comment clarifying this is intentionally using a distinct file to verify per-phase copying, or duplicatinggraph.pbwith different content.🤖 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 `@tests/test_gdi_make_task.py` around lines 57 - 58, The test uses shutil.copyfile to create graph.0.pb from "graph.pb" and graph.1.pb from "beta.lmp", which is semantically confusing; update tests/test_gdi_make_task.py to either (a) replace the second shutil.copyfile call so it duplicates "graph.pb" into graph.1.pb but alters its contents slightly (e.g., different bytes) to make a true distinct model file for MD5 comparison, or (b) keep using "beta.lmp" but add a clear inline comment next to the shutil.copyfile("beta.lmp", os.path.join(parent_dir, "graph.1.pb")) call explaining that beta.lmp is intentionally used as a distinct placeholder for testing per-phase copying and not a real DeepMD model.
🤖 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 `@docs/workflow.md`:
- Around line 44-46: The example's task "hti.gen" declares a dependency "needs":
["nvt.run"] that isn't defined in the minimal schema; either remove the "needs"
entry from the "hti.gen" object or add a corresponding task named "nvt.run"
earlier in the example so the dependency resolves (edit the JSON where "hti.gen"
and the other task definitions appear).
In `@dpti/workflow.py`:
- Around line 161-169: The step command may be empty after calling
_normalize_command(step["command"]), so before printing/using it and before
subprocess.run, validate that the returned command list is non-empty; if empty,
raise a clear exception or return an error dict (e.g., {"returncode": 1,
"error": "empty command"}) so we reject empty commands early. Locate the
validation around the code that calls _normalize_command, uses work_dir and
subprocess.run (references: _normalize_command, step["command"], work_dir,
subprocess.run) and add the guard just after normalization and before the
dry_run check.
- Around line 256-275: The loop that processes completed futures calls
future.result() which can raise and leaves the corresponding step marked
"running"; wrap the future.result() call in a try/except around the block that
updates state (the loop handling done futures) so that on any exception you
still set step_state = state["steps"][name], set step_state["finished_at"] =
_now(), set a sensible step_state["returncode"] (e.g. -1 or None), set
step_state["status"] = "failed", update state["updated_at"] = _now(), call
_dump_json(state_file, state) to persist the final state, then re-raise the
original exception; ensure this handling lives alongside the existing dry_run
and non-zero returncode branches in the same loop (referencing variables:
future, running, done, step, name, result, step_state, state_file, _dump_json,
_now).
---
Nitpick comments:
In `@tests/test_gdi_make_task.py`:
- Around line 57-58: The test uses shutil.copyfile to create graph.0.pb from
"graph.pb" and graph.1.pb from "beta.lmp", which is semantically confusing;
update tests/test_gdi_make_task.py to either (a) replace the second
shutil.copyfile call so it duplicates "graph.pb" into graph.1.pb but alters its
contents slightly (e.g., different bytes) to make a true distinct model file for
MD5 comparison, or (b) keep using "beta.lmp" but add a clear inline comment next
to the shutil.copyfile("beta.lmp", os.path.join(parent_dir, "graph.1.pb")) call
explaining that beta.lmp is intentionally used as a distinct placeholder for
testing per-phase copying and not a real DeepMD model.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26a8b6c5-5d28-4347-8dd7-e5033f7378d3
📒 Files selected for processing (10)
docs/index.rstdocs/workflow.mddpti/__init__.pydpti/gdi.pydpti/lib/utils.pydpti/main.pydpti/workflow.pytests/test_gdi_make_task.pytests/test_lib_utils.pytests/test_workflow.py
Comment on lines
+44
to
+46
| "name": "hti.gen", | ||
| "needs": ["nvt.run"], | ||
| "command": ["dpti", "hti", "gen", "hti.json", "-o", "hti", "-s", "one-step"], |
There was a problem hiding this comment.
Fix undefined dependency in the minimal schema example.
At Line 45, hti.gen uses "needs": ["nvt.run"], but nvt.run is not defined in that example JSON. This makes the “minimal schema” invalid as written.
Proposed fix (one option)
- "needs": ["nvt.run"],
+ "needs": ["nvt.gen"],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "name": "hti.gen", | |
| "needs": ["nvt.run"], | |
| "command": ["dpti", "hti", "gen", "hti.json", "-o", "hti", "-s", "one-step"], | |
| "name": "hti.gen", | |
| "needs": ["nvt.gen"], | |
| "command": ["dpti", "hti", "gen", "hti.json", "-o", "hti", "-s", "one-step"], |
🤖 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 `@docs/workflow.md` around lines 44 - 46, The example's task "hti.gen" declares
a dependency "needs": ["nvt.run"] that isn't defined in the minimal schema;
either remove the "needs" entry from the "hti.gen" object or add a corresponding
task named "nvt.run" earlier in the example so the dependency resolves (edit the
JSON where "hti.gen" and the other task definitions appear).
Comment on lines
+161
to
+169
| command = _normalize_command(step["command"]) | ||
| print(f"[workflow] run step: {name}") | ||
| print(f"[workflow] cwd: {work_dir}") | ||
| print("[workflow] command:", " ".join(shlex.quote(item) for item in command)) | ||
| if dry_run: | ||
| return {"returncode": 0} | ||
| work_dir.mkdir(parents=True, exist_ok=True) | ||
| completed = subprocess.run(command, cwd=str(work_dir)) | ||
| return {"returncode": completed.returncode} |
There was a problem hiding this comment.
Reject empty commands before spawning subprocesses.
At Line 161, an empty command list is possible (for example, command: ""), which currently fails later with a less actionable runtime error.
Proposed fix
command = _normalize_command(step["command"])
+ if not command:
+ raise ValueError(f"workflow step '{name}' has an empty command")
print(f"[workflow] run step: {name}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| command = _normalize_command(step["command"]) | |
| print(f"[workflow] run step: {name}") | |
| print(f"[workflow] cwd: {work_dir}") | |
| print("[workflow] command:", " ".join(shlex.quote(item) for item in command)) | |
| if dry_run: | |
| return {"returncode": 0} | |
| work_dir.mkdir(parents=True, exist_ok=True) | |
| completed = subprocess.run(command, cwd=str(work_dir)) | |
| return {"returncode": completed.returncode} | |
| command = _normalize_command(step["command"]) | |
| if not command: | |
| raise ValueError(f"workflow step '{name}' has an empty command") | |
| print(f"[workflow] run step: {name}") | |
| print(f"[workflow] cwd: {work_dir}") | |
| print("[workflow] command:", " ".join(shlex.quote(item) for item in command)) | |
| if dry_run: | |
| return {"returncode": 0} | |
| work_dir.mkdir(parents=True, exist_ok=True) | |
| completed = subprocess.run(command, cwd=str(work_dir)) | |
| return {"returncode": completed.returncode} |
🧰 Tools
🪛 Ruff (0.15.15)
[error] 168-168: subprocess call: check for execution of untrusted input
(S603)
🤖 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 `@dpti/workflow.py` around lines 161 - 169, The step command may be empty after
calling _normalize_command(step["command"]), so before printing/using it and
before subprocess.run, validate that the returned command list is non-empty; if
empty, raise a clear exception or return an error dict (e.g., {"returncode": 1,
"error": "empty command"}) so we reject empty commands early. Locate the
validation around the code that calls _normalize_command, uses work_dir and
subprocess.run (references: _normalize_command, step["command"], work_dir,
subprocess.run) and add the guard just after normalization and before the
dry_run check.
Comment on lines
+256
to
+275
| for future in done: | ||
| step = running.pop(future) | ||
| name = step["name"] | ||
| result = future.result() | ||
| step_state = state["steps"][name] | ||
| step_state["finished_at"] = _now() | ||
| step_state["returncode"] = result["returncode"] | ||
| if dry_run: | ||
| step_state["status"] = "dry_run" | ||
| state["updated_at"] = _now() | ||
| _dump_json(state_file, state) | ||
| continue | ||
| if result["returncode"] != 0: | ||
| step_state["status"] = "failed" | ||
| state["updated_at"] = _now() | ||
| _dump_json(state_file, state) | ||
| raise RuntimeError( | ||
| f"workflow step '{name}' failed with return code " | ||
| f"{result['returncode']}" | ||
| ) |
There was a problem hiding this comment.
Ensure step state is finalized when worker execution raises.
At Line 259, future.result() can raise (e.g., FileNotFoundError from subprocess.run). In that path, the step remains "running" in state, which breaks resumability and status reporting.
Proposed fix
for future in done:
step = running.pop(future)
name = step["name"]
- result = future.result()
step_state = state["steps"][name]
step_state["finished_at"] = _now()
+ try:
+ result = future.result()
+ except Exception as exc:
+ step_state["status"] = "failed"
+ step_state["error"] = repr(exc)
+ state["updated_at"] = _now()
+ _dump_json(state_file, state)
+ raise RuntimeError(
+ f"workflow step '{name}' crashed before returning a code"
+ ) from exc
step_state["returncode"] = result["returncode"]🤖 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 `@dpti/workflow.py` around lines 256 - 275, The loop that processes completed
futures calls future.result() which can raise and leaves the corresponding step
marked "running"; wrap the future.result() call in a try/except around the block
that updates state (the loop handling done futures) so that on any exception you
still set step_state = state["steps"][name], set step_state["finished_at"] =
_now(), set a sensible step_state["returncode"] (e.g. -1 or None), set
step_state["status"] = "failed", update state["updated_at"] = _now(), call
_dump_json(state_file, state) to persist the final state, then re-raise the
original exception; ensure this handling lives alongside the existing dry_run
and non-zero returncode branches in the same loop (referencing variables:
future, running, done, step, name, result, step_state, state_file, _dump_json,
_now).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary\n- fix Simpson integration-error estimation so fine and coarse quadratures cover the same endpoint\n- avoid a spurious large error when the final block has an even number of points\n- add a regression test for a 160-point linear integrand\n\n## Validation\n- Installed with:
conda run -n dpti python -m pip install -e .\n- Recomputedmelt_3200K/hti.refine: soft_on integration error changed from7.164e-02to8.126e-07eV/atom; total integration error changed to1.944e-06eV/atom\n-conda run -n dpti python -m pytest test_lib_utils.pyfromtests/: 12 passed\n- Fullpytestfromtests/: 110 passed, 1 pre-existing collection error intest_hti_ff_spring.py::test_spring_var_spring_multiple_elementbecause a module-level test function takesself\nSummary by CodeRabbit
Release Notes
New Features
dpti workflowCLI for orchestrating multi-step thermodynamic-integration campaigns with JSON-based configuration, concurrent execution, and state persistenceDocumentation
Improvements