gradient verification callback #465

awaelchli · 2020-12-20T10:46:22Z

What does this PR do?

Fixes #194
Adds the requested callbacks for model verification.
It's copied from my personal repository.

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? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
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

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

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 🙃

pep8speaks · 2020-12-20T10:46:27Z

Hello @awaelchli! Thanks for updating this PR.

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

Comment last updated at 2021-01-18 08:36:55 UTC

codecov · 2020-12-20T10:46:34Z

Codecov Report

Merging #465 (224de45) into master (99232c3) will decrease coverage by 52.84%.
The diff coverage is 51.92%.

@@             Coverage Diff             @@
##           master     #465       +/-   ##
===========================================
- Coverage   78.95%   26.10%   -52.85%     
===========================================
  Files         105      107        +2     
  Lines        6121     6201       +80     
===========================================
- Hits         4833     1619     -3214     
- Misses       1288     4582     +3294

Flag	Coverage Δ
cpu	`26.10% <51.92%> (+0.44%)`	⬆️
pytest	`26.10% <51.92%> (+0.44%)`	⬆️
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pl_bolts/callbacks/data_monitor.py	`40.70% <0.00%> (-46.91%)`	⬇️
pl_bolts/callbacks/verification/base.py	`46.34% <46.34%> (ø)`
pl_bolts/callbacks/verification/batch_gradient.py	`54.23% <54.23%> (ø)`
pl_bolts/callbacks/__init__.py	`100.00% <100.00%> (ø)`
pl_bolts/utils/__init__.py	`100.00% <100.00%> (ø)`
pl_bolts/models/rl/common/agents.py	`0.00% <0.00%> (-100.00%)`	⬇️
pl_bolts/models/rl/dueling_dqn_model.py	`0.00% <0.00%> (-100.00%)`	⬇️
...l_bolts/models/rl/vanilla_policy_gradient_model.py	`0.00% <0.00%> (-96.37%)`	⬇️
pl_bolts/models/rl/double_dqn_model.py	`0.00% <0.00%> (-95.66%)`	⬇️
pl_bolts/models/rl/common/gym_wrappers.py	`0.00% <0.00%> (-91.60%)`	⬇️
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99232c3...ae63dad. Read the comment docs.

Borda

LGTM, just would be nice to have tests for uncovered lines...

docs/source/info_callbacks.rst

awaelchli · 2020-12-21T07:49:26Z

@Borda ok, I will try to make it higher

TylerYep · 2020-12-21T07:53:43Z

Thank you so much for implementing this!

akihironitta

@awaelchli I love this feature! I left some minor suggestions on typing and docs, so would you mind having a look at them? They should be resolved by clicking some buttons in your browser :]

docs/source/info_callbacks.rst

pl_bolts/callbacks/verification/base.py

pl_bolts/callbacks/verification/batch_gradient.py

Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>

…nto feature/gradient-verification

tchaton · 2021-01-09T17:16:26Z

Hey @awaelchli, why not push it directly to PL ?

awaelchli · 2021-01-09T17:36:30Z

Because @williamFalcon wants only essential callbacks in core PL library, and all non-essential, specialized callbacks here in bolts. Or at least this the information I have back from when pl_bolts was created, maybe it changed. I do not want to force this into any library unless team/community wants it.

tchaton · 2021-01-09T18:52:13Z

Because @williamFalcon wants only essential callbacks in core PL library, and all non-essential, specialized callbacks here in bolts. Or at least this the information I have back from when pl_bolts was created, maybe it changed. I do not want to force this into any library unless team/community wants it.

This looks like a great feature and we need to provide more callback examples to people.

It is up to you, but I see its value for PL.

awaelchli · 2021-01-09T23:18:26Z

This looks like a great feature and we need to provide more callback examples to people.
It is up to you, but I see its value for PL.

Thank you for the kind feedback. Ok, let me adress the remaining review comments here on the PR and I will double check with William.

awaelchli · 2021-01-09T23:23:25Z

pl_bolts/callbacks/verification/base.py

+        self.model = model
+
+    @abstractmethod
+    def check(self, *args: Any, **kwargs: Any) -> bool:


