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

mypy checking as part of CI #1725

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

mypy checking as part of CI #1725

wants to merge 29 commits into from

Conversation

aarchiba
Copy link
Contributor

@aarchiba aarchiba commented Feb 25, 2024

If we do want to apply static typing to PINT (#1709 ), a possible goal is for the static type checker mypy to successfully check the entire code base.

mypy is designed for gradual typing, and in fact there is a guide to applying mypy to an existing codebase. A key first step is to enable mypy as part of CI. Of course, this requires an appropriate mypy setup; in particular it requires ignoring a lot of errors. As long as these are ignored, mypy isn't doing a particularly strict job, but this is a starting point for gradual typing.

Next steps:

  1. Get mypy working on the whole codebase and running in CI, but with sufficiently permissive settings that it passes. This PR does this.
  2. Select a small subset of the codebase (ideally something that is widely used) and enable stricter mypy checking, fixing that part of the codebase so it passes. This PR does this for everything but pint.observatory.
  3. Add type annotations to the subset where mypy is checking, making sure mypy continues to pass. This would be an incremental process going forward.
  4. Ensure new code does not require additional mypy exceptions/error suppression.

There are choices about what the target for mypy checking should be - how strict we should attempt to eventually be. There are also choices about how best to approach these targets. There is room for a lot of discussion, but this PR is intended to establish the most basic setup to begin the process.

There is a discussion about how astropy should handle typing: astropy/astropy#15170

There is also an analogue to this PR: astropy/astropy#15794

Summary of the optional rules as enabled by this PR:

  • The bodies of functions are only checked if the function parameters/return values have type annotations
  • Unreachable code is a warning
  • A function that fails to return a value on some execution paths is a warning
  • A function that is supposed to return a specific type but returns an untyped value on some paths is a warning
  • Global and class variables should have declared types
  • Redefining a variable with an incompatible type is an error
  • Values that can be None should be typed using Optional if they are typed
  • Equality/containment checks that are always false because they compare different types are an error

These are certainly up for discussion, but they seem reasonable to me.

The idea of putting mypy in CI as this PR does it now is to encourage contributors to follow these rules:

  • Don't merge code if mypy fails.
  • Don't add new ignore_ statements (except un-typed third-party packages).
  • Don't add type annotations in places they won't be checked (because of ignore_ statements).
  • If you want to improve our typing, you can remove an ignore_ rule and then make that module pass mypy.
  • If you work on a function, try to add argument and return types, and then make sure mypy still passes.
  • If you want to improve our typing, you can add type annotations to a common function or class and check mypy still passes.

The goal isn't necessarily that everything has static types, just that they become the norm where they help.

I should also say that your editor can probably run mypy (or one of its mostly equivalent alternatives) as an on-the-fly linter and get angry underlines everywhere the code won't pass mypy. Your editor may also offer "inlay hints" showing the inferred types of various things, and allow you to fill those types in by double-clicking if you want to.

@aarchiba
Copy link
Contributor Author

Python 3.12 is more strict with string syntax; previous versions were happy with something like "\Delta" because \D wasn't a recognised escape sequence; they would pass it through without complaining. Python3.12 emits a SyntaxWarning in this case, because future versions of Python will consider that an error. These SyntaxWarnings have been getting lost in our test suite's blizzard of warnings, but mypy emits them. I probably should have switched to python3.11 for running mypy but I figured we needed to fix those anyway.

Copy link

codecov bot commented Feb 25, 2024

Codecov Report

Attention: Patch coverage is 92.17082% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 69.23%. Comparing base (bf9e15b) to head (1b3b20f).

Files Patch % Lines
src/pint/utils.py 82.08% 11 Missing and 1 partial ⚠️
src/pint/simulation.py 76.47% 4 Missing ⚠️
src/pint/observatory/__init__.py 94.64% 1 Missing and 2 partials ⚠️
src/pint/observatory/topo_obs.py 96.07% 1 Missing and 1 partial ⚠️
src/pint/pintk/plk.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1725      +/-   ##
==========================================
+ Coverage   69.06%   69.23%   +0.17%     
==========================================
  Files         105      105              
  Lines       24703    24729      +26     
  Branches     4410     4410              
==========================================
+ Hits        17061    17121      +60     
+ Misses       6534     6499      -35     
- Partials     1108     1109       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aarchiba
Copy link
Contributor Author

aarchiba commented Feb 28, 2024

It's probably instructive to look at the changes I had to make to get mypy to pass:

  • A few type annotations were actually wrong
  • The try import except ImportError doesn't work very well (but we didn't need it)
  • There is some weirdness with the Fitter's covariance matrix attribute because it doesn't exist unless the value has been computed
  • There was a case where e had been used both as except ... as e and as a normal variable in the same scope
  • Some global variables needed types
  • models/__init__.py needed to have __all__ (which it should have had anyway)
  • mypy is confused by @thing.setter so I had to disable it for that line
  • mypy doesn't know about the :P f-string format introduced by the uncertainties package so I had to disable mypy for that line
  • I had to put an if TYPE_CHECKING: to cope with some of our circular importing
  • mypy did not like me passing **kwargs into a function that only accepted specific keyword arguments so I had to disable mypy for that line

