fix(sdk): make map/parallel child exec order invariant#108
Merged
1 commit merged intomainfrom Oct 29, 2025
Merged
Conversation
ghost
changed the title
ghost
force-pushed
the
quinsclr/fix-concurrency-on-map-and-parallel
branch
from
fae2007 to
93d6f68
Compare
Context: - Currently the operation_id for children in map and parallel is contingent on execution order due to internal method that always increments a counter for safety. - In map and parallel, depending on when the executor decides to run a particular branch, we may observe different ordering. Changes: - we skip using executor context's run_in_child_context to run a concurrent branch, and instead call child_handler directly. - we generate the appropriate operation_id before we call child_handler and use executable.index to make it deterministic.
ghost
force-pushed
the
quinsclr/fix-concurrency-on-map-and-parallel
branch
from
93d6f68 to
3889a42
Compare
ghost
marked this pull request as ready for review
rarepolz
approved these changes
Oct 29, 2025
yaythomas
reviewed
Oct 29, 2025
| info=self._log_info, | ||
| ) | ||
|
|
||
| def _create_step_id_for_logical_step(self, step: int) -> str: |
Contributor
There was a problem hiding this comment.
step maybe better described as counter
Contributor
There was a problem hiding this comment.
_create_step_id_with_preset_counter
|
|
||
| def _create_step_id_for_logical_step(self, step: int) -> str: | ||
| """ | ||
| Generate a step_id based on the given logical step. |
Contributor
There was a problem hiding this comment.
on the given counter.
"logical step" doesn't really have a well defined meaning.
| we generate an operation_id for the child, and then call `child_handler` | ||
| directly. This avoids the hidden mutation of the context's internal counter. | ||
| we can do this because we explicitly control the generation of step_id and do it | ||
| using executable.index. |
Contributor
There was a problem hiding this comment.
phrasing.
"This avoids the hidden mutation of the context's internal counter.
we can do this because we explicitly control the generation of step_id and do it
using executable.index."
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context:
is contingent on execution order due to internal method that
always increments a counter for safety.
to run a particular branch, we may observe different ordering.
Changes:
concurrent branch, and instead call child_handler directly.
and use executable.index to make it deterministic.
Issue #, if available:
fixes: #107
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.