[njzjz-bot]

Summary

• Accept explicit conf_idx without the previous normalization crash, and leave the caller's dictionary unchanged
• Supply the required normalization key only when neither conf_idx nor its sys_idx alias is present
• Cover absent, explicit, aliased and invalid keys; configuration selection still occurs separately through set_conf
• Copy the CALYPSO caller dictionary before removing type, with regression coverage for caller and nested-value independence
• Rename the test and describe the actual normalization/nonmutation contract

Verification

• CPU exploration suite: 110 tests and 25 subtests passed
• Mutations removing the LAMMPS copy, clobbering the supplied normalization key or removing the CALYPSO copy all fail
• Full Pyright 1.1.318 check with CI-matched NumPy 2.2.6: zero errors
• Synced current master, preserving both merge parents
• Pinned Ruff 0.1.3 formatting, isort 5.12.0 and git diff --check passed

Closes #358

Agent: dot

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 49 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: e8e11c50-ebb6-4bbb-95dd-67f5cb18e52c

📥 Commits

Reviewing files that changed from the base of the PR and between f0093fe and b592785.

📒 Files selected for processing (2)

• dpgen2/exploration/task/make_task_group_from_config.py
• tests/exploration/test_make_task_group_from_config.py

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.99%. Comparing base (f0093fe) to head (b592785).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #384   +/-   ##
=======================================
  Coverage   85.98%   85.99%           
=======================================
  Files         105      105           
  Lines        6930     6933    +3     
=======================================
+ Hits         5959     5962    +3     
  Misses        971      971           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Retracted. This review was produced without running the mandated /code-review fan-out (the loop skill's section 2); the substitute process used instead has since been shown to miss findings and, in one case, to state a verified-sounding falsehood. Re-reviewing properly.

[wanghan-iapcm]

The code change is right and I have no objection to it. Blocking on the test, which pins neither half of what you fixed, and on the framing, which describes a behaviour the code does not have.

The fix is correct

I reproduced the pre-fix failure on the merge base:

dargs.dargs.ArgumentTypeError: [at root location] key `conf_idx` gets wrong value type,
                               requires <list> but None is not a list
caller dict after: {'type': 'lmp-md', 'Ts': [100], 'conf_idx': None}

so it both crashed and left the caller's dict poisoned. The shallow copy is sufficient: dargs normalize_value deep-copies its input, so no nested aliasing survives into the task group. I checked that rather than assuming it.

The archaeology is worth recording because it explains the shape of the bug. It was born in a6f0fec (#156, 2023-07-26). Seventy-eight lines above, config_strip_confidx reads

cc.pop("conf_idx") if "conf_idx" in cc else None

where else None is a harmless discarded expression. The same shape was reused one function down on the right-hand side of an assignment, where else None stops being a no-op and becomes a write. Same commit, same author, about eighty lines apart. It stayed invisible because submit.py has done sys_idx = jconf.pop("conf_idx") since that same commit, so the only in-tree caller has always taken the [] branch.

Blocking: the test pins neither half of the fix

I mutated your own code two ways and re-ran the module:

(0) PR head as-is                       -> Ran 5 tests  OK
(a) copy kept, clobber explicit value   -> Ran 5 tests  OK    <- "preserve" not pinned
(b) setdefault kept, copy removed       -> Ran 5 tests  OK    <- "no mutation" not pinned

Both survive. The only property the new test exercises is "calling with an explicit conf_idx does not raise". The assertEqual(config["conf_idx"], [2]) line cannot fail: it is reachable only when nothing raised, and in every non-raising implementation conf_idx is either left alone or written on a copy.

Half (b) is cheap to pin. Add to the existing test_npt, on the absent path:

self.assertNotIn("conf_idx", self.config_npt)

I verified that this passes at head and fails under mutant (b) with AssertionError: 'conf_idx' unexpectedly found in {'type': 'lmp-md', 'Ts': [100], 'conf_idx': []}.

Half (a) cannot be pinned, and that is the interesting part

config_strip_confidx pops conf_idx immediately after normalization, so this function never consumes it. I built two task groups, one with conf_idx=[2] and one with the key absent, and compared them: vars() identical, conf_set false in both. NPTTaskGroup.set_md has no such parameter. Configuration selection happens later and separately, in submit.py, through tgroup.set_conf(...) using a value popped before this helper is ever called.

So the title "preserve LAMMPS configuration indices" and the test name test_npt_preserves_explicit_conf_idx describe something the code does not do. Nothing is preserved in any sense a caller can observe, which is also why mutant (a) survives and why no black-box test can catch it. What is actually fixed is a crash and a corrupted caller dict, both genuinely worth fixing. Please retitle the test to match, for example test_npt_accepts_explicit_conf_idx_without_mutating_caller, and adjust the docstring, so the next reader is not hunting for behaviour that is not there.

Not blocking

Same bug class next door: make_calypso_task_group_from_config does config.pop("type", None) on the caller's dict, permanently deleting the key. make_diffcsp_task_group_from_config normalizes into a fresh dict and this helper now copies, so that one is the holdout. Its shipped path is shielded only incidentally, because caly_normalize happens to run first. Worth the same one line, here or in a follow-up.

On merge order: this does not conflict with #382, #383 or #405. I checked with git merge-tree.

[wanghan-iapcm]

This assertion cannot fail. It is reachable only if the call did not raise, and in every non-raising implementation conf_idx is either untouched (setdefault) or written on a copy, so no code under review reaches this line with a different value.

I mutated the fix two ways and the module stayed green both times:

(a) copy kept, clobber the explicit value  -> Ran 5 tests  OK
(b) setdefault kept, copy removed          -> Ran 5 tests  OK

Neither half of the fix is pinned; the test only proves "does not raise". Half (b) is easy to cover: put self.assertNotIn("conf_idx", self.config_npt) in test_npt, which fails immediately if the copy is dropped. I checked that it passes at head and fails under mutant (b).

Half (a) is not coverable at all, because config_strip_confidx pops conf_idx before it can influence anything. See my note on the test name.

[njzjz-bot]

Added the absent-key assertion to test_npt and the template path, plus caller snapshots for explicit/aliased keys and invalid normalization input. Removing the LAMMPS copy now fails. Clobbering a supplied key is also caught through observable behavior: an invalid conf_idx must still be rejected, and sys_idx must normalize correctly. CALYPSO now copies before removing type, with its own mutation regression. All three deliberate mutants fail; 66 exploration tests pass at 83c5f30.

Agent: dot

[wanghan-iapcm]

The name and docstring claim a preservation behaviour that is not observable. config_strip_confidx removes conf_idx immediately after normalization, so this function never consumes it. I built a task group with conf_idx=[2] and one with the key absent and compared them: vars() identical, conf_set false in both, and NPTTaskGroup.set_md has no such parameter. That is why the mutant which clobbers the value still passes.

What the change really fixes is the ArgumentTypeError crash and the mutation of the caller's dict. Suggest test_npt_accepts_explicit_conf_idx_without_mutating_caller, with a docstring to match. The PR title has the same issue.

[njzjz-bot]

Renamed the test to test_npt_accepts_explicit_conf_idx_without_mutating_caller and corrected its docstring, the PR title and summary. They now describe successful normalization and caller nonmutation. The test explicitly checks conf_set is false; this helper does not select configurations, and selection remains the caller's separate set_conf step.

Agent: dot