So, not too difficult a battle, but there are some things that are okay that mypy doesn't like.

[edited to add: a lot of the broken commits here are me trying to get CI set up properly]

@aarchiba
Copy link
Contributor Author

Adding typed, documented instance variable declarations helps the docs (adds a Variables section): https://nanograv-pint--1725.org.readthedocs.build/en/1725/_autosummary/pint.fitter.Fitter.html#pint-fitter-fitter

@aarchiba
Copy link
Contributor Author

toa.py passes without changes, but this demonstrates gradual typing: none of the functions have type annotations so they pass automatically.

@aarchiba aarchiba changed the title WIP: mypy checking as part of CI mypy checking as part of CI Mar 3, 2024
@aarchiba aarchiba added the awaiting review This PR needs someone to review it so it can be merged label Mar 3, 2024
@aarchiba
Copy link
Contributor Author

aarchiba commented Mar 3, 2024

I'd actually say this is ready to go: if we want to start working on static typing for pint, I'd say apply #1718 and then this one; we can then start adding annotations and removing ignore_errors and mypy will keep us from making a mess.

@aarchiba
Copy link
Contributor Author

aarchiba commented Mar 3, 2024

Curses. Astropy's type annotations pre-6.0 clash with mypy's, and oldestdeps doesn't use astropy 6.0. I'll have to back out the changes to derived_quantities.

@aarchiba
Copy link
Contributor Author

aarchiba commented Mar 4, 2024

Didn't have to back them out entirely, just separate the actual units into quantity_input and the types (u.Quantity) into the function signature. With astropy 6.0 (5.0 maybe?) we could include the units in the form u.Quantity[u.m/u.s] on both input and output of the function, and at least in principle a static type checker could detect that we were putting a length in where a speed was needed. I don't know that any actual type checker can check this, but it would collect all the type-like information in one place tidily. And quantity_input (in sufficiently advanced astropy) can understand this. Plus, I believe, Sphinx can use this, so we should be able to drop the type annotations in the docstrings and they'll get automatically filled in in the rendered docs (might need to set something to make this happen).

@aarchiba
Copy link
Contributor Author

aarchiba commented Mar 5, 2024

There is no way to specify return types/units that makes both quantity_input and mypy happy when using Astropy 4.0.

@@ -1197,22 +1225,21 @@ def merge_dmx(model, index1, index2, value="mean", frozen=True):
)
if value.lower() == "first":
dmx = getattr(model, f"DMX_{index1:04d}").quantity
elif value.lower == "second":
elif value.lower() == "second":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug found!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review This PR needs someone to review it so it can be merged enhancement needs discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant