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 typing fixes #5

Merged
merged 14 commits into from
Jul 15, 2022
Merged

mypy typing fixes #5

merged 14 commits into from
Jul 15, 2022

Conversation

alexpovel
Copy link
Collaborator

@alexpovel alexpovel commented Jul 13, 2022

Two short fixes.

Please note I couldn't check the validity of 27267da : mypy wouldn't run. It's utterly baffling, especially since just the other day I happily ran mypy in a poetry project in a venv on code using match statements. However, for your code:

$ poetry install
...
$ poetry shell
...
$ mypy src/design_by_contract.py
src\design_by_contract.py:32: error: Match statement is not supported
Found 1 error in 1 file (errors prevented further checking)
$ python -m mypy src/design_by_contract.py   # Doesn't work either
$ python --version
Python 3.10.5

How is that possible? The mypy script points to the virtual environment. Python is on 3.10.5. This happens on Windows 10, WSL (Debian), but also in this reproducible Docker environment (initial shell is PowerShell, replace ${PWD} if needed):

$ git clone [email protected]:StefanUlbrich/design-by-contract.git
$ docker run -it -v ${PWD}:/src python:3.10.4 bash
root@78436f342573:/# curl -sSL https://install.python-poetry.org | python3 -
Retrieving Poetry metadata

# Welcome to Poetry!

This will download and install the latest version of Poetry,
a dependency and package manager for Python.

It will add the `poetry` command to Poetry's bin directory, located at:

/root/.local/bin

You can uninstall at any time by executing this script with the --uninstall option,
and these changes will be reverted.

Installing Poetry (1.1.14): Done

Poetry (1.1.14) is installed now. Great!

To get started you need Poetry's bin directory (/root/.local/bin) in your `PATH`
environment variable.

Add `export PATH="/root/.local/bin:$PATH"` to your shell configuration file.

Alternatively, you can call Poetry explicitly with `/root/.local/bin/poetry`.

You can test that everything is set up by executing:

`poetry --version`

root@78436f342573:/# export PATH="/root/.local/bin:$PATH"
root@78436f342573:/# poetry --version
Poetry version 1.1.14
root@78436f342573:/# cd src/design-by-contract/
root@78436f342573:/src/design-by-contract# poetry install
Creating virtualenv design-by-contract-mIK2sEAW-py3.10 in /root/.cache/pypoetry/virtualenvs
Installing dependencies from lock file

Package operations: 87 installs, 0 updates, 0 removals

  • Installing certifi (2021.10.8)
  • Installing charset-normalizer (2.0.12)
  • Installing idna (3.3)
  • Installing markupsafe (2.1.0)
  • Installing pyparsing (3.0.7)
  • Installing pytz (2021.3)
  • Installing six (1.16.0)
  • Installing urllib3 (1.26.8)
  • Installing alabaster (0.7.12)
  • Installing asttokens (2.0.5)
  • Installing attrs (21.4.0)
  • Installing babel (2.9.1)
  • Installing docutils (0.16)
  • Installing executing (0.8.3)
  • Installing imagesize (1.3.0)
  • Installing jinja2 (3.0.3)
  • Installing mdurl (0.1.0)
  • Installing packaging (21.3)
  • Installing parso (0.8.3)
  • Installing ptyprocess (0.7.0)
  • Installing pure-eval (0.2.2)
  • Installing pygments (2.11.2)
  • Installing requests (2.27.1)
  • Installing snowballstemmer (2.2.0)
  • Installing soupsieve (2.3.1)
  • Installing sphinxcontrib-applehelp (1.0.2)
  • Installing sphinxcontrib-devhelp (1.0.2)
  • Installing sphinxcontrib-htmlhelp (2.0.0)
  • Installing sphinxcontrib-jsmath (1.0.1)
  • Installing sphinxcontrib-qthelp (1.0.3)
  • Installing sphinxcontrib-serializinghtml (1.1.5)
  • Installing traitlets (5.1.1)
  • Installing wcwidth (0.2.5)
  • Installing backcall (0.2.0)
  • Installing beautifulsoup4 (4.10.0)
  • Installing decorator (5.1.1)
  • Installing distlib (0.3.4)
  • Installing entrypoints (0.4)
  • Installing filelock (3.6.0)
  • Installing jedi (0.18.1)
  • Installing jupyter-core (4.9.2)
  • Installing lazy-object-proxy (1.7.1)
  • Installing markdown-it-py (2.0.1)
  • Installing matplotlib-inline (0.1.3)
  • Installing nest-asyncio (1.5.4)
  • Installing pexpect (4.8.0)
  • Installing pickleshare (0.7.5)
  • Installing platformdirs (2.5.1)
  • Installing prompt-toolkit (3.0.28)
  • Installing python-dateutil (2.8.2)
  • Installing pyzmq (22.3.0)
  • Installing sphinx (4.4.0)
  • Installing stack-data (0.2.0)
  • Installing tornado (6.1)
  • Installing wrapt (1.13.3)
  • Installing astroid (2.9.3)
  • Installing cfgv (3.3.1)
  • Installing click (8.0.4)
  • Installing debugpy (1.5.1)
  • Installing identify (2.4.11)
  • Installing iniconfig (1.1.1)
  • Installing ipython (8.1.1)
  • Installing isort (5.10.1)
  • Installing jupyter-client (7.1.2)
  • Installing mccabe (0.6.1)
  • Installing mdit-py-plugins (0.3.0)
  • Installing mypy-extensions (0.4.3)
  • Installing nodeenv (1.6.0)
  • Installing numpy (1.22.3)
  • Installing pathspec (0.9.0)
  • Installing pluggy (1.0.0)
  • Installing py (1.11.0)
  • Installing pydata-sphinx-theme (0.7.2)
  • Installing pyyaml (6.0)
  • Installing toml (0.10.2)
  • Installing tomli (1.2.3)
  • Installing typing-extensions (4.1.1)
  • Installing virtualenv (20.13.3)
  • Installing black (21.12b0)
  • Installing ipykernel (6.9.1)
  • Installing mypy (0.931)
  • Installing myst-parser (0.17.0)
  • Installing pandas (1.4.1)
  • Installing pre-commit (2.17.0)
  • Installing pylint (2.12.1)
  • Installing pytest (6.2.5)
  • Installing sphinx-book-theme (0.2.0)

