quality(mypy): Add type hints and fix all mypy warnings/errors #22212

oscarbenjamin · 2021-10-03T14:04:52Z

References to other Issues or PRs

The general issue for adding type hints is #17945 so probably high-level discussion belongs there.

See also #20173 which did not get merged. At the time it fixed all mypy errors and there were only a few. The fixes in this PR are what is needed after 1 year of not running mypy in CI.

Also previous PRs that originally fixed all the mypy complaints: #20140 and #18244

Brief description of what is fixed or changed

Adds type hints and type: ignore to silence all mypy errors. There are a few places where the mypy errors indicate genuine bugs such as a method being defined in a superclass where it can only really work for a particular subclass. Otherwise mostly this is just about adding hints and adding ignores.

Other comments

Most of the ignores are needed due to python/mypy#2904.

Some of them actually indicate genuine bugs that I can't easily fix for example the tensor module has a lot of code that does inconsistent things with different types of object that probably would fail under some conditions. Ironically it is also the module with the most type hints and many of those hints are incorrect and inconsistent.

The bulk of the work in making this PR was not about adding hints but about fixing incorrect hints. I think that since contributors are beginning to add type hints anyway sympy needs to start running mypy in CI to make sure that the hints are not garbage. I'll do that in a separate PR though after this one is merged.

Release Notes

other
- Type hints were added to silence all mypy errors in the sympy codebase.

Also the number_of_states property is moved from MarkovProcess to DiscreteMarkovProcess since it does not make sense for other MarkovProcess subclasses and mypy can see the method will fail if used on those other classes.

Many type hints were incorrect and some new ones have been added.

sympy-bot · 2021-10-03T14:05:01Z

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

