docs(skills): add structured DPA4 workflow support - #5961
docs(skills): add structured DPA4 workflow support#5961SchrodingersCattt wants to merge 21 commits into
Conversation
for more information, see https://pre-commit.ci
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds DPA4/SeZM skills for training, fine-tuning, Python inference, and LAMMPS deployment. It documents ChangesDPA4/SeZM skill workflows
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Updated the description for the deepmd-finetune-dpa4 skill to simplify language and remove references to SeZM. Signed-off-by: Ming-Yu Guo <guomy26@mail2.sysu.edu.cn>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@skills/deepmd-finetune-dpa4/SKILL.md`:
- Around line 92-95: Update the LoRA example reference in the training
instructions to use ../../examples/water/dpa4/lora_ft.json, ensuring it resolves
from the skill directory to the repository-level example while preserving the
existing command guidance.
- Around line 27-36: Update skills/deepmd-finetune-dpa4/SKILL.md at lines 27-36,
49-63, and 104-115, skills/deepmd-train/models/dpa4.md at lines 73-84,
skills/deepmd-python-inference/references/model-artifacts.md at lines 63-70, and
skills/lammps-deepmd/references/model-deployment.md at lines 9-15 and 27-36 so
the selected multi-task branch is propagated through training and freezing
workflows, using --model-branch SELECTED_BRANCH for training and --head
SELECTED_BRANCH for freezing or export commands.
In `@skills/deepmd-python-inference/SKILL.md`:
- Around line 31-35: Restrict .pt2 guidance to inference commands and APIs
verified as supported by integration tests. In
skills/deepmd-python-inference/SKILL.md at lines 31-35, 58-60, and 292-293, and
references/model-artifacts.md at lines 17-22, 39-44, and 57-60, remove or revise
any universal .pt2 loading claims and update dp test or dp eval-desc examples to
supported frozen formats or explicit backend flags when .pt2 is unsupported.
Ensure all documented .pt2 paths are consistent with the installed-version
capabilities.
In `@skills/lammps-deepmd/SKILL.md`:
- Line 6: Update the online execution command in the compatibility metadata to
pin both LAMMPS and DeePMD-kit to the tested compatible versions, and apply the
same pins consistently to every command that invokes them.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f040ef9-3491-40d7-a8a4-f8142ef6b61f
📒 Files selected for processing (10)
doc/agent-skills.mdskills/deepmd-finetune-dpa3/SKILL.mdskills/deepmd-finetune-dpa4/SKILL.mdskills/deepmd-python-inference/SKILL.mdskills/deepmd-python-inference/references/model-artifacts.mdskills/deepmd-train/SKILL.mdskills/deepmd-train/models/dpa4.mdskills/lammps-deepmd/SKILL.mdskills/lammps-deepmd/references/commands-and-workflow.mdskills/lammps-deepmd/references/model-deployment.md
|
Addressed the review findings in four focused commits:
Validation: relative links and JSON examples checked, unsupported |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@skills/deepmd-python-inference/references/model-artifacts.md`:
- Around line 71-77: Update the `dp embed` example to pass `--head
SELECTED_BRANCH` when extracting embeddings from multi-task pretrained
checkpoints, preserving the intended fitting head; alternatively, explicitly
document that the example is single-head-only.
- Around line 54-57: Update the DeepPot.eval and dp test coverage statement in
model-artifacts.md to clarify that atomic virial is available only when the
archive was exported with --atomic-virial, rather than implying it is always
supported; keep the other covered outputs unchanged.
In `@skills/lammps-deepmd/SKILL.md`:
- Around line 48-52: Update the local command-line help instruction in SKILL.md
by removing the `tee /dev/tty` pipeline from the `lmp -h` command, so it writes
directly to standard output and works without a controlling terminal.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de465679-0ebf-4de7-a9c3-863538ed52fd
📒 Files selected for processing (6)
skills/deepmd-finetune-dpa4/SKILL.mdskills/deepmd-python-inference/SKILL.mdskills/deepmd-python-inference/references/model-artifacts.mdskills/deepmd-train/models/dpa4.mdskills/lammps-deepmd/SKILL.mdskills/lammps-deepmd/references/model-deployment.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/deepmd-train/models/dpa4.md
- skills/deepmd-python-inference/SKILL.md
|
Addressed the latest review comments in two focused commits:
Validation: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5961 +/- ##
==========================================
- Coverage 79.77% 79.50% -0.28%
==========================================
Files 1085 1085
Lines 127148 127148
Branches 4598 4592 -6
==========================================
- Hits 101428 101083 -345
- Misses 24067 24411 +344
- Partials 1653 1654 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Document how to obtain and verify compatible DPA4 checkpoints, decide between standard fine-tuning and LoRA, inspect active adapter state, distinguish merged best checkpoints, and avoid the incompatible bundled checkpoint/example pairing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@skills/deepmd-finetune-dpa4/SKILL.md`:
- Around line 104-111: Update the checker around model_dict, lora, and the
use_lora diagnostic to inspect each dictionary in multi-task branches for a lora
setting. Treat LoRA as enabled when either the top-level model.lora or any
branch-level lora is present, report that combined status, and reject multi-task
configurations whenever branch-level or top-level LoRA is detected.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c666cc7-3e45-49bc-9ca7-08963dd0c3b3
📒 Files selected for processing (1)
skills/deepmd-finetune-dpa4/SKILL.md
Remove smoke-model-specific discussion and detailed checkpoint inspection scripts, retaining concise general requirements for checkpoint selection and LoRA usage.
Signed-off-by: weiqichen77 <weiqchen@hku.hk>
e188dd4 to
f42fdde
Compare
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
The new skill set has a clear separation between scientific setup and platform orchestration, but the documented platform path can currently validate missing inputs, submit jobs without durable output collection or a wall-time limit, and accept empty logs as successful. The LoRA instructions can also discard an explicitly selected fitting head when the source checkpoint is multi-task. These are blocking workflow/data-retention issues; the inline comments include two direct suggestions and concrete exit-status reproductions.
Codex quota is about to reset, so I am using the remaining token budget to complete a concentrated review pass over the outstanding PRs.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Make required capability probes fail on a nonzero exit status
skills/matmaster-dpa4-workflows/scripts/check_environment.py currently treats executable discoverability as capability success. I reproduced this with a fake dp on PATH that exits 42: --probe --require-deepmd reports available: true, returncode: 42, and failures: [], then exits 0. A broken or incompatible DeePMD/LAMMPS/Bohrium runtime can therefore pass the validation gate and let automation continue packaging or submitting the job.
Please define required capability usability from the probe result when --probe is active (for example, discovered and returncode == 0) and apply that same predicate consistently to bohr, dp, and the lmp/lmp_mpi alternatives. A regression test with a discoverable executable returning nonzero would pin the contract.
OutisLi-Bot
left a comment
OutisLi-Bot
left a comment
There was a problem hiding this comment.
Review
Prior P1s on f42fddeb look fixed on bbe64e03. I re-checked the current head against the CLI and the three MatMaster scripts. No remaining documented-path blocker.
The DPA4 skills match the public PyTorch/SeZM workflow: inspect the stored descriptor (do not trust .pt), keep DPA3 on its own skill, freeze via dp --pt freeze to .pt2, keep dp embed on checkpoints, and fail closed on multi-rank .pt2 without has_comm_artifact / forward_lower_with_comm.pt2. Multi-task --model-branch / --head is now present on standard finetune, LoRA, freeze, and embed.
check_environment.py now fails on a missing model, case dir, or required file. audit_cases.py now rejects empty logs. The MatMaster skill example now passes --max-run-time and repeated --backward values.
Non-blocking
make_job_spec.pystill accepts a spec with no wall time andbackward_files=[]if--max-run-time/--backwardare omitted. The skill text is the agent contract and is now correct; requiring a positive--max-run-timeand at least one--backwardin the generator would close the remaining hole.- The three new scripts (
check_environment.py,audit_cases.py,make_job_spec.py) have no tests. - The top-level
skills/lammps-deepmd/SKILL.mdNVT example still usesgraph_compressed.pb, implicitpair_coeff * *, and noatom_modify map yes. DPA4-specific guidance inmodel-deployment.mdandexamples/water/dpa4/lmp/in.lammpsis correct; agents that copy the top-level NVT template for DPA4 will miss the atom map.
I did not re-run DPA4 train/freeze (CPU-only box). CLI help, relative links, git diff --check, and the script exit-status cases all passed.
Verdict: comment (approve-leaning). Prior CHANGES_REQUESTED can be considered addressed.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Keep required case files inside the packaged case directory
skills/matmaster-dpa4-workflows/scripts/check_environment.py documents --require-file as case-relative, but currently checks (case / name).is_file() without enforcing that boundary. I reproduced a successful exit with --case-dir parent/case --require-file ../model.pt when model.pt exists beside the case: the report marks both run.sh and ../model.pt present and leaves failures empty. An absolute path behaves the same way.
Bohrium packages only the directory passed as --input_directory, so this gate can validate a model/input that will not exist in the remote workspace. Please reject absolute paths and any resolved required path outside case_dir (including symlink escapes), retain normalized relative names in the report, and add regression coverage for .. and absolute-path inputs.
There was a problem hiding this comment.
The documented non-submitting validation command is not a verified Bohrium CLI interface. The current official bohr job submit documentation does not list either --dry-run or --output, and the available CLI reproduces this as Error: unknown flag: --dry-run; its submit help lists neither option. Therefore the workflow safety gate cannot run as written. Line 54 then tells the agent to remove --dry-run only for production, but there is no supported validation path before that production submission.
Please do not encode an unverified version-specific CLI contract. Use a version-verified, genuinely non-submitting validation interface if Bohrium provides one; otherwise keep the local job-spec/input checks as validation and leave the actual bohr job submit strictly behind explicit user authorization. If 2.5.17 really supports these flags, please add an authoritative reference or executable test that establishes that contract. Current CLI reference: https://bohrium-doc.dp.tech/docs/bohrctl/job/
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
audit_cases.py silently scans only the first 50,000,000 characters of each log. Any fatal signature after that boundary is ignored, so a failed long-running job can be reported as passes_file_audit with exit status 0.
I reproduced this with a 50,000,077-byte run.log in train mode whose final lines are Traceback (most recent call last): and RuntimeError: failed after long training. The script returned passes_file_audit, reasons: [], fatal_signatures: [], and exit code 0. The report warning about scientific acceptance does not make this safe, because this is a false result for the fatal-signature audit itself.
Please scan logs incrementally with bounded memory while accumulating fatal signatures, completion markers, and the last energy, rather than truncating the log. Reading only head plus tail would still miss failures in the middle. Please also add a regression test with a fatal signature beyond the current 50 MB boundary.
There was a problem hiding this comment.
audit_cases.py can return success while expected cases are completely absent. The earlier in.lmp reproduction is one symptom: infer_mode() recognizes in.lmp, but marker-based find_case_dirs() does not. More fundamentally, any submitted case whose download failed, whose directory is empty, or whose files use an unlisted name has no marker and therefore disappears from case_count instead of being reported as missing.
I reproduced the concrete false pass with one valid training case plus a second directory containing only in.lmp and no log. The report contained only the valid case, returned case_count: 1, passes_file_audit: 1, and exited 0. Adding in.lmp to the marker tuple would fix only this filename and leave absent or empty expected cases invisible.
The workflow already requires a manifest that records every case. Please make the completeness-sensitive audit manifest-driven, or accept an explicit repeatable expected-case list, and iterate those case directories even when they contain no recognized files. Marker discovery can remain an optional convenience mode, but it must not produce a successful completeness verdict without an expected inventory. Add regression coverage for an absent case directory, an empty case directory, and an in.lmp case without a log alongside a valid case; all must remain visible and make the audit fail.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
make_job_spec.py describes itself as generating and validating a Bohrium job specification, but it accepts any --result-path and writes it unchanged. The Bohrium bohr job submit contract limits automatic result destinations to /personal or /share: https://bohrium-doc.dp.tech/docs/bohrctl/job/
I reproduced this with --result-path /tmp/ephemeral-results; the script exited 0 and emitted that value into job.json. The generated specification therefore violates both the platform contract and this skill requirement that results use durable storage.
Please validate the normalized path before writing the specification. It must be /personal, /share, or a descendant of one of those directories. Use path-component containment rather than a raw string prefix so values such as /personal-old/run are rejected. Please cover valid durable paths plus /tmp/result, /personal-old/run, and a relative path in regression tests.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
audit_cases.py --require does not enforce its documented case-relative boundary. match_required() passes the user pattern directly to case.glob(), so .. and symlink traversal can satisfy a required output with a file outside the audited case.
I reproduced this with root/checkpoint.pt and root/case/run.log, where the case itself has no checkpoint. Running the audit with --mode train --require ../checkpoint.pt returned passes_file_audit, exit code 0, and recorded ../checkpoint.pt as the successful match.
This is the same containment invariant that check_environment.py --require-file needs. Please avoid two local patches: introduce one shared case-path containment helper used by both scripts. It should reject absolute paths and parent traversal, and verify every resolved match remains under the resolved case directory so symlink escapes are rejected as well. Please test ordinary exact paths, globs, .., absolute paths, and symlink escapes.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
The pinned online environment cannot provide the multi-rank DPA4 behavior documented by this PR. skills/lammps-deepmd/SKILL.md installs deepmd-kit==3.2.0b0, but the v3.2.0b0 PT freezer hard-codes has_comm_artifact: False and never embeds model/extra/forward_lower_with_comm.pt2: https://github.com/deepmodeling/deepmd-kit/blob/v3.2.0b0/deepmd/pt/entrypoints/freeze_pt2.py#L253-L264. The C++ runtime in that same tag rejects multi-rank message-passing inference when this artifact is absent. This directly contradicts model-deployment.md, which says the ordinary PT freeze carries the with-comm artifact and supports multi-rank LAMMPS out of the box.
Do not replace this with another fixed DeePMD-kit release. These DPA4/PT/PT-expt capabilities are evolving on upstream/master, so the skill should use the latest master source and record the resolved commit SHA in the execution/provenance report. Resolve a LAMMPS version compatible with that checkout rather than forcing DeePMD-kit back to the old beta pin. If the master bootstrap needs a separate installation step instead of the current one-line uvx command, document that explicitly; the recommended environment must actually implement every deployment capability claimed by the skill.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
The Agent responsibilities list in skills/lammps-deepmd/SKILL.md has lost its hierarchy. Under Confirm the minimum simulation inputs, only the structure/data file remains nested; the model artifact, type mapping, ensemble, and run parameters at lines 28-31 are top-level bullets, after which the numbered responsibility list resumes. The rendered document therefore splits one input contract across two unrelated list levels and reads like a patch residue.
Please indent all minimum-input bullets under the same numbered responsibility and make their punctuation/line wrapping consistent, so this section has one clear hierarchy for both human readers and agents.
7 participants
Summary
deepmd-finetune-dpa4skill while preservingdeepmd-finetune-dpa3deepmd-train.pt,.pt2,.pth, and.pbinference artifact routinglammps-deepmdDesign
The skills remain organized by user workflow rather than adding one umbrella DPA4 skill. Because DPA3 and DPA4 checkpoints both use
.pt, the fine-tuning skills require inspecting the stored descriptor instead of routing by filename. DPA4-specific guidance covers the public PyTorch/SeZM workflow, LoRA,.pt2export, Python inference, and LAMMPS deployment.Validation
git diff --checkch4-deepmdtemplate did not exposedp, while thedeepmd-kit-3.2.0-devsandbox requests timed out before creationThe documented DPA4 commands and behavior are based on this repository's
doc/model/dpa4.md, maintained examples, CLI implementation, and DPA4 tests.Summary by CodeRabbit
New Features
.pt2model workflows for Python inference, training, and LAMMPS deployment.Documentation