Installing the current project: design-by-contract (0.2.1)
root@78436f342573:/src/design-by-contract# poetry shell
Spawning shell within /root/.cache/pypoetry/virtualenvs/design-by-contract-mIK2sEAW-py3.10
root@78436f342573:/src/design-by-contract# . /root/.cache/pypoetry/virtualenvs/design-by-contract-mIK2sEAW-py3.10/bin/activate
(design-by-contract-mIK2sEAW-py3.10) root@78436f342573:/src/design-by-contract# mypy src/design_by_contract.py
src/design_by_contract.py:32: error: Match statement is not supported
Found 1 error in 1 file (errors prevented further checking)
(design-by-contract-mIK2sEAW-py3.10) root@78436f342573:/src/design-by-contract# python --version
Python 3.10.4
(design-by-contract-mIK2sEAW-py3.10) root@78436f342573:/src/design-by-contract# mypy --version
mypy 0.931

Can you reproduce this? It's baffling because I see no reason it shouldn't work. This:

[tool.mypy]
python_version = "3.10"
warn_unused_configs = true
namespace_packages = true
mypy_path = "src"

looks fine and inconspicuous, commenting that out didn't help any.

As per the line 5 lower, `case UnresolvedSymbol(None)` is a possible
value for `name`, so it should be annotated as optional
@StefanUlbrich
Copy link
Owner

StefanUlbrich commented Jul 13, 2022

Hi @alexpovel , thanks for your interest. Mypy does not support the match statement (and this was one of the new features I wanted to experiment with): python/mypy#10201
It might be resolved by now but I have not much time to check before the weekend.

I probably will replace the matching until this is fixed (and replace the decorator package) as I would love to try out mypyc to compile to native code. I received some questions regarding performance and that would certainly be the quickest and coolest way to improve

@alexpovel
Copy link
Collaborator Author

mypy introduced support for the match statement in version 0.940 (that blog is the official change log I reckon). Your project has version 0.931:

version = "0.931"

It worked in case of my project because that's using version 0.960.

So it's an easy and obvious one. I had ran poetry update to cover that case before creating the PR in the first place but it didn't update mypy (at all). So that was the source of the bamboozle. Running poetry add --dev mypy@latest allows it to run (Updating mypy (0.931 -> 0.961)). I'm not sure why poetry update leaves available and legitimate (non-conflicting) updates by the wayside. In any case, it works then (running against 4e99e28):

$ mypy .\src\design_by_contract.py
src\design_by_contract.py:31: error: Return type "UnresolvedSymbol" of "__eq__" incompatible with return type "bool" in supertype "object"
src\design_by_contract.py:44: error: Incompatible return value type (got "bool", expected "UnresolvedSymbol")
src\design_by_contract.py:62: error: The first argument to Callable must be a list of types, parameter specification, or "..."
src\design_by_contract.py:62: note: See https://mypy.readthedocs.io/en/stable/kinds_of_types.html#callable-types-and-lambdas
src\design_by_contract.py:67: error: Variable "design_by_contract.R" is not valid as a type
src\design_by_contract.py:67: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
Found 4 errors in 1 file (checked 1 source file)

Ran `poetry add --dev mypy@latest`
`error: Return type UnresolvedSymbol of __eq__ incompatible with return type bool in supertype object  [override]`:

correct, but this is overridden on purpose
Apparently, this was too much for mypy to handle statically, it
couldn't figure out that `P` is a `ParamSpec` etc:

```
src\design_by_contract.py:62: error: The first argument to Callable must be a list of types, parameter specification, or "..."  [misc]
src\design_by_contract.py:62: note: See https://mypy.readthedocs.io/en/stable/kinds_of_types.html#callable-types-and-lambdas
src\design_by_contract.py:67: error: Variable "design_by_contract.R" is not valid as a type  [valid-type]
src\design_by_contract.py:67: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
src\design_by_contract.py:82: error: Returning Any from function declared to return R?  [no-any-return]
```

All these are fixed by undoing the tuple assignment and doing it
'manually'.
We could do `np.ndarray[Any, np.dtype[A]]`, for example, but `NDArray`
is a shorthand:

https://numpy.org/doc/stable/reference/typing.html#numpy.typing.NDArray

Using `Any` is improper, a `TypeVar` would be better to signal all these
`Any` are actually the same type, but couldn't get that working.
@alexpovel
Copy link
Collaborator Author

Having mypy work allowed me to try and fix its indicated type issues. pytest still passes. Three issues remain:

$ mypy src/design_by_contract.py
src\design_by_contract.py:44: error: Incompatible return value type (got "bool", expected "UnresolvedSymbol")  [return-value]
src\design_by_contract.py:61: error: Untyped decorator makes function "contract" untyped  [misc]
src\design_by_contract.py:162: error: Untyped decorator makes function "spam" untyped  [misc]
Found 3 errors in 1 file (checked 1 source file)

The first one on line 44 is difficult, I'm not sure about your intent. It's returning True where an UnresolvedSymbol is expected.

As for the latter two: decorator is untyped, so we're losing out on that information. Perhaps a stub file would help, or seeing if the decorator dependency could be dropped.

@StefanUlbrich
Copy link
Owner

Great. I should have read the release notes. Replacing the decorators package is trivial (initially, it reduced more complexity than in the final version). Will give mypyc a shot too on the weekend

@alexpovel alexpovel changed the title Type fix and dict shortcut mypy typing fixes Jul 14, 2022
Copy link
Owner

@StefanUlbrich StefanUlbrich left a comment

Choose a reason for hiding this comment

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

Amazing! together with a modified return type for the __eq__ function, this removes the mypy errors (except for the decorator package which I will remove after merging). Big Thanks! My only ask is to revert the logging to the former style for performance reasons

pyproject.toml Show resolved Hide resolved
src/design_by_contract.py Show resolved Hide resolved
src/design_by_contract.py Show resolved Hide resolved
src/design_by_contract.py Show resolved Hide resolved
src/design_by_contract.py Show resolved Hide resolved
src/design_by_contract.py Show resolved Hide resolved
src/design_by_contract.py Outdated Show resolved Hide resolved
Also gets rid of having to ignore the type, so it's cleaner
@StefanUlbrich
Copy link
Owner

Okay, figured out the decorators (disadvantage: parantheses mandatory after @contract()–but that's ok). Waiting for the merge

@alexpovel alexpovel requested a review from StefanUlbrich July 15, 2022 20:56
@StefanUlbrich
Copy link
Owner

Thanks very much! I learned a bit here.

@StefanUlbrich StefanUlbrich merged commit 2c91be2 into StefanUlbrich:main Jul 15, 2022
@alexpovel
Copy link
Collaborator Author

Great! Glad this was useful to you. Just wanted to mention that poetry was the deciding factor: I had this up and running in seconds. Most other installation methods would've been too cumbersome to figure out 🙂

@StefanUlbrich
Copy link
Owner

Python tooling came a long way indeed. It really was enjoyable to bootstrap a complete project (albeit tiny) to experiment with all the new features. And I am pleasantly surprised by receiving support :-)

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.

2 participants