-
Notifications
You must be signed in to change notification settings - Fork 36
fix: handle empty iter_data_new_exp in auto_prob generation #368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -315,6 +315,71 @@ def test_decide_init_model_config_larger_than_yes(self): | |||||||||||||||
| ) | ||||||||||||||||
| self.assertTrue(do_init_model) | ||||||||||||||||
|
|
||||||||||||||||
| def test_auto_prob_empty_new_iter_data(self): | ||||||||||||||||
| """Test that auto_prob falls back to 'prob_sys_size' when | ||||||||||||||||
| iter_data_new_exp is empty (e.g., FP produced no labeled data). | ||||||||||||||||
|
|
||||||||||||||||
| Previously this would generate "prob_sys_size; 0:2:0.6; 2:2:0.4" | ||||||||||||||||
| which crashes dp train with "probabilities do not sum to 1". | ||||||||||||||||
|
|
||||||||||||||||
| This test exercises the real RunDPTrain.execute() code path with | ||||||||||||||||
| a mocked run_command to verify the generated training script. | ||||||||||||||||
| """ | ||||||||||||||||
| # Create an empty directory to simulate iter_data with no systems | ||||||||||||||||
| empty_iter = Path("empty_iter_data") | ||||||||||||||||
| empty_iter.mkdir(exist_ok=True) | ||||||||||||||||
|
|
||||||||||||||||
| # Create a task_path with input.json | ||||||||||||||||
| task_path = Path("input-auto-prob-test") | ||||||||||||||||
| task_path.mkdir(exist_ok=True) | ||||||||||||||||
| with open(task_path / train_script_name, "w") as fp: | ||||||||||||||||
| json.dump(self.idict_v2, fp, indent=4) | ||||||||||||||||
|
|
||||||||||||||||
| config = self.config.copy() | ||||||||||||||||
| config["init_model_policy"] = "yes" | ||||||||||||||||
| config["init_model_old_ratio"] = 0.6 | ||||||||||||||||
|
|
||||||||||||||||
| ip = OPIO( | ||||||||||||||||
| { | ||||||||||||||||
| "config": config, | ||||||||||||||||
| "task_name": "task-auto-prob", | ||||||||||||||||
| "task_path": task_path, | ||||||||||||||||
| "init_model": self.init_model, | ||||||||||||||||
| "init_data": self.init_data, | ||||||||||||||||
| "iter_data": [empty_iter], | ||||||||||||||||
| "valid_data": None, | ||||||||||||||||
| "optional_files": None, | ||||||||||||||||
| "optional_parameter": { | ||||||||||||||||
| "mixed_type": False, | ||||||||||||||||
| "finetune_mode": "no", | ||||||||||||||||
| }, | ||||||||||||||||
| } | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| op = RunDPTrain() | ||||||||||||||||
| # Mock run_command so dp train is not actually invoked | ||||||||||||||||
| with patch("dpgen2.op.run_dp_train.run_command", return_value=(0, "", "")): | ||||||||||||||||
| try: | ||||||||||||||||
| op.execute(ip) | ||||||||||||||||
| except Exception: | ||||||||||||||||
| # May fail on freeze/post-process; we only care about | ||||||||||||||||
| # the generated training script at this point. | ||||||||||||||||
| pass | ||||||||||||||||
|
Comment on lines
+362
to
+367
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 — Do not swallow unexpected failures in this regression test. Both external commands are mocked as successful, so an exception from
Suggested change
Codex quota is about to reset, so I am using the remaining token budget to review this PR now. Coding agent: Codex |
||||||||||||||||
|
|
||||||||||||||||
| # Read the generated training script and verify auto_prob | ||||||||||||||||
| script_path = Path("task-auto-prob") / train_script_name | ||||||||||||||||
| self.assertTrue(script_path.exists(), "Training script was not generated") | ||||||||||||||||
| with open(script_path) as fp: | ||||||||||||||||
| train_dict = json.load(fp) | ||||||||||||||||
| auto_prob = train_dict["training"]["training_data"]["auto_prob"] | ||||||||||||||||
| # Must be plain "prob_sys_size", NOT "prob_sys_size; 0:2:0.6; 2:2:0.4" | ||||||||||||||||
| self.assertEqual(auto_prob, "prob_sys_size") | ||||||||||||||||
|
|
||||||||||||||||
| # Cleanup | ||||||||||||||||
| shutil.rmtree("empty_iter_data", ignore_errors=True) | ||||||||||||||||
| shutil.rmtree("task-auto-prob", ignore_errors=True) | ||||||||||||||||
| shutil.rmtree("input-auto-prob-test", ignore_errors=True) | ||||||||||||||||
|
|
||||||||||||||||
| def test_update_input_dict_v1_init_model(self): | ||||||||||||||||
| odict = RunDPTrain.write_data_to_input_script( | ||||||||||||||||
| self.idict_v1, | ||||||||||||||||
|
|
||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 — Handle the symmetric empty-old-data case. If a workflow starts from supplied
init_modelswith noinit_data, the first labeled iteration can makenumb_old == 0 < numb_new. This branch then emitsprob_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 toprob_sys_size; please also generalize the warning and add an empty-old regression test.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