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: limit maximum running bars #3500

Merged
merged 3 commits into from
Mar 29, 2020
Merged

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented Mar 18, 2020

issue & fix:

(from #3453 (cc @JIoJIaJIu): script for reproducing issue & fix)

@casperdcl casperdcl added bug Did we break something? p1-important Important, aka current backlog of things to do ui user interface / interaction research labels Mar 18, 2020
@casperdcl casperdcl mentioned this pull request Mar 18, 2020
3 tasks
dvc/progress.py Outdated
@@ -35,6 +35,7 @@ class Tqdm(tqdm):
BYTES_DEFAULTS = dict(
unit="B", unit_scale=True, unit_divisor=1024, miniters=1
)
MAX_BARS = 20 # maximum parallel bars to display
Copy link
Contributor Author

@casperdcl casperdcl Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally auto-detect; see tqdm/tqdm@80be780:tqdm/utils.py#L263-L330 for inspiration

@casperdcl casperdcl requested a review from skshetry March 18, 2020 01:47
@efiop
Copy link
Contributor

efiop commented Mar 18, 2020

@casperdcl Could you show before/after capture, please?

@efiop
Copy link
Contributor

efiop commented Mar 18, 2020

@casperdcl Also, wouldn't it make sense to patch tqdm itself to tidy up unused bars as it goes?

@casperdcl
Copy link
Contributor Author

casperdcl commented Mar 18, 2020

Could you show before/after capture, please?

The script and asciinema recordings from @JIoJIaJIu (see description at the top here) still apply. I can re-generate them but they'd look identical.

Also, wouldn't it make sense to patch tqdm itself to tidy up unused bars as it goes?

This isn't about tidying up unused bars. It's about hiding running bars.

Specifically:

  • completed bars have never been an issue - they just print normally upon completion (assuming leave==True)
  • running disable=False bars could scroll off screen if there are more than SCREEN_HEIGHT of them running. This PR dynamically hides them.

By "dynamically" I mean:

  • completed leave=True bars would still print normally only after completion
  • when a displaying bar completes it may be replaced by one which was hidden

@casperdcl
Copy link
Contributor Author

The script and asciinema recordings from @JIoJIaJIu (see description at the top here) still apply. I can re-generate them but they'd look identical.

Actually not true. Updated:

@efiop
Copy link
Contributor

efiop commented Mar 18, 2020

@casperdcl By unused bars i meant blank lines, that make current pbars look like they are flickering in a mess. I like how the new approach looks like 👍 But maybe this should be done in tqdm itself, after all? Seems like a feature everyone could benefit from. To be honest, I'm not too deep into the implementation details here, so I'm simply judging by the looks.

@gurobokum
Copy link
Contributor

gurobokum commented Mar 18, 2020

Actually not true. Updated

But you launched with --tqdm flag that means you use library tqdm version.

I tried and noticed that it works! 🎉 And it look nicer from the code side
What interesting when I was working on the issue it was my first approach - usage display and clean methods, but it didn't help, but now I cannot reproduce it and everything works

One disadvantage - that it flicks too much. I think cause all pbars going up (pos -=1) on removing pbar. It would be better if we can just put hidden bar in the place of the closed one

@gurobokum gurobokum self-requested a review March 18, 2020 20:28
@casperdcl
Copy link
Contributor Author

But you launched with --tqdm flag that means you use library tqdm version.

Yes but at the end of the animation it's run again without the flag

@casperdcl
Copy link
Contributor Author

casperdcl commented Mar 18, 2020

It would be better if we can just put hidden bar in the place of the closed one

Interesting idea. I prefer moving up logically as newer bars at the bottom will be less complete and higher bars will be more complete - e.g. assuming similar file sizes:

bar10: #################
bar12: ###########
bar13: #######
bar14: ####

Rather than more randomly ordered:

bar14: ####
bar10: #################
bar13: #######
bar12: ###########

Maybe for a different issue/PR?

@casperdcl
Copy link
Contributor Author

Upstream issue for fixing this properly: tqdm/tqdm#918

@casperdcl
Copy link
Contributor Author

casperdcl commented Mar 20, 2020

updated with a ... (more hidden) ... message:

Think this is mergeable now as-is.

@casperdcl casperdcl requested a review from efiop March 20, 2020 13:37
@casperdcl casperdcl self-assigned this Mar 20, 2020
@efiop
Copy link
Contributor

efiop commented Mar 27, 2020

For the record: had a discussion about it last Sat and decided to wait for this functionality to become available in the upstream tqdm.

@casperdcl
Copy link
Contributor Author

... aaand new version!

@efiop efiop merged commit eeb6b60 into iterative:master Mar 29, 2020
casperdcl added a commit to casperdcl/dvc that referenced this pull request Apr 2, 2020
@casperdcl casperdcl mentioned this pull request Apr 2, 2020
5 tasks
casperdcl added a commit that referenced this pull request Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Did we break something? p1-important Important, aka current backlog of things to do research ui user interface / interaction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pull: going completely crazy on large dataset
3 participants