Adding types to some of datamodules

[briankosw]

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]

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]

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]

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?

[Borda]

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

[briankosw]

@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!

[briankosw]

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]

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

[akihironitta]

@briankosw I'm sorry for replying very late.

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?

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

[akihironitta]

1. 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]

@Borda Sure.

[akihironitta]

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

[Borda]

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

[akihironitta]

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

[akihironitta]

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]

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

[akihironitta]

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

[briankosw]

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

[akihironitta]

@briankosw No worries :)