[Feat] Cleanup ModelCheckpoint / EarlyStopping by moving logic to LoggerConnector #5218

tchaton · 2020-12-21T14:29:32Z

What does this PR do?

This PR introduces MetricsHolder class which is responsible to hold metrics and their convert functions.

Fixes # (issue)

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
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? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

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; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

* add test * resolve bug * udpate test * wrongly copy / paste * update test * resolve a second bug Co-authored-by: Ubuntu <[email protected]>

pep8speaks · 2020-12-21T14:29:38Z

Hello @tchaton! Thanks for updating this PR.

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

Comment last updated at 2021-01-07 14:02:34 UTC

codecov · 2020-12-21T15:44:15Z

Codecov Report

Merging #5218 (b922c73) into release/1.2-dev (7464aca) will increase coverage by 4%.
The diff coverage is 92%.

@@               Coverage Diff                @@
##           release/1.2-dev   #5218    +/-   ##
================================================
+ Coverage               89%     93%    +4%     
================================================
  Files                  146     147     +1     
  Lines                10361   10423    +62     
================================================
+ Hits                  9220    9656   +436     
+ Misses                1141     767   -374

awaelchli · 2020-12-22T01:04:39Z

this is based on the 1.2 branch but unfortunately the Metrics code that needs cleanup is on master, and so this model checkpoint code will now diverge

tchaton · 2020-12-22T08:19:34Z

1.2 branch but unfortunately the Metrics code that needs cleanup is on master, and so this model checkpoint code will now diverge

Yes, I started a previous PR from master. But after reflection, it is better we don't introduce this kind of refactoring during Christmas vacation. I will move this back to master in JAN, when the entire team is back.

Best,
T.C

SkafteNicki

PR looking good.
@tchaton just to clarify, this is only refactor and no new features right?

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py

pytorch_lightning/core/step_result.py

tchaton · 2020-12-28T10:41:47Z

just to clarify, this is only refactor and no new features right?

Hey @SkafteNicki, it is mainly a refactor + a small feature (automatic casting when user access a metric dictionary).

Reason: Users had to add conversion check within callbacks which they shouldn't have to.

This PR adds automatic conversion only on accessing properties from the trainer. For performance reasons, I moved all the metrics to private, so they don't trigger the conversion.

Best,
T.C

tchaton · 2020-12-28T10:50:52Z

Hey @SkafteNicki, It enables having all the metrics displayed in the same format. Before we had a mix between tensors and numbers.

pytorch_lightning/trainer/connectors/logger_connector/metrics_holder.py

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py

tests/trainer/logging/test_logger_connector.py

pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py

pytorch_lightning/trainer/connectors/logger_connector/metrics_holder.py

justusschock

LGTM just one minor question regarding device.

justusschock · 2021-01-04T11:22:52Z

pytorch_lightning/callbacks/model_checkpoint.py

