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 gradual #1

Closed
wants to merge 45 commits into from
Closed

Mypy gradual #1

wants to merge 45 commits into from

Conversation

ksunden
Copy link
Owner

@ksunden ksunden commented Dec 27, 2022

PR Summary

This is a massive PR and I realize that... I don't fully expect anyone to read every single line of the newly added stub files. That said, these files have zero effect at run time for the library, so even if there are things that are wrong about them, they won't cause running code to fail. I'd like to get this in early in the 3.8 cycle to allow downstream libraries to test out the type hints with sufficient time to fix them before final release.

When reviewing, read enough of the stub files to get a sense of how they work, and to understand common patterns.
I attempted to be as complete and correct as I could be, but that is mostly reliant on existing docstrings being complete and correct (and some amount of fallback to variable names).

Highlighted changes to library code:

  • __all__ added to matplotlib.__init__
    • mypy was not happy about some things being implicitly exported (things that were imported but other places use in the mpl top level namespace)
    • This makes explicit that all mpl defined things are publically accessible from the mpl namespace
    • Happy to add/subtract from the list
  • Pyplot has type hints inline
    • boilerplate.py updated to gather annotations
    • because of the syntax rules of the return annotation (cannot line break on ->) the textwrap based line wrapping has been removed and replaced with a clean-up step after writing of running black
    • This makes some of the other behaviors (such as putting null bytes to avoid line wrapping the data kwarg) obsolete, but at the cost of a new dev (and testing) dependency on black
    • The handwritten portions of pyplot are wrapped with # fmt: off/on comments, so only the autogenerated portion of the file is actually run through black

For most of the library, stub files are used rather than inline type hints.
This has the advantage of not touching literally every public signature line in one PR, but does carry a tradeoff of direct utility of mypy and some other tools in the typing space.
mypy will trust the pyi files and disregard the actual implementation, meaning that if a parameter is added/removed, but the pyi file is not updated, mypy will use the outdated signature to type check.

The two exceptions where inline type hints are used are tests and pyplot.
Tests use inline typehints, but do so relatively minimally, only when mypy complains explicitly. It doesn't make a lot of sense to write stub files for tests, which are not called by downstream libraries (who the stub files are actually mostly for) but running type checking on tests makes a fair amount of sense as it is more representative of what downstream users will see.

Pyplot uses inline type hints because it is autogenerated, and it was easier to inject the type hints into the existing autogenerated code than it would be to do parallel lookup and write two files. The hand written portions of pyplot also do have type hints.
Since pyplot has inline type hints, mypy will actually check the implementation of the functions for type consistency, which can find certain errors.

