ci: mypy config - #16312

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.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

I very much like this PR! Thank you for doing it, and to everyone else for their feedback and work towards adding type hints! The approach taken here is a much better idea than the prototype I experimented with in #15794... 😮‍💨

The mypy docs contain a guide on using mypy with an existing codebase, which I've found to be incredibly helpful for PlasmaPy. This PR implements two of the recommendations from this guide:

• Make sure everyone runs mypy with the same options. ...
• Make sure everyone type checks the same set of files. ...

[The guide also recommends adding mypy to CI as soon as possible, which made the process of adding type hints to PlasmaPy significantly easier! But that's out-of-scope here and may be best discussed in https://github.com/astropy/astropy-APEs/pull/94, which I'm hoping to get back to one of these megaseconds... ⌚]

@eerovaher, you wrote a good post about the levels of typing we want to do as a package.
I think package-wide consensus is now that we can add informative types to functions but we are not requiring the types to be perfectly correct. (I think that's your level 1). This is a great improvement over the previous state of affairs and I think it's well worth doing anywhere and everywhere.
Higher levels of typing relate to satisfying mypy (and distributing those types). I don't think us doing level 1 package-wide precludes doing level 2 in specific places, at the discretion of the package maintainer, e.g. to help Python optimize it's compilation. This PR establishes the architecture to permit level-2 (which requires some CI). If a package maintainer doesn't want to verify their type annotations, then that is the default state! It's functionally equivalent to an allow-list, but replaces the blanket ignore with package-specific ignores, which serves as a TODO log. We do this with .ruff.toml; IMO this is the best approach in this case too.

It's not clear to me who is supposed to maintain type checker configuration and how. Adding exceptions to every type checking error or warning doesn't sound very helpful, even if it is done on per-file and per-error basis. It is much simpler to ignore type checking by not running a type checker at all. An alternative that could be useful would be to try to address type checking errors as they are revealed, but no one has done that so far. Is anyone planning to start doing that? Maybe they should start by opening a pull request where they could post the type checker configuration that they used, and then we could evaluate if the improvements this enables are worth the effort.

I don't think us doing level 1 package-wide precludes doing level 2 in specific places, at the discretion of the package maintainer

A pull request that adds annotations to some set of functions that are useful for IDE tab-completion has no effect on the tab-completion of any other functions. This means that functions can be annotated independently from each other and project-level coordination is not required. Type checking is very different because adding annotations to one function could cause new type checking warnings in other functions, modules or even sub-packages, especially early on when the amount of annotated code is still very small. It seems to me type checking needs project-level commitment, or at least commitment from the key sub-packages such as units.

[...type checker configuration] serves as a TODO log. We do this with .ruff.toml

Addressing a Ruff rule violation does not cause brand new violations to appear in unedited code, but editing type annotations can cause brand new type checking violations in unedited code. So the ignores in ruff.toml can be treated as a TODO list where addressing any item removes it from the list permanently, whereas type checker configuration would be like a whack-a-mole game where errors can easily pop up again. They are quite different.

Is type-checking on the list of topics to be discussed at the Coordination Meeting yet ? it sounds more and more like it should be.

Does this count? astropy/astropy-project#349 (comment)

Thanks ! I missed it !

We should probably make sure that at least the big picture aspect of typing is discussed as a plenary just to make sure everyone is on the same page.

Since we're not actually using it ourselves properly yet, is it an idea to work a bit like ruff, where we use the "standard" location mypy.ini (or maybe .mypy.ini to keep it hidden by default) and move things over to pyproject.toml as they mature?

SGTM.

I would also prefer if type checking would be opt-in (by adding entries to files) instead of opt-out. The fraction of annotated code will remain very small in the foreseeable future.

True, but IMO that's not the rationale for having an opt-out list rather than an opt-in one. The reasons are:

1. We have a common configuration for all of Astropy. Any module that starts becoming typed should want to work towards that common configuration. The process of adding type information starts by uncommenting the module from the opt-out list, seeing a long list of problems, then adding some mypy config to suppress most of the issues, then slowly adding / correcting type hints and winnowing down the list of ignores. This is a similar approach to ruff, where we want a common configuration, so move things from global ignore, to sub package-specific ignores, to fixed.
2. It's better to be self-documenting in what we want to work on. An opt-out list and subpackage-specific configuration are that documentation.

If the opt-out approach is more popular then I can accept it.

There's a few files that we could start type checking already (with these settings, and locally, not in a CI job). Should we discuss that here?

There's a few files that we could start type checking already (with these settings, and locally, not in a CI job). Should we discuss that here?

Seems appropriate! We should get the mypy settings correct.

In addition to what @neutrinoceros already said it should also be pointed out that normally pre-commit only checks files that are being changed, but Mypy needs to check all the files because changing one file can introduce type errors in unmodified files.

Mypy should be configured to check at least one file. There are two reasons for this. The first is that it would provide an example how to unignore files. The second is that type checking anything in astropy requires a huge effort, so I am starting to get worried that the configuration that ignores everything will stay in place for years and I don't see any point in that.

Sorry to be a bummer but I'm having second thoughts about the choice of mypy as our base type-checker; specifically, with the launch of numpy 2.2, which made great strides in the typing area, it became clear to me that relying solely on mypy to type check numpy-based code is problematic:
I could anecdotally cite couple issues I've encountered with this combo myself

• TYP: np.ndarray.tolist return type seems broken in numpy 2.2.0 numpy/numpy#27944
• TYP: excessively vague return types from np.dtype and np.ndarray.dtype in numpy 2.2.0 numpy/numpy#27945

but perhaps a more convincing point is that mypy is explicitely called out as buggy in numpy 2.2's release notes, where we also find that their recommended type checker is actually basedpyright.
Given how central numpy is in our own API, I think it'd be wise to follow their guidance while we still can. I also found in my own experiments (and I don't know how this scales to astropy's size yet) that I didn't need any configuration to run (based)pyright efficiently.

To a non-typing type like me, the advantage of mypy has always been the speed-up promised by mypyc...

I don't think I ever heard of any project leveraging both mypyc and numpy together, so I'm skeptical about the existence of that possibility as far as astropy is concerned (but happy to be proven wrong !).

@nstarman and I have been thinking that for the units machinery it might be quite useful - that of course is pure python right now (i.e., calculating the conversion functions, etc.).

This doesn't exclude basedpyright, which is stricter than mypy and corrects a few mistakes/ambiguties. For most cases, what passes one passes the other. Areas we want to target with mypyc must pass mypy and should pass basedpyright.

If we want to start with basedpyright, which SGTM, why don't you open a PR to get it started. Then we can change this PR to target only specific modules / files.

@nstarman and I have been thinking that for the units machinery it might be quite useful - that of course is pure python right now (i.e., calculating the conversion functions, etc.).

Sounds interesting, I'm curious to see the outcome !

This doesn't exclude basedpyright

True, though it seems hard enough for us to start using one typechecker, I'm worried we might never get a second chance.

For most cases, what passes one passes the other.

yes, but I'd like to highlight that it doesn't take much to run into some correct numpy code that mypy will choke on (either by incorrectly reporting an error or by not applying type narrowing even when it's possible), but pyright will thrive.
I mostly want to get this point on our collective radar;

If we want to start with basedpyright, which SGTM, why don't you open a PR to get it started. Then we can change this PR to target only specific modules / files.

I'd be happy to, but don't have plans for doing this at the moment, and I'm not sure I can commit to it for the time being so please do not wait on this bit.

I realize that I'm being a bit more negative than constructive here. The point I want to get across is: mypy might not work for our needs, and the sooner we acknowledge this possibility, the less painful it'll be if we eventually need to switch to something else.

I am not at all worried about potential incompatibilities between Mypy and NumPy. The difficulties I am encountering when trying to make the code (mostly units) type-checkable are so far entirely caused by astropy. That being said, I still think that parts of astropy could be compiled with Mypyc, which would make Mypy mandatory. Also, my experience with Mypy is that each release is noticeably better than the previous one, so if we keep working on making astropy type-checkable at our current rate then I'm willing to guess Mypy and NumPy will have sorted out their problems years before they will have a chance to become a bottleneck for us.

So far, mostly we've added code annotations, which are useful even if they may contain (minor, hopefully) typing errors. But #17902 proposes to add ~100KiB worth of automatically generated stub files for astropy.units, which are only useful if we actually start to test and verify our typing, i.e., if we start to hold typing PRs to the same standards as regular ones, that they come with tests and documentation. But that requires enabling type checking in CI, i.e., something like this PR. Though perhaps based on basedpyright, since that is the numpy recommendation as well (https://numpy.org/devdocs/release/2.2.1-notes.html).

Agreed. This PR has gotten a bit away from me, in part because when mypy errors, it sometimes just catastrophically crashes, which is what is happening here. Debugging has proven onerous.
Replacement PR welcome. Perhaps based on basedpyright and with a more minimal config just selecting astropy/units.

I've personally found mypy to be challenging to configure (see PlasmaPy's mypy.ini 😅). I'm therefore quite curious about basedpyright.

The documentation page on using mypy with an existing codebase contains a lot of general advice that has made my life significantly easier whilst enabling static type checking.

I've been using (based)pyright for a couple months now, I can look into setting it up for astropy.units tomorrow.

update: #17918 👀

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

There have been a couple of new Rust based tools for static type checking that have been announced recently:

• ty from Astral (in preview, so not yet stable or feature complete)
• Pyrefly, which is from Meta (in alpha, presumably no association with Joss Whedon, cue the Firefly opening theme)

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.