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

APE 25ish: An initial roadmap for static type checking #94

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 166 additions & 0 deletions APE25.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
An initial roadmap for static type checking
-------------------------------------------

author: Nick Murphy

date-created:

date-last-revised:

date-accepted:

type: Standard Track

status: Discussion


Abstract
--------

This APE describes the initial process for gradually adding static
type checking to Astropy.

Detailed description
--------------------

Type hint annotations allow us to specify the expected data types of
function arguments, return values, and variable assignments. For
example,

.. code-block::

def f(x: int) -> float:
...

indicates that ``x`` should be an ``int`` and the return value should
be a ``float``. Type hints are not enforced by Python during
runtime. Type hints can be used by static type checkers to find
problems with the code and by Jupyter notebooks and integrated
development environments (IDEs) to help with code completion. Type
hints also serve as a form of documentation in function and method
signatures.

For most of its history, type hint annotations have not been applied
within Astropy's source code (though annotations have been used to
indicate the expected units or physical types of function arguments
and return values). Only recently have type hint annotations begun to
be applied to Astropy. However, no mechanism is in place to check the
correctness and validity of type hint annotations. ...

Most guides on adding type hint annotations and enabling static type
checking recommend starting small, running mypy consistently,
prioritize annotating widely used imports, add type hint annotations
for new code, gradually increase the fraction of the code base that is
checked my mypy, and gradually increase the strictness of the checks
performed by mypy.

.. This section describes the need for the APE. It should describe
the existing problem that it is trying to solve and why this APE
makes the situation better. It should include examples of how the
new functionality would be used and perhaps some use cases.


Benefits of static type checking
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

#. Can find programming errors that would be difficult to find
otherwise
Comment on lines +66 to +67
Copy link
Member

Choose a reason for hiding this comment

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

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.

#. Helps introspection and tab completion in Jupyter notebooks
#. Helps IDEs to make things easier for humans
#. Helpful for downstream package maintainers
Comment on lines +68 to +70
Copy link
Member

Choose a reason for hiding this comment

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

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.


Disadvantages of static type checking
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

#. Requiring type

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.
Comment on lines +77 to +80

Choose a reason for hiding this comment

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

aren't those the same thing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! This is still an early draft so I was trying out different wordings and ways to organize the doc.


1. Type hint annotations may add a barrier to entry for contributing

Choose a reason for hiding this comment

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

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.

to Astropy.





Branches and pull requests
--------------------------

.. Any pull requests or development branches containing work on this
APE should be linked to from here. (An APE does not need to be
implemented in a single pull request if it makes sense to implement
it in discrete phases). If no code is yet implemented, just put
"N/A"


Implementation
--------------

#. Create an initial ``mypy.ini`` configuration file that ignores all
existing errors and warnings from mypy. The configuration file must
enable static type checking on a single Python module, and disable
static type checking for all other modules and subpackages.

#. Create a tox environment for running mypy.

#. Create a GitHub Action that runs the tox environment as a GitHub
Action for each pull request.

#. Create a section in Astropy's developer documentation on the
process for adding type hint annotations to Astropy and using
static type checking.

#. Set the GitHub Action for running mypy as required for a pull
request to be merged.

#. Gradually enable static type checking in ``astropy.cosmology`` and
``astropy.units`` by expanding the number of files checked by mypy,
enabling new mypy rules on a per-file or per-subpackage basis,
adding type hint annotations, creating type stub files for
dynamically generated content, and using ``# type: ignore`` style
comments to indicate when static type checking should be skipped on
particular lines.

#. Update the developer documentation with lessons learned and best
practices for adding type hint annotations.

At this point, subpackage maintainers may gradually enable static type
checking for the subpackages that they maintain.



.. This section lists the major steps required to implement the APE. Where
possible, it should be noted where one step is dependent on another, and which
steps may be optionally omitted. Where it makes sense, each step should
include a link related pull requests as the implementation
progresses.


Backward compatibility
----------------------

This APE is expected to maintain backward compatibility.


Alternatives
------------

.. If there were any alternative solutions to solving the same
problem, they should be discussed here, along with a justification
for the chosen approach.

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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

2. **Take a more aggressive approach to static type checking.** Most
recommendations
3. **Delay adding a static type checker to Astropy's continuous
integration suite.**

Decision rationale
------------------

<To be filled in by the coordinating committee when the APE is accepted or rejected>