Move NaN/Inf detection to a separate utilities file #6834

ananthsub · 2021-04-05T07:54:04Z

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-04-05T07:54:10Z

Hello @ananthsub! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-08 16:42:06 UTC

codecov · 2021-04-05T08:29:56Z

Codecov Report

Merging #6834 (d34889b) into master (22a266d) will decrease coverage by 7%.
The diff coverage is 96%.

❗ Current head d34889b differs from pull request most recent head b7f4229. Consider uploading reports for the commit b7f4229 to get more accurate results

@@           Coverage Diff            @@
##           master   #6834     +/-   ##
========================================
- Coverage      91%     84%     -7%     
========================================
  Files         192     194      +2     
  Lines       12191   13129    +938     
========================================
- Hits        11145   11041    -104     
- Misses       1046    2088   +1042

awaelchli

maybe add a todo on top of the TrainingTricksMixin class that it should be removed in v1.5?

pytorch_lightning/utilities/nan.py

pytorch_lightning/trainer/training_tricks.py

carmocca · 2021-04-06T00:10:16Z

pytorch_lightning/utilities/nan.py

+log = logging.getLogger(__name__)
+
+
+def print_nan_gradients(model: nn.Module) -> None:


What do you think of these improvements? (my code)

https://github.com/jpuigcerver/PyLaia/blob/master/laia/utils/checks.py

Optional exception

Include non finite percentages

It could be useful for some users with unstable losses. Worth to consider.

I think it'd be useful! Maybe we can add it in a follow up PR? this one is mainly moving code around for parity

If you prefer, sure

tchaton

LGTM !

tchaton · 2021-04-06T07:07:08Z

pytorch_lightning/utilities/nan.py

+log = logging.getLogger(__name__)
+
+
+def print_nan_gradients(model: nn.Module) -> None:


It could be useful for some users with unstable losses. Worth to consider.

pytorch_lightning/trainer/training_loop.py

pytorch_lightning/trainer/training_tricks.py

pytorch_lightning/utilities/nan.py

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

tests/trainer/test_trainer.py

ananthsub force-pushed the nan-utils branch from 76c9cea to b0a49aa Compare April 5, 2021 07:54

[refactor] Move nan checks to a utilities file

04959f2

ananthsub force-pushed the nan-utils branch from b0a49aa to 04959f2 Compare April 5, 2021 07:59

ananthsub added 8 commits April 5, 2021 00:59

Update nan.py

7bd411e

Update nan.py

10b420a

Update nan.py

2147bff

Update nan.py

3720f88

Update training_tricks.py

b1ac088

Update training_tricks.py

06cf508

Update training_tricks.py

055f7b4

Update nan.py

68aa52f

Update CHANGELOG.md

ac61065

ananthsub changed the title ~~[WIP] Move NaN/Inf detection to a separate utilities file~~ Move NaN/Inf detection to a separate utilities file Apr 5, 2021

ananthsub marked this pull request as ready for review April 5, 2021 09:34

ananthsub requested review from awaelchli, Borda, carmocca, justusschock, SeanNaren, tchaton and williamFalcon as code owners April 5, 2021 09:34

awaelchli approved these changes Apr 5, 2021

View reviewed changes

awaelchli added the refactor label Apr 5, 2021

awaelchli added this to the 1.3 milestone Apr 5, 2021

ananthsub added 2 commits April 5, 2021 03:00

add test

c3e3790

Update training_tricks.py

9a1622a

ananthsub commented Apr 5, 2021

View reviewed changes

pytorch_lightning/utilities/nan.py Outdated Show resolved Hide resolved

carmocca reviewed Apr 6, 2021

View reviewed changes

tchaton approved these changes Apr 6, 2021

View reviewed changes

Borda reviewed Apr 6, 2021

View reviewed changes

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

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

pytorch_lightning/utilities/nan.py Outdated Show resolved Hide resolved

ananthsub and others added 3 commits April 7, 2021 12:29

Update pytorch_lightning/trainer/training_tricks.py

abf7fe0

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

Update pytorch_lightning/trainer/training_loop.py

1d3b8d6

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

raise value error

d34889b

ananthsub commented Apr 8, 2021

View reviewed changes

tests/trainer/test_trainer.py Show resolved Hide resolved

ananthsub added 2 commits April 8, 2021 09:40

rename to finite_checks

b7f4229

Update training_tricks.py

3292f59

carmocca approved these changes Apr 8, 2021

View reviewed changes

carmocca merged commit 851f9e3 into Lightning-AI:master Apr 8, 2021

This was referenced Apr 23, 2021

Move metrics_to_scalars to a dedicated utilities file #7180

Merged

Move grad_norm to a dedicated utilities file #7292

Merged

ananthsub mentioned this pull request May 7, 2021

Deprecate TrainerModelHooksMixin #7422

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move NaN/Inf detection to a separate utilities file #6834

Move NaN/Inf detection to a separate utilities file #6834

ananthsub commented Apr 5, 2021 •

edited

Loading

pep8speaks commented Apr 5, 2021 •

edited

Loading

codecov bot commented Apr 5, 2021 •

edited

Loading

awaelchli left a comment

carmocca Apr 6, 2021

tchaton Apr 6, 2021

ananthsub Apr 7, 2021

carmocca Apr 7, 2021

tchaton left a comment

tchaton Apr 6, 2021

		log = logging.getLogger(__name__)


		def print_nan_gradients(model: nn.Module) -> None:

Move NaN/Inf detection to a separate utilities file #6834

Move NaN/Inf detection to a separate utilities file #6834

Conversation

ananthsub commented Apr 5, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Apr 5, 2021 • edited Loading

Comment last updated at 2021-04-08 16:42:06 UTC

codecov bot commented Apr 5, 2021 • edited Loading

Codecov Report

awaelchli left a comment

Choose a reason for hiding this comment

carmocca Apr 6, 2021

Choose a reason for hiding this comment

tchaton Apr 6, 2021

Choose a reason for hiding this comment

ananthsub Apr 7, 2021

Choose a reason for hiding this comment

carmocca Apr 7, 2021

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

tchaton Apr 6, 2021

Choose a reason for hiding this comment

ananthsub commented Apr 5, 2021 •

edited

Loading

pep8speaks commented Apr 5, 2021 •

edited

Loading

codecov bot commented Apr 5, 2021 •

edited

Loading