lengthen bars unless nested/threaded #2857
Conversation
Partially fixes iterative#2487
casperdcl
added
enhancement
|
|
Combining threaded actions into one would be even better for sure. This will be really big. Will make uploading and downloading dirs predictable. But what's the point aligning nested bars? These perform independent actions and don't need to be aligned. |
|
8 threads downloading independent files with progress bars on top of each other imho look bad when unaligned looks better than from random import randint, seed
from dvc.progress import Tqdm
seed(1337)
bars = []
fmt = (
"{percentage:3.0f}%|{bar}|"
"{desc} {n_fmt}/{total_fmt}"
" [{elapsed}<{remaining}, {rate_fmt:>11}{postfix}]"
)
#fmt = None
for i in range(8):
filename = "f" * randint(10, 50)
bars.append(Tqdm(desc=filename, total=10, leave=True, ncols=100, bar_format=fmt))
bars[-1].update(randint(1, 9))
bars[-1].refresh()
input() |
|
@casperdcl I think @Suor is bring up a good point. We've also discussed not using 1 progress bars for each file we are downloading, but instead using 1 progress bar for all of them, right? |
|
yes but that's a next step. for now (with the top-level bar for file count and then nested bars for bytes) I feel the nested ones should be aligned. |
|
@casperdcl E.g. showing the amount of data being downloaded instead of showing individual files. |
|
@casperdcl But we are going to remove those, why try to align them? |
|
Well they were already aligned, I'm just leaving them that way. This PR removes the 10-bar width on all top-level bars only. Making the nested bars look bad with the explanation that "we will one day soon remove them" doesn't sound like something which would make users happy. Would make devs happy. Would make my life easier. But not users. |
|
@casperdcl Thanks for clarifying! Looks like we won't get rid of all nested progress bars anyway, so this PR will still be useful later when we optimize most of the stuff. 👍 |
|
Waiting for bars to be combined is ok. What are other situations we have several pbars? Like add several things and calc checksums maybe. This looks ugly now anyway - misaligned and labels on different sides. |
Yes @Suor I agree. I'm still not happy about it either (even tried with 78c9e32) but couldn't come up with a nice solution.
|
|
One solution is to dynamically align them. You get the shortest pbar and redraw all the earlier ones to fit it, then go from where. a non-related thing, we obviously need to cap it, because text might be however long. Can we just cap it so that we don't get 10 char pbars at all? Or maybe we can set that minimum pbar dependent on overall width, like |
not sure I follow. Right now I cap the |
|
@casperdcl the formula might be more complex:
We may distribute the rest of the width between text and bar however we like, e.g.: free_width = width - fixed_stuff - numbers
# If free_width < 0 then we can't do much,
# let's suppose we always have free_width >= 30
# then we can use:
text_width = max(20, free_width * 0.3)
pbar_width = free_width - text_width # pbar is 10+, but generally 70% of free space.We can even have separate formula for damage control, e.g if free_width <= 15: # the very low limit for text
text_width = free_width
pbar_width = 0 # hide pbar, rely on %
elif free_width < 30:
text_width = ...
...
else:
# normal scenario |
|
@Suor I'm not sure it's wise to share space between Surely I'd prefer # truncate bar down to 10 min
bar_width = max(10, free_width - desc_width)
# truncate desc down to 0 min
desc_width = max(0, free_width - bar_width)
# truncate bar down to 0 min
bar_width = max(0, free_width - desc_width) |
I disagree. After say 40-50 chars desc is not that helpful. And bar is very important, it makes waiting less annoying. The very point of progress bar is bar, we don't call it progress description after all. |
|
Hmm I think we need more opinions on this one and then go with the majority... @efiop @shcheklein @MrOutis? Also regarding truncating |
|
Agreed with @casperdcl on this (though would love to see a GIF with it). It went into matter of personal preferences it looks like. I think the most important part is to show that something is happening + explain what exactly is happening. There a lot of ways to do the first part, and I'm not very opinionated unless it looks really bad. For example, I've shared recently https://asciinema.org/a/rUwhTMR06Rz7MIFEeFxFsjrHd which does not use classical progress bar at the last stage (multithreaded) at all. So, I'm fine with a (reasonable) number of progress bars and even if we sacrifice some width for now. Especially considering that we are going to think how to make one shared bar or something else as the next step, right? |
|
I agree with @casperdcl and @shcheklein 🙂 |
|
Combining threads into single bar is more important I agree on that. On bar and text, having 140 chars in a screen and only 10 chars for bar doesn't make sense to me at all. |
|
@Suor We have percentage, data and time counts, so having a high def progress bar is not vital there. |
|
@efiop This is not about vital, it's about ugly. If we can't make it > 10 chars then let's remove it. |
|
Then again, if we solve threading - by combining them into single pbar - then we should simply use default non-fixed tqdm pbars after that. Do you guys agree with that? |
Yeah, functionally it becomes pretty close to a spinner :) Though, small pbar is still more visually pleasant 🙂
I am not sure we will get rid of all pbars in favor of one, that is why this might still be useful later. But this optimization indeed feels a bit premature, even though it improves the look, but it is definitely not a game-changer. Maybe we could wait with it until we are done with combining progress bars and stuff. |
After that those will be some nested or unrelated pbars, which I think should not be aligned. Anyway, we may postpone this decision until after that. The one option is still to dynamically align them, just mentioning it to not get it lost. |
|
@Suor I don't see this PR as a final solution, it's just a patch for now that I think is better in all ways than the current version. I'm hesitant to try to fix the remaining issues until we see about combining bars later; as @efiop says it seems premature. I also don't see a bar of width 10 on a 300 character screen as ugly in the same way as people who have wide screens get used to 80 char code line lengths. I actually like the "simple important info" on the left and then the "advanced stats" on the right. For future maybe as you say a more fine grained approach is needed. In which case I'd suggest maybe: if wide_screen:
# truncate or pad to ensure desc is exactly 50
desc = "%50s" % (desc[:50])
# rest is bar (which will be the same fixed length for all bars)
bar_width = free_space - 50
elif narrow_screen:
bar_width = 0
desc_width = max(0, free_width)
else:
# truncate bar down to 10 min
bar_width = max(10, free_width - desc_width)
# truncate desc down to 0 min
desc_width = max(0, free_width - bar_width)
# truncate bar down to 0 min
bar_width = max(0, free_width - desc_width)But I feel that should be debated and implemented in a different PR - especially if we opt for an alternative as major as dynamic alignment. |
|
Btw, why are we only changing the proportions of the nested bars? Seems a bit odd looking to have some bars aligned and some not. |
|
Btw, talking about conda: (the image is taken from the google search, sorry) |
|
@efiop now they have https://github.com/conda/conda/blob/89ff48c2d20dad390ff9fa46b8e20aaff0d53d72/conda/common/io.py#L465:
producing: I guess they pre-compute the size in the Elsewhere they use the default format. |
|
@casperdcl Got it. @Suor I suggest we move on with this implementation and merge this PR, see how we feel about it and how it looks in further UI improvements. I think it looks nice enough to give it a try and the potential harm is very minor, so I don't see a point in fighting over it here. 🙂 |
issue:
fix: