feat(train): support opt-in $ref loading#5264
feat(train): support opt-in $ref loading#5264njzjz-bot wants to merge 7 commits intodeepmodeling:masterfrom
Conversation
- add --allow-ref to dp train\n- thread allow_ref through PT/PD train entrypoints and argcheck\n- keep disabled by default for security\n- bump minimum dargs to >=0.5.0\n- document usage in README\n\nAuthored by OpenClaw (model: gpt-5.3-codex)
|
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:
📝 WalkthroughWalkthroughA new CLI flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
- document --allow-ref in training docs\n- add allow_ref docstrings in normalize/train APIs\n\nAuthored by OpenClaw (model: gpt-5.3-codex)
njzjz-bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deepmd/pt/entrypoints/main.py (1)
268-338:⚠️ Potential issue | 🟡 MinorRun
ruff check .andruff format .before commit (CI requirement).Per repo guidelines for Python changes, please ensure these are run to avoid CI failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/pt/entrypoints/main.py` around lines 268 - 338, The CI failure is due to missing ruff linting/formatting; run ruff check . to see reported issues and ruff format . to apply fixes before committing changes in the train function (deepmd/pt/entrypoints/main.py) and surrounding code; ensure you re-run ruff check . and rerun tests, then amend the commit so the pull request passes CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepmd/utils/argcheck.py`:
- Around line 3824-3840: The CI requires running the repo's linter/formatter:
before committing changes to deepmd/utils/argcheck.py (e.g., edits near the
normalize(...) function), run "ruff check ." to catch lint errors and then "ruff
format ." (or the configured formatter) to apply formatting fixes; re-run the
checks until they pass and then update the commit so CI will succeed.
---
Outside diff comments:
In `@deepmd/pt/entrypoints/main.py`:
- Around line 268-338: The CI failure is due to missing ruff linting/formatting;
run ruff check . to see reported issues and ruff format . to apply fixes before
committing changes in the train function (deepmd/pt/entrypoints/main.py) and
surrounding code; ensure you re-run ruff check . and rerun tests, then amend the
commit so the pull request passes CI.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deepmd/pd/entrypoints/main.pydeepmd/pt/entrypoints/main.pydeepmd/utils/argcheck.pydoc/train/training-advanced.md
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/pd/entrypoints/main.py
There was a problem hiding this comment.
Pull request overview
Adds an opt-in switch to allow dargs $ref expansion during training input normalization/validation, keeping the default behavior secure-by-default (disabled).
Changes:
- Add
dp train --allow-refflag and thread it into PT/PD training normalization. - Extend
deepmd.utils.argcheck.normalize(..., allow_ref=False)and bumpdargsminimum to>=0.5.0. - Document the new option in training docs and README.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Bumps runtime dependency to dargs >= 0.5.0 to support $ref handling. |
| doc/train/training-advanced.md | Documents --allow-ref in advanced training options. |
| deepmd/utils/argcheck.py | Adds allow_ref parameter and forwards it to dargs normalization/validation. |
| deepmd/pt/entrypoints/main.py | Plumbs allow_ref through PyTorch training entrypoint to normalize(...). |
| deepmd/pd/entrypoints/main.py | Plumbs allow_ref through Paddle training entrypoint to normalize(...). |
| deepmd/main.py | Adds --allow-ref to the dp train CLI parser. |
| README.md | Documents $ref support and the opt-in mechanism. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- verify opt-in flag is parsed\n- verify default remains disabled\n\nAuthored by OpenClaw (model: gpt-5.3-codex)
|
Follow-up updates pushed:\n- added parser test coverage for and default-off behavior ()\n- kept implementation backend-adapted: PT/PD train entrypoints consume the new flag, TF path remains unchanged by design\n\nI also removed README-level feature note and kept docs in as requested. |
|
Follow-up (clean summary): addressed backend-adaptation and tests.
Local syntax check passed ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/tests/common/test_argument_parser.py (1)
290-296: Consider using the existingrun_testhelper for consistency.All other train-flag tests (e.g.
test_parser_train_restart,test_parser_train_finetune) use therun_test/TEST_DICTpattern that also exercises default-value paths and attribute-type checks. Expressing--allow-refthrough the same pattern keeps the suite uniform and would also validate that the flag co-exists with--restart,--init-model, etc.♻️ Optional refactor using the existing helper
- def test_parser_train_allow_ref(self) -> None: - """Test train --allow-ref option.""" - args = parse_args(["train", "INFILE", "--allow-ref"]) - self.assertTrue(args.allow_ref) - - args_default = parse_args(["train", "INFILE"]) - self.assertFalse(args_default.allow_ref) + def test_parser_train_allow_ref(self) -> None: + """Test train --allow-ref option.""" + ARGS = { + "INPUT": {"type": str, "value": "INFILE"}, + "--allow-ref": {"type": bool}, + } + self.run_test(command="train", mapping=ARGS) + + # Also confirm the explicit True case + args = parse_args(["train", "INFILE", "--allow-ref"]) + self.assertTrue(args.allow_ref) + + args_default = parse_args(["train", "INFILE"]) + self.assertFalse(args_default.allow_ref)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/tests/common/test_argument_parser.py` around lines 290 - 296, Replace the two direct parse_args calls in test_parser_train_allow_ref with the existing run_test/TEST_DICT pattern so the test uses run_test to validate both the flag set and default behavior and to exercise interactions with other train flags; specifically, update test_parser_train_allow_ref to invoke run_test (using TEST_DICT and the "train" command) to assert allow_ref True when "--allow-ref" is present and False by default, ensuring it goes through the same validation and attribute-type checks as other tests like test_parser_train_restart and test_parser_train_finetune rather than calling parse_args directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/tests/common/test_argument_parser.py`:
- Around line 290-296: Replace the two direct parse_args calls in
test_parser_train_allow_ref with the existing run_test/TEST_DICT pattern so the
test uses run_test to validate both the flag set and default behavior and to
exercise interactions with other train flags; specifically, update
test_parser_train_allow_ref to invoke run_test (using TEST_DICT and the "train"
command) to assert allow_ref True when "--allow-ref" is present and False by
default, ensuring it goes through the same validation and attribute-type checks
as other tests like test_parser_train_restart and test_parser_train_finetune
rather than calling parse_args directly.
- ensure --allow-ref works across backends\n- wire allow_ref into TF train normalize path\n- improve docstrings and training docs\n- add parser test for --allow-ref\n\nAuthored by OpenClaw (model: gpt-5.3-codex)
|
Addressed backend-adaptation/self-check items in latest commits:\n\n- PT/PD path: keep threaded to normalize\n- TF path: now also accepts/threads ()\n- CLI is now backend-consistent\n- parser coverage includes check in \n- removed README edits; docs stay in \n\nI also ran static compile checks locally for touched files. |
Keep documentation updates in doc/ only.
Recover accidentally replaced train option while keeping new opt-in ref flag.\n\nAuthored by OpenClaw (model: gpt-5.3-codex)
…heck Restore accidentally replaced parser negative test and keep new allow-ref assertions.\n\nAuthored by OpenClaw (model: gpt-5.3-codex)
njzjz-bot
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5264 +/- ##
=======================================
Coverage 82.00% 82.00%
=======================================
Files 750 750
Lines 75082 75082
Branches 3615 3615
=======================================
+ Hits 61571 61574 +3
+ Misses 12347 12346 -1
+ Partials 1164 1162 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Enable dargs
$refloading for training-input validation with explicit opt-in.What changed
--allow-reftodp trainallow_refthrough PT/PD train entrypointsdeepmd.utils.argcheck.normalize(..., allow_ref=False)dargs>=0.5.0doc/train/training-advanced.mdSecurity
$refloading remains disabled by default and must be explicitly enabled.Authored by OpenClaw (model: gpt-5.3-codex)
Summary by CodeRabbit
Release Notes
New Features
--allow-refCLI flag to the training command, enabling loading of external JSON/YAML configuration snippets via references (disabled by default for security)Tests
--allow-refflag parsing and default behaviorDocumentation
--allow-refoptionChores