Skip to content

fix(concurrency): resolve impossible-to-succeed TODO and align with TypeScript behavior#72

Closed
ghost wants to merge 2 commits intomainfrom
map-parallel-sweep
Closed

fix(concurrency): resolve impossible-to-succeed TODO and align with TypeScript behavior#72
ghost wants to merge 2 commits intomainfrom
map-parallel-sweep

Conversation

@ghost
Copy link

@ghost ghost commented Oct 21, 2025

  • [Parity]: Add completion reason inference
  • fix(concurrency): resolve impossible-to-succeed TODO and align with TypeScript behavior

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

- Adds completion reason inference based on the rules outlined in the
  typescript implementation / spec.

fixes: #36
…ypeScript behavior

- Remove impossible-to-succeed early termination logic to match TypeScript implementation
- Refactor ExecutionCounters with separate should_continue() and is_complete() methods
- Add comprehensive documentation explaining TypeScript parity decisions
- Update completion logic to properly handle ALL_COMPLETED with failures when all tasks finish
- Maintain existing _get_completion_reason logic for correct completion reason inference

Resolves TODO at line 416 by implementing TypeScript-compatible completion behavior
where operations continue until normal completion criteria are met rather than
terminating early on impossible-to-succeed conditions.
return CompletionReason.MIN_SUCCESSFUL_REACHED

# Otherwise, assume failure tolerance was exceeded
return CompletionReason.FAILURE_TOLERANCE_EXCEEDED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment in other PR for this: this logic can be in from_items rather than in this arbitrary function

items: Iterable[BatchItem[R]],
completion_config: CompletionConfig | None = None,
):
items = list(items)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

necessary?

@ghost ghost closed this Oct 21, 2025
@wangyb-A wangyb-A deleted the map-parallel-sweep branch December 9, 2025 22:31
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants