change progress bar backend to tqdm by casperdcl · Pull Request #2333 · treeverse/dvc

casperdcl · 2019-07-28T15:17:07Z

Substitute current progress with a thin wrapper around tqdm

fixes ui: better progress bars #2148 progress bar refactoring
fixes progress: flickers when updating to fast #2005 flickering/bottleneck progress
fixes Progress callback new line for long strings #2109 long-descriptions (also fixes progress: not clearing line correctly on small screens #2051)
fixes misc incorrect progress handling

details

most optional. Necessary starred (*)

review TODO list

* fix remaining 0% bar when dvc pulling
* increase description length to 25
- or truncate description from right?
have a fix bar width?
* remove MD5 descriptions
* add units (file, md5, B) for all bars

removes

adds

progress.Tqdm(tqdm.tqdm)
progress.TqdmThreadPoolExecutor(concurrent.futures.ThreadPoolExecutor)
logger.LoggerHandler.emit
remote.base.RemoteBASE.get_files_number

modifies

many things (refactoring)

depends on

tqdm>=4.34.0 (Release 4.34.0: nested closing, and = CLI option syntax, docs & framework updates tqdm/tqdm#797 <- shifting nested/parallel bars when closing tqdm/tqdm#795)

Suor

Hello, @casperdcl and welcome to the project! It looks way cleaner now) I've added a few comments below.

- progress.Progress - progress.ProgressCallback - progress.progress

casperdcl · 2019-08-17T23:30:34Z

Ah just opened a new issue, at least in part due to tqdm/tqdm#795

casperdcl · 2019-08-18T23:30:49Z

incidentally dvc pull fails now even on master with

WARNING: Some of the cache files do not exist neither locally nor on remote. Missing cache files

efiop · 2019-08-19T06:36:38Z

incidentally dvc pull fails now even on master with

Looks like it is due to redirection settings being changed on dvc.org. I'm looking into it.

efiop · 2019-08-19T06:44:01Z

@casperdcl updated the url on master, please rebase. Pull should be working now.

casperdcl · 2019-08-19T10:52:04Z

Right looks about done now. Had to release a new version of tqdm for this!

efiop · 2019-08-19T12:05:49Z

@casperdcl Once again, sorry for that one, it is WIP #2407 . Will merge soon.

efiop · 2019-08-19T12:25:11Z

@casperdcl Should be fixed now. Sorry for that.

treeverse#2333 (comment)

casperdcl · 2019-08-19T12:35:43Z

will merge master later (it does fix the test)

Suor · 2019-08-20T05:18:45Z

                return ret

-        return list(set(checksums) & set(self.all()))
+            pbar.update_desc("", 0)  # clear path name description


Add .clear_desc()? This at least would make the comment not needed.

probably need a better description though.. maybe pbar.update_desc("Cache lookup", 0)?

@casperdcl Btw, it actually seems pretty weird here, is it clearing the progress bars from batch_exists? You have another similar line pbar.update_desc("Checkout", 0) # clear path name description up above. Maybe pbar.finish() would be better? Is this line used to just leave the trace of the progress bar? If that is the case, we could definitely just drop them, those operations are kinda internal ones, so there is no real need to leave them(those leftover progress bars) in the final output, after they've already served the purpose of notifiying user about what was going on.

