docs(skills): add structured DPA4 workflow support - #5961
docs(skills): add structured DPA4 workflow support#5961SchrodingersCattt wants to merge 30 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.10% 78.84% -0.26%
==========================================
Files 1105 1105
Lines 130981 130981
Branches 4771 4765 -6
==========================================
- Hits 103609 103270 -339
- Misses 25686 26026 +340
+ Partials 1686 1685 -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
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.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Handle valid datasets without type_map.raw instead of assuming it exists
The held-out admission rule states that type.raw is always indexed into the system type_map.raw, but type_map.raw is optional in the DeePMD system format. When it is absent, DeePMD interprets type.raw directly in the model type-map order. The current rule therefore either rejects valid systems or, worse, encourages an agent to invent an element-identity mapping that the dataset does not contain; a wrong mapping can produce scientifically invalid held-out metrics while dp test still completes.
Please express the two actual contracts explicitly: when type_map.raw exists, compare/remap by element identity; when it is absent, require provenance that the dataset indices already use the candidate model type-map order (and fail closed if that cannot be established). This should remain an admission invariant rather than being inferred from filenames or index ranges.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Preserve the selected head when evaluating a multi-task checkpoint
The complete held-out command always invokes dp --pt test on selected.pt without --head. A native multi-task checkpoint cannot generally be evaluated this way: DeepEval asserts that a head must be selected unless the checkpoint happens to define a Default alias. The reference is linked as the production-admission procedure, and DPA4 training in this same skill set explicitly supports multi-task models, so the documented path fails before evaluating any frame for a valid multi-task candidate.
Please make the artifact contract explicit here: inspect model-branch for a multi-task checkpoint and pass --head SELECTED_BRANCH to every per-system dp test invocation. A frozen .pt2 produced with --head is already single-head and should not receive another branch argument.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Keep the reusable LAMMPS asset consistent with the DPA4-safe example
The updated top-level skill fixes its NVT example with atom_modify map yes, an explicit pair_coeff element mapping, and matching dump element labels, but it still points readers to assets/input.nvt.lammps as the reusable template. That asset remains on the old contract: no atom map, pair_coeff * * with implicit ordering, and no element labels. An agent following the referenced asset for DPA4 can therefore fail the required atom-map check or silently use an unaudited type mapping, even though the prose example is correct.
Please make the skill expose one canonical template rather than two divergent copies. At minimum, update the asset to the same safe mapping contract and cover the asset in the consistency test; preferably generate or verify both rendered examples from one source so future edits cannot leave this patch-style split again.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Make the advertised minimal DPA4 model section pass argcheck
The new DPA4 training reference labels its JSON as a minimal model configuration but omits model.fitting_net and then says model.type=dpa4 selects a default energy fitting network. That is not the current schema contract: the dpa4 model variant gives fitting_net.type a default only after a fitting_net object exists, while fitting_net itself is required. Normalizing the shown input fails with ArgumentKeyError: at model, key fitting_net is required. An agent copying the advertised minimal block therefore cannot start training.
Please either include an explicit fitting_net object in the minimal configuration or change the schema deliberately if omission is intended. Add a test that parses the actual fenced JSON and runs argcheck normalization; substring assertions cannot catch an invalid example.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Create the detail-output directory before the documented test command
The complete held-out example writes -d details/system.000 but never creates details/. dp test does not create parent directories: save_txt_file opens the derived .e.out/.f.out paths directly with Path.open. In a clean evaluation workspace, the production-admission command therefore evaluates the model and then fails with FileNotFoundError while trying to retain the required evidence.
Please make directory creation an explicit step before the per-system commands (and keep the existing no-overwrite check). A focused workflow test should execute the documented path from a clean temporary directory rather than only asserting that -d text is present.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Do not claim that use-pretrain-script restores the complete DPA4 model configuration
The fine-tuning skill recommends --use-pretrain-script when the architecture is unknown and says it inherits the stored model configuration except type_map. The current fine-tune builder does not do that for DPA4: _apply_pretrained_model_params copies only descriptor and fitting_net. Model-level architecture/semantics such as type, spin, bridging_method, bridging_r_inner/outer, and related specialized settings remain whatever input.json supplied. I reproduced this by applying the builder to a native-spin, ZBL-bridged pretrained config and a plain DPA4 target; the returned config contained only the copied descriptor/fitting and silently dropped spin and bridging.
This is unsafe guidance for exactly the unknown-architecture case: training may fail during state loading or proceed with a different physical model. Please either make the core option actually preserve the complete model contract (with type_map and intentional trainability/LoRA exceptions defined centrally), or narrow the skill and require every DPA4 model-level field to be inspected and reproduced explicitly. A bounded initial run is not a substitute for constructing the right model.
OutisLi
left a comment
OutisLi
left a comment
There was a problem hiding this comment.
[P1] Make the inference precision/performance policy explicit before freezing DPA4
Every DPA4 freeze recipe runs dp --pt freeze without inspecting or recording DP_TRITON_INFER, DP_TF32_INFER, and DP_AMP_INFER. These are not ordinary runtime knobs for .pt2: the freezer reads them while constructing/tracing the model and bakes the selected kernels and precision policy into the AOTInductor archive. Existing shell exports can therefore silently change force smoothness and numerical behavior, while the default DP_TRITON_INFER=0 also leaves the production artifact on the unfused slow path. Moving freeze/run to the same node does not establish a reproducible policy by itself; at level >=2 it only makes the hardware-specific tuning relevant.
Please add one explicit pre-freeze decision shared by the training, fine-tuning, inference, and LAMMPS routes: inspect/reset these variables, choose a validated policy (levels 1/2 retain fp32 accumulation; level 3 and AMP/TF32 require task-specific validation), and record the values with the artifact provenance. The command examples should not inherit an unknown shell environment, and the production workflow should not silently bake the slow default.
|
Addressed the latest review in fe8168c and dd3fb6a (plus pre-commit auto-fix 3a871cd): optional type_map.raw is provenance-gated; native multi-task held-out tests require the selected --head while frozen pt2 stays single-head; the LAMMPS asset now matches the documented mapping/dump contract; the minimal DPA4 JSON includes fitting_net and has an argcheck normalization test; detail directories are created collision-free and exercised from a clean temporary directory; --use-pretrain-script is limited to descriptor/fitting_net replacement; and DPA4 freeze-time Triton/TF32/AMP policy is explicit. Focused local result after the auto-fix: 13 passed, 1 skipped only because this source checkout has no built deepmd.lib; the normalization test executes where the extension is built. Full CI is still running. |
…nto docs/add-deepmd-dpa4-skill
for more information, see https://pre-commit.ci
|
Added DPA4C training guidance in
Validation: |
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. Checkpoint routing is based on stored model metadata instead of filenames. DPA4/SeZM guidance covers PyTorch training and fine-tuning, LoRA, held-out evaluation,
.pt2export, Python inference, and LAMMPS deployment. DPA4C guidance covers the public PyTorch Exportable training, freeze, compression, export-time inference policy, and LAMMPS deployment contracts.The changed skills contain no MatMaster, Bohrium, private image, private library path, or platform-specific helper dependency.
Validation
python -m pytest -q -p no:cacheprovider source/tests/common/test_agent_skills.py: 15 passed, 1 skipped; the skipped normalization test requires a built DeePMD checkoutpython -m black --check source/tests/common/test_agent_skills.pygit diff --checkdoc/model/dpa4c.md, reject the invalid--pt_exptspelling, and preserve the same-node export contract