[njzjz-bot]

Summary

• Replace the incorrect overlap guard and unbounded rejection loop with randomized augmenting-path matching
• Accept every satisfiable species family and report impossible choices with the offending name_of_atoms
• Exercise all 399 nonempty families of one to three candidate lists over three symbols against an independent exhaustive oracle
• Cover reassignment, duplicate candidates, empty candidates, controlled randomness and task generation; guard integration tests against regression to the old retry loop

Verification

• CPU exploration suite: 112 tests and 432 subtests passed
• Four deliberate mutants fail: overlap guard, identical-list guard, greedy-only matching, and restored dead guard/retry loop
• 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

Tests are now in a dedicated module, avoiding the shared insertion-point conflict with #383. #405 still touches the removed old predicate; preserve the matching implementation when resolving that overlap.

Closes #356

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: 116b2bd5-ea24-4cb1-b454-e58c8f4d4ce9

📥 Commits

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

📒 Files selected for processing (2)

• dpgen2/exploration/task/caly_task_group.py
• tests/exploration/test_caly_atom_choices.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 86.01%. Comparing base (f0093fe) to head (f7be3a4).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
+ Coverage   85.98%   86.01%   +0.02%     
==========================================
  Files         105      105              
  Lines        6930     6935       +5     
==========================================
+ Hits         5959     5965       +6     
+ Misses        971      970       -1     

☔ 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]

You are right that (set(s) - overlap) == 0 compares a set to an int and has never fired — that part of #356's diagnosis is correct, and I confirmed the guard has been dead since it was written in 07df321 (#217, 2024-04-30). But the replacement predicate is not the right test, and turning it on is a net regression: it rejects configurations that work today while still hanging on the ones #356 is about.

The predicate is not the condition the loop needs

overlap is the intersection of all sub-lists, so overlap ⊆ L holds for every L. That makes not (set(L) - overlap) equivalent to set(L) == overlap — "some sub-list equals the global intersection". The loop below needs one element per sub-list, all distinct, which exists exactly when Hall's marriage condition holds. The two are unrelated.

Brute-forcing every family over a 3-symbol universe (399 configs, satisfiability decided by exhaustive search):

sub-lists   total  ok-accept  ok-reject  WRONGLY REJECTED  WRONGLY ACCEPTED
    1          7        0          0            7                0
    2         49       18          3           28                0
    3        343      165         51           82               45
  total      399      183         54          117               45

117 of 399 (29%) are satisfiable configs the guard now rejects. 45 (11%) are impossible configs it still lets through, where the loop hangs exactly as #356 describes.

Run against the real make_calypso_task_group_from_config:

[["Li"]]                              d3ca156~1: SUCCESS            head: ValueError
[["Li","Na","K"],["Na","K"],["K"]]    d3ca156~1: ['Li','Na','K']   head: ValueError
[["Li"],["Li"],["Na","K"]]            head: killed at 8s, exit 137 (still spinning)

Note the first row: every single-sub-list config is now rejected, because with one sub-list the intersection is that sub-list. name_of_atoms: [["Li","Na","K"]] — pick one species at random for a 1-species search — is the simplest use of this feature and it now fails at submit.

And the second row is the example printed in the error message itself. Details inline.

Suggested direction

Hall's condition is exact and cheap at CALYPSO species counts:

from itertools import combinations
sets = [set(s) for s in name_of_atoms]
n = len(sets)
if any(
    len(set().union(*(sets[i] for i in sub))) < k
    for k in range(1, n + 1)
    for sub in combinations(range(n), k)
):
    raise ValueError(
        f"cannot pick {n} distinct species from {name_of_atoms}: "
        "some group of sub-lists has fewer candidates than sub-lists"
    )

I checked this against exhaustive search over the same sweep: zero mismatches. Better still would be to replace the while True rejection sampling with a direct matching that constructs the assignment — then the guard is unnecessary and the hang is impossible by construction.

Merge-order warning, not a change request

PR #405 ("Enable remaining Ruff rules", open) rewrites this same line to any((set(s) - overlap) == 0 for s in name_of_atoms) — a pure map→genexp change that keeps the == 0 bug. git merge-tree confirms a direct conflict on this line. If #405 lands after this and the conflict is resolved carelessly, the always-false guard comes back. Worth sequencing them deliberately. (#383 also touches this file but only conflicts on the test file's insertion point — trivial.)

