[Data] Progress Reporting Refactor 2#59629
Conversation
Signed-off-by: kyuds <kyuseung1016@gmail.com>
Signed-off-by: kyuds <kyuseung1016@gmail.com>
Signed-off-by: Daniel Shin <kyuseung1016@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the progress bar system in Ray Data by introducing abstract base classes, BaseProgressBar and BaseExecutionProgressManager, to standardize progress reporting. Common progress bar logic, including block_until_complete and fetch_until_complete methods, along with utility functions like _extract_num_rows and truncate_operator_name, are moved into new dedicated _internal/progress modules. Existing progress bar implementations (ProgressBar, RichExecutionProgressManager, TqdmExecutionProgressManager) are updated to inherit from these new base classes, and type hints across various files are adjusted to reflect these changes. A review comment identified a bug in BaseProgressBar.fetch_until_complete where cancellation could lead to a KeyError due to an incomplete ref_to_result dictionary, suggesting that a RuntimeError should be raised instead of breaking the loop to handle cancellation explicitly.
| with _canceled_threads_lock: | ||
| if t in _canceled_threads: | ||
| break |
There was a problem hiding this comment.
The break statement here will cause a KeyError on line 87 if the operation is cancelled, because ref_to_result will not contain all the refs. This is a bug. To handle cancellation correctly, you should raise an exception. This will make the cancellation explicit to the caller and prevent the unhandled exception.
| with _canceled_threads_lock: | |
| if t in _canceled_threads: | |
| break | |
| with _canceled_threads_lock: | |
| if t in _canceled_threads: | |
| raise RuntimeError("The fetch operation was cancelled.") |
There was a problem hiding this comment.
this is as per original implementation of progress bars...
Signed-off-by: Daniel Shin <kyuseung1016@gmail.com>
## Description Follow up to ray-project#59350 - motivation: better abstraction for progress bars and type checking in general. ## Related issues N/A ## Additional information N/A --------- Signed-off-by: kyuds <kyuseung1016@gmail.com> Signed-off-by: Daniel Shin <kyuseung1016@gmail.com> Signed-off-by: jasonwrwang <jasonwrwang@tencent.com>
## Description Follow up to ray-project#59350 - motivation: better abstraction for progress bars and type checking in general. ## Related issues N/A ## Additional information N/A --------- Signed-off-by: kyuds <kyuseung1016@gmail.com> Signed-off-by: Daniel Shin <kyuseung1016@gmail.com>
## Description Follow up to ray-project#59350 - motivation: better abstraction for progress bars and type checking in general. ## Related issues N/A ## Additional information N/A --------- Signed-off-by: kyuds <kyuseung1016@gmail.com> Signed-off-by: Daniel Shin <kyuseung1016@gmail.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
Description
Follow up to #59350
Related issues
N/A
Additional information
N/A