Skip to content

WIP: 476 - 20200602 type hints (Being broken apart into subsets) #473

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
wants to merge 36 commits into from

Conversation

hjmjohnson
Copy link
Contributor

@hjmjohnson hjmjohnson commented Jun 3, 2020

Related to #476

Description

The next round of adding typehints

Status

Ready

Types of changes

  • Docstrings/Documentation updated

@hjmjohnson hjmjohnson force-pushed the 20200602-type-hints branch from 4c38c55 to 8f0efdd Compare June 3, 2020 12:14
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @hjmjohnson and the team, this PR is quite large, it'll take some time to finish the review. we'll make comments as we go through the changes, instead of collecting all the comments and release in one go...

Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your great work on this topic.
I haven't finished review as this PR is very big.
Put several comments inline for discussion first.
Thanks.

@hjmjohnson hjmjohnson force-pushed the 20200602-type-hints branch from 8f0efdd to ccb60f4 Compare June 3, 2020 12:48
@hjmjohnson
Copy link
Contributor Author

@wyli @Nic-Ma I am concerned that if a PR like this is evaluated for every place where type-hinting could be better, that it will consume a huge amount of time resolving merge conflicts in the future.

We have 3 developers working on making typehint changes for the next week, and are attempting to have a typehint related PR every day with incremental improvements.

My fear is that if we try to do an entire weeks worth of changes before evolutionary improvements towards the final solution that we will need to spend extraordinary efforts resolving merge conflicts.

@wyli
Copy link
Contributor

wyli commented Jun 3, 2020

My fear is that if we try to do an entire weeks worth of changes before evolutionary improvements towards the final solution that we will need to spend extraordinary efforts resolving merge conflicts.

true, how about we split the commits of this PR into multiple PRs? I believe many of them are independent changes

@hjmjohnson
Copy link
Contributor Author

@wyli I understand. I will work on that now. Let's remember that time taken to break these items apart is time taken away from the end goals. You will soon find many small PR's coming through.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jun 3, 2020

Hi @hjmjohnson and @wyli ,

I agree with you guys to split the PR into smaller ones.
We should not merge a "big" but "draft" PR, it's very dangerous and unnecessary.
Maybe you can ask your 3 developers to submit 3 PRs for 3 different code folders, then let's review and refine them until merging, I think most of them are independent.
After merged, then try to submit another 3 PRs for another 3 folders in iterations.

What do you think?

Thanks.

@hjmjohnson
Copy link
Contributor Author

These are not completely independent. Type hinting is good precisely because it can identify unforeseen relationships between different modules.

These are NOT independent. I've spent about 2 hours identifying the checks that need to be disabled in order to create a smaller patch set that can get us started.

I expect to spend all day today breaking this into smaller units to be reviewed.

NOTE: I do not think this is a "DRAFT", It is the first completed chapter.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jun 3, 2020

Hi @hjmjohnson ,

Plan sounds good!
Thanks for your explanation.

@hjmjohnson hjmjohnson changed the title 20200602 type hints WIP: 20200602 type hints (Being broken apart into subsets) Jun 3, 2020
@hjmjohnson
Copy link
Contributor Author

Please see #477 for the critical first step necessary for building other less dependant PR's.

@hjmjohnson hjmjohnson force-pushed the 20200602-type-hints branch from ccb60f4 to 8f414d2 Compare June 3, 2020 18:49
This was referenced Jun 3, 2020
@hjmjohnson hjmjohnson changed the title WIP: 20200602 type hints (Being broken apart into subsets) WIP: 476 - 20200602 type hints (Being broken apart into subsets) Jun 3, 2020
@hjmjohnson hjmjohnson force-pushed the 20200602-type-hints branch 2 times, most recently from 0db0311 to 71fa8d8 Compare June 5, 2020 16:13
hjmjohnson and others added 9 commits June 5, 2020 18:23
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.
One often wants to run the very fast codeformatting tests without
running any of the unit tests. Add a sub-command to allow this
behavior with `./runtests.sh --codeformat-only`.

One often wants the `black` formatting tool to automatically
fix formatting violations without requiring explicit
manual call.

These two subcommand features allow one to write
scripts that allow walking through a series of
refactorings addressing modifications one by
one.
hjmjohnson and others added 26 commits June 5, 2020 22:02
These types were injected by pyre with manual review.
These types were injected by pyre with manual review.
Co-authored-by: Cameron Trentz <[email protected]>
(expression has type "float", variable has type "int")

Co-authored-by: Cameron Trentz <[email protected]>
From compose.py

Co-authored-by: Cameron Trentz <[email protected]>
 (expression has type "List[Any]", variable has type "None")

Co-authored-by: Cameron Trentz <[email protected]>
"Union[int, float, Tuple[Any, ...], List[Any]]" is not indexable

Co-authored-by: Cameron Trentz <[email protected]>
(expression has type "Union[Tuple[Any, ...], List[Any]]", variable has type "Tuple[float, Any]")
(expression has type "Tuple[Any, ...]", variable has type "List[Any]")
(expression has type "Union[Tuple[Any, ...], List[Any]]", variable has type "Tuple[float, Any]")
(expression has type "PNGSaver", variable has type "NiftiSaver")

Co-authored-by: Cameron Trentz <[email protected]>
for + ("List[Any]" and "Tuple[Any]")

Co-authored-by: Cameron Trentz <[email protected]>
Co-authored-by: Cameron Trentz <[email protected]>
Corner case warnings are suppressed until more beneficial
changes are incorporated.
@hjmjohnson hjmjohnson force-pushed the 20200602-type-hints branch from 71fa8d8 to 1644fe3 Compare June 6, 2020 12:34
@hjmjohnson
Copy link
Contributor Author

Abandoning. This work will now need to be redone it is too far diverged to justify working on it.

@hjmjohnson hjmjohnson closed this Jun 9, 2020
@hjmjohnson hjmjohnson deleted the 20200602-type-hints branch June 15, 2020 15:41
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 this pull request may close these issues.

3 participants