[data] MapOperator.num_active_tasks should exclude pending actors #46364
Conversation
This file contains 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
raulchen
requested review from
ericl,
scv119,
c21,
amogkam,
scottjlee,
bveeramani,
stephanie-wang and
omatthew98
as code owners
bveeramani
approved these changes
Jul 1, 2024
| @@ -414,6 +414,17 @@ def implements_accurate_memory_accounting(self) -> bool: | |||
| def supports_fusion(self) -> bool: | |||
| return self._supports_fusion | |||
|
|
|||
| def num_active_tasks(self) -> int: | |||
There was a problem hiding this comment.
If I'm understanding correctly, this change makes len(get_active_tasks()) != num_active_tasks for map operators. I think that might get confusing.
@raulchen do have any ideas for how we can address the issues while avoiding this discrepancy?
There was a problem hiding this comment.
I thought of this as well. It's indeed a bit confusing.
I also thought of separating data and metadata tasks. But that seems overkill.
I think maybe I'll just update the comments to clarify for now.
scottjlee
reviewed
Jul 2, 2024
| * Displaying active task info in the progress bar. | ||
| Thus, the return value can be less than `len(get_active_tasks())`, | ||
| if some tasks are not needed for the above purposes. E.g., for the | ||
| actor pool map operator, readiness checking tasks can be excluded |
There was a problem hiding this comment.
what is "readiness checking task" referring to in this case?
There was a problem hiding this comment.
it's _MapWorker.get_location.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why are these changes needed?
MapOperator.num_active_tasksshould exclude the pending actors. BecausePhysicalOperator.completedchecksnum_active_tasks. The operator should be considered completed if there are still pending actors.Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.