Deprecate progress_bar_refresh_rate from Trainer constructor

[daniellepintz]

What does this PR do?

Fixes #9500

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

• Was this discussed/approved via a GitHub issue? (not for typos and docs)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes? (if necessary)
• Did you write any new necessary tests? (not for typos and docs)
• Did you verify new and existing tests pass locally with your changes?
• Did you list all the breaking changes introduced by this pull request?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

• Is this pull request ready for review? (if not, please submit in draft mode)
• Check that all items from Before submitting are resolved
• Make sure the title is self-explanatory and the description concisely explains the PR
• Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

[ananthsub]

One of the roles that progress bar refresh rate is serving is to disable the progress bar by setting the rate to 0.

With this flag deprecated, we still need a way for users to disable this setting.

The pattern for features that are by default enabled has been to have a boolean flag for this. For example enable_progress_bar: bool = True on the trainer constructor makes this easy to toggle on/off.

This would match what we've done for checkpointing and what we will do for the model summary.

@awaelchli what do you think?

[daniellepintz]

In what cases would a user want to disable progress bar? Also, what do you think about #9500 (comment)

[ananthsub]

In what cases would a user want to disable progress bar? Also, what do you think about #9500 (comment)

1. user preference: in case people find it unhelpful or visually unappealing or spamming outputs for whatever reason, they should be able to disable it
2. unit tests: its never looked at there, so we could disable it for marginally faster tests and reduced log output in the tests

[tchaton]

One of the roles that progress bar refresh rate is serving is to disable the progress bar by setting the rate to 0.

With this flag deprecated, we still need a way for users to disable this setting.

The pattern for features that are by default enabled has been to have a boolean flag for this. For example enable_progress_bar: bool = True on the trainer constructor makes this easy to toggle on/off.

This would match what we've done for checkpointing and what we will do for the model summary.

@awaelchli what do you think?

I would be fine with this option, but to be consistent, let's call it progress_bar_callback: bool = True/False,

[daniellepintz]

sounds good I will add a boolean flag to enable the progress_bar. @tchaton I am not a big fan of calling it progress_bar_callback because then it sounds like the argument is literally a progress bar callback, and it is not clear that it is a boolean flag. I believe we have the same issue with checkpoint_callback, and I think it would make sense to change that flag to something like enable_checkpoint_callback

[tchaton]

sounds good I will add a boolean flag to enable the progress_bar. @tchaton I am not a big fan of calling it progress_bar_callback because then it sounds like the argument is literally a progress bar callback, and it is not clear that it is a boolean flag. I believe we have the same issue with checkpoint_callback, and I think it would make sense to change that flag to something like enable_checkpoint_callback

I agree. Ideally, we would have enable_checkpointing and enable_progress_bar

[Borda]

I am not much a fan of this deprecation I feel this shall stay in Trainer...
Progressbar is a basic feature, this pushed all users to learn and define special progress bar callback just for trivial adjustment... 🤒
cc: @PyTorchLightning/core-contributors

temp block for a day till I hear some more thoughts 🐰

[Borda]

I agree. Ideally, we would have enable_checkpointing and enable_progress_bar

what would be the logic then, how many progressBar callbacks you would have to define, assuming you have a bar per step in validation, training?

[daniellepintz]

@Borda please see my comment here for a summary of why I think this makes sense: #9500 (comment)

this pushed all users to learn and define special progress bar callback just for trivial adjustment

whether trivial or not, I think all adjustments should go in callbacks, otherwise the Trainer constructor gets cluttered with lots of different "trivial adjustments"

[daniellepintz]

what would be the logic then, how many progressBar callbacks you would have to define, assuming you have a bar per step in validation, training?

Not sure what you mean by multiple progress bar callbacks... I believe we only allow one. Once this deprecation is complete and we have enable_progress_bar (#9664), the logic will be if enable_progress_bar is True, create a default ProgressBar with a refresh_rate of 1 (20 if in Colab notebook) upon Trainer initialization.

.

[SeanNaren]

Great work @daniellepintz!