Refactor PhysicalOperator.completed to fix side effects #58915
Conversation
…n_finished() to has_execution_finished(). Make has_execution_finished() and complete() pure functions to remove side effects. Signed-off-by: Simeet Nayan <[email protected]>
bveeramani
left a comment
bveeramani
left a comment
There was a problem hiding this comment.
High-level change LGTM.
I think you'll need update references to execution_finished with has_execution_finished.
Would you mind renaming completed to has_completed to make it clear that the method doesn't modify state, and also ensure that this PR passes the microcheck status check?
…nished in limit_operator Signed-off-by: Simeet Nayan <[email protected]>
…nished in test_operators.py Signed-off-by: Simeet Nayan <[email protected]>
Signed-off-by: Simeet Nayan <[email protected]>
|
Sure thing @bveeramani. Will do the changes. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
python/ray/data/_internal/execution/operators/limit_operator.py
Outdated
Show resolved
Hide resolved
ray-gardener
bot
added
data
python/ray/data/_internal/execution/operators/limit_operator.py
Outdated
Show resolved
Hide resolved
… has_execution_finished() Signed-off-by: Simeet Nayan <[email protected]>
…refactor-PhysicalOperator Signed-off-by: Simeet Nayan <[email protected]>
Can I take this in a follow-up pr? Regarding the test failing, it's related to test_stats::test_large_args_scheduling_strategy, particularly at |
Yeah, ofc. |
2 participants
Description
This PR refactors the
PhysicalOperatorclass to eliminate hidden side effects in thecompleted()method. Previously, callingcompleted()could inadvertently modify the internal state of the operator, which could lead to unexpected behavior. This change separates the logic for checking if the operator is marked as finished from the logic that computes whether it is actually finished.Key changes include:
_execution_finishedto_is_execution_marked_finishedto clarify its purpose.execution_finished()tohas_execution_finished()and making it a pure computed property without side effects.completed()method to callhas_execution_finished()instead of modifying internal state.mark_execution_finished()correctly sets the renamed field.Related issues
Fixes #58884
Additional information
This refactor ensures that both
has_execution_finished()andcompleted()are pure query methods, allowing them to be called multiple times without altering the state of the operator. T