fix: keep partial relaxation groups

[njzjz-bot]

Summary

• calculate relaxation task counts with ceiling division
• reject non-positive relax_group_size values
• test partial groups and inputs smaller than one group

Tests

• python -m unittest -v tests.op.test_prep_relax
• git diff --check

Closes #350

Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh

Summary by CodeRabbit

• Bug Fixes
  • Relaxation preparation now includes remaining CIF files in a final partial task group.
  • Invalid non-positive group sizes are rejected with a clear error instead of proceeding.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fe7b7b72-134a-4fa5-822d-bc0e7010ad8c

📥 Commits

Reviewing files that changed from the base of the PR and between 6b01f29 and 0255a20.

📒 Files selected for processing (2)

• dpgen2/op/prep_relax.py
• tests/op/test_prep_relax.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

PrepRelax.execute now validates relax_group_size and uses ceiling division so every CIF is assigned to a task. Tests cover complete groups, partial final groups, oversized groups, invalid sizes, and generalized cleanup.

Changes

PrepRelax task grouping

Layer / File(s)	Summary
Validate group size and calculate task count dpgen2/op/prep_relax.py	PrepRelax.execute raises ValueError for non-positive group sizes and creates a task for a partial final group.
Cover grouping edge cases tests/op/test_prep_relax.py	Parameterized helpers and tests cover standard, partial, oversized, and invalid group sizes. Cleanup now removes all generated task directories and CIF files.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 0255a

This localized change validates group sizes and ensures final partial relaxation groups are processed without changing deployment or workflow contracts. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the primary change: preserving partial relaxation groups.
Linked Issues check	✅ Passed	The changes satisfy issue #350. PrepRelax uses ceiling division, validates positive relax_group_size values, and adds tests for remainder groups, undersized inputs, and invalid group sizes.
Out of Scope Changes check	✅ Passed	The production and test changes remain within issue #350. The test helpers and cleanup changes support the required regression coverage.

• Fix all pre-merge checks with AI

---

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 84.44%. Comparing base (6b01f29) to head (0255a20).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #375   +/-   ##
=======================================
  Coverage   84.43%   84.44%           
=======================================
  Files         104      104           
  Lines        6110     6112    +2     
=======================================
+ Hits         5159     5161    +2     
  Misses        951      951           

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

Correct fix. The min(group_size * (i + 1), ncifs) clamp in the loop below has been there since the file was created in #251, but floor division made it unreachable for every i <= ntasks - 1 — so the guard against a short final group was dead code from day one. Ceiling division restores the original intent rather than changing behaviour.

Worth recording the production impact, since it is larger than the issue suggests: relax_group_size defaults to 100, so any DiffCSP iteration generating a number of structures that is not a multiple of 100 silently discarded up to 99 of them, and an iteration generating fewer than 100 produced zero relax tasks. No error, no log line.

I verified the new tests are not decoration. Reverting both changed lines and re-running:

test_keeps_partial_final_group                    AssertionError: 2 != 3
test_creates_task_when_group_is_larger_than_input AssertionError: 0 != 1
test_rejects_non_positive_group_size              ZeroDivisionError

All four pass at head. The reason this sat undetected is that tests/op/test_prep_relax.py has exactly one commit in its history and has always used ncifs=4, group_size=2 — an exact multiple, the one input class where floor and ceiling division agree.

Approving. One non-blocking note on the new test inline, and two smaller things below that are yours to take or leave.

Not blocking

1. ValueError vs FatalError. dflow's generated pod script maps only TransientError to exit 1 and FatalError to exit 2; any other exception propagates and CPython also exits 1. The Argo retry expression dflow emits is asInt(lastRetry.exitCode) == 1, so with retry_on_transient_error set, this deterministic config error gets retried until the budget runs out. #366 made exactly this conversion for the same reason, and FatalError is used this way at 17 sites in the package.

2. Placement. relax_group_size is declared in dpgen2/entrypoint/args.py as a plain Argument(..., int, optional=True, default=100) with no constraint, so this guard is genuinely needed and not redundant. But it fires inside the prep-relax container, after diffcsp-gen has already run. extra_check=lambda v: v > 0 on that Argument would reject the value during normalize_args at submit time instead. Note the repo currently uses extra_check nowhere, so this would be a new pattern rather than following one.