-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Bugfix] Add LightningOptimizer parity test and resolve AMP bug #5191
Conversation
…PyTorchLightning/pytorch-lightning into bugfix/5165_enable_pl_optimizer
…PyTorchLightning/pytorch-lightning into bugfix/5165_enable_pl_optimizer
tests/trainer/optimization/test_parity_automatic_optimization.py
Outdated
Show resolved
Hide resolved
tests/trainer/optimization/test_parity_automatic_optimization.py
Outdated
Show resolved
Hide resolved
@@ -489,6 +489,10 @@ def optimizer_step(self, optimizer, opt_idx, batch_idx, train_step_and_backward_ | |||
'native PyTorch amp and lbfgs are not compatible.' | |||
' To request, please file a Github issue in PyTorch and tag @mcarilli') | |||
|
|||
if not isinstance(optimizer, LightningOptimizer): | |||
# wraps into LightingOptimizer only for running step | |||
optimizer = LightningOptimizer.to_lightning_optimizer(optimizer, self.trainer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tchaton you can move the if statement inside the to_lightning_optimizer
function to achieve idempotence and avoid code duplication.
…thub.com/PyTorchLightning/pytorch-lightning into bugfix/5165_enable_pl_optimizer_refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests are a bit complex, I don't really understand them but I was able to verify that they fail on the master branch with the wrong loss values so I suppose the scaling bug is fixed 👍
The test are performing parity comparison with vanilla training in several scenarios and making sure everything matches properly when using enable_pl_optimizer=True/False. We will need to add more parity test such as Apex, DDP modes, multi optimisers. Best, |
What does this PR do?
Fixes #5165
Fixes #5159
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