feat: limit in-flight operations in map/parallel#533
Conversation
9f04c8d to
3aa2bdc
Compare
69100b1 to
419d14e
Compare
b144ab0 to
500507e
Compare
500507e to
e6a45d9
Compare
021cf06 to
c23f02a
Compare
c23f02a to
31ab29f
Compare
max_concurrency now bounds in-flight branches via a coordinator loop, the completion decision is recorded in an SDK-owned summary envelope and obeyed on replay, and completion config bounds are validated. BREAKING CHANGE: max_concurrency=0 and min_successful=0 now raise ValidationError (previously unlimited / all-items). min_successful greater than the item count now raises ValidationError. BatchResult omits never-started branches. Large-result summary payloads use the SDK envelope format; custom summary_generator output is stored under the summary key instead of replacing the payload. The internal MapSummaryGenerator and ParallelSummaryGenerator classes are removed; the envelope supersedes them. CompletionConfig.all_completed() now tolerates all failures instead of failing fast.
31ab29f to
5077544
Compare
| f"min_successful cannot be greater than total items: " | ||
| f"{min_successful} > {len(items)}" | ||
| ) | ||
| raise ValidationError(msg) |
There was a problem hiding this comment.
Should ValidationError be checkpointed?
There was a problem hiding this comment.
The raise occurs inside map_handler, which runs inside child_handler's try block, so it is checkpointed and the map context FAILs terminally with error_type: ValidationError, surfacing as a catchable ChildContextError.
That's deterministic (replay re-raises the recorded error), loud, and visible in history. And the current placement means the error behaves exactly like every other failure inside a map.
The alternative is to validate in context.py before the child context starts, so no STARTED-then-FAILED context appears in history, which is closer to matching Java's constructor validation + throw IllegalArgumentException?
So the diffs is
a) ChildContextError wrapping ValidatorError, with a STARTED/FAILED checkpoint for the map (current)
vs
b) a bare exception at execution level?
There's prob an argument to make for either. I instinctively would've thought a STARTED/FAILED on the map makes sense since this can only rear its head at runtime rather than design time (like a compiler err), but happy to reconsider if you think otherwise?
| # when completion criteria are met (e.g., min_successful). | ||
| # Running threads continue in the background and raise | ||
| # OrphanedChildException on their next attempt to checkpoint. | ||
| pool.shutdown(wait=False, cancel_futures=True) |
There was a problem hiding this comment.
Would this cause the threads and resources to accumulate if users heavily use early return with long running tasks
Description of changes:
BREAKING CHANGE -- see the behavior changes section; config validation, result shape, and the large-result summary payload format change in this PR.
max_concurrencypreviously sized the thread pool only, so branches that suspended (invoke, wait, callbacks) released their thread and the next item started immediately. All items were initiated at once regardless of the configured limit.Rework the concurrency executor around a single coordinator loop:
max_concurrencynow bounds in-flight branches. A suspended branch keeps its slot until it reaches a terminal state, matching the JS and Java SDKs. When every in-flight branch is suspended, the parent suspends with the earliest resume timestamp, or indefinitely.TimerSchedulerbackground thread.CompletionPolicy, replacingExecutionCountersand the duplicated logic inBatchResult.type,totalCount,completionReason, the started-branch index set (startedIndexes, orcompletedIndexeswhen smaller), and stats fields (startedCount,successCount,failureCount,status). Replay reads the record and reconstructs the exact live result — same items, same statuses, same reason — instead of re-deriving from child checkpoints. A configuredsummary_generatorno longer produces the payload: its output is stored verbatim under the envelope'ssummarykey for observability and is never read by the SDK, so custom generators get exact replay and cannot displace the record. An exception raised by a custom generator propagates and fails the operation, matching the JS SDK. The envelope is checkpointed as provided; a payload exceeding the checkpoint size limit fails the operation with the service's payload validation error (terminal map failure, no retry loop -- checkpoint API 4xx errors are classified non-retryable). The envelope is written and parsed with plain JSON, independent of any configured serdes. Payloads without the record (checkpoints written by earlier SDK versions) fall back to checkpoint derivation, preserving old behavior. This also makes FLAT-nesting results reconstructable on replay, which checkpoint derivation cannot discriminate.Behavior changes
BatchResultomits never-started branches (matching the JS SDK). Previously every item appeared as STARTED even when an early completion meant it never ran;total_countnow reflects branches that actually started.min_successfulset no longer fail fast on the first failure. The old tolerance check ignoredmin_successfulwhen deciding whether criteria existed, so a single failure stopped scheduling; scheduling now continues past failures untilmin_successfulis reached or all items finish, matching the JS and Java SDKs.max_concurrencyandmin_successfulmust be at least 1,tolerated_failure_countnon-negative,tolerated_failure_percentagewithin 0–100. Previouslymax_concurrency=0silently meant unlimited andmin_successful=0silently meant all items.min_successfulgreater than the item count now raisesValidationErrorat the operation call, matching the Java SDK. Previously it silently degraded to "complete when everything finishes".CompletionConfig.all_completed()now tolerates all failures (tolerated_failure_percentage=100), matching its documentation and the Java SDK. Previously its all-Nonefields selected the fail-fast default, so a single failure stopped scheduling -- the opposite of the factory's name.MapSummaryGeneratorandParallelSummaryGeneratorclasses are removed (not exported from the package root; reachable only by deep import, referenced by no examples or docs, and never invoked on the released large-result path). The envelope writes their fields directly.startedCountnow on map as well as parallel). Output from a customsummary_generatorappears under thesummarykey instead of replacing the whole payload.Testing
aws-durable-execution-sdk-python-conformance-tests, deployed against this branch's SDK): all 9 suites pass with 145/145 validations — map 20/20 (including 9-16 MapLargeResult, which exercises the summarized-replay path, and the early-stop cases for fail-fast, min-successful, and failure-tolerance semantics), parallel 22/22, callback 19/19, child 17/17, invoke 16/16, step 20/20, wait 5/5, wait_for_callback 15/15, wait_for_condition 11/11.Issue #, if available:
Closes #279
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.