Skip to content
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

progress: add main bars #3594

Merged
merged 4 commits into from
Apr 25, 2020
Merged

progress: add main bars #3594

merged 4 commits into from
Apr 25, 2020

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented Apr 5, 2020

@casperdcl casperdcl requested review from gurobokum and efiop April 5, 2020 14:05
@casperdcl casperdcl self-assigned this Apr 5, 2020
@casperdcl casperdcl added ui user interface / interaction enhancement Enhances DVC labels Apr 5, 2020
@casperdcl casperdcl changed the title progress: add main push/pull bar progress: add main bars Apr 5, 2020
@shcheklein shcheklein self-requested a review April 5, 2020 20:42
@shcheklein
Copy link
Member

Example: https://asciinema.org/a/YEsnExZypmhtq6k2aDSYi5nvr (start at 20th s)

Main progress bar looks great.

UI/UX comments, some possible optimizations (not sure if some of them are related this PR particularly cc @efiop @pmrowla):

  1. Querying cache happens two times, second time it blinks at some progress and moves on - quite confusing. This it probably related to the changes discussed with @pmrowla.
  2. Downloads. It's expected no progress bars for them yet. But they still blink. I would still try an option when we just don't clean the line for a while.
  3. Downloads - the last large file takes time to download, meaning that we see static blank lines at the end, sometimes 20 lines.
  4. Possible optimization. Btw, we should be sorting files we download/upload by their size before pass them into the queue - it can speedup pull/push significantly in some cases.
  5. Checkout - it's a total mess - I just don't understand it - sometimes two progress bars (one is at 0%), then one progress bar without description (?)
  6. I agree with one of the tickets that 1 added is very confusing. At the end.

@casperdcl
Copy link
Contributor Author

Was just about to post a recording; thanks.

  1. Downloads. It's expected no progress bars for them yet. But they still blink. I would still try an option when we just don't clean the line for a while.

There's no way to do this automatically - what if the last 50 jobs finish at exactly the same time? All will leave these messages behind. The only solution in that case would be to manually clear TERM_HEIGHT lines at the end of the ThreadPool. If you think that would be fine to do everywhere then let me know/open a new issue.

  1. Downloads - the last large file takes time to download, meaning that we see static blank lines at the end, sometimes 20 lines.

Same problem as before. Either we move running bars upwards when other things finish (old tqdm behaviour, lots of flickering), or we never move a displaying bar (current tqdm behaviour, no flickering but some blank spaces near the end when there are a few jobs left). The current state is not improvable for the same reason as above.

  1. Possible optimization. Btw, we should be sorting files we download/upload by their size before pass them into the queue - it can speedup pull/push significantly in some cases.

Yes, this should be a separate issue.

  1. Checkout - it's a total mess - I just don't understand it - sometimes two progress bars (one is at 0%), then one progress bar without description (?)

There's one main bar (initial description "Checkout") which updates its description based on the last successfully checked out file. If item to be checked out is large, a nested bar appears showing its progress.

  1. I agree with one of the tickets that 1 added is very confusing. At the end.

#1838 <- This one? (Also related: #2869; #1622.)

@shcheklein
Copy link
Member

Thanks for the explanation, @casperdcl !

Ok, I see. We can create a ticker to try to remove blank lines.

There's one main bar (initial description "Checkout") which updates its description based on the last successfully checked out file. If item to be checked out is large, a nested bar appears showing its progress.

Probably the most confusing part here is this switch from Checkout 0% to some file name. (similar to the ticket with Git). My 2cs on this - I would try keep the Checkout message stable (above, below, or in the progress bar itself). File names should appear somewhere (e.g. below) "Processing: ". So, it's similar to the nested case - always show the file below the main progress bar. In some cases with progress on its own, in some cases w/o.

@casperdcl
Copy link
Contributor Author

casperdcl commented Apr 5, 2020

@efiop why was #528 closed? Seems like we need to re-open it.

@shcheklein I just opened #3597.

@efiop
Copy link
Contributor

efiop commented Apr 6, 2020

@efiop why was #528 closed? Seems like we need to re-open it.

Was no longer relevant. Please feel free to reopen.

@shcheklein
Copy link
Member

Was no longer relevant. Please feel free to reopen.

I think it was not clear why was it not relevant anymore :)

@efiop
Copy link
Contributor

efiop commented Apr 7, 2020

@shcheklein Because it had had questionable effectiveness, that wasn't worth the effort. At least that is what our consensus was back then, IIRC. I am still not sure if it is worth it, so have to trust @casperdcl 's research 🙂

@casperdcl
Copy link
Contributor Author

Ok, it seems like there should have been a slight speed improvement. Anyway I think a secondary gain here is reducing whitespace between bars by ordering them in advance.

@shcheklein
Copy link
Member

Yeah, but if we have file sizes it's (for pull we don't have them yet, might need them for other tickets #3568 ) we can improve pretty significantly in certain cases for free. And indeed improve UI along the way.

@efiop
Copy link
Contributor

efiop commented Apr 9, 2020

Well, we might just use 1 progress bar as we've discussed earlier. Doing the sorting to compensate for flickery pbar sounds strange to me, I would probably reconsider single progress bar again. But could go with the proposed approach too

@efiop
Copy link
Contributor

efiop commented Apr 22, 2020

@casperdcl Any progress on this?

@casperdcl
Copy link
Contributor Author

@efiop Updated description just to be clear: a lot of discussion is probably for other issues. I think this can me merged as-is. Maybe need a new issue to collate things blocked on the import/get refactoring?

@casperdcl
Copy link
Contributor Author

ah srry didn't notice the merge conflicts :)

@efiop efiop merged commit b312895 into iterative:master Apr 25, 2020
@efiop
Copy link
Contributor

efiop commented Apr 25, 2020

@casperdcl No worries :) Btw, this indeed greatly improves the experience for pull/push/fetch. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhances DVC ui user interface / interaction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

progress: inconsistent nested format when no main bar
3 participants