Optimization docs #6907

akihironitta · 2021-04-08T22:54:30Z

What does this PR do?

Fixes #<issue_number> Follow-up of #6825 (comment). Also requested by the community in #5780.

Updated docs links (`23803e3`)

Optimization page
Current: https://pytorch-lightning.readthedocs.io/en/latest/common/optimizers.html
Updated: https://108739-178626720-gh.circle-artifacts.com/0/html/common/optimizers.html

LightningModule page
Current: https://pytorch-lightning.readthedocs.io/en/stable/common/lightning_module.html
Updated: https://108739-178626720-gh.circle-artifacts.com/0/html/common/lightning_module.html

Motivation

In my opinion, the current docs about optimization (especially about manual optimization) are somewhat messy. There are quite a number of tip/note/warning which are randomly distributed. Also, I found a few outdated/wrong examples in the docs, so I'll try to address them here.

Description of the changes

Restructure the optimization page
Verify that examples are correct
Other small fixes/improvements of typo, indent, ...

Before submitting

[n/a] 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?
[n/a] Did you make sure to update the documentation with your changes? (if necessary)
[n/a] Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
[n/a] Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

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:

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 🙃

akihironitta · 2021-04-09T08:11:38Z

docs/source/common/optimizers.rst

-        def closure():
-            # Only zero_grad on the first batch to accumulate gradients
-            is_first_batch_to_accumulate = batch_idx % 2 == 0
-            if is_first_batch_to_accumulate:
-                opt.zero_grad()


Removing this gradient accumulation in a closure here.

When using LBFGS-like optimizers which require a closure function to reevaluate the model, the closure always has to run zero_grad to make them work properly, so "gradient accumulation + closure" is not supported in PL as well as PyTorch, I think.

akihironitta · 2021-04-11T00:16:45Z

docs/source/common/optimizers.rst

-
----------
-
-Using the closure functions for optimization


I removed this section ("Using the closure functions for optimization") because

from 1.2.2, this isn't needed even when using LBFGS-like optimizers

It doesn't include zero_grad in the closure function, so this example is wrong anyway.

Borda

lgtm 🐰

awaelchli

love this doc update! thanks

docs/source/common/optimizers.rst

carmocca

Awesome work! We really needed this

docs/source/common/optimizers.rst

Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]>

carmocca · 2021-04-15T13:58:42Z

docs/source/common/optimizers.rst

+   * ``self.optimizers()`` will return :class:`~pytorch_lightning.core.optimizer.LightningOptimizer` objects. You can
+     access your own optimizer with ``optimizer.optimizer``. However, if you use your own optimizer to perform a step,
+     Lightning won't be able to support accelerators and precision for you.
+   * Be careful where you call ``optimizer.zero_grad()``, or your model won't converge.
+     It is good practice to call ``optimizer.zero_grad()`` before ``self.manual_backward(loss)``.


Remove this now that we have it at the bottom?

Suggested change

* ``self.optimizers()`` will return :class:`~pytorch_lightning.core.optimizer.LightningOptimizer` objects. You can

access your own optimizer with ``optimizer.optimizer``. However, if you use your own optimizer to perform a step,

Lightning won't be able to support accelerators and precision for you.

* Be careful where you call ``optimizer.zero_grad()``, or your model won't converge.

It is good practice to call ``optimizer.zero_grad()`` before ``self.manual_backward(loss)``.

Be careful where you call ``optimizer.zero_grad()``, or your model won't converge.

It is good practice to call ``optimizer.zero_grad()`` before ``self.manual_backward(loss)``.

I would leave it in both manual and automatic optimization sections as one might only look at a section of their interest, but the decision is yours. Should I apply your suggestion here?

akihironitta · 2021-04-16T18:40:23Z

pytorch_lightning/core/lightning.py

-                    if batch_idx % 2 == 0 :
-                        optimizer.step(closure=optimizer_closure)
-                        optimizer.zero_grad()
+                    optimizer.step(closure=optimizer_closure)

-                # update discriminator opt every 4 steps
+                # update discriminator opt every 2 steps
                if optimizer_idx == 1:
-                    if batch_idx % 4 == 0 :
+                    if (batch_idx + 1) % 2 == 0 :


Just for the note, either optimizer needs to update its parameters every step. Otherwise, half of the training dataset will be ignored in this case...

akihironitta · 2021-04-16T18:41:24Z

pytorch_lightning/core/lightning.py

-        .. tip:: In manual mode we still automatically clip grads if Trainer(gradient_clip_val=x) is set
+        See :ref:`manual optimization<common/optimizers:Manual optimization>` for more examples.

