Adding types to some of datamodules #462

briankosw · 2020-12-18T11:02:26Z

What does this PR do?

Adding types to pl_bolts.datamodules.

related to #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-18T11:02:33Z

Hello @briankosw! Thanks for updating this PR.

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

Comment last updated at 2021-01-20 12:45:02 UTC

codecov · 2020-12-18T11:07:10Z

Codecov Report

Merging #462 (af563a5) into master (6b2136b) will increase coverage by 0.02%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
+ Coverage   77.53%   77.55%   +0.02%     
==========================================
  Files         114      114              
  Lines        6664     6671       +7     
==========================================
+ Hits         5167     5174       +7     
  Misses       1497     1497

Flag	Coverage Δ
cpu	`25.88% <76.92%> (+0.07%)`	⬆️
pytest	`25.88% <76.92%> (+0.07%)`	⬆️
unittests	`77.03% <84.61%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
pl_bolts/callbacks/byol_updates.py	`100.00% <ø> (ø)`
pl_bolts/callbacks/variational.py	`95.91% <ø> (ø)`
pl_bolts/datamodules/async_dataloader.py	`21.21% <80.00%> (+1.21%)`	⬆️
pl_bolts/datamodules/experience_source.py	`95.90% <100.00%> (ø)`
pl_bolts/models/self_supervised/amdim/datasets.py	`56.89% <0.00%> (-0.74%)`	⬇️
pl_bolts/datasets/__init__.py	`100.00% <0.00%> (ø)`

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 6b2136b...af563a5. Read the comment docs.

briankosw · 2020-12-19T18:37:37Z

A couple things that I need some guidance on:

Modules async_dataloader, experience_source, vocdetection_datamodule, and sklearn_datamodule have a few places where I wasn't able to infer the types.
Since torchvision is not a strict dependency, parameters and return values that rely on torchvision.transforms cannot have types. I considered adding Callable. What's the best practice here?

Borda · 2020-12-20T23:10:39Z

@briankosw how is it going, still wip or ready to review?

briankosw · 2020-12-21T01:09:50Z

@briankosw how is it going, still wip or ready to review?

Just need some guidance on the comment I've left above, but otherwise I'm done!

pl_bolts/datamodules/kitti_datamodule.py

pl_bolts/datamodules/binary_mnist_datamodule.py

…rch-lightning-bolts into types/datamodules

pl_bolts/datamodules/binary_mnist_datamodule.py

briankosw · 2020-12-22T22:55:54Z

A couple things that I need some guidance on:
1. Modules async_dataloader, experience_source, vocdetection_datamodule, and sklearn_datamodule have a few places where I wasn't able to infer the types.
2. Since torchvision is not a strict dependency, parameters and return values that rely on torchvision.transforms cannot have types. I considered adding Callable. What's the best practice here?

@akihironitta would love to get your feedback on these points!

Borda · 2021-01-02T21:25:49Z

@akihironitta mind re-review so we can land this one :]

akihironitta · 2021-01-04T08:53:58Z

@briankosw I'm sorry for replying very late.

Since torchvision is not a strict dependency, parameters and return values that rely on torchvision.transforms cannot have types. I considered adding Callable. What's the best practice here?

For types using optional packages, can you have a look at: #444 (review)?

akihironitta · 2021-01-04T09:08:34Z

Modules async_dataloader, experience_source, vocdetection_datamodule, and sklearn_datamodule have a few places where I wasn't able to infer the types.

For complicated ones, I guess, for now, we can just annotate them with Any as adding complicated types can lower readability of the code... @Borda Does it sound reasonable?

For functions returning self, let's use forward references as explained in PEP 484 (https://www.python.org/dev/peps/pep-0484/#forward-references). For example in our case, we can add types to AsynchronousLoader.__iter__ as the following:

class AsynchronousLoader(object):
    ...
    def __iter__(self) -> "AsynchronousLoader":
        # We don't want to run the thread more than once
        # Start a new thread if we are at the beginning of a new epoch, and our current worker is dead
        if (not hasattr(self, 'worker') or not self.worker.is_alive()) and self.queue.empty() and self.idx == 0:
            self.worker = Thread(target=self.load_loop)
            self.worker.daemon = True
            self.worker.start()
        return self

akihironitta · 2021-01-19T16:43:08Z

@Borda Sure.

akihironitta · 2021-01-19T21:52:32Z

This is a lot more work than I expected. Hopefully, I'll finish this PR today...

Borda · 2021-01-19T22:02:44Z

you can just pin the failing files so spilt the datamodiles into particular files and lets ignore the failing ones only

akihironitta · 2021-01-20T11:33:16Z

pl_bolts/datamodules/binary_mnist_datamodule.py

@@ -76,7 +78,7 @@ def __init__(
                "You want to use transforms loaded from `torchvision` which is not installed yet."
            )

-        super().__init__(
+        super().__init__(  # type: ignore[misc]


mypy will raise errors without these # type: ignore[misc] due to the bug reported in python/mypy#6799.

akihironitta · 2021-01-20T13:13:43Z

you can just pin the failing files so spilt the datamodiles into particular files and lets ignore the failing ones only

@Borda Some datamodule files are still included in the ignore list, but I think this PR is ready for review for now.

akihironitta · 2021-01-21T02:38:57Z

pl_bolts/datamodules/async_dataloader.py

+        # yapf: disable
+        if (not hasattr(self, 'worker') or not self.worker.is_alive()) and self.queue.empty() and self.idx == 0:  # type: ignore[has-type] # noqa: E501
            self.worker = Thread(target=self.load_loop)
+            # yapf: enable


Just for the record, I had a collision here between yapf and flake8. Lightning-AI/pytorch-lightning#5591

Applying yapf will cause flake8's error [W503] line break before binary operator if without these ignores.

briankosw · 2021-01-22T07:04:07Z

Thank you for finishing this up and my apologies @akihironitta. It was a hectic week, so I couldn't get to it.

akihironitta · 2021-01-22T11:03:47Z

@briankosw No worries :)

Adding types to datamodules

34f5f07

briankosw marked this pull request as draft December 18, 2020 11:03

Fixing typing imports

2b55b32

akihironitta mentioned this pull request Dec 18, 2020

Add type annotations #434

Open

17 tasks

akihironitta added the refactoring label Dec 19, 2020

briankosw added 2 commits December 20, 2020 03:03

Removing torchvision.transforms from return typing

ac3377d

Remove more torchvision.transforms typing

a4c39c7

Borda requested a review from akihironitta December 20, 2020 23:10

briankosw marked this pull request as ready for review December 21, 2020 01:08

Removing return typing

ffa0cb9

hassiahk reviewed Dec 21, 2020

View reviewed changes

pl_bolts/datamodules/kitti_datamodule.py Outdated Show resolved Hide resolved

hassiahk reviewed Dec 21, 2020

View reviewed changes

pl_bolts/datamodules/binary_mnist_datamodule.py Outdated Show resolved Hide resolved

briankosw added 4 commits December 21, 2020 16:01

Add None for optional arguments

5e6c5d4

Remove unnecessary import

3a5a0ab

Remove unnecessary import

30579ed

Merge branch 'types/datamodules' of https://github.com/briankosw/pyto…

5de590c

…rch-lightning-bolts into types/datamodules

akihironitta suggested changes Dec 21, 2020

View reviewed changes

pl_bolts/datamodules/binary_mnist_datamodule.py Outdated Show resolved Hide resolved

Add None return type

c675931

Borda requested a review from akihironitta January 2, 2021 21:25

Merge branch 'master' into briankosw-types/datamodules

bd97183

akihironitta changed the title ~~Adding types to datamodules~~ Adding types to datamodules [wip] Jan 19, 2021

akihironitta added 4 commits January 20, 2021 20:11

Add types and skip mypy checks on some files

b9c910d

Fix setup.cfg

9e222d0

Add missing import

0c54fdd

isort

8b2e196

akihironitta reviewed Jan 20, 2021

View reviewed changes

akihironitta added 8 commits January 20, 2021 20:34

yapf

9c5dd5c

mypy please...

c6c97e1

Please be quiet mypy and flake8

4ac8a5b

yapf...

e847ead

Disable all of yapf, flake8, and mypy

1839438

Use Callable

097df6d

Use Callable

9e00c5d

Add missing import

af563a5

akihironitta changed the title ~~Adding types to datamodules [wip]~~ Adding types to datamodules Jan 20, 2021

akihironitta changed the title ~~Adding types to datamodules~~ Adding types to some of datamodules Jan 20, 2021

akihironitta self-requested a review January 20, 2021 13:14

Borda approved these changes Jan 20, 2021

View reviewed changes

Borda merged commit f0cc60b into Lightning-Universe:master Jan 20, 2021

akihironitta reviewed Jan 21, 2021

View reviewed changes

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

Adding types to some of datamodules #462

Adding types to some of datamodules #462

briankosw commented Dec 18, 2020 •

edited by Borda

Loading

pep8speaks commented Dec 18, 2020 •

edited

Loading

codecov bot commented Dec 18, 2020 •

edited

Loading

briankosw commented Dec 19, 2020

Borda commented Dec 20, 2020

briankosw commented Dec 21, 2020

briankosw commented Dec 22, 2020 •

edited

Loading

Borda commented Jan 2, 2021

akihironitta commented Jan 4, 2021

akihironitta commented Jan 4, 2021

akihironitta commented Jan 19, 2021

akihironitta commented Jan 19, 2021

Borda commented Jan 19, 2021

akihironitta Jan 20, 2021

akihironitta commented Jan 20, 2021 •

edited

Loading

akihironitta Jan 21, 2021

akihironitta Jan 21, 2021 •

edited

Loading

briankosw commented Jan 22, 2021

akihironitta commented Jan 22, 2021

Adding types to some of datamodules #462

Adding types to some of datamodules #462

Conversation

briankosw commented Dec 18, 2020 • edited by Borda Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Dec 18, 2020 • edited Loading

Comment last updated at 2021-01-20 12:45:02 UTC

codecov bot commented Dec 18, 2020 • edited Loading

Codecov Report

briankosw commented Dec 19, 2020

Borda commented Dec 20, 2020

briankosw commented Dec 21, 2020

briankosw commented Dec 22, 2020 • edited Loading

Borda commented Jan 2, 2021

akihironitta commented Jan 4, 2021

akihironitta commented Jan 4, 2021

akihironitta commented Jan 19, 2021

akihironitta commented Jan 19, 2021

Borda commented Jan 19, 2021

akihironitta Jan 20, 2021

Choose a reason for hiding this comment

akihironitta commented Jan 20, 2021 • edited Loading

akihironitta Jan 21, 2021

Choose a reason for hiding this comment

akihironitta Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

briankosw commented Jan 22, 2021

akihironitta commented Jan 22, 2021

briankosw commented Dec 18, 2020 •

edited by Borda

Loading

pep8speaks commented Dec 18, 2020 •

edited

Loading

codecov bot commented Dec 18, 2020 •

edited

Loading

briankosw commented Dec 22, 2020 •

edited

Loading

akihironitta commented Jan 20, 2021 •

edited

Loading

akihironitta Jan 21, 2021 •

edited

Loading