@@ -605,7 +602,7 @@ def _update_best_and_save(
            del_list.append(delpath)

        # do not save nan, replace with +/- inf
-        if torch.isnan(current):
+        if isinstance(current, torch.Tensor) and torch.isnan(current):
            current = torch.tensor(float('inf' if self.mode == "min" else '-inf'))


should this always be on cpu? or should it be on current.device?

Great question ! I am not sure. What do you think ?

IMO it should live on current.device, since all the other tensors (especially current if not nan) also live on this device.

pytorch_lightning/trainer/connectors/logger_connector/metrics_holder.py

…holder.py Co-authored-by: Roger Shieh <[email protected]>

…ng/pytorch-lightning into cleanup/metrics_2

tchaton and others added 3 commits December 21, 2020 14:12

[bug-fix] Metric reduction with Logging (#5150)

a48ca18

* add test * resolve bug * udpate test * wrongly copy / paste * update test * resolve a second bug Co-authored-by: Ubuntu <[email protected]>

iupdate

884a454

resolve bugs

f30de4c

tchaton changed the title ~~CleanUp metrics~~ [Feat] Cleanup ModelCheckpoint / EarlyStopping by moving logic to LoggerConnector Dec 21, 2020

Ubuntu and others added 2 commits December 21, 2020 14:31

add test back

d6bae34

correct flake8

303e85d

tchaton marked this pull request as ready for review December 21, 2020 15:00

tchaton requested review from awaelchli, Borda, justusschock, SeanNaren and williamFalcon as code owners December 21, 2020 15:00

tchaton self-assigned this Dec 21, 2020

tchaton added priority: 1 Medium priority task refactor labels Dec 21, 2020

tchaton added 2 commits December 22, 2020 09:20

Merge branch 'release/1.2-dev' into cleanup/metrics_2

c6ef2f1

resolve flake8

3af20fe

SkafteNicki reviewed Dec 23, 2020

View reviewed changes

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py Show resolved Hide resolved

pytorch_lightning/core/step_result.py Outdated Show resolved Hide resolved

tchaton and others added 2 commits December 28, 2020 11:51

update on comments

5d3d7ce

update tests

fe95959

awaelchli reviewed Dec 28, 2020

View reviewed changes

Borda reviewed Dec 28, 2020

View reviewed changes

Ubuntu and others added 2 commits December 28, 2020 16:33

add a test

bf2e78e

add test

89f901e

tchaton requested a review from awaelchli December 28, 2020 16:40

awaelchli approved these changes Dec 28, 2020

View reviewed changes

tests/trainer/logging/test_logger_connector.py Outdated Show resolved Hide resolved

update to Callable

f2ffa52

SkafteNicki approved these changes Dec 29, 2020

View reviewed changes

Borda reviewed Jan 4, 2021

View reviewed changes

pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py Show resolved Hide resolved

pytorch_lightning/trainer/connectors/logger_connector/metrics_holder.py Show resolved Hide resolved

justusschock approved these changes Jan 4, 2021

View reviewed changes

tchaton enabled auto-merge (squash) January 4, 2021 12:25

tchaton requested review from Borda, justusschock and awaelchli January 4, 2021 14:02

tchaton added the ready PRs ready to be merged label Jan 5, 2021

tchaton disabled auto-merge January 5, 2021 13:10

tchaton enabled auto-merge (squash) January 5, 2021 13:10

awaelchli approved these changes Jan 5, 2021

View reviewed changes

s-rog reviewed Jan 6, 2021

View reviewed changes

pytorch_lightning/trainer/connectors/logger_connector/metrics_holder.py Outdated Show resolved Hide resolved

tchaton and others added 2 commits January 7, 2021 11:07

Update pytorch_lightning/trainer/connectors/logger_connector/metrics_…

97c9e51

…holder.py Co-authored-by: Roger Shieh <[email protected]>

Merge branch 'release/1.2-dev' into cleanup/metrics_2

b922c73

SeanNaren approved these changes Jan 7, 2021

View reviewed changes

tchaton added 5 commits January 7, 2021 12:53

Merge branch 'release/1.2-dev' into cleanup/metrics_2

2696cf1

add changelog

46de65b

Merge branch 'cleanup/metrics_2' of https://github.com/PyTorchLightni…

3b37aa6

…ng/pytorch-lightning into cleanup/metrics_2

resolve flake8

2650ae2

Merge branch 'release/1.2-dev' into cleanup/metrics_2

a957d19

tchaton merged commit 5f94900 into release/1.2-dev Jan 7, 2021

tchaton deleted the cleanup/metrics_2 branch January 7, 2021 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat] Cleanup ModelCheckpoint / EarlyStopping by moving logic to LoggerConnector #5218

[Feat] Cleanup ModelCheckpoint / EarlyStopping by moving logic to LoggerConnector #5218

tchaton commented Dec 21, 2020 •

edited

Loading

pep8speaks commented Dec 21, 2020 •

edited

Loading

codecov bot commented Dec 21, 2020 •

edited

Loading

awaelchli commented Dec 22, 2020

tchaton commented Dec 22, 2020 •

edited

Loading

SkafteNicki left a comment

tchaton commented Dec 28, 2020

tchaton commented Dec 28, 2020 •

edited

Loading

justusschock left a comment

justusschock Jan 4, 2021

tchaton Jan 4, 2021

justusschock Jan 7, 2021

[Feat] Cleanup ModelCheckpoint / EarlyStopping by moving logic to LoggerConnector #5218

[Feat] Cleanup ModelCheckpoint / EarlyStopping by moving logic to LoggerConnector #5218

Conversation

tchaton commented Dec 21, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Dec 21, 2020 • edited Loading

Comment last updated at 2021-01-07 14:02:34 UTC

codecov bot commented Dec 21, 2020 • edited Loading

Codecov Report

awaelchli commented Dec 22, 2020

tchaton commented Dec 22, 2020 • edited Loading

SkafteNicki left a comment

Choose a reason for hiding this comment

tchaton commented Dec 28, 2020

tchaton commented Dec 28, 2020 • edited Loading

justusschock left a comment

Choose a reason for hiding this comment

justusschock Jan 4, 2021

Choose a reason for hiding this comment

tchaton Jan 4, 2021

Choose a reason for hiding this comment

justusschock Jan 7, 2021

Choose a reason for hiding this comment

tchaton commented Dec 21, 2020 •

edited

Loading

pep8speaks commented Dec 21, 2020 •

edited

Loading

codecov bot commented Dec 21, 2020 •

edited

Loading

tchaton commented Dec 22, 2020 •

edited

Loading

tchaton commented Dec 28, 2020 •

edited

Loading