Skip to content

Instrument code with typehints #476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
9 of 11 tasks
hjmjohnson opened this issue Jun 3, 2020 · 7 comments
Closed
9 of 11 tasks

Instrument code with typehints #476

hjmjohnson opened this issue Jun 3, 2020 · 7 comments

Comments

@hjmjohnson
Copy link
Contributor

hjmjohnson commented Jun 3, 2020

Is your feature request related to a problem? Please describe.

We believe statically typing what are essentially fully dynamic languages will give MONAI code stability and improve developer productivity.

Attempt to use modern python preferred pep 484 support.
This allows advanced linters to more aggressively perform static
validation checking.

Using typehints as annotations allow for a more DRY (Do not repeat
yourself) implementation that supports various type checking linters and
documentation support.

Some pytype warnings will require more extensive corrections to fully
resolve them. These few cases are suppressed in the interest of
allowing more static checks without imposing a huge burden at the moment.

Describe the solution you'd like
Instrument code with type hint static checkers and PEP484 type hints.

This task will not be complete until

the last two steps could happen in parallel

@hjmjohnson
Copy link
Contributor Author

The plan is to make MONAI code base pass tests with pytype static analysis, and mypy static analysis tools, along with flake8-mypy integrations.

The MONAI package should be sufficiently instrumented with type hints so that all the docstring types used to indicate expected types to developers can be statically checked and validated by the above-mentioned tools.

Eventually, the entire MONAI toolkit should be compatible with type hinting for all public-facing interfaces.

@wyli
Copy link
Contributor

wyli commented Jun 3, 2020

thanks,~ the flake8-mypy CI tests currently works fine~
for a concrete plan to enable mypy, pytype, flake8-mypy:

the last two steps could happen in parallel

Edit: Please ignore this comment and follow the tasks listed in the ticket

@hjmjohnson
Copy link
Contributor Author

@wyli The flake8-mypy does NOT currently work fine. They are disabled!!! as was done explicitly to get the separate PR's working.

@wyli
Copy link
Contributor

wyli commented Jun 3, 2020

thanks for pushing this forward @hjmjohnson, we also need

@hjmjohnson
Copy link
Contributor Author

thanks for pushing this forward @hjmjohnson, we also need

@wyli We have some of that locally, but as we figure out how to use pytype, mypy, and other validators (i.e. we spend a lot of time with pyre, but it is really hard to install, and does not seem to produce meaningfully useful failures at this point in our type hinting journey).

@crtrentz @benjamin-gorman I know you have been working notes for how to use, configure, and resolve problems identified with pytype and mypy. Please modify the recommended CONTRIBUTING.md file with your notes as an independant PR on the master branch.

@hjmjohnson hjmjohnson mentioned this issue Jun 5, 2020
2 tasks
hjmjohnson added a commit to BRAINSia/MONAI that referenced this issue Jun 5, 2020
Run the complete mypy static checking tool during evaluations
of code format style checks.

Related to Project-MONAI#476 subtask for enable mypy
hjmjohnson added a commit to BRAINSia/MONAI that referenced this issue Jun 5, 2020
Run the complete mypy static checking tool during evaluations
of code format style checks.

Related to Project-MONAI#476 subtask for enable mypy
hjmjohnson added a commit to BRAINSia/MONAI that referenced this issue Jun 5, 2020
Run the complete mypy static checking tool during evaluations
of code format style checks.

Related to Project-MONAI#476 subtask for enable mypy
hjmjohnson added a commit to BRAINSia/MONAI that referenced this issue Jun 5, 2020
Run the complete mypy static checking tool during evaluations
of code format style checks.

Related to Project-MONAI#476 subtask for enable mypy
hjmjohnson added a commit to BRAINSia/MONAI that referenced this issue Jun 5, 2020
Run the complete mypy static checking tool during evaluations
of code format style checks.

Related to Project-MONAI#476 subtask for enable mypy
hjmjohnson added a commit to BRAINSia/MONAI that referenced this issue Jun 5, 2020
One often wants to run the very fast codeformatting tests without
running any of the unit tests. Add a commands to allow this
behavior with "./runtests.sh [--black|--pytype|--mypy|--flake8]".

One often wants the `black` formatting tool to automatically
fix formatting violations without requiring explicit
manual call. Use "./runtests.sh --black-fix"

Add mypy static checking tool to monai

Run the complete mypy static checking tool during evaluations
of code format style checks.

Related to Project-MONAI#476 subtask for enable mypy

Expand the documentation for runtest.sh script.  Provide more expressive
documentation for `-h`

The dryrun request to print the commands used for static
and format checking without actually running the commands.
hjmjohnson added a commit to BRAINSia/MONAI that referenced this issue Jun 6, 2020
One often wants to run the very fast codeformatting tests without
running any of the unit tests. Add a commands to allow this
behavior with "./runtests.sh [--black|--pytype|--mypy|--flake8]".

One often wants the `black` formatting tool to automatically
fix formatting violations without requiring explicit
manual call. Use "./runtests.sh --black-fix"

Add mypy static checking tool to monai

Run the complete mypy static checking tool during evaluations
of code format style checks.

Related to Project-MONAI#476 subtask for enable mypy

Expand the documentation for runtest.sh script.  Provide more expressive
documentation for `-h`

The dryrun request to print the commands used for static
and format checking without actually running the commands.
wyli pushed a commit that referenced this issue Jun 6, 2020
* Add subcommands to the individual codeformat test

One often wants to run the very fast codeformatting tests without
running any of the unit tests. Add a commands to allow this
behavior with "./runtests.sh [--black|--pytype|--mypy|--flake8]".

One often wants the `black` formatting tool to automatically
fix formatting violations without requiring explicit
manual call. Use "./runtests.sh --black-fix"

Add mypy static checking tool to monai

Run the complete mypy static checking tool during evaluations
of code format style checks.

Related to #476 subtask for enable mypy

Expand the documentation for runtest.sh script.  Provide more expressive
documentation for `-h`

The dryrun request to print the commands used for static
and format checking without actually running the commands.

* Temporarily add branch needed to trigger CI of this codebase.

* Suppress mypy warnings about incompatible superclass signature

Related to #495

* Suppress mypy error: Cannot assign to a method

Related to #494

* Mypy potential type mismatch resolved with assertion.

* Mypy type ambiguity resolved.
This was referenced Jun 6, 2020
This was referenced Jun 9, 2020
@wyli
Copy link
Contributor

wyli commented Jun 10, 2020

perhaps a new ticket is needed, but a quick question here first, could we remove 'flake8-mypy' since we have both 'flake8-mypy' and 'mypy'?

when there's a type error, running ./runtests.sh --codeformat --quick raises a flake8-mypy error but lacks error contexts. mypy provides much better messages.

This was referenced Jun 10, 2020
@wyli
Copy link
Contributor

wyli commented Sep 14, 2020

Major APIs and docstrings are now revised to fully support typehints. closing this ticket -- please create new specific tickets to address any further typehint related issues.

@wyli wyli closed this as completed Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants