[Bug-Fix]:properties `current_epoch` and `global_step` between model and trainer same always #3785

nrupatunga · 2020-10-02T02:25:10Z

What does this PR do?

This PR makes sure that properties current_epoch and global_step shared between the lightning module and the trainer remain the same always.

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?

Did you have fun?

Yes I had fun learning from Adrian 🙃

update to master central repo

… restore.

Update code from remote

update master

Use `raise .. from ..` to explicitly chain exceptions (Lightning-AI#3750)

[pytest]: test function to check common properties shared between lightning model and trainer

ananthsub · 2020-10-02T05:45:12Z

pytorch_lightning/core/lightning.py

@@ -122,6 +116,14 @@ def __init__(self, *args, **kwargs):
    def example_input_array(self) -> Any:
        return self._example_input_array

+    @property
+    def current_epoch(self):
+        return self.trainer.current_epoch if self.trainer else 0


should these be -1 if self.trainer isn't set? that could indicate that this is currently outside of training

This could be a nice addition but isn't trainer = None, when it isn't set and that could be an indication too?

have set it to 0 as previously it was self.current_epoch = 0 in __init__

pytorch_lightning/core/lightning.py

type check, better clarity Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

ydcjeff · 2020-10-02T06:43:06Z

Can you please re-add those properties comments?

Borda

mind share motivation of this addition since it is just one-to-one mapping to trainer attribute...?

tests/models/test_restore.py

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

nrupatunga · 2020-10-03T10:38:32Z

Yes, this doesn't affect users.
But I think if we mention in the docs, other users or newcomers can know this they are the same and don't need to ask questions or try comparing them.

What do you think?

@ydcjeff

Should I mention that self.current_epoch will be same as trainer.current_epoch. (similarly for global_step)

I was thinking if that information is really needed for the user to know? As the user will care only about if he could access those at any point in time and expect it to be right.

ydcjeff · 2020-10-03T10:46:13Z

I am suggesting that I prefer to have them in the docs. But, core contributors will decide to add it or not.

nrupatunga · 2020-10-03T10:48:21Z

I am suggesting that I prefer to have them in the docs. But, core contributors will decide to add it or not.

I agree, I think adding has an advantage that the user will have clarity that self.current_epoch = trainer.current_epoch

nrupatunga · 2020-10-03T10:51:47Z

@ydcjeff @ananthsub
Also, now adding change logs resulted in 4 failed check, Can you help me on why this might be happening

one of the logs:
Failed to fetch https://developer.download.nvidia.com/compute/machine-learning/repos/ubuntu1804/x86_64/Packages.gz File has unexpected size (47871 != 49498). Mirror sync in progress? [IP: 152.195.19.142 443]

Is there a way to rerun them

ydcjeff · 2020-10-03T10:56:20Z

Don't worry about the failed ones, these are unrelated to this PR.

awaelchli

👍 nice

CHANGELOG.md

Borda · 2020-10-04T09:19:24Z

Seems that some tests are failing...

tests/models/test_restore.py

rohitgr7 · 2020-10-04T13:31:02Z

tests/models/test_restore.py

+def test_resume_from_checkpoint(tmpdir):
+    """ Test that properties like `current_epoch` and `global_step`
+    in model and trainer are always the same. """


I think we should extend this test somehow to check for all the things that are required to be restored. Not here maybe.

nrupatunga and others added 10 commits October 1, 2020 18:36

Merge pull request #2 from PyTorchLightning/master

c4526e0

update to master central repo

make current_epoch and global_step to be same as trainer, after model…

330e7ac

… restore.

Merge pull request #3 from PyTorchLightning/master

8303f98

Update code from remote

Merge branch 'master' of https://github.com/nrupatunga/pytorch-lightning

f6b745f

remove assignment here

cc16f1b

Merge pull request #4 from PyTorchLightning/master

6d11e64

update master

test

2bda381

Merge pull request #5 from PyTorchLightning/master

411ae5e

Use `raise .. from ..` to explicitly chain exceptions (Lightning-AI#3750)

Merge pull request #6 from PyTorchLightning/bugfix/test-resume-step

1e092b3

[pytest]: test function to check common properties shared between lightning model and trainer

minor modification

45f256f

mergify bot requested a review from a team October 2, 2020 02:25

ananthsub reviewed Oct 2, 2020

View reviewed changes

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

ananthsub reviewed Oct 2, 2020

View reviewed changes

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

nrupatunga and others added 2 commits October 2, 2020 11:40

Update pytorch_lightning/core/lightning.py

f2192d5

type check, better clarity Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

Update pytorch_lightning/core/lightning.py

b6ace34

type check, better clarity Co-authored-by: ananthsub <ananth.subramaniam@gmail.com>

nrupatunga removed the request for review from a team October 2, 2020 06:13

mergify bot requested a review from a team October 2, 2020 06:13

nrupatunga removed the request for review from a team October 2, 2020 06:14

mergify bot requested a review from a team October 2, 2020 06:15

Borda added the bug Something isn't working label Oct 2, 2020

Borda reviewed Oct 2, 2020

View reviewed changes

tests/models/test_restore.py Outdated Show resolved Hide resolved

comments for current_epoch and global_step properties

3752f6b

mergify bot requested a review from a team October 2, 2020 06:53

Update tests/models/test_restore.py

6c082bb

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

nrupatunga requested review from Borda and removed request for a team October 2, 2020 07:07

mergify bot requested a review from a team October 2, 2020 07:08

update comments according to the changes made

e0fd2f8

nrupatunga requested review from awaelchli and removed request for a team October 3, 2020 16:50

mergify bot requested a review from a team October 3, 2020 16:51

nrupatunga removed the request for review from a team October 3, 2020 16:51

mergify bot requested a review from a team October 3, 2020 16:51

awaelchli approved these changes Oct 3, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

34d98f8

mergify bot requested a review from a team October 4, 2020 00:01

nrupatunga removed the request for review from a team October 4, 2020 02:31

mergify bot requested a review from a team October 4, 2020 02:31

awaelchli reviewed Oct 4, 2020

View reviewed changes

tests/models/test_restore.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team October 4, 2020 13:04

Update tests/models/test_restore.py

2e82757

rohitgr7 reviewed Oct 4, 2020

View reviewed changes

mergify bot requested a review from a team October 4, 2020 13:31

rohitgr7 approved these changes Oct 4, 2020

View reviewed changes

mergify bot requested a review from a team October 4, 2020 13:33

edenlightning added the v1.0 allowed label Oct 4, 2020

edenlightning added this to the 1.0 milestone Oct 4, 2020

justusschock approved these changes Oct 5, 2020

View reviewed changes

williamFalcon merged commit 7d47ed1 into Lightning-AI:master Oct 5, 2020

Borda modified the milestones: 1.0, 0.10.0 Oct 7, 2020

snyk-bot mentioned this pull request Mar 9, 2023

[Snyk] Upgrade axios from 0.25.0 to 0.27.2 slumericanBx/lightning#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug-Fix]:properties `current_epoch` and `global_step` between model and trainer same always #3785

[Bug-Fix]:properties `current_epoch` and `global_step` between model and trainer same always #3785

nrupatunga commented Oct 2, 2020 •

edited

Loading

ananthsub Oct 2, 2020 •

edited

Loading

nrupatunga Oct 2, 2020 •

edited

Loading

ydcjeff commented Oct 2, 2020

Borda left a comment

nrupatunga commented Oct 3, 2020

ydcjeff commented Oct 3, 2020

nrupatunga commented Oct 3, 2020 •

edited

Loading

nrupatunga commented Oct 3, 2020 •

edited

Loading

ydcjeff commented Oct 3, 2020

awaelchli left a comment

Borda commented Oct 4, 2020

rohitgr7 Oct 4, 2020

[Bug-Fix]:properties current_epoch and global_step between model and trainer same always #3785

[Bug-Fix]:properties current_epoch and global_step between model and trainer same always #3785

Conversation

nrupatunga commented Oct 2, 2020 • edited Loading

Before submitting

Did you have fun?

ananthsub Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

nrupatunga Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

ydcjeff commented Oct 2, 2020

Borda left a comment

Choose a reason for hiding this comment

nrupatunga commented Oct 3, 2020

ydcjeff commented Oct 3, 2020

nrupatunga commented Oct 3, 2020 • edited Loading

nrupatunga commented Oct 3, 2020 • edited Loading

ydcjeff commented Oct 3, 2020

awaelchli left a comment

Choose a reason for hiding this comment

Borda commented Oct 4, 2020

rohitgr7 Oct 4, 2020

Choose a reason for hiding this comment

[Bug-Fix]:properties `current_epoch` and `global_step` between model and trainer same always #3785

[Bug-Fix]:properties `current_epoch` and `global_step` between model and trainer same always #3785

nrupatunga commented Oct 2, 2020 •

edited

Loading

ananthsub Oct 2, 2020 •

edited

Loading

nrupatunga Oct 2, 2020 •

edited

Loading

nrupatunga commented Oct 3, 2020 •

edited

Loading

nrupatunga commented Oct 3, 2020 •

edited

Loading