@efiop I wrote this to match the old behavior precisely, where the bar was left behind without any description (the callback would've injected various descriptions). The same is sort of true of the "Checkout" case, where I've replaced a description-less bar + "Checkout finished!" message with simply a completed bar labelled "Checkout".

Basically the new version is either identical to or slightly less verbose than the original.

Happy to change if you prefer.

@casperdcl I understand, and the idea for this PR was precisely that: to make a migration happen and then polish everything in the next PRs, but looks like making a few changes along the way is inevitable. If my understanding of those update_desc() described above is correct, then we could simply remove those lines and be done with this 🙂

Although, we can indeed deal with it on top. Ok.

Sure but to be more explicit

old version:

>>> progress_callback.finish("") [##############################] 100%

new version:

>>> pbar.update_desc("", 0) # clear path name description 100%|██████████| 3/3 [00:01<00:00, 1.96it/s]

if removed:

...f17c72d: 100%|██████████| 3/3 [00:01<00:00, 1.96it/s]

Actually (unrelated) there's a slight logic error (the line needs to be moved up a little before the function returns).

@efiop I made a minor change just after you merged. It's now rebased on master as e701221 (if you want to cherry-pick or something).

@casperdcl Sure, please create a new PR for it.

efiop

Thank you! Let's proceed with the UI on top of this.

casperdcl self-assigned this Jul 28, 2019

casperdcl changed the title ~~Tqdm~~ chane progress bar backend to tqdm Jul 28, 2019

casperdcl changed the title ~~chane progress bar backend to tqdm~~ change progress bar backend to tqdm Jul 28, 2019

casperdcl added the ui user interface / interaction label Jul 28, 2019

casperdcl requested review from efiop and shcheklein July 28, 2019 15:18

casperdcl added refactoring Factoring and re-factoring enhancement Enhances DVC labels Jul 28, 2019

efiop changed the title ~~change progress bar backend to tqdm~~ [WIP] change progress bar backend to tqdm Jul 28, 2019

Suor suggested changes Jul 29, 2019

View reviewed changes

Comment thread dvc/progress.py Outdated

Comment thread dvc/remote/base.py Outdated

Comment thread dvc/remote/s3.py Outdated

Comment thread dvc/repo/checkout.py Outdated

casperdcl added 20 commits August 10, 2019 14:11

add preliminary tqdm work

1511fd6

create dvc.progress.Tqdm, migrate dvc.remote.base

0ebce9c

disable old progress

70ee313

minor bugfix

c4b9d7d

tqdm repo.checkout

cc28155

tqdm remote.http

62cbeed

add progress.Tqdm(bytes)

6aee6b6

add progress.Tqdm(truncate)

3f0b85c

tqdm remote.s3, flake8

d460d7e

tqdm remote.azure

f61e7d8

add progress.Tqdm(desc_truncate)

85d4f7e

tqdm remote.ssh.connection

bac3d93

tqdm remote.local

efd8a07

tqdm dvc.utils

82bb550

remove old progress

d6f816b

- progress.Progress - progress.ProgressCallback - progress.progress

tqdm remote.oss

c02807d

add progress.Tqdm.update_desc(truncate=True)

ca68eb4

pylint

aed3451

black

9143f67

tqdm auto-disabling with dynamic log level

f88bf92

casperdcl added 2 commits August 18, 2019 00:19

Tqdm auto leave=False for nested bars

34dc163

Tqdm longer descriptions

9a2dea8

casperdcl added 2 commits August 18, 2019 23:43

update tqdm version

f90fd17

suppress MD5 description prefix

13e76de

casperdcl commented Aug 18, 2019

View reviewed changes

Comment thread dvc/repo/checkout.py

hopefully temp fix for uncalled checkout progress_callback

6102caa

casperdcl added 2 commits August 19, 2019 11:15

Merge remote-tracking branch 'upstream/master' into tqdm

1c8bf11

fix uncalled progress_callbacks

bee9064

fix logic

8f7a7a5

efiop reviewed Aug 19, 2019

View reviewed changes

Comment thread dvc/remote/base.py

This comment has been minimized.

Sign in to view

avoid duplication

9b73ff7

treeverse#2333 (comment)

Suor suggested changes Aug 20, 2019

View reviewed changes

casperdcl added 2 commits August 20, 2019 14:18

quick review fixes

8faa02e

get_files_number test fix

4bd2ce2

efiop approved these changes Aug 21, 2019

View reviewed changes

efiop merged commit 5aa9a2f into treeverse:master Aug 21, 2019

This was referenced Aug 21, 2019

minor unreachable code move #2422

Merged

progressbar cleanup #2434

Closed

progressbar cleanup #2436

Merged

casperdcl added the performance improvement over resource / time consuming tasks label Aug 27, 2019

jorgeorpinel mentioned this pull request Oct 18, 2019

docs: update/remove progress bar samples treeverse/dvc.org#707

Closed

Conversation

casperdcl commented Jul 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

details

review TODO list

removes

adds

modifies

depends on

Uh oh!

Suor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

casperdcl commented Aug 17, 2019

Uh oh!

Uh oh!

casperdcl commented Aug 18, 2019

Uh oh!

efiop commented Aug 19, 2019

Uh oh!

efiop commented Aug 19, 2019

Uh oh!

casperdcl commented Aug 19, 2019

Uh oh!

Uh oh!

This comment has been minimized.

efiop commented Aug 19, 2019

Uh oh!

efiop commented Aug 19, 2019

Uh oh!

casperdcl commented Aug 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Suor Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

casperdcl Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casperdcl Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Aug 21, 2019

Choose a reason for hiding this comment

Uh oh!

efiop Aug 21, 2019

Choose a reason for hiding this comment

Uh oh!

casperdcl Aug 21, 2019

Choose a reason for hiding this comment

Uh oh!

casperdcl Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

casperdcl commented Jul 28, 2019 •

edited

Loading

casperdcl Aug 20, 2019 •

edited

Loading

efiop Aug 21, 2019 •

edited

Loading

casperdcl Aug 21, 2019 •

edited

Loading

casperdcl Aug 21, 2019 •

edited

Loading

efiop Aug 21, 2019 •

edited

Loading