other
- Type hints were added to silence all mypy errors in the sympy codebase. (#22212 by @oscarbenjamin)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.10.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->

The general issue for adding type hints is #17945 so probably high-level discussion belongs there.

See also #20173 which did not get merged. At the time it fixed all mypy errors and there were only a few. The fixes in this PR are what is needed after 1 year of not running mypy in CI.

Also previous PRs that originally fixed all the mypy complaints: #20140 and #18244

#### Brief description of what is fixed or changed

Adds type hints and `type: ignore` to silence all mypy errors. There are a few places where the mypy errors indicate genuine bugs such as a method being defined in a superclass where it can only really work for a particular subclass. Otherwise mostly this is just about adding hints and adding ignores.

#### Other comments

Most of the ignores are needed due to https://github.com/python/mypy/issues/2904.

Some of them actually indicate genuine bugs that I can't easily fix for example the tensor module has a lot of code that does inconsistent things with different types of object that probably would fail under some conditions. Ironically it is also the module with the most type hints and many of those hints are incorrect and inconsistent.

The bulk of the work in making this PR was not about adding hints but about fixing incorrect hints. I think that since contributors are beginning to add type hints anyway sympy needs to start running mypy in CI to make sure that the hints are not garbage. I'll do that in a separate PR though after this one is merged.

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* other
    * Type hints were added to silence all mypy errors in the sympy codebase.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

oscarbenjamin · 2021-10-03T14:17:41Z

To test the effect of this PR try running mypy sympy on master. With this PR mypy sympy does not report any errors. On master you get 168 errors but actually once I started fixing some of them more and more appeared. It seems like mypy can't handle the sympy codebase on current master and just bails after a while.

github-actions · 2021-10-03T16:01:23Z

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

asmeurer · 2021-10-05T01:22:37Z

sympy/stats/stochastic_process_types.py

@@ -2,15 +2,16 @@
 import random

 import itertools
-from typing import Sequence as tSequence, Union as tUnion, List as tList, Tuple as tTuple
+from typing import (Sequence as tSequence, Union as tUnion, List as tList,
+        Tuple as tTuple, Set as tSet)


This naming convention isn't the greatest (not that I can think of a better one). Maybe we should require all typing imports to come from sympy.typing which could provide common alias names. Even the fact that some files import Tuple or Dict as is because they don't use the SymPy Tuple or Dict is confusing.

Given the ubiquity of typing now in common editors and so on I wonder if sympy needs to bite the bullet and start calling them symTuple or BasicTuple or something so that Tuple can just be for typing. Or actually maybe there was a PEP that was going to change it so you could just do tuple[int] etc...

I'm also adding type hint, and I expect an official naming convention to be documented somewhere.

I don't want to add a new package sympy.typing for this. Ideally there would be a better place to add things like this like maybe utilities or external (related to the reorganisation that @oscargus has been doing #22291).

like maybe utilities or external

I think one may argue for both of these. utilities since it is, sort of, a utility, external since it is an external (as in non-SymPy). But as it is part of Python, I'd probably say utilities (but do not have any strong opinions really).

An additional package may also be an option of course, but it seems to be a single, quite small, file there so maybe hard to argue that it is worth it.

asmeurer · 2021-10-05T01:24:10Z

sympy/tensor/array/expressions/conv_matrix_to_array.py

    MatPow, symbols, Dummy, Lambda, HadamardProduct, HadamardPower, S
 from sympy.matrices.expressions.matexpr import MatrixExpr
 from sympy.tensor.array.expressions.array_expressions import ArrayDiagonal, ArrayTensorProduct, \
    PermuteDims, ArrayAdd, ArrayContraction, ArrayElementwiseApplyFunc


-def convert_matrix_to_array(expr: MatrixExpr) -> Basic:
+def convert_matrix_to_array(expr: Basic) -> Basic:


Why is Expr not good enough here?

The function calls itself recursively with the args of its argument. In general the args of Expr are not known to be Expr e.g.:

In [11]: M = MatrixSymbol('X', 2, 2) In [12]: isinstance(M, Expr) Out[12]: True In [13]: isinstance(M.args[0], Expr) Out[13]: False

It should be possible to add type hints to each of the different classes here like MatMul could have

args: Tuple[Expr, ...]

I expect that adding those type hints will throw up more errors though when I'm actually trying to silence the errors first so that we can run mypy and then add type-hints properly later.

oscarbenjamin · 2021-10-17T23:15:51Z

I realise that there can be improvements here but really I would just like to get this in and enable mypy in CI and then further improvements can be made while CI is running mypy. I have fixed all mypy errors several times now and I just want mypy to be running in CI so that the next time I come to look at this there won't be a load of new errors to fix.

As I said in the OP:

See also #20173 which did not get merged. At the time it fixed all mypy errors and there were only a few. The fixes in this PR are what is needed after 1 year of not running mypy in CI.

Does anyone object to this being merged as it is?

redeboer

Great work, good to see that mypy can soon be enforced through CI!

Could you perhaps add (and install) a file py.typed next to sympy/__init__.py? Then mypy sees SymPy as typed when it checks downstream packages.
https://mypy.readthedocs.io/en/stable/installed_packages.html#creating-pep-561-compatible-packages

oscarbenjamin · 2021-10-18T00:21:29Z

Could you perhaps add (and install) a file py.typed next to sympy/__init__.py?

I can but can that also be done in a subsequent PR? The intention in this PR is to silence warnings from mypy sympy. My next planned PR is to enable mypy checking in GitHub actions. After that many changes can be made by many contributors to improve the typing situation but we can then do those with mypy enabled in CI. This PR already has a sizeable diff and I don't want to add to that any more than is needed.

redeboer

Okay LGTM

oscargus · 2021-10-18T08:35:23Z

Let's merge this and then the repackaging and CI integration can come in subsequent PRs.

oscarbenjamin · 2021-10-18T13:15:11Z

Okay great. Thanks for the reviews. I'll merge this and then we can enable mypy in CI.

oscarbenjamin added 9 commits October 2, 2021 14:34

quality(mypy): use type:ignore for dispatch handlers

ff0044c

quality(mypy): ignore pysat under mypy

cb401a9

quality: add type hints in various modules

b62a369

quality: add type hints for args and free_symbols

84f955e

quality: add type hints for Symbol and solvers

4d28a54

quality: add type hints for sympy/stats

6d23cb3

Also the number_of_states property is moved from MarkovProcess to DiscreteMarkovProcess since it does not make sense for other MarkovProcess subclasses and mypy can see the method will fail if used on those other classes.

quality: add type hints in testing and unify

087adfd

quality: add type hints in utilities

0385e03

quality: add/fix type hints in tensor

d202ea3

Many type hints were incorrect and some new ones have been added.

oscarbenjamin requested a review from moorepants as a code owner October 3, 2021 14:04

oscarbenjamin added Code quality mypy labels Oct 3, 2021

quality: remove redundant imports

48794fb

This was referenced Oct 3, 2021

WIP: Add return type hints for some functions #22180

Closed

Look into using type hints #17945

Open

fix(stats): re-add number_of_states for ContinuousMarkovChain

432f0fd

asmeurer reviewed Oct 5, 2021

View reviewed changes

eagleoflqj mentioned this pull request Oct 16, 2021

Fix handling of zoo in evalf #22211

Merged

redeboer suggested changes Oct 18, 2021

View reviewed changes

redeboer approved these changes Oct 18, 2021

View reviewed changes

oscarbenjamin merged commit ef37f78 into sympy:master Oct 18, 2021

oscarbenjamin deleted the pr_mypy2 branch October 18, 2021 13:15

oscarbenjamin mentioned this pull request Oct 18, 2021

Run mypy in GitHub Actions on all pull requests #22320

Merged

oscargus mentioned this pull request Oct 19, 2021

maint(ci): add mypy type checking to Travis #20173

Closed

redeboer mentioned this pull request Oct 21, 2021

Make SymPy PEP 561 compatible #22337

Merged

oscarbenjamin mentioned this pull request Jun 21, 2022

Errors on multi-variable assignments of types python/mypy#10101

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quality(mypy): Add type hints and fix all mypy warnings/errors #22212

quality(mypy): Add type hints and fix all mypy warnings/errors #22212

oscarbenjamin commented Oct 3, 2021 •

edited

sympy-bot commented Oct 3, 2021 •

edited

oscarbenjamin commented Oct 3, 2021

github-actions bot commented Oct 3, 2021

asmeurer Oct 5, 2021

oscarbenjamin Oct 5, 2021

eagleoflqj Oct 5, 2021

oscarbenjamin Oct 17, 2021

oscargus Oct 18, 2021

asmeurer Oct 5, 2021

oscarbenjamin Oct 5, 2021

oscarbenjamin commented Oct 17, 2021

redeboer left a comment •

edited

oscarbenjamin commented Oct 18, 2021

redeboer left a comment

oscargus commented Oct 18, 2021

oscarbenjamin commented Oct 18, 2021

quality(mypy): Add type hints and fix all mypy warnings/errors #22212

quality(mypy): Add type hints and fix all mypy warnings/errors #22212

Conversation

oscarbenjamin commented Oct 3, 2021 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Oct 3, 2021 • edited

oscarbenjamin commented Oct 3, 2021

github-actions bot commented Oct 3, 2021

asmeurer Oct 5, 2021

Choose a reason for hiding this comment

oscarbenjamin Oct 5, 2021

Choose a reason for hiding this comment

eagleoflqj Oct 5, 2021

Choose a reason for hiding this comment

oscarbenjamin Oct 17, 2021

Choose a reason for hiding this comment

oscargus Oct 18, 2021

Choose a reason for hiding this comment

asmeurer Oct 5, 2021

Choose a reason for hiding this comment

oscarbenjamin Oct 5, 2021

Choose a reason for hiding this comment

oscarbenjamin commented Oct 17, 2021

redeboer left a comment • edited

Choose a reason for hiding this comment

oscarbenjamin commented Oct 18, 2021

redeboer left a comment

Choose a reason for hiding this comment

oscargus commented Oct 18, 2021

oscarbenjamin commented Oct 18, 2021

oscarbenjamin commented Oct 3, 2021 •

edited

sympy-bot commented Oct 3, 2021 •

edited

redeboer left a comment •

edited