-        .. tip:: In manual mode we still automatically accumulate grad over batches if


As pointed out by @rubencart, this tip is outdated.

akihironitta · 2021-04-16T19:03:20Z

docs/source/common/optimizers.rst

+.. warning::
+   * Before 1.3, Lightning automatically called ``lr_scheduler.step()`` in both automatic and manual optimization. From
+     1.3, ``lr_scheduler.step()`` is now for the user to call at arbitrary intervals.


The manual lr_scheduler.step PR (#6825) couldn't make it to 1.3? If so, this needs to be updated.

In my opinion they should both make it in
cc @edenlightning

Borda · 2021-04-18T09:39:10Z

@edenlightning @carmocca are we set here?

tchaton

LGTM !

.

8058a81

akihironitta added feature Is an improvement or enhancement docs Documentation related labels Apr 8, 2021

akihironitta added this to the 1.3 milestone Apr 8, 2021

akihironitta commented Apr 9, 2021

View reviewed changes

.

4477f46

akihironitta commented Apr 11, 2021

View reviewed changes

akihironitta added 2 commits April 11, 2021 09:26

Fix link to the section

05b6304

Fix link to the section

cf30741

akihironitta changed the title ~~Update optimization docs [WIP]~~ [RFC][WIP] Optimization docs Apr 11, 2021

akihironitta added 2 commits April 11, 2021 09:42

Consistent indent

1627da4

Update docs

43e8ee6

akihironitta changed the title ~~[RFC][WIP] Optimization docs~~ Optimization docs Apr 13, 2021

akihironitta marked this pull request as ready for review April 13, 2021 21:09

akihironitta requested review from Borda, awaelchli, edenlightning and tchaton as code owners April 13, 2021 21:09

Borda approved these changes Apr 13, 2021

View reviewed changes

Borda requested review from a team, SeanNaren and carmocca April 13, 2021 21:14

awaelchli approved these changes Apr 14, 2021

View reviewed changes

docs/source/common/optimizers.rst Show resolved Hide resolved

carmocca reviewed Apr 15, 2021

View reviewed changes

akihironitta and others added 2 commits April 15, 2021 11:31

Apply suggestions from code review

6e9bd4e

Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]>

Add note for optimizer.optimizer

29f70c0

akihironitta requested a review from carmocca April 15, 2021 02:47

carmocca reviewed Apr 15, 2021

View reviewed changes

carmocca approved these changes Apr 15, 2021

View reviewed changes

akihironitta marked this pull request as draft April 16, 2021 13:48

akihironitta added 5 commits April 17, 2021 03:03

.

5d6bb27

Update hooks

e0b3633

Update closure docstring

4512334

Update optimizer methods

eb9ecc3

Update optimizer

715bb2c

akihironitta commented Apr 16, 2021

View reviewed changes

akihironitta marked this pull request as ready for review April 16, 2021 19:03

akihironitta requested review from justusschock, kaushikb11 and williamFalcon as code owners April 16, 2021 19:03

akihironitta requested a review from carmocca April 16, 2021 19:03

akihironitta mentioned this pull request Apr 17, 2021

gradient_clip_val+manual_backward isn't working on PL1.2.1 #6328

Closed

Remove manopt + grad clipping (by @flukeskywalker)

23803e3

Borda added the ready PRs ready to be merged label Apr 18, 2021

carmocca approved these changes Apr 19, 2021

View reviewed changes

tchaton approved these changes Apr 19, 2021

View reviewed changes

tchaton enabled auto-merge (squash) April 19, 2021 14:07

tchaton added the _Will label Apr 19, 2021

lexierule disabled auto-merge April 19, 2021 14:08

lexierule merged commit d1529c2 into master Apr 19, 2021

lexierule deleted the docs/clean-optimization branch April 19, 2021 14:08

akihironitta mentioned this pull request Apr 19, 2021

Disable lr_scheduler.step() in manual optimization #6825

Merged

11 tasks

Optimization docs #6907

Optimization docs #6907

Uh oh!

Conversation

akihironitta commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Updated docs links (23803e3)

Motivation

Description of the changes

Before submitting

PR review

Did you have fun?

Uh oh!

akihironitta Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akihironitta Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Borda left a comment

Choose a reason for hiding this comment

Uh oh!

awaelchli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carmocca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carmocca Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

akihironitta Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

akihironitta Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

akihironitta Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

akihironitta Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

awaelchli Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

Borda commented Apr 18, 2021

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

akihironitta commented Apr 8, 2021 •

edited

Loading

Updated docs links (`23803e3`)

akihironitta Apr 9, 2021 •

edited

Loading

akihironitta Apr 11, 2021 •

edited

Loading