add types in bolts/callbacks #444

ydcjeff · 2020-12-11T06:42:31Z

What does this PR do?

Part of #434

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-11T06:42:36Z

Hello @ydcjeff! Thanks for updating this PR.

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

Comment last updated at 2021-01-04 11:08:33 UTC

ydcjeff · 2020-12-11T06:43:40Z

I need help with the failed mypy check.

codecov · 2020-12-11T06:46:52Z

Codecov Report

Merging #444 (798f68b) into master (efac971) will decrease coverage by 0.04%.
The diff coverage is 90.27%.

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
- Coverage   79.15%   79.10%   -0.05%     
==========================================
  Files         102      102              
  Lines        5906     5921      +15     
==========================================
+ Hits         4675     4684       +9     
- Misses       1231     1237       +6

Flag	Coverage Δ
cpu	`25.57% <65.27%> (+0.20%)`	⬆️
pytest	`25.57% <65.27%> (+0.20%)`	⬆️
unittests	`78.61% <90.27%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
pl_bolts/utils/shaping.py	`40.00% <ø> (+3.63%)`	⬆️
pl_bolts/callbacks/vision/confused_logit.py	`76.92% <66.66%> (-23.08%)`	⬇️
pl_bolts/callbacks/printing.py	`71.18% <69.23%> (+0.49%)`	⬆️
pl_bolts/callbacks/byol_updates.py	`100.00% <100.00%> (ø)`
pl_bolts/callbacks/data_monitor.py	`87.61% <100.00%> (-1.68%)`	⬇️
pl_bolts/callbacks/ssl_online.py	`96.42% <100.00%> (+0.13%)`	⬆️
pl_bolts/callbacks/variational.py	`95.91% <100.00%> (+0.26%)`	⬆️
pl_bolts/callbacks/vision/image_generation.py	`91.30% <100.00%> (ø)`
pl_bolts/utils/__init__.py	`100.00% <100.00%> (ø)`
pl_bolts/datasets/kitti_dataset.py	`32.69% <0.00%> (-1.93%)`	⬇️
... and 2 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 efac971...db6637d. Read the comment docs.

ydcjeff · 2020-12-11T06:50:34Z

matplotlib is needed for adding types in confused logits callbacks. how shall we do it?

pl_bolts/callbacks/byol_updates.py

pl_bolts/callbacks/data_monitor.py

setup.cfg

akihironitta · 2020-12-14T04:35:39Z

@ydcjeff

matplotlib is needed for adding types in confused logits callbacks. how shall we do it?

I'm adding matplotlib to the (optional) requirements in #450, so after having it merged, it should work without any issue I suppose :]

Borda · 2020-12-16T14:31:46Z

@ydcjeff can we resolve conflits?

.github/workflows/ci_test-base.yml

pl_bolts/callbacks/data_monitor.py

pl_bolts/callbacks/variational.py

pl_bolts/callbacks/vision/confused_logit.py

pl_bolts/callbacks/byol_updates.py

akihironitta

LGTM other than the below comment! Thank you for your work!

pl_bolts/callbacks/data_monitor.py

Co-authored-by: Akihiro Nitta <[email protected]>

Borda · 2021-01-03T11:20:45Z

the docs fail, probably we need to add matplotlib also to docs requirements

…lightning-bolts into types/callbacks

ydcjeff · 2021-01-03T15:14:36Z

I have tested locally, the docs build still fail even if installed matplotlib.
The thing is autodoc tracks _MATPLOTLIB_AVAILABLE to the _module_available function. so when we print _MATPLOTLIB_AVAILABLE, it shows pytorch_lightning.utilities._module_available, so resulting import warning.
any other solution in mind?

akihironitta

@ydcjeff Sorry for the delay! I looked through how other OSS projects deal with types of optional packages, and I found that, in Optuna, they assign object to the types (class names) when optional packages are unavailable. https://github.com/optuna/optuna/blob/392088c25d661b997c4143fd18ccdb7480ffa825/optuna/integration/pytorch_lightning.py#L4-L12

Do you think following this convention sound reasonable?

pl_bolts/callbacks/vision/confused_logit.py

Co-authored-by: Akihiro Nitta <[email protected]>

…lightning-bolts into types/callbacks

akihironitta

@ydcjeff lgtm. Thank you!

awaelchli · 2021-01-04T11:39:46Z

pl_bolts/callbacks/data_monitor.py

+            self,
+            trainer: Trainer,
+            pl_module: LightningModule,
+            batch: Sequence,


awaelchli · 2021-01-04T11:41:51Z

pl_bolts/callbacks/data_monitor.py

        self._train_batch_idx = batch_idx

-    def log_histograms(self, batch, group="") -> None:
+    def log_histograms(self, batch: Sequence, group: str = "") -> None:


awaelchli · 2021-01-04T11:45:01Z

pl_bolts/callbacks/data_monitor.py

        self._trainer = trainer

    def on_train_batch_start(
-        self, trainer, pl_module, batch, batch_idx, dataloader_idx
-    ):
+        self, trainer: Trainer, pl_module: LightningModule, batch: Sequence, batch_idx: int, dataloader_idx: int


for batch it is too strict, this could be almost anything (Any)

Borda · 2021-01-04T12:04:12Z

@ydcjeff mind send another PR addressing @awaelchli comments?

add types in bolts/callbacks

95cb8b1

flake8/pep8

660856d

Borda added the refactoring label Dec 11, 2020

Borda reviewed Dec 11, 2020

View reviewed changes

pl_bolts/callbacks/byol_updates.py Outdated Show resolved Hide resolved

pl_bolts/callbacks/data_monitor.py Outdated Show resolved Hide resolved

pl_bolts/callbacks/data_monitor.py Outdated Show resolved Hide resolved

Borda reviewed Dec 11, 2020

View reviewed changes

setup.cfg Show resolved Hide resolved

akihironitta mentioned this pull request Dec 14, 2020

Add missing optional packages to requirements/*.txt #450

Merged

8 tasks

akihironitta self-assigned this Dec 14, 2020

ydcjeff changed the title ~~add types in bolts/callbacks~~ blocked by #450 add types in bolts/callbacks Dec 14, 2020

ydcjeff changed the title ~~blocked by #450 add types in bolts/callbacks~~ [blocked by #450] add types in bolts/callbacks Dec 14, 2020

akihironitta mentioned this pull request Dec 15, 2020

Add type annotations #434

Open

17 tasks

Borda changed the title ~~[blocked by #450] add types in bolts/callbacks~~ add types in bolts/callbacks Dec 16, 2020

ydcjeff added 7 commits December 19, 2020 22:43

Merge remote-tracking branch 'upstream/master' into types/callbacks

d9601f3

merge master

3fd7e18

suggestions

9a985ff

type: ignore more specific

0a0fb72

type: ignore more specific

1a8288f

type: ignore more specific

e1f7dda

install matplotlib

6770bc3

ydcjeff requested a review from Borda December 20, 2020 17:21

install matplotlib

5eb8e20

Borda requested a review from akihironitta December 20, 2020 21:54

Borda approved these changes Dec 20, 2020

View reviewed changes

Borda reviewed Dec 20, 2020

View reviewed changes

pl_bolts/callbacks/byol_updates.py Outdated Show resolved Hide resolved

Apply suggestions from code review

f69a505

ydcjeff requested a review from ananyahjha93 as a code owner January 3, 2021 10:38

ydcjeff requested a review from akihironitta January 3, 2021 10:39

ydcjeff added 2 commits January 3, 2021 17:10

flake8 and isort

a9b6f72

install req

c1916fb

akihironitta reviewed Jan 3, 2021

View reviewed changes

pl_bolts/callbacks/data_monitor.py Outdated Show resolved Hide resolved

Apply suggestions from code review

3b4a7f3

Co-authored-by: Akihiro Nitta <[email protected]>

Borda requested a review from akihironitta January 3, 2021 11:17

Borda approved these changes Jan 3, 2021

View reviewed changes

ydcjeff added 2 commits January 3, 2021 17:54

install req

fe24fcc

Merge branch 'types/callbacks' of https://github.com/ydcjeff/pytorch-…

2099feb

…lightning-bolts into types/callbacks

ydcjeff requested a review from edenlightning as a code owner January 3, 2021 11:24

ydcjeff mentioned this pull request Jan 3, 2021

Add missing , to pl_bolts/models/self_supervised/__init__.py #492

Merged

8 tasks

akihironitta suggested changes Jan 4, 2021

View reviewed changes

pl_bolts/callbacks/vision/confused_logit.py Outdated Show resolved Hide resolved

akihironitta mentioned this pull request Jan 4, 2021

Adding types to some of datamodules #462

Merged

8 tasks

Jeff Yang and others added 5 commits January 4, 2021 16:57

Update pl_bolts/callbacks/vision/confused_logit.py

0ef7f82

Co-authored-by: Akihiro Nitta <[email protected]>

Merge remote-tracking branch 'upstream/master' into types/callbacks

928419b

isort

e2e8a36

Merge branch 'types/callbacks' of https://github.com/ydcjeff/pytorch-…

798f68b

…lightning-bolts into types/callbacks

flake8

db6637d

ydcjeff requested a review from akihironitta January 4, 2021 11:09

akihironitta approved these changes Jan 4, 2021

View reviewed changes

Borda merged commit ce3187d into Lightning-Universe:master Jan 4, 2021

ydcjeff deleted the types/callbacks branch January 4, 2021 11:23

awaelchli reviewed Jan 4, 2021

View reviewed changes

ydcjeff mentioned this pull request Jan 4, 2021

change to type Any for batch argument #495

Merged

8 tasks

akihironitta mentioned this pull request Jan 9, 2021

Tighten up mypy config #505

Merged

8 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add types in bolts/callbacks #444

add types in bolts/callbacks #444

ydcjeff commented Dec 11, 2020 •

edited by Borda

Loading

pep8speaks commented Dec 11, 2020 •

edited

Loading

ydcjeff commented Dec 11, 2020

codecov bot commented Dec 11, 2020 •

edited

Loading

ydcjeff commented Dec 11, 2020

akihironitta commented Dec 14, 2020

Borda commented Dec 16, 2020

akihironitta left a comment

Borda commented Jan 3, 2021

ydcjeff commented Jan 3, 2021

akihironitta left a comment

akihironitta left a comment

awaelchli Jan 4, 2021

awaelchli Jan 4, 2021

awaelchli Jan 4, 2021

Borda commented Jan 4, 2021

add types in bolts/callbacks #444

add types in bolts/callbacks #444

Conversation

ydcjeff commented Dec 11, 2020 • edited by Borda Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Dec 11, 2020 • edited Loading

Comment last updated at 2021-01-04 11:08:33 UTC

ydcjeff commented Dec 11, 2020

codecov bot commented Dec 11, 2020 • edited Loading

Codecov Report

ydcjeff commented Dec 11, 2020

akihironitta commented Dec 14, 2020

Borda commented Dec 16, 2020

akihironitta left a comment

Choose a reason for hiding this comment

Borda commented Jan 3, 2021

ydcjeff commented Jan 3, 2021

akihironitta left a comment

Choose a reason for hiding this comment

akihironitta left a comment

Choose a reason for hiding this comment

awaelchli Jan 4, 2021

Choose a reason for hiding this comment

awaelchli Jan 4, 2021

Choose a reason for hiding this comment

awaelchli Jan 4, 2021

Choose a reason for hiding this comment

Borda commented Jan 4, 2021

ydcjeff commented Dec 11, 2020 •

edited by Borda

Loading

pep8speaks commented Dec 11, 2020 •

edited

Loading

codecov bot commented Dec 11, 2020 •

edited

Loading