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

Add LambdaCallback #5347

Merged

Conversation

marload
Copy link
Contributor

@marload marload commented Jan 4, 2021

What does this PR do?

Fixes #5343

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?
  • Did you make sure to update the documentation with your changes [if needed]?
  • Did you write any new necessary tests [no need for typos, docs]?
  • 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?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified
  • Check that target branch and milestone are aligned!

Did you have fun?

Make sure you had fun coding 🙃

@marload marload changed the title Feature/lambdacallback Add LambdaCallback Jan 4, 2021
@marload
Copy link
Contributor Author

marload commented Jan 4, 2021

@tchaton

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Great work ! Could you update the raise error by providing a better message.
Can you make sure it works with all callback functions (auto-generate them and assert they are being called)

pytorch_lightning/callbacks/lambda_cb.py Outdated Show resolved Hide resolved
@tchaton tchaton changed the base branch from master to release/1.2-dev January 4, 2021 13:02
@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #5347 (7ed3eea) into release/1.2-dev (dfbb592) will increase coverage by 0%.
The diff coverage is 94%.

@@               Coverage Diff                @@
##           release/1.2-dev   #5347    +/-   ##
================================================
  Coverage               93%     93%            
================================================
  Files                  146     152     +6     
  Lines                10315   10824   +509     
================================================
+ Hits                  9549   10044   +495     
- Misses                 766     780    +14     

@SkafteNicki
Copy link
Member

@marload could you please rebase on the release/1.2-dev branch as this PR right now contains a lot of changes from master

@tchaton tchaton changed the base branch from release/1.2-dev to master January 4, 2021 13:09
@Borda Borda force-pushed the feature/lambdacallback branch from 1a1ea36 to 7863e67 Compare January 4, 2021 13:14
@Borda Borda changed the base branch from master to release/1.2-dev January 4, 2021 13:17
@Borda Borda added this to the 1.2 milestone Jan 4, 2021
@Borda Borda added the feature Is an improvement or enhancement label Jan 4, 2021
@Borda
Copy link
Member

Borda commented Jan 4, 2021

Would you mind creating this PR from release/1.2-dev branch which is the feature branch ?

@marload I have fixed it so no action needed
cc: @tchaton @SkafteNicki

@Lightning-AI Lightning-AI deleted a comment from tchaton Jan 4, 2021
pytorch_lightning/callbacks/lambda_cb.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/lambda_cb.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/lambda_cb.py Outdated Show resolved Hide resolved
@marload
Copy link
Contributor Author

marload commented Jan 8, 2021

I should test the on_keyboard_interrrup and on_load_checkpoint hooks, but i dont know what to do. Who can help me?

@tchaton
Copy link
Contributor

tchaton commented Jan 9, 2021

Hey @marload ,

You have 1 test failing. Can you resolve it ?

@pep8speaks
Copy link

pep8speaks commented Jan 9, 2021

Hello @marload! Thanks for updating this PR.

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

Comment last updated at 2021-01-13 09:03:01 UTC

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.

@marload on_load_checkpoint was a bit tricky since to trigger that it requires on_save_checkpoint to return something. So tried something. Mind check?

tests/callbacks/test_lambda_cb.py Outdated Show resolved Hide resolved
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.

again, can you please rename the files? else LGTM.

docs/source/callbacks.rst Show resolved Hide resolved
@SkafteNicki SkafteNicki enabled auto-merge (squash) January 12, 2021 12:58
@SkafteNicki SkafteNicki merged commit 61f415f into Lightning-AI:release/1.2-dev Jan 13, 2021
Borda added a commit that referenced this pull request Jan 13, 2021
* Add LambdaCallback

* docs

* add pr link

# Conflicts:
#	CHANGELOG.md

* convention

* Fix Callback Typo

* Update pytorch_lightning/callbacks/lambda_cb.py

Co-authored-by: Nicki Skafte <[email protected]>

* Update pytorch_lightning/callbacks/lambda_cb.py

Co-authored-by: Nicki Skafte <[email protected]>

* Update pytorch_lightning/callbacks/lambda_cb.py

Co-authored-by: Nicki Skafte <[email protected]>

* use Misconfigureation

* update docs

* sort export

* use inspect

* string fill

* use fast dev run

* isort

* remove unused import

* sort

* hilightning

* highlighting

* highlighting

* remove debug log

* eq

* res

* results

* add misconfig exception test

* use pytest raises

* fix

* Apply suggestions from code review

Co-authored-by: Rohit Gupta <[email protected]>

* Update pytorch_lightning/callbacks/lambda_cb.py

Co-authored-by: Rohit Gupta <[email protected]>

* hc

* rm pt

* fix

* try fix

* whitespace

* new hook

* add raise

* fix

* remove unused

* rename

Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Nicki Skafte <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
Co-authored-by: chaton <[email protected]>
@marload marload deleted the feature/lambdacallback branch January 14, 2021 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
callback feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add LambdaCallback
8 participants