Fix scheduler reporting #30169
Fix scheduler reporting #30169
Conversation
| @@ -2255,17 +2255,21 @@ def _inner_training_loop( | |||
| # Optimizer step | |||
| self.optimizer.step() | |||
| optimizer_was_run = not self.accelerator.optimizer_step_was_skipped | |||
| # Store the current LR before stepping | |||
| learning_rate = self._get_learning_rate() | |||
There was a problem hiding this comment.
I had to modify the order slightly, as we need to grab the LR before scheduling, but we need to step() it before calling on_step_end
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| self._maybe_log_save_evaluate( | ||
| tr_loss, grad_norm, self._get_learning_rate(), model, trial, epoch, ignore_keys_for_eval | ||
| ) |
There was a problem hiding this comment.
Just for my own understanding - why is self._maybe_log_save_evaluate called twice in the training loop?
There was a problem hiding this comment.
First is at n steps, the second is at the end of the epoch
| self._maybe_log_save_evaluate( | ||
| tr_loss, grad_norm, learning_rate, model, trial, epoch, ignore_keys_for_eval | ||
| ) |
There was a problem hiding this comment.
Why not move this line above L2260? It seems it's logging a few values before there's any change to the global state e.g. epoch
There was a problem hiding this comment.
That changes any behavior if users want to modify something with on_step_end and fully adjusts when that callback is called. Seems far too reaching for this IMO :) We want that to be a thing
There was a problem hiding this comment.
(And if we do this, it will make some tests fail IIRC because reported vs expected changes)
There was a problem hiding this comment.
If I've understood correctly, this means that the elements being logged aren't all in sync i.e. some will be reporting values before a step and some after
There was a problem hiding this comment.
Yes, which I think for the scheduler is important because stepping it only reports what the next LR will be when grabbing, which is not what we want. We want the applied scheduler. For the rest not as much I think, I'll look back through some eval reports to make sure that it is just an isolated incident.
There was a problem hiding this comment.
Sorry, I don't think I've fully understood what should be happening.
Yes, which I think for the scheduler is important because stepping it only reports what the next LR will be when grabbing, which is not what we want. We want the applied scheduler.
Three questions:
- In terms of what we "want" - do you mean what we want to pass to
maybe_log_save_evaluatehere, or something else? - What is the difference between the scheduler and the applied scheduler?
- Why can't we use the applied scheduler
I think it would be good to have @pacman100 review this first, as he's more familiar with all of these objects
For the rest not as much I think, I'll look back through some eval reports to make sure that it is just an isolated incident.
Were you able to review reports and confirm behaviour is as expected?
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
muellerzr
added
the
WIP
What does this PR do?
Fixes the issue of the wrong iterations scheduler being reported and adds a test
Solves #28124
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@amyeroberts