[Data] Progress Bar: Sort sample in "rows" and remove the duplicate Sort sample.

[Bye-legumes]

Why are these changes needed?

Currently, the sort sample is not in rows and there is a duplicate sort sample progress bar.

With this modification, Sort sample will be also in rows and the additional progress bar will be removed.

In fact there should only one sort sample progress bar which is created at

ray/python/ray/data/_internal/planner/exchange/sort_task_spec.py

Line 166 in e066289

SortTaskSpec.SORT_SAMPLE_SUB_PROGRESS_BAR_NAME,

while the one created in

sub_progress_bar_names=[
                SortTaskSpec.SORT_SAMPLE_SUB_PROGRESS_BAR_NAME,
                ExchangeTaskSpec.MAP_SUB_PROGRESS_BAR_NAME,
                ExchangeTaskSpec.REDUCE_SUB_PROGRESS_BAR_NAME,
            ],

should be deleted.

Related issue number

Checks

• [√] I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• [√] I've run scripts/format.sh to lint the changes in this PR.
• [ √ I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • [√] Unit tests
  • [√] Release tests
  • This PR is not tested :(

[Bye-legumes]

@scottjlee With this fix, all progress bar (expect two when read the file) are replaced with rows. And the sort sample problem is also fixed.

[scottjlee]

could you consolidate this logic and the one in block_until_complete() as a static method in ProgressBar or a utility method in the file?

[scottjlee]

Thanks for looking into this. I think we actually want to keep this bar here, so that it is initialized as a sub-progress bar for the AbstractAllToAll op, and we should remove the one that is created in SortTaskSpec.sample_boundaries(). We want the "Sort Sample" to be a sub-bar of the overall Sort bar, like this:

In terms of the concrete change, we can update sample_boundaries() to reference the sub-progress bar that is part of the operator. This will also require us to either:

(1) change sample_boundaries from a static method to class method, and use the object's sub-progress bar
(2) add a sub_progress_bar parameter to sample_boundaries(), and at the callers of this method, pass the SortTaskSpec instance's progress bar (e.g. here)

[omatthew98]

Changes lgtm, can you upload a new image of what the progress bars look like with these changes + whatever script you used for the example for posterity?

[Bye-legumes]

Changes lgtm, can you upload a new image of what the progress bars look like with these changes + whatever script you used for the example for posterity?

Sure!, let me make it better and I will @ you and scott when I finished!

[Bye-legumes]

Hi, @scottjlee and @omatthew98 here is what look like now.
During exctuion

After

TOOD: There is one missing step that is: I tried different solutions but the sort sample bar doesn't update although it finished in fetch_until_complete. I double checked the _bar.total and progress in ray/python/ray/data/_internal/progress_bar.py can be update correctly, but it doesn't show.

But at least we have the sort sample inside the main now.

FYI
sample_bar.close() need to be deleted as otherwise, since the sort sample finished early, the sort sample bar will always above the main bar, and looks like this

[scottjlee]

@Bye-legumes Nice, the updated bars look great!

I tried different solutions but the sort sample bar doesn't update although it finished in fetch_until_complete.

For this part, which update are you referring to? In the "after" screenshot you shared, it looks like the Sort Sample bar updated progress to 100%, and shows numerator == denominator?

Also another small question/request. It looks like in the initially created progress bar for Sort Sample (in the "before execution" screenshot), it shows up as its own bar, and not a sub-bar of Sort, and it also shows the index of the bar (Sort Sample 2). This is later corrected by the time execution completes, in the "after" screenshot. Could you take a look at if the bar is being initialized incorrectly?

[Bye-legumes]

@Bye-legumes Nice, the updated bars look great!

I tried different solutions but the sort sample bar doesn't update although it finished in fetch_until_complete.

For this part, which update are you referring to? In the "after" screenshot you shared, it looks like the Sort Sample bar updated progress to 100%, and shows numerator == denominator?

Also another small question/request. It looks like in the initially created progress bar for Sort Sample (in the "before execution" screenshot), it shows up as its own bar, and not a sub-bar of Sort, and it also shows the index of the bar (Sort Sample 2). This is later corrected by the time execution completes, in the "after" screenshot. Could you take a look at if the bar is being initialized incorrectly?

1. During the excution, the sort sample is always 0/1 and cannot be update.
2. Yeah, that is the question that I left a TODO there. I tried to solve it. But in fact, I passed the sub progress bar to the and I checked the object number. The sub progress bar is passed to the sort sample with fetch_until_finished. But it will keeps as that before finished. Also when the bar starts before any updating. They will all has the number, but it becomes "*- {name} " after update (or set description later.)
   

[scottjlee]

I think what's going on with the 0/1 in Sort bar is that:

• When we initialize the sub-progress bar in initialize_sub_progress_bar(), the total is set to 1.
• After the Sort Sample finishes, we get the row count from the resulting output block, which updates the numerator and denominator accordingly.

So, I think we can initialize the sub progress bar (or call .update() with the updated total) using the n_samples from here.

Regarding the indices in the most recent screenshot you posted looks good actually, since it shows the operators in order with the indices. The description is initialized with <op_name> <position> in tqdm_ray here, then later updated in here. I think it's OK to leave this behavior as is for now. We can decide later if the indices appearing/disappearing is confusing behavior.

[Bye-legumes]

I think what's going on with the 0/1 in Sort bar is that:

• When we initialize the sub-progress bar in initialize_sub_progress_bar(), the total is set to 1.
• After the Sort Sample finishes, we get the row count from the resulting output block, which updates the numerator and denominator accordingly.

So, I think we can initialize the sub progress bar (or call .update() with the updated total) using the n_samples from here.

Regarding the indices in the most recent screenshot you posted looks good actually, since it shows the operators in order with the indices. The description is initialized with <op_name> <position> in tqdm_ray here, then later updated in here. I think it's OK to leave this behavior as is for now. We can decide later if the indices appearing/disappearing is confusing behavior.

OK. Maybe I can try to solve it later. I just fix any where it calls sort sample,i.e. in aggragate, so it's also a sub_progress bar now

[scottjlee]

hmm, if we are setting the unit as block here, how come the screenshot in #47106 (comment) shows the unit as row for the Sort Sample bars?

[scottjlee]

oh i see, this is only for the case where the bar is not passed from calling sample_boundaries. understood

[scottjlee]

i think if we use row as the unit here, it should still be supported right? since you implemented extract_num_rows() which will get the num_rows from blocks?

[scottjlee]

Thanks for the fix!