Conversation
Configuring a default list of files for Mypy to check helps ensure that modules without typing errors stay without typing errors. Configuring `follow_imports = "silent"` ensures that Mypy does not report errors from modules it is not instructed to check. In the long term type checking with Mypy can allow compiling (parts of) `astropy` with Mypyc.
The new CI job helps ensure that typing errors are not introduced to modules Mypy already approves. Mypy approval is a requirement for compiling anything with Mypyc.
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
There was a problem hiding this comment.
I think this a great first step! 🚀
There was a problem hiding this comment.
This looks good!
One question about the best way to approach this: is there any way to run mypy only when one of the files it guards is actually changed? I recall we discussed adding it to pre-commit earlier - would that ensure it? (Note that unless there is a clear good answer to this, I'm fine with going ahead and merge -- we can always revisit how things are tested later.)
|
Let's take as a starting point f40ba98 (which is where #19011 is at right now). Mypy reports Suppose for the sake of example that we now edit @@ -77,7 +77,7 @@ For more examples see the :mod:`numpy.typing` definition of
"""
-UnitPower: TypeAlias = int | float | Fraction
+UnitPower: TypeAlias = int | float | Fraction | str
"""Alias for types that can be powers of the components of a
`~astropy.units.UnitBase` instance"""
UnitPowerLike: TypeAlias = UnitPower | np.integer | np.floatingAfter that single change mypy reports You can see that changes in one file can cause typing errors in another, so it is not helpful to only run mypy in CI if a module it guards has been changed. |
There was a problem hiding this comment.
Thanks for that example of why a guard-by-file doesn't work. With that, approving, though I'd like someone more versed in CI/tox to review before merging.
|
The log https://github.com/astropy/astropy/actions/runs/20110655410/job/57706767249?pr=19044 seems straight forward. But now people who don't care about typing won't touch that file because the CI would fail unless everything in it in typed? |
That seems a fair worry longer term, though my sense is that those interested in typing are willing to help out. Maybe a failure of this run can lead to an (automatic?) ping to a new "astropy-typing" group? Though I'm personally happy to just wait and see how it goes. If really worried, maybe have the CI run not be required for merging for now? |
Description
In the December developer telecon speeding up parts of
astropyby compiling them with mypyc was discussed (see also #19011). The conclusion was that right now providing wheels with mypyc-compiled code is not worthwhile, one of the main reasons being that mypyc will only compile code that mypy approves and the fraction of code with type annotations is still very low. However, mypyc compilation might eventually become worthwhile if more code gets annotated, so there should be a CI job that prevents typing errors from being introduced to modules mypy already approves.There have been several attempts to configure type checkers for
astropy(#12971, #15794, #15815, #16312, #17918). Those PRs attempted to nominally turn on type checking for all or large fractions ofastropy, but that necessitates listing a large number of modules and rules to be ignored. This PR takes the opposite approach where mypy only checks modules it is explicitly told to check, but none of the default rules are ignored.During local development I recommend running mypy directly instead of running it through tox, but as far as I know the tox environment is required for CI.