Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds the option of saving the last model on checkpoint #1908

Merged
merged 5 commits into from
May 25, 2020

Conversation

lgvaz
Copy link
Contributor

@lgvaz lgvaz commented May 20, 2020

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 to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Implements part of #1658 , option to always save the latest model.
Fixes #1926

@pep8speaks
Copy link

pep8speaks commented May 20, 2020

Hello @lgvaz! Thanks for updating this PR.

Line 259:111: E501 line too long (113 > 110 characters)

Comment last updated at 2020-05-22 03:44:38 UTC

@mergify mergify bot requested a review from a team May 20, 2020 21:33
@lgvaz
Copy link
Contributor Author

lgvaz commented May 20, 2020

I thought by reading the contributing guide that the max line length was 120, bot does not like that my line is 113, should I make the bot happy?

@awaelchli
Copy link
Contributor

awaelchli commented May 21, 2020

the bot warns after 110 characters and the test will fail for >120
the intention should not be to make the bot happy, but the reader of the code :))

@mergify mergify bot requested a review from a team May 21, 2020 15:00
@awaelchli
Copy link
Contributor

could you also add an entry in changelog?

@lgvaz
Copy link
Contributor Author

lgvaz commented May 21, 2020

Does the changelog makes sense?

@awaelchli awaelchli added the feature Is an improvement or enhancement label May 21, 2020
@mergify mergify bot requested a review from a team May 21, 2020 18:31
@Borda Borda added the ready PRs ready to be merged label May 21, 2020
@mergify mergify bot requested a review from a team May 21, 2020 18:49
@mergify mergify bot requested a review from a team May 21, 2020 23:51
Co-authored-by: Jeremy Jordan <13970565+jeremyjordan@users.noreply.github.com>
@justusschock
Copy link
Member

Do we want to expose this argument in the trainer?

@Borda
Copy link
Member

Borda commented May 22, 2020

Do we want to expose this argument in the trainer?

what do you mean, I think that it quite natural also save the last state especially when your training accidentally stops...

@lgvaz
Copy link
Contributor Author

lgvaz commented May 22, 2020

I think that it quite natural also save the last state

Is it a good idea to make save_last default to True?

@Borda
Copy link
Member

Borda commented May 22, 2020

I think that it quite natural also save the last state

Is it a good idea to make save_last default to True?

good point, probably False is better option since every save would slow down...
lets fix also #1926 in this PR :]

@lgvaz
Copy link
Contributor Author

lgvaz commented May 22, 2020

lets fix also #1926 in this PR :]

Should I do something or is it automatically going to close that once this is merged?

@Borda
Copy link
Member

Borda commented May 22, 2020

lets fix also #1926 in this PR :]

Should I do something or is it automatically going to close that once this is merged?

now it is linked and with merging this PR it will automatically close the linked issue

@williamFalcon williamFalcon merged commit 112dd5c into Lightning-AI:master May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Save checkpoint for the last epoch
7 participants