fix: handle empty iter_data_new_exp in auto_prob generation - #368
fix: handle empty iter_data_new_exp in auto_prob generation#368SchrodingersCattt wants to merge 5 commits into
Conversation
When FP labeling fails on all conformations and continue_on_success_ratio allows the workflow to proceed, iter_data contains directory entries that expand to zero systems. This caused auto_prob to generate an empty range like "prob_sys_size; 0:2:0.6; 2:2:0.4" which crashes dp train with "ValueError: probabilities do not sum to 1". Fix: guard with `if numb_new > numb_old` before generating the two-group auto_prob string. When there is no new data, fall back to plain "prob_sys_size" and log a warning. Fixes the scenario: extreme systems (e.g., energetic materials at high temperature) where teacher model cannot label any conformations from the first exploration iteration.
dosubot
Bot
added
size:S
for more information, see https://pre-commit.ci
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesAuto probability fallback
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 |
![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
🤖 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/op/test_run_dp_train.py`:
- Around line 349-355: Update the test around RunDPTrain.execute to exercise the
production fallback rather than recomputing auto_prob locally. Mock run_command,
invoke execute with iter_data=[empty_iter], then inspect the generated training
script and assert its auto_prob value is "prob_sys_size"; remove the duplicate
numb_new/numb_old conditional and assertion based on it.
🪄 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: 0e6873f7-f44c-48b1-a2b0-d1a2d779e58a
📒 Files selected for processing (2)
dpgen2/op/run_dp_train.pytests/op/test_run_dp_train.py
Address review: replace local logic re-computation with an actual call to RunDPTrain.execute() using mocked run_command, then inspect the generated training script's auto_prob value. This ensures the test fails if the production guard is removed.
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Functional finding on the current head.
| numb_old = len_init + len(iter_data_old_exp) | ||
| numb_new = numb_old + len(iter_data_new_exp) | ||
| auto_prob_str = f"prob_sys_size; 0:{numb_old}:{old_ratio}; {numb_old}:{numb_new}:{1.-old_ratio:g}" | ||
| if numb_new > numb_old: |
There was a problem hiding this comment.
P2 — Handle the symmetric empty-old-data case. If a workflow starts from supplied init_models with no init_data, the first labeled iteration can make numb_old == 0 < numb_new. This branch then emits prob_sys_size; 0:0:0.6; 0:N:0.4; DeePMD assigns only the 0.4 block, so the probabilities still do not sum to 1. Require both ranges to be nonempty before using the two-block form, otherwise fall back to prob_sys_size; please also generalize the warning and add an empty-old regression test.
| if numb_new > numb_old: | |
| if numb_old > 0 and numb_new > numb_old: |
Codex quota is about to reset, so I am using the remaining token budget to review this PR now.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| try: | ||
| op.execute(ip) | ||
| except Exception: | ||
| # May fail on freeze/post-process; we only care about | ||
| # the generated training script at this point. | ||
| pass |
There was a problem hiding this comment.
P3 — Do not swallow unexpected failures in this regression test. Both external commands are mocked as successful, so an exception from execute() is a test failure. Catching every exception allows the test to pass when execution breaks after writing the input file and weakens the claimed end-to-end coverage.
| try: | |
| op.execute(ip) | |
| except Exception: | |
| # May fail on freeze/post-process; we only care about | |
| # the generated training script at this point. | |
| pass | |
| op.execute(ip) |
Codex quota is about to reset, so I am using the remaining token budget to review this PR now.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
The empty-new-data fallback works, but the symmetric empty-old-data case still produces an invalid two-range probability expression. I also left a non-blocking inline suggestion to keep the regression test from swallowing unexpected execution failures.
Codex quota is about to reset, so I am using the remaining token budget to review this PR now.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
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 `@tests/op/test_run_dp_train.py`:
- Around line 328-376: Make the directory cleanup in this test failure-safe by
registering self.addCleanup for empty_iter_data, task_path, and the generated
task-auto-prob directory immediately after they are created or otherwise
wrapping the test body in try/finally. Remove reliance on the final
shutil.rmtree block, following the cleanup pattern used by
test_auto_prob_empty_old_data.
🪄 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: 13fecdf8-3f54-485a-9679-f6ed0832c4c5
📒 Files selected for processing (2)
dpgen2/op/run_dp_train.pytests/op/test_run_dp_train.py
🚧 Files skipped from review as they are similar to previous changes (1)
- dpgen2/op/run_dp_train.py
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Code review
Approving. The fix is correct and the tests genuinely prove it.
The production hunk is sound. The counts line up exactly with the systems list write_data_to_input_script emits (init_data + iter_data_old_exp + iter_data_new_exp), so the old block really does occupy [0, numb_old). And in both degenerate cases plain prob_sys_size is the mathematical limit of the intended distribution rather than a different policy — with an empty block there is no old/new mixture left to weight, so no configured init_model_old_ratio is being silently discarded. logging.warning is the right severity: an FP iteration yielding nothing is a legitimate, if suspicious, workflow state, and the training OP should not kill the run over it.
I verified the tests fail pre-fix, rather than reading the diff and assuming. Reverting only the source line in a scratch worktree, with your tests kept verbatim:
FAIL: test_auto_prob_empty_new_iter_data
AssertionError: 'prob_sys_size; 0:2:0.6; 2:2:0.4' != 'prob_sys_size'
FAIL: test_auto_prob_empty_old_data
AssertionError: 'prob_sys_size; 0:0:0.6; 0:2:0.4' != 'prob_sys_size'
Ran 27 tests ... FAILED (failures=2)
Exactly the two malformed strings, no collateral failures, all 27 green at HEAD. Neither test is vacuous — the assertion is a literal string round-tripped through JSON from the input.json that execute() itself wrote, sharing no guard with the code under test, and only run_command is patched so the real code path runs. I also checked test_auto_prob_empty_old_data against the intermediate commit 435b9bb: it still fails there, which confirms the two tests cover genuinely distinct defects and that the second one is what forced c689e99.
Why this survived three years, for the record: the untested cell was init_model_policy="yes" crossed with an iter_data entry expanding to zero systems. TestRunDPTrainNullIterData — the class commit 09e7e2e (#61) added for exactly the empty-dir case — pins init_model_policy="no" in all three of its tests, so decide_init_model returns False and this expression is never evaluated; its test_exec_v2_empty_dir asserts auto_prob == "prob_sys_size" and gets the right answer for the wrong reason. Meanwhile TestRunDPTrain, the only class with the policy on, hardcodes fully populated data (numb_old=4, numb_new=7). The bug lived precisely at the crossing of the two classes' setups. Root cause goes back to 1ca9aaa, which hard-coded numb_old:numb_old+1 and so assumed the last iteration contributes exactly one system; #61 falsified that invariant five months later and nobody revisited the trainer.
Two things to be aware of, neither blocking:
-
See the inline note on the doubly-empty case.
-
The unit tests never actually ran on this PR.
gh pr checksshows 3 passing checks; merged #343 had 10.Python unit-testsandType checkerare bothconclusion=action_requiredon4ef4258— the outside-contributor workflow gate. Someone needs to approve the run before merge; the green checkmarks currently say nothing about the suite. (Also,njzjz-bot'sCHANGES_REQUESTEDfrom 435b9bb is stale — both its points were applied in c689e99 and 4ef4258 — and may need dismissing if branch protection requires it.)
Minor: the diff carries some unrelated f-string reformatting ({1.-old_ratio:g} -> {1.0 - old_ratio:g}, and {self.old_data_size-1} in the test). The pinned ruff-format v0.1.3 in .pre-commit-config.yaml does not touch f-string internals, so these came from a newer local formatter and CI will not normalize them either way. Harmless, just diff noise.
| "Cannot build two non-empty auto_prob ranges " | ||
| "(numb_old=%d, numb_new=%d). " | ||
| "Falling back to auto_prob='prob_sys_size'. " | ||
| "Training will proceed with all available data.", |
There was a problem hiding this comment.
Non-blocking follow-up: there is a third degenerate case this branch catches but does not really handle — numb_old == numb_new == 0.
With init_data: [] in the config (a finetune-style run: pretrained init_model, no initial data) plus an iteration that expands to zero systems, we land here with no data at all. auto_prob becomes "prob_sys_size", but training_data.systems is [] and dp train is still launched. I reproduced it end-to-end against this HEAD:
WARNING Cannot build two non-empty auto_prob ranges (numb_old=0, numb_new=0)...
training_data: {"systems": [], "batch_size": "auto", "auto_prob": "prob_sys_size"}
command: ['dp', 'train', '--init-frz-model', 'bar.pb', 'input.json']
Before this PR that state produced prob_sys_size; 0:0:0.9; 0:0:0.1 and died fast with the clear "probabilities do not sum to 1". Now it gets further and fails deeper inside deepmd-kit with a murkier message. Note skip_training cannot rescue it either — it tests len(iter_data) == 0 on the unexpanded list, which from iteration 1 onward is never true.
Related, and the reason I am flagging it on this line: the message text is false in that case. "Training will proceed with all available data" is printed when there is no data. Something like "... proceed with all available data (numb_new=0: nothing to train on)", or an explicit early bail when numb_new == 0, would read correctly in all three cases.
Happy for this to be a separate PR — it does not affect the crash you are fixing here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #368 +/- ##
==========================================
+ Coverage 84.17% 84.20% +0.03%
==========================================
Files 104 104
Lines 6111 6110 -1
==========================================
+ Hits 5144 5145 +1
+ Misses 967 965 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Problem
When FP labeling fails on all conformations and
continue_on_success_ratioallows the workflow to proceed,iter_datacontains directory entries that expand to zero systems.This causes
auto_probto generateprob_sys_size; 0:2:0.6; 2:2:0.4(empty range2:2) which crashes withValueError: probabilities do not sum to 1.Fix
Guard with
if numb_new > numb_old+ fallback to plainprob_sys_size+ warning log.Test
Added
test_auto_prob_empty_new_iter_data.Summary by CodeRabbit
Bug Fixes
Tests