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

update docs on checkpoint_callback Trainer argument #4461

Merged
merged 10 commits into from
Nov 2, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Oct 31, 2020

What does this PR do?

Fixes #4446
Adds docs update to deprecation introduced in #4336

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

@awaelchli awaelchli added the docs Documentation related label Oct 31, 2020
@awaelchli awaelchli added this to the 1.0.x milestone Oct 31, 2020
@pep8speaks
Copy link

pep8speaks commented Oct 31, 2020

Hello @awaelchli! Thanks for updating this PR.

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

Comment last updated at 2020-11-02 04:01:20 UTC

@awaelchli awaelchli changed the title Docs/checkpoint callback docs update docs on checkpoint_callback Trainer argument Oct 31, 2020
@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

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

@@           Coverage Diff           @@
##           master   #4461    +/-   ##
=======================================
+ Coverage      91%     93%    +2%     
=======================================
  Files         113     113            
  Lines        8397    8192   -205     
=======================================
- Hits         7655    7624    -31     
+ Misses        742     568   -174     

@rohitgr7
Copy link
Contributor

rohitgr7 commented Nov 1, 2020

what if both a checkpoint_callback is passed in callbacks and checkpoint_callback = True, should we raise a warning here?

@awaelchli
Copy link
Contributor Author

@rohitgr7 no, because checkpoint_callback = True is the default, so it would always lead to a warning if we did that. True is ignored if we pass in a callback through the list.

@rohitgr7
Copy link
Contributor

rohitgr7 commented Nov 1, 2020

@awaelchli not always I think. Only if someone passes a ModelCheckpoint explicitly in the callbacks.

@awaelchli
Copy link
Contributor Author

awaelchli commented Nov 1, 2020

@rohitgr7

# this is now a warning
Trainer(checkpoint_callback=ModelCheckpoint(...))  

# this should not throw a warning, because it is now the recommended way
Trainer(callbacks=[ModelCheckpoint(...)]) 

# equivalent
Trainer(checkpoint_callback=True (default), callbacks=[ModelCheckpoint(...)]) 

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.

oh okay got it, where the hell is my head 😅
just one more regarding the deprecation. @Borda said it's always +v0.2 while deprecating something but this thing is marked as deprecated in v1.1 and removed in v1.4

@awaelchli awaelchli merged commit 6ae4c6e into master Nov 2, 2020
@awaelchli awaelchli deleted the docs/checkpoint-callback-docs branch November 2, 2020 05:18
Borda pushed a commit that referenced this pull request Nov 4, 2020
* docs update

* update callbacks docs

* docs

* notebook examples

* warning

* line lenght

* update deprecation

Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>
Co-authored-by: Roger Shieh <55400948+s-rog@users.noreply.github.com>
(cherry picked from commit 6ae4c6e)
rohitgr7 pushed a commit that referenced this pull request Nov 21, 2020
* docs update

* update callbacks docs

* docs

* notebook examples

* warning

* line lenght

* update deprecation

Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>
Co-authored-by: Roger Shieh <55400948+s-rog@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkpoint docs update
5 participants