change progress bar backend to tqdm #2333
Conversation
casperdcl
changed the title
efiop
changed the title
There was a problem hiding this comment.
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
|
Ah just opened a new issue, at least in part due to tqdm/tqdm#795 |
|
incidentally WARNING: Some of the cache files do not exist neither locally nor on remote. Missing cache files |
Looks like it is due to redirection settings being changed on dvc.org. I'm looking into it. |
|
@casperdcl updated the url on master, please rebase. Pull should be working now. |
|
Right looks about done now. Had to release a new version of |
This comment has been minimized.
This comment has been minimized.
|
@casperdcl Once again, sorry for that one, it is WIP #2407 . Will merge soon. |
|
@casperdcl Should be fixed now. Sorry for that. |
|
will merge |
| return ret | ||
|
|
||
| return list(set(checksums) & set(self.all())) | ||
| pbar.update_desc("", 0) # clear path name description |
There was a problem hiding this comment.
Add .clear_desc()? This at least would make the comment not needed.
There was a problem hiding this comment.
probably need a better description though.. maybe pbar.update_desc("Cache lookup", 0)?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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 🙂
There was a problem hiding this comment.
Although, we can indeed deal with it on top. Ok.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@casperdcl Sure, please create a new PR for it.
Substitute current
progresswith a thin wrapper aroundtqdmdetails
most optional. Necessary starred (
*)*addprogress.Tqdm(tqdm.tqdm)*progress-aware logging*update/fix old testslogging.getLogger().getEffectiveLevel() >= WARNING?tqdmis very efficient and there are now at mostN_WORKERSbars which will be cleared upon completionif pbar.n > LARGE_FILE_SIZE: pbar.write(str(pbar)))*under which appears a per-file download thread bar*which is cleared away correctlyreview TODO list
*fix remaining0%bar whendvc pulling*increase description length to 25*remove MD5 descriptions*add units (file,md5,B) for all barsremoves
logger.ColorFormatter._progress_awareprogress.Progressprogress.ProgressCallbackprogress.progressprogress.CLEARLINE_PATTERNremote.azure.Callbackremote.http.ProgressBarCallbackremote.s3.Callbackremote.ssh.connection.percent_cbremote.ssh.connection.create_cbrepo.checkout.get_progress_callbacktests.func.test_checkout.TestCheckoutShouldHaveSelfClearingProgressBaradds
progress.Tqdm(tqdm.tqdm)progress.TqdmThreadPoolExecutor(concurrent.futures.ThreadPoolExecutor)logger.LoggerHandler.emitremote.base.RemoteBASE.get_files_numbermodifies
depends on
tqdm>=4.34.0(Release 4.34.0: nested closing,=CLI option syntax, docs & framework updates tqdm/tqdm#797 <- shifting nested/parallel bars when closing tqdm/tqdm#795)