Type annotations and mypy

[gianscarpe]

🚀 Feature

Introduce mypy in pre-commit pipeline

Motivation

Static type checking is an interesting and powerful technique to assure high code quality and standard. I believe it should be unvaluable to start introducing type annotation and add mypy among pre-commit hooks

Pitch

I want to check the correctness of types annotation with a static type checking. When I commit, the checker should verify and eventually notify the contributer to check his code

Alternatives

Additional context

I ran mypy onto tests and it heighights some deprecated calls (such as checkpoint_callback, now in callbacks list). These checks could be useful to static check tests and find avoid obsolescent tests

[github-actions]

Hi! thanks for your contribution!, great first issue!

[carmocca]

Would the checks fail the build or just be informative?
Would it be run only for the files modified?

[gianscarpe]

Hi @carmocca , I think it would be just informative in the beginning. But in the long run it should fail the builds.
I believe it could be run only for modified files, as in pre-commit pipeline!

[Borda]

@gianscarpe what would be the difference to pyright which we are using now? #855

[gianscarpe]

@gianscarpe what would be the difference to pyright which we are using now? #855

Hi @Borda, Thanks for your replay! I didn't notice pyright configuration in the root repository. I think (correct me if I'm wrong) that pyright check is not explicitly mandatory neither it's part of pre-commit pipeline. I would consider moving the project in this direction and add static type checking for tests code as well

[Borda]

we have this test in CI - https://github.com/PyTorchLightning/pytorch-lightning/blob/8601268c70649f49767001098adbf665a93843df/.github/workflows/code-formatting.yml#L41
but as you can see in the config we are skipping a major part of our codebase because we have not had time to propagate the fix everywhere...
So is there any advantage of mypy, eventually other checkers - 4 Python type checkers to keep your code clean?
If not much, I would stay for now with pyright and focus on code fixes to have typing right 🐰

[gianscarpe]

we have this test in CI - https://github.com/PyTorchLightning/pytorch-lightning/blob/8601268c70649f49767001098adbf665a93843df/.github/workflows/code-formatting.yml#L41

but as you can see in the config we are skipping a major part of our codebase because we have not had time to propagate the fix everywhere...
So is there any advantage of mypy, eventually other checkers - 4 Python type checkers to keep your code clean?
If not much, I would stay for now with pyright and focus on code fixes to have typing right rabbit

My issue was more about type checking in general. So I totally agree! I would focus on fix the typing in the code and eventually introduce it as a pre-commit hook!

[Borda]

My issue was more about type checking in general. So I totally agree! I would focus on fix the typing in the code and eventually introduce it as a pre-commit hook!

COOL, let's follow the per-partez strategy as we also have for imports fixing #4805
so fix just a few files in each PR and then drop these files from skip pattern in type check :]

[Borda]

I feel we can close this one and continue type improvements talks in #855

[Borda]

@gianscarpe it seems we have some problems with pyright

An internal error occurred while type checking file "/home/runner/work/pytorch-lightning/pytorch-lightning/pytorch_lightning/loggers/base.py": TypeError: Cannot read property 'category' of undefined
    at w (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:474439)
    at w (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:474753)
    at F (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:471590)
    at A (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:474075)
    at F (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:472663)
    at Object.T [as applySolvedTypeVars] (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:467845)
    at Un (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:442783)
    at Ln (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:441406)
    at /home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:310374
    at Object.c [as mapSubtypes] (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:465952)
Error performing analysis: TypeError: Cannot read property 'category' of undefined
    at w (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:474439)
    at w (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:474753)
    at F (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:471590)
    at A (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:474075)
    at F (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:472663)
    at Object.T [as applySolvedTypeVars] (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:467845)
    at Un (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:442783)
    at Ln (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:441406)
    at /home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:310374
    at Object.c [as mapSubtypes] (/home/runner/work/pytorch-lightning/pytorch-lightning/node_modules/pyright/dist/pyright-internal.js:1:465952)

I think that maybe it would be easier to move to mypy? @akihironitta

[akihironitta]

@Borda #5021 sounds reasonable.

[gianscarpe]

I totally agree! @Borda find it reasonable. Moreover, I think extending static type checking coverage should be taken into consideration in future milestones :)

[Borda]

@gianscarpe do you mean #5023?

[gianscarpe]

Exactly! @Borda