Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Remove {running,accumulated}_loss #9372

Closed
carmocca opened this issue Sep 8, 2021 · 8 comments
Closed

[RFC] Remove {running,accumulated}_loss #9372

carmocca opened this issue Sep 8, 2021 · 8 comments

Comments

@carmocca
Copy link
Contributor

carmocca commented Sep 8, 2021

Proposed refactoring or deprecation

Remove the following code: a979944

Motivation

The running loss is a running window of loss values returned by the training_step. It has been present since the very beginning of Lightning and has become legacy code.

Problems:

  • Users are sometimes confused by the value when they don't know it's a running window and compare it to the actual loss value they self.loged.
  • Often users self.log their actual loss which makes them see two "loss" values in the progress bar.
  • To disable it, you have to override the get_progress_bar_dict hook which is inconvenient.
  • The running window configuration is opaque to the user as it's hard-coded in the TrainingBatchLoop.__init__.

Alternative:

Pitch

Remove the code, I don't think there's anything to deprecate here.

cc @awaelchli @ananthsub


If you enjoy Lightning, check out our other projects! ⚡

  • Metrics: Machine learning metrics for distributed, scalable PyTorch applications.

  • Flash: The fastest way to get a Lightning baseline! A collection of tasks for fast prototyping, baselining, finetuning and solving problems with deep learning

  • Bolts: Pretrained SOTA Deep Learning models, callbacks and more for research and production with PyTorch Lightning and PyTorch

  • Lightning Transformers: Flexible interface for high performance research using SOTA Transformers leveraging Pytorch Lightning, Transformers, and Hydra.

@carmocca carmocca added this to the v1.5 milestone Sep 8, 2021
@carmocca carmocca changed the title [RFC] Remove {running,accumulated}_loss [RFC] Remove {running,accumulated}_loss Sep 8, 2021
@SkafteNicki
Copy link
Member

@carmocca nothing yet, but just created PR Lightning-AI/torchmetrics#506 in torchmetrics that implements simple aggregation metrics (sum, mean, max, min, cat) :]

@ananthsub
Copy link
Contributor

+1 I'm in favor of this! Good find @carmocca !

@awaelchli
Copy link
Contributor

awaelchli commented Sep 9, 2021

I agree with the window accumulation for the regular loss, it's not really needed and the value is anyway not configurable.
However, I don't think its good to ask users to log the loss manually just to appear in the progress bar. Since automatic optimization is the default and the loss needs to be returned, Lightning should show it in the progress bar. Please keep this. The progress bar callback could take care of that.

What will we do with the loss accumulation for gradient accumulation phases? Will you also remove that?

@tchaton
Copy link
Contributor

tchaton commented Sep 9, 2021

Yes, sounds like a great idea !

@tchaton tchaton added the let's do it! approved to implement label Sep 9, 2021
@carmocca
Copy link
Contributor Author

carmocca commented Sep 9, 2021

I don't think its good to ask users to log the loss manually just to appear in the progress bar.

Almost all users (if not all?) are logging the loss explicitly already to include it in the loggers.

Since automatic optimization is the default and the loss needs to be returned, Lightning should show it in the progress bar

The progress bar and the concept of automatic optimization don't need to be linked like this. Also, it raises the question: "what about manual? do I need to return the loss there too?"
On the other hand, there's a clear relation between "logging" and showing the progress bar, given the presence of log(prog_bar=bool).

This goes with the theme of avoiding multiple ways to do the same thing.

The progress bar callback could take care of that.

It does take care of it already by getting values from progress_bar_metrics which are managed by the LoggerConnector.

What will we do with the loss accumulation for gradient accumulation phases? Will you also remove that?

Yes, the point is to show in the progress bar the same that the users will see when they open TensorBoard, whatever that is.

@jinyoung-lim
Copy link
Contributor

i can take a look at this issue if no one has started yet.

@carmocca
Copy link
Contributor Author

carmocca commented Oct 9, 2021

After some offline discussion, we decided to split this into separate PRs:

  1. (Agreed) Remove the running accumulation mechanism. The difference in how this average is computed becomes confusing for users when they compare it to the values they logged, we should probably use an average metric instead.
  2. (Up for debate): Remove the automatic inclusion of the loss in the progress bar by returning it from training_step.

The main arguments for (2) are:

  • The loss is returned for optimization, not visualization. Using this mechanism to automatically add it into the progress bar is somewhat of a leak of responsibility. This responsibility should belong to the LoggerConnector which the user interacts with by calling self.log()
  • The current loss mechanism is easy to opt-in (by returning the loss) but hard to opt-out (you need to subclass the progress bar and override a hook, then pass this instance to your trainer). The alternative of self.log would be easy to opt-in and opt-out (change a boolean value)
  • Most people self.log their loss already, and self.log is a widely known and used mechanism.
  • Back when the return-to-add mechanism was added, self.log did not exist which might explain why it was done that way.

Now, if (2) is approved, there are things we could do to improve the experience:
a. if the user logs something with "loss" in the key and prog_bar=True, then we disable the return mechanism
b. if the user logs something with "loss" in the key and prog_bar=False, we print an info message (only once) suggesting to set prog_bar=True

@carmocca carmocca modified the milestones: v1.5, v1.6 Nov 2, 2021
@rohitgr7
Copy link
Contributor

b. if the user logs something with "loss" in the key and prog_bar=False, we print an info message (only once) suggesting to set prog_bar=True

I think an info msg is not required. The user chooses not to show it in the prog_bar, so it seems reasonable that they don't want it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Waiting
Development

No branches or pull requests

7 participants