There are some tools to help discover those problems, but I have found that they are too noisy and not configurable enough to be useful to us (yet).
I may end up writing a script to find the most commonly anticipated discrepancies, but for getting started, the heuristic is "if a signature is touched, make sure to update the pyi file" (which are already a higher bar of review/etc as anything that changes a signature is an API change which should carry a release note.

There are some areas left for followup PRs as they are a) relatively complicated ideas that should get particular attention on review that would be lost in this massive PR or b) ideas that can be added incrementally

  • data arg preprocessor
  • typing shapes and dtypes of arrays (most everything is ArrayLike or np.ndarray, but these types do support being more specific. Using the full power that seems to be available is not as well documented as I would have liked, so may wish to reach out to somebody who knows more about this (and encourage/possibly write the docs that I think should exist for typing with numpy)
  • Some methods (e.g. tri*, streamplot, etc) are defined outside of the Axes class, and so the pyplot generation code does not currently get their type hints. The code could be updated to be smarter about looking at attributes and getting their type hints, but that would make it more complicated, so cover the 90% case first.

Additionally, we may wish to be more relaxed or more specific in certain areas.

Dask has an interesting set of guidelines that perhaps we should adopt some or all of.
In particular, for this pass I was being as complete as I could, but they recommend things like "if the return type is a union of things, don't actually type it, because that can be more burdensome to downstream users than just saying Any"
Also some places where we specify callback functions may be overly specified or hard to follow with the full specification, so relaxing the type hint may aid readability.

I certainly don't follow every one of those guidelines yet, but I think they are mostly well reasoned and perhaps we should adopt them/go through and clean up (things like the return annotation on __init__ or importing collections.abc over typing may be better as followup PRs.

I make fairly heavy use of typing.Literal. There may be some places where that is more unwieldy than we'd like and it should be str, but for the most part it has a reasonable correspondence to where we use _api.check_in_list (and its similar siblings). There may also be places where I went more generic but we can be more specific by using Literal.

Some types, particularly things like tuple[float, float, float, float] are a little unwieldy and may be better understandable replaced by a type alias (e.g. RGBA or RectangleSpec (names negotiable), because despite being the same type, their usage is logically different).
Also, tuple types are sometimes used when what we really mean is "unpackable thing with N elements" but the type system doesn't handle that super well at this time (though perhaps ArrayLike can get us there...). I also think it is fair in those cases that we say "you should give us a tuple, and for the best guarantee of forward compatibility, maybe cast it to one just to be sure".

The sphinx build is mostly unaffected, as sphinx does not read pyi stub files (yet)... though pyplot does get all of the info in the signature lines (and is currently spouting warnings that I have to deal with before this can be merged)

Todos before merge:

  • Move items from _typing.pyi to their more natural homes, and document them. This was a bit of a catchall, that ended up not having all that many things in it, but I wanted to be able to use the types and move them later. It is part of why sphinx is unhappy, since these are not documented.
  • Fix sphinx build
  • Add documentation, especially of expectations moving forward (e.g. keeping these up to date)
  • Write release note[s]

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@ksunden
Copy link
Owner Author

ksunden commented Jan 7, 2023

Some additional tooling beyond mypy to consider using in the future (and why I've not set up to use them now)

  • stubtest (provided by mypy package)
    • Imports code and checks the implementation against the type stubs (sounds perfect... but...)
    • Way too noisy and not easily configurable
    • Tries to import all of the code, including platform specific code on alternate platforms without graceful fallback to ignoring
    • Has over 5000 "errors", many of which are missing private method stubs/type hints or things like **kwargs when subclasses don't have the fully spelled out signatures but pass all kwargs to the superclass.
  • pyright (microsoft's static code analyzer)
    • Not too far off from usable, but it is stricter than mypy
    • by default will check code without explicit annotations (unlike mypy), which especially means the tests directory is full of errors simply because annotations are not provided.
    • Once you ignore tests plus the set of things configured to be ignored, it is still at ~100 errors identified (plus some warnings), mostly involving private methods.
    • Could be reasonable to get it working,
    • There are other checks that can be done with pyright besides the default static check (including things like mypy's stubtest) but they are also noisier than I'd like and not as configurable.
  • pyre (facebook's entry)
    • Not so bad, but noisier than mypy (~80 errors when configured similarly)
    • seems to not respect # type: ignore comments
    • Not as widely used as mypy/pyright, from what I've seen
  • pytype (google)
    • Does not support running on 3.11, for some reason... by default installed a 2020 version since that is when they added the cap on python version...
    • Failed complaining about a missing attribute on Any (and stopped checking)... That is the whole point of Any, to be overly permissive
    • Can be told to keep going with a cli argument, but still fails

If the user explicitly passes a levels array (the default is
auto-determined), let's assume that they know what they are doing.
oscargus and others added 7 commits January 12, 2023 21:11
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
…rs (matplotlib#24254)

* Expire deprecations in widgets and keyword only arguments for Selectors
Allow non-default scales on polar axes
Remove contour warning for "no-valid-levels".
oscargus and others added 21 commits January 13, 2023 10:11
Remove additional deprecations from 3.5
Percent signs needed to be escaped as %% in the past when using 
'%' formatting to generate default matplotlibrc file.
The 4 remaining double percent signs were overlooked in 5206cb0.
MNT: Fix double % signs in matplotlibrc
…gine

FIX: adjust_bbox should not modify layout engine
Transforms pyi, remove hints from transforms.py, minimal required _path.pyi for imports

Type stubs for artists.pyi

bezier and hatch pyi files

gridspec pyi

A few extra hints/import

axis.pyi file

scale.pyi

style public api type stubs

Fix up style typehints (remove redundant pyi for __init__, add/fix core)

Lines pyi

Triangulation type hints

backend tools stubs

projections stubs

Backend bases/managers pyis, move some shared types

Missing imports

container.pyi

layout_engine.pyi

Ticker, including updating polar

mlab.pyi

relax type hint on FuncFormatter callable

dviread.pyi

animation.pyi

collections.pyi

rcsetup.pyi

mathtext.pyi

figure.pyi and associated changes

fuller ft2font

stackplot pyi

spelling

markers.pyi

image.pyi

font_manager

path effects, spines

texmanager, text pyi files

Some additional modifications for consistency

cbook, textpath pyi

colorbar, sankey pyi

path pyi

quiver pyi

widgets pyi

contour pyi

Colors, cm pyi stubs

offsetbox, table pyi stubs

streamplot type stubs

legend and legendhandler stubs

patches type stubs

_enums pyi, ignores setting attr after instantiation

Axes except AxesBase type stubs

Axes Base and associated updates to type stubs

_format_approx stub and igonore setattr in mathtext

Add some type ignores and fix some imports

more type ignores for mpl_toolkits

Finish typing Axes

Remove TypeAlias since its newer than py3.8

missing/incorrect axes and figure annotations

Corrections and private method annotations used in pyplot
adjusts some imports in tests
add numpy to mypy and explicitly use pyproject.toml for config

update reviewdog filter

Add explicit inline excludes

Add deps to mypy run

remove numpy from mypy, add python-dateutil

Add default dependencies to mypy deps

mypy requirements fix python version requirement

psutil stubs plus sphinx install

Add ignore missing imports (cycler and a handful of others do not _yet_ have released type hints)

add follow imports skip to mypy ci

remove explicit extra excludes in mypy ci, rely on config

Add back pipe mypy ci

follow imports silent mypy ci

Add comment about ignore missing imports in mypy ci
comment about methods without type hints in boilerplate
STY: whitespace in comment
@ksunden ksunden closed this Feb 1, 2023
ksunden pushed a commit that referenced this pull request Oct 11, 2024
… and bump cibuildwheel

This is the commit message #1:

> Merge pull request matplotlib#28293 from QuLogic/py313
>
> BLD: Enable building Python 3.13 wheels for nightlies

(cherry picked from commit 725ee99)

This is the commit message #2:

> Merge pull request matplotlib#28668 from matplotlib/dependabot/github_actions/actions-167bd8b160
>
> Bump the actions group with 2 updates

(cherry picked from commit fd42e7d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants