[Data] Do not show AllToAll progress bar if the disable flag is set #45136
Conversation
jrosti
requested review from
ericl,
scv119,
c21,
amogkam,
scottjlee,
bveeramani,
raulchen,
stephanie-wang and
omatthew98
as code owners
Signed-off-by: Jari Rosti <jari@iprally.com>
jrosti
force-pushed
the
fix/progress-bars-visible
branch
from
b411547
to
1c94ebe
Compare
| enabled = is_all_to_all or ( | ||
| DataContext.get_current().enable_progress_bars and verbose_progress | ||
| progress_bar_enabled = DataContext.get_current().enable_progress_bars and ( | ||
| is_all_to_all or verbose_progress | ||
| ) | ||
| self.progress_bar = ProgressBar( |
There was a problem hiding this comment.
ProgressBar is bit confusing name because it has responsibilities in the monitoring lifecycle: e.g. if sub_progress_bars are not initialized the program does not work.
I'd change name to ProgressMonitor - which can then have progress-bar property as it currently has - I think it makes the intention bit more clear: it is not just a visual detail, but it tells the completion of the underlying operation which is then coupled to the control flow.
There was a problem hiding this comment.
Got it, I agree with you in general, I think it's a nit for now, so we can live with ProgressBar without updating too many places.
| # Initialize must be called for sub progress bars, even the | ||
| # bars are not enabled via the DataContext. | ||
| num_progress_bars += self.op.initialize_sub_progress_bars(index + 1) | ||
| return num_progress_bars if progress_bar_enabled else 0 |
There was a problem hiding this comment.
Original code wants to return number of enabled progress bars, although I think that number of created "ProgressBar" instances would be more meaningful.
I preserved original behaviour since I did not follow the calls if the visibility of the progress bar is really what client wants to know.
There was a problem hiding this comment.
yes, let's keep existing behavior.
c21
changed the title
…ay-project#45136) AllToAll main progress bar is shown even if progress bars are disabled. Signed-off-by: Jari Rosti <jari@iprally.com>
…ay-project#45136) AllToAll main progress bar is shown even if progress bars are disabled. Signed-off-by: Jari Rosti <jari@iprally.com>
…ay-project#45136) AllToAll main progress bar is shown even if progress bars are disabled. Signed-off-by: Jari Rosti <jari@iprally.com> Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
…ay-project#45136) AllToAll main progress bar is shown even if progress bars are disabled. Signed-off-by: Jari Rosti <jari@iprally.com>
Why are these changes needed?
AllToAll main progress bar is shown even if progress bars are disabled.
Related issue number
Closes #44770
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.