Skip to content

Conversation

@schwobr
Copy link

@schwobr schwobr commented Dec 6, 2019

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
  • Did you read the contributor guideline?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

As discussed in #556 I'd like to propose adding model hooks for training start and end. As I noticed a hook already exists for training start (on_sanity_check_start), I basically just renamed to on_train_start it as I find it clearer (they still coexist for now). If that is ok with you I could then add deprecation warnings for on_sanity_check_start. We could also let it be as it was if it is just me who finds it clearer that way.
I also added on_train_end that is executed at the end of the training loop just before logger experiment is closed. It would be a time to log weights, maybe reload best model or anything we could be wanting to do when training ends. I know it is a bit confusing with training_end that happens after each batch, but I didn't find a better name yet and I am obviously open to ideas.
I changed tests from on_sanity_check_start to on_train_start but didn't add any for on_train_end as I am not sure what test I could create.
As for the docs, it seems to me that model hooks are undocumented. I could try to document them as I find them very useful, but I am not sure how docs are written in this project.

I am obviously open to feedback, as this PR is pretty much a personal quality of life improvement, so I am not sure it has many use cases.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Of course !

@williamFalcon williamFalcon merged commit 2f01c03 into Lightning-AI:master Dec 7, 2019
def on_sanity_check_start(self):
"""
Called before starting evaluate
.. warning:: will be deprecated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shall add info when it will be removed, e.g. v0.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants