Add type hints For eth/utils module #1404

Bhargavasomu · 2018-10-17T14:11:41Z

What was wrong?

The eth module was not type hinted till now. This needs to be fixed and also the tox.ini file needs to be updated.
Issue : #1398

How was it fixed?

Adding type hints to the arguments and the function. The phase of adding the type hints is as follows

eth.utils (Started previously and hence wanted to finish it)
eth.tools
eth.vm
Everything else which is leftover

Cute Animal Picture

Bhargavasomu · 2018-10-17T15:29:27Z

@cburgdorf I have done the type hinting for eth.utils module. Please verify it.

cburgdorf

Great start! Main problem seems that you silenced a bunch of errors in tox.ini which are caused by these changes. Also left some inline comments.

cburgdorf · 2018-10-17T19:41:58Z

tox.ini

@@ -104,7 +104,8 @@ commands=
    {[common-lint]commands}
    flake8 {toxinidir}/tests --exclude="trinity,p2p"
    # TODO: Drop --ignore-missing-imports once we have type annotations for eth_utils, coincurve and cytoolz
-    mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs -p eth
+	mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs --disallow-untyped-defs --disallow-any-generics eth/utils


This silences all type errors in the eth module except those that are in eth.utils. The tox.ini setup should continue to be what it is today. As soon as you have finished one of the bigger chunks (eth.tools, eth.vm) you can add an additional check for those. This will essentially result in slightly increased type checking time as it means we are checking the entire eth module with less restrictive checks and then eth.tools (or eth.vm) again but with stricter checks. That's why I consider it impractical to add additional checks for each and every submodule now.

In the ideal case, it will just be a short period of days where type checks are less strict in CI compared to the actual type hints being used. Then, once the entire library has been properly typed, we activate the stricter checks for the entire eth module.

@cburgdorf how about just two mypy runs? one that has the current config and one that we slowly expand to cover only the modules which have been updated.

@pipermerriam yes, that's my plan. I just think we do not need to do it for each and every submodule (hence,eth.tools, eth.vm, then the rest) especially if we consider this whole thing to be finished over the course of the next, say, two weeks.

@cburgdorf got it. But since I have started on eth.utils first unfortunately, how about we have one mypy for the normal configuration that we see today, one for the eth.utils submodule and one for the presently working submodule (eth.tools and eth.vm)? Please correct me if I am missing something.

Cool with me!

cburgdorf · 2018-10-17T19:45:39Z

eth/utils/bls.py

    final_exponentiation = final_exponentiate(
        pairing(decompress_G2(sig), G1, False) *
        pairing(hash_to_G2(m), neg(decompress_G1(pub)), False)
    )
    return final_exponentiation == FQ12.one()


-def aggregate_sigs(sigs):
+def aggregate_sigs(sigs: List[Tuple[int, int]]) -> Tuple[int, int]:


This can be def aggregate_sigs(sigs: Iterable[Tuple[int, int]]) -> Tuple[int, int]:

See: https://github.com/ethereum/ethereum-dev-tactical-manual/blob/master/style-guide.md#function-variance

cburgdorf · 2018-10-17T19:46:18Z

eth/utils/bls.py

    o = Z2
    for s in sigs:
        o = add(o, decompress_G2(s))
    return compress_G2(o)


-def aggregate_pubs(pubs):
+def aggregate_pubs(pubs: List[int]) -> int:


Same here, can be Iterable[int]

cburgdorf · 2018-10-17T19:57:01Z

eth/utils/db.py

    for account, account_data in state_dict.items():
+        assert isinstance(account_data["balance"], int)


These assert statements may be problematic as they involve a runtime cost. This may not be a big problem for this particular piece of code but I think we could get rid of them if we used something like a TypedDict here. /cc @pipermerriam @carver

cburgdorf · 2018-10-17T20:02:13Z

eth/utils/numeric.py

@@ -69,12 +72,13 @@ def is_odd(value: int) -> bool:
    return value % 2 == 1


-def get_highest_bit_index(value):
+def get_highest_bit_index(value: int) -> int:


I think the preferred way is:

def get_highest_bit_index(value: int) -> int: value >>= 1 for bit_length in itertools.count(): if not value: return bit_length value >>= 1 raise Exception("Invariant: unreachable code path")

cburgdorf · 2018-10-17T20:03:15Z

eth/utils/rlp.py

+    BaseBlock,
+)
+
+DiffObjectType = Union[None, Iterable[Tuple[str, str, str]]]


Union[None, Something] should be equivalent to Optional[Something] but the latter reads more naturally

cburgdorf · 2018-10-17T20:05:10Z

eth/utils/state.py

    for account, account_data in sorted(expected_state.items()):
+        assert isinstance(account_data['balance'], int)


Same here about the assert statements. TypedDict should help.

Bhargavasomu · 2018-10-18T08:46:10Z

I added the mypy-extensions module to setup.py, but still travis is showing "No module named mypy_extensions"

cburgdorf · 2018-10-18T09:40:06Z

@Bhargavasomu it comes with mypy by default and is already being used here:

py-evm/trinity/protocol/common/handlers.py

Lines 59 to 69 in e68e7c3

    
           from mypy_extensions import DefaultArg 
        
           BlockHeadersCallable = Callable[ 
        
               [ 
        
                   BaseExchangeHandler, 
        
                   BlockIdentifier, 
        
                   int, 
        
                   DefaultArg(int, 'skip'), 
        
                   DefaultArg(int, 'reverse') 
        
               ], 
        
               Awaitable[Tuple[BlockHeader, ...]] 
        
           ]

cburgdorf · 2018-10-18T09:42:04Z

eth/utils/state.py

+# 'balance', 'nonce' -> int
+# 'code' -> bytes
+# 'storage' -> Dict[int, int]
+AccountDetails = TypedDict('AccountDetails',


cburgdorf · 2018-10-18T09:43:00Z

tox.ini

@@ -104,7 +104,9 @@ commands=
    {[common-lint]commands}
    flake8 {toxinidir}/tests --exclude="trinity,p2p"
    # TODO: Drop --ignore-missing-imports once we have type annotations for eth_utils, coincurve and cytoolz
-    mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs -p eth
+	mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs -p eth


Looks like you accidentally used tabs instead of four spaces here.

Bhargavasomu · 2018-10-18T09:57:44Z

@cburgdorf, since the whole of eth is not type hinted the following command would fail right?
mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs -p eth
This is present in the present tox.ini file, but it would fail eventually because the whole eth module is not yet type hinted fully. So shouldn't we remove that command till we finish the whole type hinting? Is there something I am missing?

cburgdorf · 2018-10-18T13:25:21Z

@Bhargavasomu The type hinting command that is currently already used should stay in place until it is fully converted. We can incrementally add additional checks but should not lower the already existing checks.

Bhargavasomu · 2018-10-18T16:34:37Z

@cburgdorf then how would we know exactly why the testcases are failing for, because of the requirement of the type hinting of the whole eth module or the faults in the linting or some scenario in the present module for which we have made changes for.
Also the above test cases are failing because we haven't put the typing for all of the eth module. Is that ok for them to fail, till we have completed the type hinting for the whole module.
If yes, then can I move onto Type Hinting the other modules (eth.tools)?

cburgdorf · 2018-10-18T17:49:46Z

Stricter type hints do automatically qualify less strict checks. That said, if you add stricter type hints in e.g. eth.utils that may unveil issues in other parts of the code base, hence you see that now some other things outside of eth.utils fail. Fixing these can either mean:

a.) something in eth.utils is typed incorrectly
b.) something outside of eth.utils is typed incorrectly

This is why, even if you aim to just add type hints for module x it may involve changing things in module y in the same PR.

Is that ok for them to fail, till we have completed the type hinting for the whole module.

The rules are:

We do not lower type checks at any point
We do never merge PRs that fail in CI

To elaborate on the second point. If we decided to merge a PR that fails CI, even if it is only for type hints, it becomes hard / impossible to spot any other CI failures as master would be broken for everyone else.

Bhargavasomu · 2018-10-19T12:15:11Z

@cburgdorf I have made the necessary changes so that the present eth type hinting is not breaked. I have also added the necessary type hinting for eth.utils and eth.tools.builder. But the test cases are failing because of ImportError: No module named 'mypy_extensions'. Then I took a look at this

Eventhough mypy is part of the setup, it is not being installed here. Could you help me out here?
Because I have tested it thoroughly in my local machine, but when I push it, throwing a mypy_extensions module not found error.

cburgdorf · 2018-10-19T12:30:25Z

Eventhough mypy is part of the setup, it is not being installed here

Right, I guess mypy is currently only installed for the linting jobs itself. I guess there are two ways to go from here:

Add mypy as a dependency to eth

py-evm/setup.py

Lines 7 to 19 in e68e7c3

    
           'eth': [ 
        
               "cryptography>=2.0.3,<3.0.0", 
        
               "cytoolz>=0.9.0,<1.0.0", 
        
               "eth-bloom>=1.0.0,<2.0.0", 
        
               "eth-keys>=0.2.0b3,<1.0.0", 
        
               "eth-typing>=2.0.0,<3.0.0", 
        
               "eth-utils>=1.3.0b0,<2.0.0", 
        
               "lru-dict>=1.1.6", 
        
               "py-ecc>=1.4.2,<2.0.0", 
        
               "pyethash>=0.1.27,<1.0.0", 
        
               "rlp>=1.0.3,<2.0.0", 
        
               "trie>=1.3.5,<2.0.0", 
        
           ],

Use if TYPE_CHECKING to only import from mypy_extensions during mypy runs

py-evm/trinity/protocol/common/handlers.py

Lines 58 to 59 in e68e7c3

    
           if TYPE_CHECKING: 
        
               from mypy_extensions import DefaultArg

Honestly, I have no preference. Since we already use if TYPE_CHECKING to guard around the mypy_extensions you may just continue doing that. On the other hand I don't see why we should treat this library any different than other libraries we depend on.

Feel free to try and pick any of both options. Chances are @carver or @pipermerriam have a stronger preference for one or the other way but it will be trivial to move back and forth so whatever moves this forward is fine by me for now.

cburgdorf

This is looking better and better! Just a couple of minor things left! 👍

cburgdorf · 2018-10-19T12:38:28Z

eth/utils/rlp.py


 @to_tuple
-def diff_rlp_object(left, right):
+def diff_rlp_object(left: BaseBlock, right: BaseBlock) -> DiffObjectType:


Since this is only being used once, I'd prefer to use the type in it longer form and just reformat as:

def diff_rlp_object(left: BaseBlock, right: BaseBlock) -> Optional[Iterable[Tuple[str, str, str]]] ...

cburgdorf · 2018-10-19T12:42:53Z

eth/utils/state.py

+    Union,
+)
+
+from mypy_extensions import (


Based on what you choose from the stated options you may need to add an if TYPECHECKING here to only do that import under mypy runs.

cburgdorf · 2018-10-19T12:43:51Z

eth/utils/state.py

 from eth_utils import (
    to_tuple,
 )

+from eth_typing import (


nitpick: eth_typing should be above eth_utils (alphabetical order within 3rd party libraries)

cburgdorf · 2018-10-19T12:44:55Z

eth/utils/state.py

+)
+
+
+# Mapping from address to account state.


I think we can get rid of this comment now that we are using a TypedDict

cburgdorf · 2018-10-19T12:46:06Z

eth/utils/state.py

+                            })
+AccountState = Dict[Address, AccountDetails]
+
+DiffType = Iterable[Tuple[Address, str, Union[int, bytes], Union[int, bytes]]]


Let's rename this to AccountDiff.

cburgdorf · 2018-10-19T12:47:43Z

tox.ini

@@ -105,6 +105,9 @@ commands=
    flake8 {toxinidir}/tests --exclude="trinity,p2p"
    # TODO: Drop --ignore-missing-imports once we have type annotations for eth_utils, coincurve and cytoolz
    mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs -p eth
+    mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs --disallow-untyped-defs --disallow-any-generics eth/utils


Rather than using eth/utils and eth/tools/builder can you use -p eth.utils and -p eth.tools.builder

Bhargavasomu · 2018-10-19T13:43:47Z

@cburgdorf sorry for pinging you a lot, but these errors don't seem to be related to the type hinting. Some assert statements are failing because of the fixtures which I am not able to understand properly. Could you please tell me what to do?

cburgdorf · 2018-10-19T14:05:00Z

but these errors don't seem to be related to the type hinting. Some assert statements are failing because of the fixtures

The reason you are see these is because you (accidentally) changed the fixed commit of the fixtures sub repository in your PR to point to newer test cases.

These newer test cases do fail unless you rebase your branch against latest master where changes have been made to make these test cases work.

cburgdorf · 2018-10-19T14:31:06Z

@Bhargavasomu have a look at #1412

Resolved fixtures issue + rebased against latest master
Add "mypy_extensions>=0.4.1,<1.0.0", to the eth section (instead of mypy
Change tox.ini do install the eth dependencies

cburgdorf · 2018-10-19T15:12:42Z

@Bhargavasomu I'm going to drive this home in #1412 and apply some fine tuning on top of it.

You can consider this done and move on adding type hints to other parts. Thank you for pushing this forward. Great work! 👍 I guess the learnings from this PR will make the upcoming PR chunks easier.

One thing regarding commit history. I'm quite keen on getting clean semantic commits, which means if there are many wip commits, I'll squash them all together into one semantic commit. You can avoid this by cutting semantic commits yourself and force pushing into the PR.

Bhargavasomu · 2018-10-19T15:16:36Z

@cburgdorf sorry for bugging you a lot through this PR. I definitely learnt a lot which would help me type hinting the rest of the parts. Thankyou

cburgdorf · 2018-10-19T15:41:38Z

sorry for bugging you a lot through this PR. I definitely learnt a lot which would help me type hinting the rest of the parts. Thankyou

No problem! I know that adding types can be quite challenging in the beginning. Especially in py-evm. I'm super happy to have you do this and I'm excited for py-evm soon be fully typed!

cburgdorf requested changes Oct 17, 2018

View reviewed changes

cburgdorf reviewed Oct 18, 2018

View reviewed changes

Bhargavasomu added 13 commits October 19, 2018 17:17

Add type hints For eth/utils module

56f5637

Add type hints For eth/utils module

5684470

Add type hints For eth/utils module

dbb9f60

Add type hints For eth/utils module

2acb5bf

Add type hints For eth/utils module

e366a93

Add type hints For eth/utils module

38fd02d

Add type hints For eth/utils module

7721b81

Add Better Type Hinting for

a3eb77b

Add Better Type Hinting for

a9503ea

Add mypy-extensions to setup

f9d0d4e

Add mypy-extensions to setup

6f0f438

Enable Type Hints for eth.utils and eth.tools.builder

0c7b878

Enable type hinting for eth.utils and eth.tools.builder

64030b8

Bhargavasomu force-pushed the eth_type_hinting branch from 4cf7e18 to 64030b8 Compare October 19, 2018 11:52

Enable type hinting for eth.utils and eth.tools.builder

a3fbd16

cburgdorf reviewed Oct 19, 2018

View reviewed changes

Enable type hinting for eth.utils and eth.tools.builder modules

8eb09eb

cburgdorf mentioned this pull request Oct 19, 2018

Christoph/fix/typehints #1412

Merged

cburgdorf closed this Oct 19, 2018

Bhargavasomu deleted the eth_type_hinting branch November 8, 2018 06:28

		for account, account_data in state_dict.items():
		assert isinstance(account_data["balance"], int)

		for account, account_data in sorted(expected_state.items()):
		assert isinstance(account_data['balance'], int)

Add type hints For eth/utils module #1404

Add type hints For eth/utils module #1404

Conversation

Bhargavasomu commented Oct 17, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

Bhargavasomu commented Oct 17, 2018 • edited Loading

cburgdorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhargavasomu Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhargavasomu commented Oct 18, 2018

cburgdorf commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhargavasomu commented Oct 18, 2018

cburgdorf commented Oct 18, 2018

Bhargavasomu commented Oct 18, 2018

cburgdorf commented Oct 18, 2018

Bhargavasomu commented Oct 19, 2018 • edited Loading

cburgdorf commented Oct 19, 2018

cburgdorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bhargavasomu commented Oct 19, 2018

cburgdorf commented Oct 19, 2018

cburgdorf commented Oct 19, 2018

cburgdorf commented Oct 19, 2018

Bhargavasomu commented Oct 19, 2018

cburgdorf commented Oct 19, 2018

Bhargavasomu commented Oct 17, 2018 •

edited

Loading

Bhargavasomu commented Oct 17, 2018 •

edited

Loading

Bhargavasomu Oct 18, 2018 •

edited

Loading

Bhargavasomu commented Oct 19, 2018 •

edited

Loading