Progress bar callback #1450

awaelchli · 2020-04-11T00:11:13Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure to update the docs?
Did you write any new necessary tests?
If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Moves the progress bar from training code into a callback. This enables customization of progress bar display as desired in #765, #1386, #1121, #1500, Lightning-Universe/lightning-bolts#2
A custom progress bar callback can be passed like so:

# is on by default and can still turn off as before:
Trainer(progress_bar_refresh_rate=0)

# a custom bar
bar = ProgressBar(...)
Trainer(callbacks=[bar])

The progress bar callback is used by default. The tqdm is displayed exactly as before as far as I can tell. Did not test on ddp. If anyone could check, I would appreciate that.
Also had to add additional callback methods as proposed in More granular callbacks #1440 and No Callbacks for Validation Batch Step - How To Get Progress of Validation? #1165. I can split this into a different PR, let me know.
There is a base class and there is a ProgressBar class which is the progress bar we currently have. The base class allows the user to implement a progress bar that is not based on tqdm. The current implementation gives maximum flexibility while imposing minimal structure through the base class.
New tests that the progress gets updated correctly

This is a draft. There are some open questions I have for the core team:

I append the progress bar callback the list of callbacks. Is this the right way? Or should I call the individual methods in the training loop as we do for the the checkpoint and early stopping callbacks? (answered by @jeremyjordan)
Should the progress_bar_refresh_rate and process_position trainer args be deprecated and instead moved to the ProgressBar callback? Right now it is a hybrid.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

jeremyjordan · 2020-04-11T14:07:14Z

I append the progress bar callback the list of callbacks. Is this the right way? Or should I call the individual methods in the training loop as we do for the the checkpoint and early stopping callbacks?

Yes, this is the preferred way. The plan is to migrate the checkpoint and early stopping callbacks to do this as well.

pep8speaks · 2020-04-11T15:51:04Z

Hello @awaelchli! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-23 22:38:40 UTC

hadim · 2020-04-11T21:36:17Z

Would that make sense to modify the default progress bar by adding a global progress bar (while keeping the existing ones too)? What I mean by global is a progress bar over epochs.

tqdm can handle nested progress bars so that should not be an issue on that side. The question is more do you guys like it as a default?

awaelchli · 2020-04-11T23:24:53Z

@hadim I wouldn't be against it, just note that the training tqdm contains the epoch number in the description already. Anyway, given the current implementation, this would be super easy to add!

hadim · 2020-04-11T23:45:50Z

The thing I like about the "global" progress bar is the ETA.

awaelchli · 2020-04-12T16:21:57Z

@hadim I'd like that :)
@alexeykarnachev I have a doctest failing in this method. Do you know what I have to change in the argparse setup when adding a new arg to the Trainer?

awaelchli · 2020-04-13T18:31:02Z

@Borda should a new trainer arg be added at the end or before the "deprecated" block of args?

Borda · 2020-04-13T19:05:19Z

@Borda should a new trainer arg be added at the end or before the "deprecated" block of args?

before deprecated :]

awaelchli · 2020-04-13T19:07:10Z

thanks! I was not sure because there was one added after.

awaelchli · 2020-04-13T23:38:17Z

~~It's driving me crazy. Tests are fine. Then, I only change the refresh_rate=1 in this line of the test~~
https://github.com/PyTorchLightning/pytorch-lightning/blob/dd2d49b280c5f4d414bb54cd4cfc36f4daf291a5/tests/trainer/test_callbacks.py#L301
~~and it passes but then many OTHER tests start to fail!~~
~~How can it be that a single test can interfere with others? Anybody knows?~~
Edit: problem found and fixed. It originated from the problem in #1534.

codecov · 2020-04-13T23:38:46Z

Codecov Report

Merging #1450 into master will increase coverage by 0%.
The diff coverage is 95%.

@@           Coverage Diff           @@
##           master   #1450    +/-   ##
=======================================
  Coverage      88%     89%            
=======================================
  Files          68      69     +1     
  Lines        3956    4128   +172     
=======================================
+ Hits         3501    3665   +164     
- Misses        455     463     +8

justusschock

LGTM, although I'm not sure we really need the ProgressBarBase class. Can't we just integrate all of that to the tqdm progress bar and maybe rename the tqdm parts to be more neutral? So that one just has to overwrite these methods. This would provide a more rigid structure of how such a callback should be defined.

alexeykarnachev · 2020-04-14T09:28:19Z

@awaelchli Sorry, I missed your message.
the problem is still there?

awaelchli · 2020-04-14T11:46:54Z

@alexeykarnachev No, I was able to fix it. Thanks for checking!
@justusschock Thanks! I had it like this first, but I wanted to allow more flexibility. For example, let's say we wanted to implement a progress bar that doesn't use tqdm, then we would have to override almost every callback method to get "rid" of the base class's use of tqdm calls, even if we say wanted to implement a simple progress bar only for training progess. I thought, if we are anyway decoupling the tqdm bar from the Trainer class, why not allow the user to easily use a different progress bar library of their choice without having to think about our default implementation with tqdm.
Either way, this propably concerns <1% of users so I would not be against to change it like you said @justusschock.

What do the other core devs think?

justusschock · 2020-04-14T12:51:09Z

@awaelchli I think this is a valid argument. Tbh, I didn't think that much about these details, just thought that it is somewhat heavy to introduce a base class, when there is only one child class.

mergify · 2020-04-16T03:19:12Z