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

remove deprecated early_stop_callback #3982

Merged
merged 1 commit into from
Oct 8, 2020
Merged

Conversation

Borda
Copy link
Member

@Borda Borda commented Oct 8, 2020

What does this PR do?

remove deprecated early_stop_callback

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?

Make sure you had fun coding 🙃

@Borda Borda added the refactor label Oct 8, 2020
@mergify mergify bot requested a review from a team October 8, 2020 08:24
@Borda Borda added this to the 1.0 milestone Oct 8, 2020
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #3982 into master will decrease coverage by 2%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #3982    +/-   ##
=======================================
- Coverage      89%     87%    -2%     
=======================================
  Files         216     216            
  Lines       15569   15600    +31     
=======================================
- Hits        13870   13619   -251     
- Misses       1699    1981   +282     

Copy link
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

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

Why is it removed BTW?

@mergify mergify bot requested a review from a team October 8, 2020 10:36
@awaelchli
Copy link
Contributor

@rohitgr7 because the 1) we can't guess the monitor key to use for early stopping, it is better for the user to explicitly set it. 2) there should be only one way to pass in callbacks (via callbacks arg)

@rohitgr7
Copy link
Contributor

rohitgr7 commented Oct 8, 2020

@awaelchli what about ModelCheckpoint? It's a callback too right?

@awaelchli
Copy link
Contributor

yes, but Lightning does checkpoints by default, and if we removed that argument then there would be no way to turn checkpointing off (i.e. checkpoint_callback=None)

@rohitgr7 rohitgr7 deleted the refactor/rm-depr-escall branch October 8, 2020 15:21
@chrismaliszewski
Copy link

So, should one pass ModelCheckpoint inside callbacks or still via checkpoint_callback?

@awaelchli
Copy link
Contributor

awaelchli commented Oct 22, 2020

sorry, missed your question.
Via callbacks list is the preferred way for all callbacks. Passing ModelCheckpoint in to callbacks "should" already work on the latest master branch. However there are some edge cases we need to sort out with this usecase. I am currently looking into this.
I personally recommend still using checkpoint_callback until ~v1.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants