feat: add should_complete to CompletionConfig - #605
Conversation
862e017 to
c72785b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c72785b to
306c81c
Compare
306c81c to
5483a62
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5483a62 to
fffee6d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fffee6d to
7ad4242
Compare
7ad4242 to
302330d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| if self.policy.should_complete is not None | ||
| else () | ||
| ) | ||
| decision = self.policy.evaluate(succeeded, failed, items_snapshot) |
There was a problem hiding this comment.
Codex AI review
[P1] Restore prior branch state before invoking the predicate
On a resumed invocation, all branches start in memory as PENDING and prior terminal checkpoints are replayed concurrently. The predicate is therefore evaluated against partial, synthetic snapshots whose order depends on worker scheduling. For example, if A previously succeeded before B failed, replay may report B first; a deterministic predicate such as “fail when B fails unless A succeeded” can now choose a different outcome and omit previously started branches. Add a restoration barrier that reconstructs all branch state present at invocation start before evaluating should_complete, then add a resume test that reverses prior checkpoint event order.
Codex AI reviewFound one high-severity replay-determinism defect. Current resume coverage only exercises a monotonic predicate and cannot detect restoration-order changes across multiple prior branches. Reviewed commit |
yaythomas
left a comment
There was a problem hiding this comment.
just two minor noin-func nits :)
| The predicate must be deterministic and side-effect-free, and | ||
| depend only on the CompletionStatus provided, never on external | ||
| state. For a completion that must survive a mid-run resume, | ||
| prefer a monotonic decision (for example success_count >= n) so |
There was a problem hiding this comment.
nit: "prefer" is a "must"? You argued on the codex review comments that resume determinism relies on monotonicity as a hard contract. A predicate that is false on the suspension time terminal set is false on every subset of it, but only if monotonicity is required.
| assert result.status is InvocationStatus.SUCCEEDED | ||
| assert result.result is not None | ||
|
|
||
| import json |
There was a problem hiding this comment.
inline `import. Please move it to the top of the module. Google style §3.13 via CONTRIBUTING → General style.
Issue #, if available: #519
Description of changes:
Adds a
should_completepredicate to CompletionConfig for map and parallel operations, giving full control over when a batch completes early.The predicate receives a CompletionStatus snapshot (counts + per-item statuses) and returns a CompletionDecision - either continue_batch() or complete_batch(outcome). The outcome determines whether
CUSTOM_COMPLETION_SUCCEEDEDorCUSTOM_COMPLETION_FAILEDis reported.Key design decisions -
Testing -
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.