-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[data] Improve execution progress rendering #56992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 37 commits
626c87b
225f4f2
c828f85
2b7e58b
d627f3e
bfc08d7
27551e0
814228b
fc04b23
b794aca
5b1c3f1
a3dd050
d043eec
fb23fc6
d8e1c49
c5fdfa9
e084a9f
c57dfe4
a9a5c52
b5893ab
677e1f9
9c5483a
fdec716
f236bb9
c932142
62365c4
36783f3
55df375
793bee0
b2f8316
e2d5b96
d405f97
cd54f7b
66e6347
e869e0b
e8a8167
46a6d73
01f00bd
3d3ab72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ | |
| _create_sub_pb, | ||
| estimate_total_num_of_blocks, | ||
| ) | ||
| from ray.data._internal.execution.operators.sub_progress import SubProgressBarMixin | ||
| from ray.data._internal.logical.interfaces import LogicalOperator | ||
| from ray.data._internal.stats import OpRuntimeMetrics | ||
| from ray.data._internal.table_block import TableBlockAccessor | ||
|
|
@@ -363,7 +364,7 @@ def combine(one: "_PartitionStats", other: "_PartitionStats") -> "_PartitionStat | |
| ) | ||
|
|
||
|
|
||
| class HashShuffleProgressBarMixin(abc.ABC): | ||
| class HashShuffleProgressBarMixin(SubProgressBarMixin): | ||
| @property | ||
| @abc.abstractmethod | ||
| def shuffle_name(self) -> str: | ||
|
|
@@ -374,26 +375,29 @@ def shuffle_name(self) -> str: | |
| def reduce_name(self) -> str: | ||
| ... | ||
|
|
||
| def _validate_sub_progress_bar_names(self): | ||
| assert self.shuffle_name is not None, "shuffle_name should not be None" | ||
| assert self.reduce_name is not None, "reduce_name should not be None" | ||
|
|
||
| def initialize_sub_progress_bars(self, position: int) -> int: | ||
| """Display all sub progres bars in the termainl, and return the number of bars.""" | ||
| """Display all sub progress bars in the termainl, and return the number of bars.""" | ||
| self._validate_sub_progress_bar_names() | ||
|
|
||
| # shuffle | ||
| progress_bars_created = 0 | ||
| self.shuffle_bar = None | ||
| if self.shuffle_name is not None: | ||
| self.shuffle_bar, position = _create_sub_pb( | ||
| self.shuffle_name, self.num_output_rows_total(), position | ||
| ) | ||
| progress_bars_created += 1 | ||
| self.shuffle_bar, position = _create_sub_pb( | ||
| self.shuffle_name, self.num_output_rows_total(), position | ||
| ) | ||
| progress_bars_created += 1 | ||
| self.shuffle_metrics = OpRuntimeMetrics(self) | ||
|
|
||
| # reduce | ||
| self.reduce_bar = None | ||
| if self.reduce_name is not None: | ||
| self.reduce_bar, position = _create_sub_pb( | ||
| self.reduce_name, self.num_output_rows_total(), position | ||
| ) | ||
| progress_bars_created += 1 | ||
| self.reduce_bar, position = _create_sub_pb( | ||
| self.reduce_name, self.num_output_rows_total(), position | ||
| ) | ||
| progress_bars_created += 1 | ||
| self.reduce_metrics = OpRuntimeMetrics(self) | ||
|
|
||
| return progress_bars_created | ||
|
|
@@ -403,6 +407,27 @@ def close_sub_progress_bars(self): | |
| self.shuffle_bar.close() | ||
| self.reduce_bar.close() | ||
|
|
||
| def get_sub_progress_bar_names(self) -> Optional[List[str]]: | ||
| self._validate_sub_progress_bar_names() | ||
|
|
||
| # shuffle | ||
| self.shuffle_bar = None | ||
| self.shuffle_metrics = OpRuntimeMetrics(self) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this reinitializing the metrics and resetting the bar?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is called once upon initializing progress manager, not called at all when using the tqdm progress bar implementation. btw, when initializing the progress manager, the However, I do understand the naming of the function may cause confusion. Would you have some suggestions for an alternative name?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. some ideas that come into mind: |
||
|
|
||
| # reduce | ||
| self.reduce_bar = None | ||
| self.reduce_metrics = OpRuntimeMetrics(self) | ||
|
|
||
| return [self.shuffle_name, self.reduce_name] | ||
kyuds marked this conversation as resolved.
Show resolved
Hide resolved
kyuds marked this conversation as resolved.
Show resolved
Hide resolved
kyuds marked this conversation as resolved.
Show resolved
Hide resolved
kyuds marked this conversation as resolved.
Show resolved
Hide resolved
kyuds marked this conversation as resolved.
Show resolved
Hide resolved
kyuds marked this conversation as resolved.
Show resolved
Hide resolved
kyuds marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def set_sub_progress_bar(self, name, pg): | ||
| # No type-hints due to circular imports. `name` should be a `str` | ||
| # and `pg` should be a `SubProgressBar` | ||
| if self.shuffle_name is not None and self.shuffle_name == name: | ||
| self.shuffle_bar = pg | ||
| elif self.reduce_name is not None and self.reduce_name == name: | ||
| self.reduce_bar = pg | ||
|
|
||
kyuds marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def _derive_max_shuffle_aggregators( | ||
| total_cluster_resources: ExecutionResources, | ||
|
|
@@ -705,7 +730,7 @@ def _on_partitioning_done(cur_shuffle_task_idx: int): | |
| self.shuffle_metrics.on_task_finished(cur_shuffle_task_idx, None) | ||
|
|
||
| # Update Shuffle progress bar | ||
| self.shuffle_bar.update(i=input_block_metadata.num_rows) | ||
| self.shuffle_bar.update(increment=input_block_metadata.num_rows or 0) | ||
|
|
||
| # TODO update metrics | ||
| task = self._shuffling_tasks[input_index][ | ||
|
|
@@ -823,7 +848,7 @@ def _on_bundle_ready(partition_id: int, bundle: RefBundle): | |
|
|
||
| # Update Finalize progress bar | ||
| self.reduce_bar.update( | ||
| i=bundle.num_rows(), total=self.num_output_rows_total() | ||
| increment=bundle.num_rows() or 0, total=self.num_output_rows_total() | ||
| ) | ||
|
|
||
| def _on_aggregation_done(partition_id: int, exc: Optional[Exception]): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| from abc import ABC, abstractmethod | ||
| from typing import List, Optional | ||
|
|
||
|
|
||
| class SubProgressBarMixin(ABC): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So based on this implementation, we can only have 1 level of subprogress bars right? OOC, how would we extend this implementation for this example (tree reduction case): I know we don't implement this today, but we might for larger datasets.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. some potential approaches: Therefore, the "levels" of progress bars are going to just involve adding more indents when adding progress bars. Currently, the indent is hardcoded, but making variable indents is trivial. The big change would be in the ** note that this is assuming that the tree reduction case is implemented within a single operator (ie: single operator spawns the nested/tree subprogress bars), as discussed in offline |
||
| """Abstract class for operators that support sub-progress bars""" | ||
|
|
||
| @abstractmethod | ||
| def get_sub_progress_bar_names(self) -> Optional[List[str]]: | ||
| """ | ||
| Returns list of sub-progress bar names | ||
| This is used to create the sub-progress bars in the progress manager. | ||
| Note that sub-progress bars will be created in the order returned by | ||
| this method. | ||
| """ | ||
| ... | ||
|
|
||
| @abstractmethod | ||
| def set_sub_progress_bar(self, name, pg): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add typing |
||
| """ | ||
| Sets sub-progress bars | ||
| name: name of sub-progress bar | ||
| pg: SubProgressBar instance (progress_manager.py) | ||
| """ | ||
| # Skipping type-checking for circular imports | ||
| ... | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to set shuffle_bar to none and immediately overwrite it