Skip to content
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

Roadmap to strict typing with Mypy #6279

Open
11 of 18 tasks
user27182 opened this issue Jun 19, 2024 · 10 comments
Open
11 of 18 tasks

Roadmap to strict typing with Mypy #6279

user27182 opened this issue Jun 19, 2024 · 10 comments
Labels
maintenance Low-impact maintenance activity

Comments

@user27182
Copy link
Contributor

user27182 commented Jun 19, 2024

Describe what maintenance you would like added.

Type annotations make code easier to read and help prevent bugs. Having 100% annotations would also make it easy to switch to using autodoc-typehints (see #5665) and simplify the documentation. To this extent, I propose working towards using the --strict option with mypy. This issue can be used to track progress towards this goal.

This section in the Mypy docs shows how this can be worked up to by enabling some options one at a time. The options for these are shown in "Add config options" below.

In addition there are some other outstanding issues with running mypy CI checks for the project. I believe these should be addressed first before enabling more options.

Fix outstanding issues

Add config options

Start off with these

Getting these passing should be easy

Strongly recommend enabling this one as soon as you can

These shouldn't be too much additional work, but may be tricky to get passing if you use a lot of untyped libraries

These next few are various gradations of forcing use of type annotations

  • disallow_untyped_calls = True
  • disallow_incomplete_defs = True
  • disallow_untyped_defs = True

This one isn't too hard to get passing, but return on investment is lower

  • no_implicit_reexport = True

This one can be tricky to get passing if you use a lot of untyped libraries

  • warn_return_any = True

Links to source code.

No response

Pseudocode or Screenshots

No response

@user27182 user27182 added the maintenance Low-impact maintenance activity label Jun 19, 2024
@user27182
Copy link
Contributor Author

There are some tools which may help to add annotations to old code. See https://mypy.readthedocs.io/en/stable/existing_code.html#automate-annotation-of-legacy-code.

In particular I have tried autotyping and it's pretty easy/straightforward to use.

There is also mypy_clean_slate, which can fast-forward the use of --strict by simply ignoring all current type errors rather than fixing them (and fixing them later instead).

@MatthewFlamm
Copy link
Contributor

IMO the biggest hurdle here is not the mypy execution options, i.e. mypy pyvista vs. pre-commit run mypy --all-files. This could be harmonized between doing mypy --option1 --option2 or putting in some config. The biggest hurdle is that only recently has vtk shipped types with the wheels. This means that including vtk in our mypy config throws a TON of errors that used to not be there.

I suspect a large number of errors are related to vtk version differences. The only way I know how to fix this is to declare one version of the upstream library suitable for typing and then slap # type: ignore everywhere else for version compatability code. The rest of the errors will probably uncover some bugs that we didn't know about.

@MatthewFlamm
Copy link
Contributor

MatthewFlamm commented Jun 20, 2024

I'm not trying to argue against some of the proposals here, but I'd focus first on getting the dependencies aligned with typing here and only then adding in more mypy options. Otherwise, the number of errors when including vtk in the mypy dependencies will continue to grow.

Edit: I missed this sentence in your description " I believe these should be addressed first before enabling more options." My apologies. You already have this covered.

@akaszynski
Copy link
Member

@user27182, type hints have been on my "would love to have" list for a while since I have a few downstream projects that need it. If you figure out the approach and want me to work on a module/directory, let me know. It's a bit annoying to have my dependent projects warn about extract_surface not being typed.

@MatthewFlamm
Copy link
Contributor

It's a bit annoying to have my dependent projects warn about extract_surface not being typed.

This might be a slightly separate issue, but very related. Because of our inheritance choices, it is extremely difficult to type the filters correctly. This is because the filters require the self object to be of type DataSet, but yet the inheritance is DataSet[DataSetFilters]. We need to reorg the inheritance to make this easier.

When I have tried in the past, without having the inheritance sorted, it requires slapping a # type:ignore on so many lines that it makes the typing assurance very low.

@user27182
Copy link
Contributor Author

If you figure out the approach and want me to work on a module/directory, let me know.

@akaszynski sounds good, we'll let you know. I think we're still at the "figure out the approach" stage.

This might be a slightly separate issue, but very related. Because of our inheritance choices, it is extremely difficult to type the filters correctly. This is because the filters require the self object to be of type DataSet, but yet the inheritance is DataSet[DataSetFilters]. We need to reorg the inheritance to make this easier.

When I have tried in the past, without having the inheritance sorted, it requires slapping a # type:ignore on so many lines that it makes the typing assurance very low.

This will be important to resolve. The exact mechanism for solving this (e.g. protocols vs. inheritance) can probably be discussed in #4774. For now, we'll just need to continue # type ignoreing these. Once that's resolved, however, mypy should automatically flag these ignores as unused, which is nice, so at least we shouldn't need to otherwise track these ignores.

@user27182
Copy link
Contributor Author

IMO the biggest hurdle here is not the mypy execution options, i.e. mypy pyvista vs. pre-commit run mypy --all-files. This could be harmonized between doing mypy --option1 --option2 or putting in some config. The biggest hurdle is that only recently has vtk shipped types with the wheels. This means that including vtk in our mypy config throws a TON of errors that used to not be there.

I suspect a large number of errors are related to vtk version differences. The only way I know how to fix this is to declare one version of the upstream library suitable for typing and then slap # type: ignore everywhere else for version compatability code. The rest of the errors will probably uncover some bugs that we didn't know about.

I have been able to isolate 219 errors attributed exclusively to including VTK as a dependency. See #6740 for a list of errors. Indeed there are many errors in _vtk_core caused by redefs and missing imports from older VTK versions. But I think most are legitimate type errors that should probably be addressed.

@user27182
Copy link
Contributor Author

The outstanding issues related to having a bad mypy config have been addressed by #6741 and #6738, so we can safely begin to fix existing typing errors and/or add update config options

@user27182
Copy link
Contributor Author

@user27182, type hints have been on my "would love to have" list for a while since I have a few downstream projects that need it. If you figure out the approach and want me to work on a module/directory, let me know.

I have found an approach that seems to work: use autotyping to automate adding annotations on a per-file basis, then resolve any outstanding type errors that are reported after the changes. I have found that this is relatively straightforward if you restrict the number of "auto-typed" files such that there are fewer than 10-15 type errors to resolve. See #6802 and #6803. Once the "obvious" cases are auto-typed, more serious type errors that were previously ignored can be addressed.

@akaszynski I am willing to add type hints and fix some of the existing type errors but this will need support in terms of reviews and consensus from maintainers/contributors. E.g. I think #6795 needs to be approved before a serious effort to add type hints can be made. If I push typing-related PRs, should I request a review from you?

@akaszynski
Copy link
Member

Yes please, tough lately to get time for PR reviews, but it's high priority and I'll just need to be reminded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

No branches or pull requests

3 participants