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

Add SSZ utils to mypy checking scope #1707

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from
Draft

Add SSZ utils to mypy checking scope #1707

wants to merge 7 commits into from

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Apr 6, 2020

Issue

  1. Since --ignore-missing-imports was True and --disallow-subclassing-any was False, from pyspec side, it considers the types (e.g., uint64, Bytes32, Container) from the un-mypy-ized remerkleable are Any. (See: https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-disallow-subclassing-any for the details)
  2. We didn't strictly convert all Python integers in the spec to uint64. Stricter mypy setting can help us to detect the required conversion.

This current draft PR:

  1. It shows the lint configuration changes in Makefile and mypy configuration file mypy.ini.
  2. I used stubgen to create the remerkleable and eth2spec.utils.ssz mypy stubs.

Error log: https://gist.github.com/hwwhww/cacdaa8842ec62241c8ff03e1ca56ba3

Things we can do to clean the errors and improve the typings:

  1. For the errors like eth2spec/phase0/spec.py:466: error: Argument 1 to "int_to_bytes" has incompatible type "int"; expected "uint64"
    • We can solve by modifying spec easily.
  2. For the errors of eth2spec/phase0/spec.py:48: error: Class cannot subclass 'Bytes32' (has type 'Any') on class Root(Bytes32): pass:
    • It required to solve mypy errors in remerkleable, or we can try to ignore it.
    • Fixing in the side of the stubs can fix some errors, but maybe not all of them.
  3. For the errors of complex types length assignment, e.g., Variable "eth2spec.phase0.spec.MAX_VALIDATORS_PER_COMMITTEE" is not valid as a type on attesting_indices: List[ValidatorIndex, MAX_VALIDATORS_PER_COMMITTEE]:
    • it seems we need to either modify remerkleable typing, or we can try to ignore it.

@protolambda what do you think about the stricter SSZ typing checks? :)

@protolambda
Copy link
Collaborator

I'll look into Mypy for remerkleable. The tricky part here is that integers in generics don't work in python without the verbosity of Literal or other workarounds. And Mypy only liking Generic out of the box, while the code of Generic, and ABC typing, is honestly a mess (subclasses registering, forced metaclass with ABC, and other pains).
I think the Protocol feature in python 3.8 does a much better job: provide static ducktyping instead of faking Java patterns. That is, you can be explicit about interfaces you need and expect, but free to implement it the way it works best.

The powerful part of Python to me is not its type checking or concurrency (both only have workarounds, not actual solutions). It's the ability to script and express things with minimal overhead. Syntax sugar and ability to define and interact easily with SSZ and the spec are special; no other client/tool makes it that easy to build/test some consensus thing. I am happy to introduce typing closer to mypy, but prefer not to reduce Python to Java.

Also, we can definitely make more constants typed, but I also like the idea of constants getting their type automatically from their usage. But that may be too much of a Go-lang thing.

@hwwhww
Copy link
Contributor Author

hwwhww commented Apr 10, 2020

@protolambda
Thanks for the feedback! Very much agree.
The benefit of getting remerkleable mypy-compatible is helping us to detect potential bugs, and also making remerkleable stronger. However, I understand the syntax sugar in remerkleable makes it difficult to fit mypy rules easily without explicit modification; also, sometimes the problems are in mypy side. So if it's really too annoying to make mypy checks pass in remerkleable, IMO it's okay to take remerkleable as a black box by handling it in PySpec side. See if we can use some workaround (e.g., ignoring some cases + only modifying the stubs) since it probably doesn't worth too much time for it.

@hwwhww hwwhww changed the base branch from dev to v012x April 23, 2020 15:01
@djrtwo djrtwo changed the base branch from v012x to dev May 20, 2020 23:13
@protolambda protolambda added the general:typing Spec typing, no breaking changes label Jul 8, 2020
@hwwhww hwwhww added this to the 🔵 v1.0.0 Nice-to-have milestone Aug 21, 2020
@hwwhww hwwhww force-pushed the hwwhww/mypy_ssz branch from a4f049b to db139c9 Compare May 11, 2021 13:29
@hwwhww hwwhww force-pushed the hwwhww/mypy_ssz branch from e3dbfbc to a77f782 Compare April 4, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:typing Spec typing, no breaking changes scope:CI/tests/pyspec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants