Add warning to trainstep output #7779

justusschock · 2021-05-31T11:28:02Z

What does this PR do?

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 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 🙃

pep8speaks · 2021-05-31T11:28:05Z

Hello @justusschock! Thanks for updating this PR.

In the file pytorch_lightning/trainer/training_loop.py:

Line 276:18: W503 line break before binary operator

Comment last updated at 2021-06-04 07:07:41 UTC

for more information, see https://pre-commit.ci

codecov · 2021-05-31T11:29:04Z

Codecov Report

Merging #7779 (ccacbb9) into master (d47173b) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #7779   +/-   ##
======================================
- Coverage      93%     92%   -0%     
======================================
  Files         199     199           
  Lines       12985   13020   +35     
======================================
- Hits        12018   12017    -1     
- Misses        967    1003   +36

for more information, see https://pre-commit.ci

pytorch_lightning/trainer/training_loop.py

ethanwharris

LGTM 😃

pytorch_lightning/trainer/training_loop.py

Co-authored-by: Ethan Harris <ewah1g13@soton.ac.uk>

for more information, see https://pre-commit.ci

kaushikb11 · 2021-06-02T23:43:45Z

@justusschock Could you look into failing tests?

pytorch_lightning/trainer/training_loop.py

Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

for more information, see https://pre-commit.ci

kandluis · 2021-06-07T16:24:47Z

pytorch_lightning/trainer/training_loop.py

+        elif self.trainer.lightning_module.automatic_optimization:
+            if not any((
+                isinstance(training_step_output, torch.Tensor),
+                (isinstance(training_step_output, Mapping)
+                 and 'loss' in training_step_output), training_step_output is None
+            )):
+                raise MisconfigurationException(
+                    "In automatic optimization, `training_step` must either return a Tensor, "
+                    "a dict with key 'loss' or None (where the step will be skipped)."
+                )


How does this validation work with distributed training?

eg, I noticed a few examples where the loss is not calculated in training_step but is instead computed in training_step_end after collating the outputs of training_step.

Should we maybe swap L299 and L301?

cc @ananthsub

* Update training_loop.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update test_training_loop.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update test_training_loop.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update training_loop.py * Update pytorch_lightning/trainer/training_loop.py Co-authored-by: Ethan Harris <ewah1g13@soton.ac.uk> * Update test_training_loop.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update training_loop.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update pytorch_lightning/trainer/training_loop.py Co-authored-by: ananthsub <ananth.subramaniam@gmail.com> * Update training_loop.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update test_training_loop.py * Update training_loop.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * escape regex Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ethan Harris <ewah1g13@soton.ac.uk> Co-authored-by: ananthsub <ananth.subramaniam@gmail.com> (cherry picked from commit 6a0d503)

Update training_loop.py

f5e8000

[pre-commit.ci] auto fixes from pre-commit.com hooks

4269dc6

for more information, see https://pre-commit.ci

justusschock and others added 4 commits May 31, 2021 13:35

Update test_training_loop.py

987bbed

[pre-commit.ci] auto fixes from pre-commit.com hooks

ddfbb8d

for more information, see https://pre-commit.ci

Update test_training_loop.py

6542e5a

[pre-commit.ci] auto fixes from pre-commit.com hooks

9d7cc11

for more information, see https://pre-commit.ci

awaelchli reviewed May 31, 2021

View reviewed changes

pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved

carmocca reviewed May 31, 2021

View reviewed changes

pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved

Update training_loop.py

371c058

ethanwharris approved these changes Jun 1, 2021

View reviewed changes

ethanwharris reviewed Jun 1, 2021

View reviewed changes

pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved

justusschock and others added 3 commits June 1, 2021 13:31

Update pytorch_lightning/trainer/training_loop.py

7493747

Co-authored-by: Ethan Harris <ewah1g13@soton.ac.uk>

Update test_training_loop.py

7253cdd

[pre-commit.ci] auto fixes from pre-commit.com hooks

4c05db9

for more information, see https://pre-commit.ci

kaushikb11 approved these changes Jun 1, 2021

View reviewed changes

justusschock marked this pull request as ready for review June 2, 2021 05:38

justusschock requested review from Borda, SeanNaren, tchaton and williamFalcon as code owners June 2, 2021 05:38

justusschock and others added 2 commits June 2, 2021 11:09

Update training_loop.py

5034d5d

[pre-commit.ci] auto fixes from pre-commit.com hooks

6f09c2b

for more information, see https://pre-commit.ci

ananthsub suggested changes Jun 3, 2021

View reviewed changes

pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved

justusschock and others added 4 commits June 4, 2021 07:30

Update pytorch_lightning/trainer/training_loop.py

fa3a5b0

Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

Update training_loop.py

e39ee56

[pre-commit.ci] auto fixes from pre-commit.com hooks

4b62840

for more information, see https://pre-commit.ci

Update test_training_loop.py

7f5673a

justusschock and others added 3 commits June 4, 2021 08:30

Update training_loop.py

de9e282

[pre-commit.ci] auto fixes from pre-commit.com hooks

45b174d

for more information, see https://pre-commit.ci

escape regex

ccacbb9

justusschock enabled auto-merge (squash) June 4, 2021 07:36

justusschock requested a review from ananthsub June 4, 2021 07:36

ananthsub approved these changes Jun 4, 2021

View reviewed changes

justusschock merged commit 6a0d503 into master Jun 4, 2021

justusschock deleted the warning/trainstep_output branch June 4, 2021 15:34

kandluis reviewed Jun 7, 2021

View reviewed changes

SeanNaren mentioned this pull request Jun 8, 2021

1.3.x cherries 🍒 #7467

Closed

carmocca added this to the v1.3.x milestone Jun 17, 2021

carmocca added the bug Something isn't working label Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning to trainstep output #7779

Add warning to trainstep output #7779

justusschock commented May 31, 2021

pep8speaks commented May 31, 2021 •

edited

Loading

codecov bot commented May 31, 2021 •

edited

Loading

ethanwharris left a comment

kaushikb11 commented Jun 2, 2021

kandluis Jun 7, 2021

Add warning to trainstep output #7779

Add warning to trainstep output #7779

Conversation

justusschock commented May 31, 2021

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented May 31, 2021 • edited Loading

Comment last updated at 2021-06-04 07:07:41 UTC

codecov bot commented May 31, 2021 • edited Loading

Codecov Report

ethanwharris left a comment

Choose a reason for hiding this comment

kaushikb11 commented Jun 2, 2021

kandluis Jun 7, 2021

Choose a reason for hiding this comment

pep8speaks commented May 31, 2021 •

edited

Loading

codecov bot commented May 31, 2021 •

edited

Loading