APE 25ish: An initial roadmap for static type checking#94
APE 25ish: An initial roadmap for static type checking#94namurphy wants to merge 24 commits intoastropy:mainfrom
Conversation
There was a problem hiding this comment.
Thank you for starting this ! I feel that two important points may be missing at the moment:
- we ought to acknowledge and emphasis the expected gains (and costs ?) of having type-hints, and of static checking, from a downstream developers/maintainers perspective
- some tools (including mypy, last time I checked) will not attempt to discover type hints in a package unless it contains a
py.typedmarker file. This means that as long as we don't include it anywhere, our type annotations, while not statically checked, are also invisible from the perspective of someone checking downstream code. I think we'll have to include it to make our effort visible and useful, and that running a type checker ourselves is a necessary step that needs to happen before it. Meanwhile, as long as we don't include it, we can treat type hints as private details, which I assume will help with early implementation.
APE25.rst
Outdated
| 1. Requiring type hint annotations that mypy doesn't complain about | ||
| adds an extra step to contributing to Astropy... | ||
|
|
||
| 1. An additional step is required to contribute to Astropy. |
There was a problem hiding this comment.
Yep! This is still an early draft so I was trying out different wordings and ways to organize the doc.
|
|
||
| 1. An additional step is required to contribute to Astropy. | ||
|
|
||
| 1. Type hint annotations may add a barrier to entry for contributing |
There was a problem hiding this comment.
This also feels similar to the first two points, but I guess maybe you mean to say that existing type hints may make the code harder to read from a newcomer's perspective ? This is something I've heard in similar contexts, coming from seasoned devs, and I'm honestly not convinced it's a valid point, but it's nonetheless an opinion that exists.
| #. Helps introspection and tab completion in Jupyter notebooks | ||
| #. Helps IDEs to make things easier for humans | ||
| #. Helpful for downstream package maintainers |
There was a problem hiding this comment.
These are benefits of adding type annotations, and I enthusiastically agree with these benefits. Static type checking is not required to see these benefits for humans.
| #. Can find programming errors that would be difficult to find | ||
| otherwise |
There was a problem hiding this comment.
It would be useful to know what level of mypy checking is required to realize this benefit, along with an example of a module that is typed to that level.
One of my initial bad experiences with enabling type checking was using VS Code pylance/pyright on a small module at the "basic" setting. After several hours trying and failing to get to zero errors I gave up. Maybe mypy is better and the technology has advanced.
APE25.rst
Outdated
|
|
||
| 1. **Do not enable static type checking of Astropy.** Type hint | ||
| annotations have begun to be added to Astropy. At present, there is | ||
| no way to check the validity or accuracy of type hint annotations. |
There was a problem hiding this comment.
Some of the comments and type hints in astropy/astropy#15914 seem to highlight that checking validity of type annotations is sometimes not possible or useful. I'm thinking of object or UnitLike or PhysicalTypeLike annotations. Basically we have some protocol that is documented in a long paragraph and implemented in code, but is difficult/impossible to express as a type annotation.
This is certainly common in my subpackages table, time, io.ascii. To be sure there are many places where the types are reasonably narrow, but often they are not.
|
It's going to be a while before I can get back to this PR, so if anyone else wants to work on this, please let me know! My main goal here has been to encapsulate the advice given in the mypy docs on enabling static type checking on an existing code base. |
|
With the acceptance of the revisions to APE 1, we're going to start following the new APE proposal process. While all of the APE editors (for now exactly the same as the CoCo) will contribute to the process, one of the APE editors will be the point-person (chief APE editor) shepherding your team through the process. Your chief APE editor is Clara Brasseur (@ceb8). You can review the next steps in the APE process here, but just to briefly summarize them here:
Please let us (either your chief APE editor or the full CoCo) know if you have any questions! |
|
Thank you for the update, Kyle! It's still likely that I won't be able to work on this for a while, so I plan to close this pull request unless anyone else wants to take the lead on this. It's also possible that this may be obsolete at this point, since I haven't been following the discussions about this that closely. |
|
An additional note on this topic — for PlasmaPy, I'm hoping to switch us over to using ty for static type checking once it becomes a bit more mature, both because of performance and also some capabilities that will likely make it more contributor-friendly than mypy. |
No worries. Thanks, Nick! I'll leave it to you to decide when/if to close the PR. |
|
With the new editorial process I think merging this APE (even as rejected or undecided) would be useful; this topic has been controversial and the effort needed to move it forward is certainly big enough to warrant fundings. So, anytime this is brought up again in any serious capacity, having this APE/discussion as a precedent will be relevant. |
I somewhat disagree, based in particular on the current language in APE 1: IMO, merging the APE constitutes the action of actively seeking approval with the intent (and resources) to implement the APE. |
|
Thinking more about this, it wouldn't take me that long to get this to a point where it would be worth merging, including adding something like a rationale section for why I'm withdrawing it. With respect to whether or not to merge this as a withdrawn APE, the main questions I have are:
It's pretty common for a PEP to be written that proposes an idea and gets declined or withdrawn but merged, and another PEP to come along with improvements to the original idea. There have already been a few PRs submitted that largely take the approach described here. My favorite is astropy/astropy#16312 by @nstarman, and there were also astropy/astropy#12971 and astropy/astropy#15815. (I also submitted astropy/astropy#15794, but that took an approach I used in PlasmaPy that I have since come to regret. 😅) |
|
@namurphy - regardless of whether this PR gets merged into the formal list of APE's, the work you've put into it will always remain in this PR. This is citable and won't be forgotten any time soon. Nevertheless I do appreciate the two questions you posed as being entirely reasonable. I think this is really a question that should go up to the CoCo, namely whether an APE PR should be merged already withdrawn. |
|
I think this PEP is worth merging, withdrawn or otherwise. Some of these considerations are being addressed outside of the APE mechanism as we decided that some sub-package type checking can happen without an APE, but if (🤞 |
|
@nstarman - Sounds OK, assuming you are speaking for the CoCo with a focus on process rather than the technical merits of the APE. In that case I might suggest making the |
This will match the usage in the Python docs and be shorter.
This carefully considered and not completely pseudorandomly selected sequence of emojis accurately conveys my true thoughts and feelings on enabling static type checking in Astropy.
Thank you for the encouragement! I started working on this again, and am reconsidering the decision to withdraw it. One change is that I'm switching this to proposing to use ty instead of mypy. I've used mypy for a few years now and don't think it's the best tool at present, and I've been wanting to try out ty for PlasmaPy. The performance of ty is amazing, and has a helpful feature that |
|
well it's already in much better shape than early alphas, where checking astropy would hit a runaway memory leak. I'm also much more enhusiast about ty than mypy, especially for a large code base. The performance is one thing (as you noted, it's quite a feat), but what I think takes the cake are much better gradual promises: ty is supposed to be much more resilient than mypy when checking code that contains little to no type info. |
@namurphy - without a prototype implementation or identified funding, this will may well earn you the distinction of writing the first APE to get rejected. Just saying... 😄. |
| A limitation of static type checking is that the analysis is performed | ||
| statically (i.e., without running the code being analyzed). Dynamically | ||
| generated objects (e.g., ``astropy.units.zettajansky``) therefore cannot | ||
| be directly analyzed with a static type checker. |
There was a problem hiding this comment.
Suggesting (as units maintainer, not CoCo) to say that 'directly' is strictly true, but there are ways to ameliorate this, e.g. by generating type stubs. We have mechanisms for this now in astropy.units, so it would be good to distinguish between code that:
- is static so can easily be typed
- is dynamic but fixed at installation, so can also be typed, though this is more complex
- is dynamic at runtime, is much more challenging / impossible to statically type.
This PR will add an APE that proposes a path forward for enabling static type checking in Astropy's continuous integration suite. If anyone is interested in being a co-author, please let me know!
The process would be more or less to:
astropy.cosmologyandastropy.units?)It's currently in an early draft state, and might be best though of as being in the brainstorming stage 🧠 🌩️. Big picture feedback would be especially appreciated! In particular, we will need to address the really valid concerns and considerations that have been brought up in astropy/astropy#15170.
I'm also hoping to avoid being over-prescriptive, so as to give us flexibility and avoid the need to amend this APE when ruff implements a full static type checker. 🙃
Closes #92.