Deprecate LightningModule.get_progress_bar_dict

[daniellepintz]

What does this PR do?

Deprecates get_progress_bar_dict from lightning module and moves it to progress bar callback
Fixes #8742

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 🙃

[codecov]

Codecov Report

Merging #8985 (52a6045) into master (a79c351) will decrease coverage by 4%.
The diff coverage is 96%.

@@           Coverage Diff           @@
##           master   #8985    +/-   ##
=======================================
- Coverage      92%     89%    -4%     
=======================================
  Files         178     179     +1     
  Lines       14899   14924    +25     
=======================================
- Hits        13752   13252   -500     
- Misses       1147    1672   +525     

[ananthsub]

I don't think we can deprecate the method in this manner. In fact, there's further cleanup we can do, which is kind of related to #8946

LightningModule.get_progress_bar_dict is called here: https://github.com/PyTorchLightning/pytorch-lightning/blob/6de66eb110a63d67dc2ebb74e149981cd93aa431/pytorch_lightning/trainer/properties.py#L296-L313

which is then used here inside the progress bar callback:

https://github.com/PyTorchLightning/pytorch-lightning/blob/6de66eb110a63d67dc2ebb74e149981cd93aa431/pytorch_lightning/callbacks/progress.py#L410-L437

Here's what I think we could do:

• Make the default implementation of LightningModule.get_progress_bar_dict return an empty dict
• Carry the existing implementation forward into a private utility function of the progress bar callback, which accepts a LightningModule and trainer as input args
• Deprecate progress_bar_metrics from the Trainer properties

@awaelchli

To justify these changes, we can argue this is only a temporary state and we will invest efforts in making the trainer less a god class in the future.

this is one of those ways we can start whittling away at this property list and make the trainer less of a god class

[daniellepintz]

@ananthsub thanks so much for the helpful feedback! A few questions

Make the default implementation of LightningModule.get_progress_bar_dict return an empty dict

1. That will make the functionality stop working in the next version, rather than leaving the functionality but giving dep warning, and then later removing it. Is that ok?

Carry the existing implementation forward into a private utility function of the progress bar callback, which accepts a LightningModule and trainer as input args

2. Does it have to take the trainer as an arg since it already has access to the trainer through the base class ProgressBarBase?
3. If it is a private function, how can I call it in properties.py?

Deprecate progress_bar_metrics from the Trainer properties

4. is that just on a separate note, because it isn't used?

[awaelchli]

I agree with @ananthsub , an intermediate function that merges the metrics from trainer and get_progress_bar_dict hook is probably the best here

[daniellepintz]

@ananthsub is this ready to be merged?

[tchaton]

@ananthsub is this ready to be merged?

Some conflicts to be resolved.

[tchaton]

LGTM !

[kaushikb11]

@daniellepintz Thank you for this PR! But blocking the merge for now, as I would need to take a look at the changes for RichProgressBar and test it locally and think of a better design. Will get back to it by tomorrow morning (IST)!

[kaushikb11]

Hi @daniellepintz, the changes were breaking for RichProgressBar callback. I have pushed some updates for it and the PR is good to go. We could have one more iteration to improve the MetricsTextColumn component. Will create an issue around it. Thanks!

[daniellepintz]

@kaushikb11 thanks for fixing that and sorry I didn't notice in the first place!