@akihironitta this mypy tool is not very smart :( It is not liking that the subclass has different args than here. I want this abstract method to be as general as possible and not specify concrete arguments and types. It should only act as an interface. Any suggestions how to proceed? I believe I have to add # type: ignore

I found a related issue in mypy repo, and it basically says that an easy workaround would be to add # type: ignore[override], so shall we just ignore it then?
python/mypy#1237 (comment)

no chance, it won't work. I tried to put it everywhere: at the top of method, on the same line as signature, below it, on top of the class, on top of the file, both in subclass and superclass.

I had to spam #type: ignore everywhere to make it work.
This mypy tool, I don't understand it. I spent hours now studying the docs of this tool trying to figure out what the error messages mean. I tried everything, but the type: ignore are unavoidable, yet they pollute the code unnecessarily. It's unbelievably frustrating, and I can no longer work on this, sorry.

I completely understand your frustration. Let's ignore them all for now.

pl_bolts/callbacks/verification/batch_gradient.py

tests/callbacks/verification/test_batch_gradient.py

awaelchli added callback enhancement New feature or request labels Dec 20, 2020

Borda requested review from williamFalcon and akihironitta December 20, 2020 22:59

Borda approved these changes Dec 20, 2020

View reviewed changes

docs/source/info_callbacks.rst Outdated Show resolved Hide resolved

awaelchli and others added 9 commits December 21, 2020 00:04

initial commit

b0e76b8

docs cleanup

44f8ade

isort

52a5134

black

64c389b

top level imports

8b8548e

rst docs

1b77ce5

update chlog

21cb8a1

isort again

b97a65a

format

9f2adcd

Borda force-pushed the feature/gradient-verification branch from c0d4f19 to 9f2adcd Compare December 20, 2020 23:05

akihironitta suggested changes Jan 3, 2021

View reviewed changes

awaelchli and others added 2 commits January 9, 2021 17:53

Merge branch 'master' into feature/gradient-verification

15a3240

Apply suggestions from code review

c7b610c

Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>

awaelchli requested review from ananyahjha93 and edenlightning as code owners January 9, 2021 17:12

awaelchli added 2 commits January 9, 2021 18:13

fix import

028be19

Merge remote-tracking branch 'origin/feature/gradient-verification' i…

188bf09

…nto feature/gradient-verification

increase coverage

e0dc1fb

stop complain

4ff9630

awaelchli commented Jan 9, 2021

View reviewed changes

awaelchli added 7 commits January 14, 2021 03:22

try type ignore

beba0b9

try ignore

af7db67

try ignore

15e8991

try ignore

6f52175

try ignore

b8ff39e

stupid mypy

590b722

stupid mypy

df8c746

Borda force-pushed the master branch from 8ea02ab to c4dbbfd Compare January 15, 2021 22:44

awaelchli added 3 commits January 16, 2021 02:06

stupid mypy

f4d9866

stupid mypi

791f4fa

stupid mypy

b6402e7

akihironitta self-requested a review January 16, 2021 05:57

akihironitta approved these changes Jan 16, 2021

View reviewed changes

awaelchli added the ready label Jan 18, 2021

awaelchli and others added 5 commits January 18, 2021 07:07

ugly yapf

2638e3e

Merge branch 'master' into feature/gradient-verification

4a1defc

yapf :(

3101e06

yapffffffff

84d98bb

chlog

224de45

Borda reviewed Jan 18, 2021

View reviewed changes

pl_bolts/callbacks/verification/batch_gradient.py Outdated Show resolved Hide resolved

tests/callbacks/verification/test_batch_gradient.py Outdated Show resolved Hide resolved

Borda and others added 2 commits January 18, 2021 09:35

Apply suggestions from code review

8439bd9

yapf

ae63dad

Borda merged commit 42cfa8f into master Jan 18, 2021

Borda deleted the feature/gradient-verification branch January 18, 2021 08:37

Borda added this to the v0.3 milestone Jan 18, 2021

This was referenced Mar 13, 2021

[tune](deps): Bump pytorch-lightning-bolts from 0.2.5 to 0.3.0 in /python/requirements suquark/ray#8

Closed

[tune](deps): Bump pytorch-lightning-bolts from 0.2.5 to 0.3.0 in /python/requirements sven1977/ray#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gradient verification callback #465

gradient verification callback #465

awaelchli commented Dec 20, 2020 •

edited

pep8speaks commented Dec 20, 2020 •

edited

codecov bot commented Dec 20, 2020 •

edited

Borda left a comment

awaelchli commented Dec 21, 2020

TylerYep commented Dec 21, 2020

akihironitta left a comment

tchaton commented Jan 9, 2021

awaelchli commented Jan 9, 2021

tchaton commented Jan 9, 2021

awaelchli commented Jan 9, 2021

awaelchli Jan 9, 2021

akihironitta Jan 12, 2021 •

edited

awaelchli Jan 14, 2021

awaelchli Jan 16, 2021

akihironitta Jan 16, 2021

gradient verification callback #465

gradient verification callback #465

Conversation

awaelchli commented Dec 20, 2020 • edited

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Dec 20, 2020 • edited

Comment last updated at 2021-01-18 08:36:55 UTC

codecov bot commented Dec 20, 2020 • edited

Codecov Report

Borda left a comment

Choose a reason for hiding this comment

awaelchli commented Dec 21, 2020

TylerYep commented Dec 21, 2020

akihironitta left a comment

Choose a reason for hiding this comment

tchaton commented Jan 9, 2021

awaelchli commented Jan 9, 2021

tchaton commented Jan 9, 2021

awaelchli commented Jan 9, 2021

awaelchli Jan 9, 2021

Choose a reason for hiding this comment

akihironitta Jan 12, 2021 • edited

Choose a reason for hiding this comment

awaelchli Jan 14, 2021

Choose a reason for hiding this comment

awaelchli Jan 16, 2021

Choose a reason for hiding this comment

akihironitta Jan 16, 2021

Choose a reason for hiding this comment

awaelchli commented Dec 20, 2020 •

edited

pep8speaks commented Dec 20, 2020 •

edited

codecov bot commented Dec 20, 2020 •

edited

akihironitta Jan 12, 2021 •

edited