Not a problem, so nobody re-raises it

The bare ValueError is correct here. I traced the call chain — submit_concurrent_learning → workflow_concurrent_learning → make_naive_exploration_scheduler → make_calypso_task_group_from_config → set_params — and it all runs client-side during dpgen2 submit, before wf.submit(), not inside a dflow OP. FatalError would be wrong at this layer.

[wanghan-iapcm]

Two things go wrong on this line, and they are the same root cause.

It rejects valid configurations. Since overlap ⊆ atom_choices always holds, this fires whenever a sub-list equals the global intersection. That is the shape of any "narrow the choices as you go" config, and of every single-sub-list config. Verified against the shipped API:

[["Li"]]                             before: SUCCESS   after: ValueError
[["Li","Na"]]                        before: SUCCESS   after: ValueError
[["Li","H"],["La","H"],["H"]]        before: ok       after: ValueError   (Li, La, H is valid)

The example in the error message two lines below is itself valid. [[A,B,C],[B,C],[C]] assigns C→B→A. I ran the real equivalent, [["Li","Na","K"],["Na","K"],["K"]]: at d3ca156~1 it returned ['Li','Na','K']; at this head it raises. So the message has documented a legal config as forbidden since #217, and this change is what makes the code enforce that. Whatever predicate you land on, that sentence needs to go or be corrected — it is the only user-facing description of the rule, and it is wrong.

It still hangs on genuinely impossible configs. [["Li"],["Li"],["Na","K"]] has an empty global intersection, so no sub-list equals it, the guard stays silent, and the loop spins forever — I killed it at 8 seconds, exit 137. That is the same failure #356 reports, one sub-list larger. 45 of 399 swept configs behave this way.

Hall's condition is the exact test; see the review body for a drop-in that I verified has zero mismatches against exhaustive search. Whichever way you go, it would be worth putting the offending name_of_atoms and the computed intersection into the message — as written it prints neither, so a user cannot tell which sub-list tripped it.

[njzjz-bot]

Replaced the guard and retry loop together in bbaf242 with a randomized augmenting-path matching. It constructs a distinct assignment or raises ValueError after a finite search, including the offending name_of_atoms in the error. The valid single-list and nested examples now work; impossible families with empty global intersection are rejected. An independent exhaustive oracle agrees on all 399 families (300 satisfiable, 99 impossible).

Agent: dot

[wanghan-iapcm]

[["Li"],["Li"]] happens to be a case where the wrong rule and the right rule agree, so this test cannot tell them apart. I patched three different predicates into the guard and ran only this test:

PR's rule  (sub-list == global intersection)        PASSED
Hall's condition (correct)                          PASSED
"raise iff any two sub-lists are identical" (wrong)  PASSED

All three. So it would not catch a wrong fix, which is the thing worth catching here.

Separately: if the guard ever regresses, this test hangs rather than fails. I reverted the predicate to the pre-PR always-false form and ran it under timeout -k 2 15:

Terminated
EXIT_CODE=124

No pytest verdict at all — assertRaisesRegex is wrapping a call that enters an unbounded while True. A wedged CI job is a worse signal than a red one.

To be fair to it, the test does pin something real: that some guard fires before the retry loop for this input. Three additions would make it discriminating and safe:

• a positive case that must succeed — [["Li","Na","K"],["Na","K"],["K"]], or just [["Li"]] — which fails today;
• a true negative with an empty intersection — [["Li"],["Li"],["Na"]] — which currently hangs;
• and asserting on the predicate directly, or adding a timeout, so a regression reports instead of wedging.

[njzjz-bot]

Replaced the single regression with a dedicated test module covering all 399 families, valid narrowing/identical candidate lists, empty candidates, reassignment and actual task generation. Integration cases patch random.choice to fail immediately if the old retry sampler returns. Verified four deliberate mutants fail: the old overlap predicate, identical-list predicate, greedy-only matching, and restored dead guard/retry loop. CPU exploration suite: 68 tests + 407 subtests passed at bbaf242.

Agent: dot