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

Enable type checking and adding typing_copilot to dev/CI environment #1496

Closed
ColCarroll opened this issue Jan 11, 2021 · 5 comments
Closed
Labels
Discussion Issue open for discussion, still not ready for a PR on it

Comments

@ColCarroll
Copy link
Member

ColCarroll commented Jan 11, 2021

This is to help track and discuss adding typing_copilot as a dev dependency, and as part of the CI for arviz.

TL;DR

We add type checking to pull request checks, and another tool to enforce that our type checking is only getting stronger.

Slightly longer

This is a discussion over whether we want to do two things:

  1. Add a type check step to CI
  2. Help migrate to this using typing_copilot

I've sketched some possible wins and losses as I see them, and I'm tagging @obi1kenobi (author of typing_copilot) who might be able to help us think through whether this makes sense. Note that he has already submitted #1397, #1398, #1491, #1492, #1493, and #1494. @OriolAbril and @obi1kenobi have some great conversations in there about what is going on, and what the benefits are.

The general idea of typing_copilot is that it checks your mypy configuration against the amount of checking you've already done, and will tighten the config file, if possible. To me, it seems like a no-brainer that if we decide type checking is a good idea, to use this.

Possible wins:

  • Strong typing helps downstream users and maintainers
  • Find existing bugs
  • Extra layer of testing and compatibility (especially for arviz, which may have contributors for a specific library, unaware of conventions/return types elsewhere in the codebase)

Possible losses:

  • Higher threshold for contributing / maintainers may need to get more familiar with typing to help contributors
  • More noise during merges
@ColCarroll ColCarroll added the Discussion Issue open for discussion, still not ready for a PR on it label Jan 11, 2021
@ColCarroll
Copy link
Member Author

My initial thought, for what it is worth, is that we aim to get this in after the next release, and give it a test run. If nothing else, I bet the experience of a maturing code base adding typing will be an interesting experiment, and we'll all learn something. It also seems easy to get rid of.

@canyon289
Copy link
Member

Do it :D

@obi1kenobi
Copy link
Contributor

obi1kenobi commented Jan 19, 2021

If #1498, #1503, and #1505 get merged, then typing_copilot is able to generate a minimal mypy.ini that will pass and can be used as a starting point for the type quality ratchet:

$ typing_copilot init
typing_copilot v0.5.4

Running mypy once with laxest settings to establish a baseline. Please wait...

Ran into a known issue with mypy.
If this GitHub issue is resolved, try upgrading your mypy version:
  https://github.com/python/mypy/issues/9437
For now, going to attempt a workaround, please wait...

Collecting mypy errors from strictest check configuration. Please wait...

Strict run completed and uncovered 3973 mypy errors. Building the strictest mypy config such that all configured mypy checks still pass...

[ ... some import warnings elided, I might not have had all the 3rd party dependencies installed ... ]

> Mypy was unable to find type hints for some 3rd party modules, configuring mypy to ignore them.
    More info: https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
    Affected modules: ['IPython', 'bokeh', 'cmdstanpy', 'emcee', 'jax', 'matplotlib', 'mpl_toolkits.axes_grid1', 'netCDF4', 'numba', 'numpy', 'numpyro', 'pandas', 'pyjags', 'pymc3', 'pyro', 'pystan', 'scipy', 'setuptools', 'stan', 'tensorflow', 'tensorflow_probability', 'theano', 'torch']

> Constructed 84 mypy error suppression rules across 54 modules.

Config generated (279 lines). Verifying the last few mypy settings and validating that the new configuration does not produce mypy errors. Please wait...

Validation complete. Your mypy.ini file has been updated. Happy type-safe coding!

To reproduce the above, make sure you have arviz itself installed in your local venv (e.g. via pip install -e .) from which you are running the typing_copilot commands. The error message typing_copilot shows when the analyzed package is not installed is currently not ideal, I opened obi1kenobi/typing_copilot#5 for it.

Happy to post the generated mypy.ini in a draft PR if you'd like to see it. It won't be able to be merged until #1498, #1503, and #1505 are all merged, since without the fixes in those PRs, mypy will continue to find errors even at its loosest possible configuration.

@OriolAbril
Copy link
Member

I am completely on board with using typing_copilot, my only concern is that as it has been shown in all the typing related PRs, I barely know anything about typing (thank you so much @obi1kenobi for all the explanations by the way) and will probably not be able to help contributors fix typing issues that come up.

I know it's still a work in progress at a very early stage, but could we also add/summarize all these conversations and instrucctions to the developer guide? https://arviz-devs.github.io/arviz/contributing/index.html

@OriolAbril
Copy link
Member

This is done already 😄 so I'll close the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issue open for discussion, still not ready for a PR on it
Projects
None yet
Development

No branches or pull requests

4 participants