feat(skills): add dpgen-run skill for concurrent learning workflow - #1885
feat(skills): add dpgen-run skill for concurrent learning workflow#1885SchrodingersCattt wants to merge 9 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds DP-GEN workflow documentation for separate ChangesDP-GEN Run Skill
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1885 +/- ##
=======================================
Coverage 49.80% 49.81%
=======================================
Files 83 83
Lines 14986 14986
=======================================
+ Hits 7464 7465 +1
+ Misses 7522 7521 -1 ☔ 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
🤖 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/dpgen-run/SKILL.md`:
- Around line 325-357: The numbered checklist in SKILL.md after step 4 uses "1."
for four subsequent items; update those list markers to continue the sequence as
steps 5, 6, 7, and 8 (i.e., change the four "1." occurrences following "verify
`type_map.raw` content matches `param.json` `type_map` ordering" to "5.", "6.",
"7.", and "8.") so the validation steps read as a proper ordered sequence.
- Around line 104-116: Update the descriptor/backend guidance text to remove the
incorrect absolute claim that "dpgen (v0.13.x) only supports the TensorFlow
backend" and instead state that the codebase includes both train_backend values
"tensorflow" and "pytorch", so backend support may include PyTorch; soften the
PyTorch-only wording by either citing the authoritative source or rephrasing to
say users requiring PyTorch-only models or DPA-2/DPA-3 should prefer dpgen2;
keep the descriptor recommendations for se_e2_a, se_atten, and se_atten_v2
(spell se_atten_v2 exactly) and add a short note that users should verify
backend/descriptor compatibility against the repo's train_backend options
("tensorflow", "pytorch") if unsure.
- Around line 369-384: Update the guardrail text in SKILL.md so it no longer
requires specifically "[]" for training.training_data.systems; instead state
that training.training_data.systems should be left unset or empty (dpgen fills
it) — reference dpgen/generator/run.py behavior and the default_training_param
name when editing the rule text to mention both unset or empty as acceptable.
🪄 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: 5b559805-4192-4c79-acde-08145257e989
📒 Files selected for processing (1)
skills/dpgen-run/SKILL.md
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/dpgen-run/SKILL.md (1)
341-346:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the configured
init_data_prefixhere.The validation example hardcodes
init_data/, which conflicts with the earlierinit_data_prefixinput and will mislead users whose project layout uses a different prefix.🛠 Suggested fix
- ls init_data/*/type_map.raw + ls "${init_data_prefix}"*/type_map.raw🤖 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 `@skills/dpgen-run/SKILL.md` around lines 341 - 346, In the validation example in the SKILL.md file around lines 341-346, the bash command uses a hardcoded directory path `init_data/` instead of the configurable `init_data_prefix` value that was introduced earlier in the documentation. Replace the hardcoded `init_data/` reference in the ls command with the appropriate placeholder or variable reference that represents the configured `init_data_prefix`, so the example remains consistent with the actual configuration users provide and doesn't mislead them into using incorrect paths.
🤖 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.
Outside diff comments:
In `@skills/dpgen-run/SKILL.md`:
- Around line 341-346: In the validation example in the SKILL.md file around
lines 341-346, the bash command uses a hardcoded directory path `init_data/`
instead of the configurable `init_data_prefix` value that was introduced earlier
in the documentation. Replace the hardcoded `init_data/` reference in the ls
command with the appropriate placeholder or variable reference that represents
the configured `init_data_prefix`, so the example remains consistent with the
actual configuration users provide and doesn't mislead them into using incorrect
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8abfb50-acd6-4802-baac-90db57a1cd18
📒 Files selected for processing (1)
skills/dpgen-run/SKILL.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/dpgen-run/SKILL.md (1)
71-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit user confirmation requirement before auto-executing
dpgen run.The agent responsibilities include "validate the workflow before execution" and "provide the exact command for running," but lack an explicit requirement to ask the user for confirmation before actually executing the command. This creates an autonomous execution path without user verification—a concern flagged by the SkillSpector analyzer (EA2: Excessive Agency).
To mitigate this risk, either:
- Add a new responsibility step: "ask the user for explicit confirmation before executing the command," or
- Add a guardrail: "Never auto-execute
dpgen runwithout explicit user confirmation after validation."This ensures the agent validates thoroughly but defers the final execution decision to the user.
💡 Suggested additions
Add to agent responsibilities (after line 72):
1. validate the workflow before execution 1. provide the exact command for running +1. ask the user for explicit confirmation before executingOr add to guardrails section (after line 384):
- Do not assume outer-shell activation is inherited by stage jobs; for scheduler execution, require explicit `source_list` per stage. +- Never auto-execute `dpgen run param.json machine.json` without explicit user confirmation after validation passes.🤖 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 `@skills/dpgen-run/SKILL.md` around lines 71 - 72, The SKILL.md file lacks an explicit requirement for user confirmation before executing the dpgen run command, creating a potential autonomous execution risk. Add either a new responsibility item after line 72 that states "ask the user for explicit confirmation before executing the command," or add a guardrail in the guardrails section (after line 384) that states "Never auto-execute dpgen run without explicit user confirmation after validation." This ensures the agent validates the workflow and provides the command but defers the final execution decision to the user for explicit approval.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@skills/dpgen-run/SKILL.md`:
- Around line 71-72: The SKILL.md file lacks an explicit requirement for user
confirmation before executing the dpgen run command, creating a potential
autonomous execution risk. Add either a new responsibility item after line 72
that states "ask the user for explicit confirmation before executing the
command," or add a guardrail in the guardrails section (after line 384) that
states "Never auto-execute dpgen run without explicit user confirmation after
validation." This ensures the agent validates the workflow and provides the
command but defers the final execution decision to the user for explicit
approval.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b543243-4fa4-4894-b52d-054cefc40be9
📒 Files selected for processing (1)
skills/dpgen-run/SKILL.md
There was a problem hiding this comment.
Verdict: Changes requested. This draft skill references resources that are not included, does not require explicit user authorization before launching an expensive DP-GEN/HPC workflow, and contains two hard requirements that do not match current dpdispatcher behavior. CI passes, but it does not validate relative resource links or the instructions against the real schema.
Note: The Codex quota is about to reset, so I am using the remaining tokens to review all open PRs in this repository.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| 1. generate or patch `machine.json` | ||
| 1. explain important parameters in plain language when asked | ||
| 1. validate the workflow before execution | ||
| 1. provide the exact command for running |
There was a problem hiding this comment.
[Blocking] This skill covers both preparation/explanation and execution, while dpgen run can submit substantial HPC and first-principles workloads. Please state explicitly that the agent may execute dpgen run only when the user has requested execution and confirmed the validated command. Requiring validation and providing the command does not by itself prevent autonomous execution.
|
|
||
| Use these bundled files: | ||
|
|
||
| - `assets/param.template.json` |
There was a problem hiding this comment.
[Blocking] This PR changes only SKILL.md, but the six assets/* files and four references/* files referenced here and above are not included. Following the skill will fail as soon as it attempts to read a template. Add all referenced resources, or point to real repository examples/docs, and add a check that every repository-relative link in the skill exists.
| - Keep `type_map` ordering consistent with `init_data_sys` type_map.raw files. | ||
| - If required inputs are missing, stop and ask instead of guessing. | ||
| - Always spell `se_atten_v2` correctly (not `se_attn_v2`). | ||
| - Always include `remote_root` in machine config — it is mandatory even for local execution. |
There was a problem hiding this comment.
This absolute requirement is inaccurate. remote_root is optional in the dpdispatcher schema, and whether it is needed depends on the context; some lazy/local execution paths can omit it. Please describe the requirement by context_type so the skill does not rewrite an existing working local configuration.
| - If required inputs are missing, stop and ask instead of guessing. | ||
| - Always spell `se_atten_v2` correctly (not `se_attn_v2`). | ||
| - Always include `remote_root` in machine config — it is mandatory even for local execution. | ||
| - Always capitalize `batch_type` values: `"Shell"`, `"Slurm"`, `"PBS"`. |
There was a problem hiding this comment.
This casing rule is also inaccurate. dpdispatcher registers canonical names and lowercase aliases, and repository configurations/tests use "shell". Please advise users to use a value accepted by the installed dpdispatcher version and preserve an existing working value instead of claiming lowercase values always fail.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/dpgen-run/SKILL.md`:
- Line 269: Update the machine-example links near the existing local example
references so they point to examples matching the schema produced by
convert_mdata, or document the required version-specific mapping for legacy
flattened fields. Ensure nested train, model_devi, and fp blocks are represented
and validate both linked examples with the current parser.
In `@tests/test_dpgen_run_skill.py`:
- Around line 16-18: Update the link validation around target_path to resolve
each target and reject it unless it remains inside repository_root, before
checking whether it exists. Preserve the existing missing-target collection
behavior for paths within the repository, including fragment-stripped targets.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 573c066c-6d3e-4621-bb80-4e9a704fdb4e
📒 Files selected for processing (2)
skills/dpgen-run/SKILL.mdtests/test_dpgen_run_skill.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/test_dpgen_run_skill.py`:
- Around line 35-48: Update test_linked_machine_examples_use_current_schema to
derive its machine JSON targets by extracting the documented
examples/machine/*.json links from skills/dpgen-run/SKILL.md, then validate
those resolved paths instead of maintaining the hard-coded DeePMD-kit entries.
Reuse the existing link-parsing approach in the test module if available.
- Around line 50-60: Strengthen the test around convert_mdata by validating each
input stage’s machine and resources are non-empty dictionaries and command is a
non-empty string before conversion, including assertions for the required
machine/resource fields. Retain the existing converted-key checks, and add
assertions that converted stage values preserve the expected nested types and
non-empty command.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b3443ba-c98f-4054-b828-b7253907815d
📒 Files selected for processing (2)
skills/dpgen-run/SKILL.mdtests/test_dpgen_run_skill.py
2 participants
Summary by CodeRabbit
Documentation
param.jsonandmachine.jsonconfiguration, setup requirements, runtime environments, validation steps, expected outputs, and configuration guidance.Tests