Deprecate `progress_bar_refresh_rate` from Trainer constructor

[daniellepintz]

Proposed refactoring or deprecation

Deprecate progress_bar_refresh_rate from Trainer constructor and use default refresh rate of 1 if no progress bar callback is passed

Motivation

We are trying to minimize the number of arguments on Trainer constructor: #9006
progress_bar_refresh_rate makes more sense to be part of the Progress Bar callback (in fact it already is). If someone wants to customize the progress bar refresh rate, they must create a Progress Bar callback, just like if they wanted to customize any other part of the the progress bar.

Pitch

Deprecate progress_bar_refresh_rate from Trainer constructor:
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/trainer/trainer.py#L117

Don't send progress_bar_refresh_rate here: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/trainer/trainer.py#L415
and update the relevant functions in callback_connector.py

Considerations

One implication of this is there will no longer be a way to specify no progress bar callback - one will always be created by default (currently if you dont want a progress bar you can set progress_bar_refresh_rate equal to 0, which is a bit hacky IMO). Do people see this as a problem?

[daniellepintz]

I see some similarities between progress_bar_refresh_rate, logger, and checkpoint_callback args of Trainer constructor where their purpose seems to be specifying the case where we don't want a progress bar, logger, and checkpointing, although the majority of the time we do want them. What if instead of these three arguments we had an argument called test_run or something like that which would ensure we don't create a progress bar, logger, and checkpointing callback? What about using fast_dev_run for this?

[ananthsub]

furthermore, progress_bar_refresh_rate is typed as an Optional[int] on the trainer constructor, which matches the expectation for the tqdm-based progress bar, for the frequency of updates in terms of steps: https://github.com/PyTorchLightning/pytorch-lightning/blob/ec828b826717cd3b5beabcb6d0cacf41b2320a98/pytorch_lightning/callbacks/progress/tqdm_progress.py#L101

But this is different from what the RichProgressBar expects:
https://github.com/PyTorchLightning/pytorch-lightning/blob/ec828b826717cd3b5beabcb6d0cacf41b2320a98/pytorch_lightning/callbacks/progress/rich_progress.py#L113

Where the refresh_rate is a float representing the number of updates per second.

This inconsistency is exactly why we ought to not have this argument on the trainer, as different progress bar callbacks can set this accordingly. It's the exact same reasoning as why #8991 around flushing for loggers needs to be configured on the logger and not on the trainer.

cc @kaushikb11

[williamFalcon]

Thank you for the PR! Although it looks like this simplifies the trainer constructor, it does it at a major complication in user experience.

In general, trainer constructors are “shortcuts” to not having to know anything about that thing other than you want to use it.

The number 1 driving principle of Lightning is “don’t make the user think”

This now drives us back in usability in a big way. Now here’s what will happen:

• A notebook user uses lightning
• the progress bar collapses their machine because it overwhelms the browser
• they try to figure out how to stop it from happening
• they figure out the refresh rate needs to be slower
• but they can’t just set it (ie: a usability shortcut), and now they have to hunt for how exactly this can be done
• they have to understand callbacks
• and understand the progressbar callback
• and understand that argument
• suddenly the user spent hours researching something that should have taken a few minutes.

the problems with this change are:

1. Most of us who write lightning don’t use notebooks much. thus we don’t innately relate the a huge portion of users who do... otherwise, we’d know that this is a huge issue for those users and one we spent months solving and arrived at this solution after much frustration from the community.
2. it violates the core design principle of not forcing the user to think.

In general, if a refactor takes a user experience that happens in 1 step and converts it to something that now happens in > 1 step, the refactors will not be accepted because it’s going in the opposite way. Refactors that affect the user experience need to simplify the user experience, not complicate it.

So, while the effort is well intentioned, i’m going to insist that this remain as is unless there is a proposal that makes the usability simpler, not more complicated.

cc @tchaton @edenafek

[daniellepintz]

Hi @williamFalcon thanks for your comment! just to clarify here, I am not proposing we get rid of these lines of code:

https://github.com/PyTorchLightning/pytorch-lightning/blob/0421f087428b339af7ac43c786c562b421e75b4f/pytorch_lightning/trainer/connectors/callback_connector.py#L119-L121

(We would probably just replace the if refresh_rate is None check with if no progress bar callback is passed)

If the user is using a notebook we would still absolutely make the default 20, rather than 1. My bad for not making that clear in the description.

Besides that, do you see any other issues with this proposal?

[tchaton]

Hey @daniellepintz,

I believe doing this automatically when running within a notebook (not colab GPU only) would be enough to depreceate this argument.

Best,
T.C

[daniellepintz]

sounds great! will work on it this week

[awaelchli]

@daniellepintz @ananthsub I think it's important to be able to turn off the progress bar. 1) for testing 2) some users or cloud providers redirect stdout to a file and progress bars don't print well there. progress_bar_refresh_rate=0 is working perfectly fine for that purpose, I don't see enough reason to remove or rename it.

[daniellepintz]

@awaelchli If we still want to be able to turn off the progress bar I think we should have a flag disable_progress_bar or something, but I do think it makes sense to deprecate progress_bar_refresh_rate from Trainer constructor for the following reasons:

1. I think one of the Lightning design principles is (or if not then I believe should be) to keep core functionality in Trainer and extra customizations in callbacks. progress_bar_refresh_rate would fit into the category of extra customization. This matches the approach taken in other issues like Deprecate flush_logs_every_n_steps on Trainer #8991
2. We already have a ProgressBar callback with a refresh_rate flag. It is confusing to have two different places where the progress bar refresh rate can be set. Someone might set the refresh rate in both places with different values, and not know which one will be respected.
3. Now that we have two different types of progress bar callbacks both with the refresh rate flag after the introduction of RichProgressBar, it can be even more confusing to the user. Because of the typing inconsistency mentioned by Ananth in Deprecate progress_bar_refresh_rate from Trainer constructor #9500 (comment), the user might try setting the progress_bar_refresh_rate to a float to create a RichProgressBar, but this will cause an error.

[awaelchli]

@daniellepintz as long as we don't have @williamFalcon's approval I don't think we can move forward unfortunately.
cc @ananthsub @tchaton

[daniellepintz]

I believe @williamFalcon @ananthsub and @tchaton had a sync last week and agreed it makes sense to do this issue

[tchaton]

Hey @awaelchli,

Yes, if the behaviour is respected and user still have an opt-out solution with progress_bar_callback=True/False, we should have very similar behaviour but less confusing API.

Best,
T.C