Eth leftover type hinting #1456

Bhargavasomu · 2018-11-09T13:53:57Z

What was wrong?

Completes the type hinting in the eth module.
Closes #1398

How was it fixed?

Adding the type hints manually and adjusting in other modules like trinity and p2p.

Cute Animal Picture

Bhargavasomu · 2018-11-09T14:06:54Z

@cburgdorf I have tried my level best to tighten the types. I have used type: ignore in the following conditions.

Using type: ignore in eth.chains.base because the following error is coming
Invalid self argument "Chain" to attribute function "gas_estimator" with type "Callable[[BaseState, BaseTransaction], int]"
Using type:ignore where __contains__ is defined and that particular class inherits MutableMapping or Mapping
Where the breakage of LSP is causing mypy problem (Mentioned in code comments)

Apart from that in some places I was forced to use VM instead of BaseVM because the BaseVM class didn't have the apply_all_transactions method.

Will make the improvements as per the review.

cburgdorf

Looking pretty good! My comments are mostly smaller things and then I'll give that another round of review where I try to wrestle with some cast / ignore / Any cases to see if we can tighten things further. These are hard for me to tell by just skimming over the PR.

eth/chains/base.py

cburgdorf · 2018-11-09T16:19:27Z

eth/chains/header.py

@@ -99,15 +110,15 @@ def from_genesis_header(cls,
        """
        Initializes the chain from the genesis header.
        """
-        headerdb = cls.get_headerdb_class()(base_db)
+        headerdb = cls.get_headerdb_class()(cast(BaseAtomicDB, base_db))


eth/db/cache.py

cburgdorf · 2018-11-09T16:25:14Z

eth/db/journal.py

@@ -121,7 +121,7 @@ def commit_changeset(self, changeset_id: uuid.UUID) -> Dict[bytes, bytes]:
    #
    # Database API
    #
-    def __getitem__(self, key: bytes) -> Union[bytes, DeletedEntry]:
+    def __getitem__(self, key: bytes) -> Union[bytes, DeletedEntry]:    # type: ignore # Breaks LSP


I'll double check this and other cases of # type: ignore in another round of review when I get my hands on the code. Just reviewing through github now.

tox.ini

trinity/chains/light.py

Bhargavasomu · 2018-11-09T18:38:36Z

@cburgdorf made the changes requested. Please take a look.

cburgdorf · 2018-11-12T15:42:55Z

eth/db/diff.py

        else:
-            return result
+            return cast(bytes, result)


@pipermerriam @carver I'm worried about the usage of cast in performance sensitive code paths.

Here's @gvanrossum from 2016

I would really hope we won't need to use or encourage casts here, since it's an expensive runtime operation (at least it is until we teach CPython about it -- right now it is a user-defined function, which is incredibly slow compared to a plain assignment)

I'm not sure if anything has changed about this in the meantime but if not then I assume that even if cast is a noop at runtime it is still one extra function call and hence will slow things down. Therefore I'd advocate to use type: ignore in such performance sensitive code paths. Thoughts?

If the best solution we have is a cast, then yes, a type: ignore seems like a fine alternative to me. (given that it's in a performance-sensitive line, and the line is doing exactly one thing -- we don't want to accidentally ignore other type checks on the same line).

cburgdorf

@Bhargavasomu This looks pretty good! I pushed one more commit into this PR which uses a neat trick (StaticMethod) that @carver came up with to get rid of some ignores. I believe we could apply the same trick to a bunch of other places that currently use type: ignore.

However, I have one main concern which I'd like to get @pipermerriam @carver thoughts on. It's about the usage of cast() for performance sensible code paths (those that are called very often). I believe that there's a (small) runtime cost of cast (even though the mypy docs say there isn't). Not sure.

Bhargavasomu · 2018-11-12T16:25:36Z

@cburgdorf I highly doubt that there would be considerable performance changes. Doing a cast(A, B) would be the same as Address(<some var>) right? So what difference would that cause?

cburgdorf · 2018-11-12T17:36:20Z

@Bhargavasomu well, it shouldn't matter for most cases but unless CPython optimized out the cast it is still a noop (a function that does nothing) and if a function is called in performance critical code which is called many times in a loop then that may still be a problem.

See also my citation of Guido van Rossum (creator of Python)

I would really hope we won't need to use or encourage casts here, since it's an expensive runtime operation (at least it is until we teach CPython about it -- right now it is a user-defined function, which is incredibly slow compared to a plain assignment)

I'm just trying to play it safe here. So my personal preference for performance critical code would be to use type: ignore over cast if needed (at all).

Bhargavasomu · 2018-11-13T17:30:31Z

@cburgdorf could you please tell me which files are performance critical, so that I could replace cast with type: ignore?

cburgdorf · 2018-11-13T19:14:53Z

@Bhargavasomu I'll take care of that. Applying some final finish.

cburgdorf · 2018-11-13T19:57:28Z

@Bhargavasomu solid work! 👍

I wrangled with some corner cases to get rid of some cast and type: ignore and also migrated perf critical casts to type: ignore (as discussed).

I believe there are still plenty of cases where we could do better so if you like that sort of stuff feel free to continue wrangling with type: ignore, cast and Any and send follow up PRs :)

Bhargavasomu · 2018-11-14T02:16:31Z

Thankyou for helping me out @cburgdorf

rayrapetyan · 2018-11-14T02:21:04Z

hey guys, what about other bounty-marked tickets merged recently lol ))

cburgdorf · 2018-11-14T09:54:09Z

@rayrapetyan are you saying we missed paying out a bounty? Can you point me to it?

rayrapetyan · 2018-11-14T15:41:48Z

@cburgdorf, e.g. #1402, thanks!

cburgdorf · 2018-11-15T09:56:58Z

@rayrapetyan I'm afraid that ticket did not have a Gitcoin bounty assigned. I assume that the bounty label was added with the intention to channel it through the bounty process but then it didn't happen for whatever reasons (probably just forgotten)

I'm afraid we can't retroactively give out money for work that has already happened and had no bounty associated at that time. But I recognize that you've been doing great work and there's plenty of opportunity for you to do paid work on this project.

How would you feel about writing a Proof of Concept GraphQL plugin as described in https://github.com/ethereum/py-evm/issues/1462. Is this something that you'd have fun with?

rayrapetyan · 2018-11-16T17:24:31Z

@cburgdorf, no worries, I understand. I'm currently stuck with issue #62, once completed I can start with #1462. Thanks.

cburgdorf reviewed Nov 9, 2018

View reviewed changes

Bhargavasomu force-pushed the eth_leftover_type_hinting branch from 270452d to 2c2b568 Compare November 9, 2018 18:30

cburgdorf reviewed Nov 12, 2018

View reviewed changes

Bhargavasomu and others added 10 commits November 13, 2018 20:30

Enable complete type hinting for eth.beacon

298feda

Enable complete type hinting for eth.precompiles

00a7539

Enable complete type hinting for eth.rlp

d39f511

Enable complete type hinting for eth.chains

5f4ea87

Enable complete type hinting for eth.estimators

f876af0

Enable complete type hinting for eth.db

aa78ff9

Adjust Type Hinting in other modules, Update tox.ini

3e995f5

Move and reuse StaticMethod helper tighten type support

b71bad9

Make perf critical places use ignore over cast

5ed1c4f

Get rid of ignores regarding SpoofTransaction

1a36a01

cburgdorf force-pushed the eth_leftover_type_hinting branch from b812a12 to 1a36a01 Compare November 13, 2018 19:43

cburgdorf approved these changes Nov 13, 2018

View reviewed changes

cburgdorf merged commit 9870841 into ethereum:master Nov 13, 2018

Bhargavasomu deleted the eth_leftover_type_hinting branch December 17, 2018 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eth leftover type hinting #1456

Eth leftover type hinting #1456

Bhargavasomu commented Nov 9, 2018

Bhargavasomu commented Nov 9, 2018

cburgdorf left a comment

cburgdorf Nov 9, 2018

cburgdorf Nov 9, 2018

Bhargavasomu commented Nov 9, 2018

cburgdorf Nov 12, 2018 •

edited

Loading

carver Nov 12, 2018

cburgdorf left a comment

Bhargavasomu commented Nov 12, 2018 •

edited

Loading

cburgdorf commented Nov 12, 2018 •

edited

Loading

Bhargavasomu commented Nov 13, 2018

cburgdorf commented Nov 13, 2018

cburgdorf commented Nov 13, 2018

Bhargavasomu commented Nov 14, 2018

rayrapetyan commented Nov 14, 2018

cburgdorf commented Nov 14, 2018

rayrapetyan commented Nov 14, 2018

cburgdorf commented Nov 15, 2018

rayrapetyan commented Nov 16, 2018

Eth leftover type hinting #1456

Eth leftover type hinting #1456

Conversation

Bhargavasomu commented Nov 9, 2018

What was wrong?

How was it fixed?

Cute Animal Picture

Bhargavasomu commented Nov 9, 2018

cburgdorf left a comment

Choose a reason for hiding this comment

cburgdorf Nov 9, 2018

Choose a reason for hiding this comment

cburgdorf Nov 9, 2018

Choose a reason for hiding this comment

Bhargavasomu commented Nov 9, 2018

cburgdorf Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

carver Nov 12, 2018

Choose a reason for hiding this comment

cburgdorf left a comment

Choose a reason for hiding this comment

Bhargavasomu commented Nov 12, 2018 • edited Loading

cburgdorf commented Nov 12, 2018 • edited Loading

Bhargavasomu commented Nov 13, 2018

cburgdorf commented Nov 13, 2018

cburgdorf commented Nov 13, 2018

Bhargavasomu commented Nov 14, 2018

rayrapetyan commented Nov 14, 2018

cburgdorf commented Nov 14, 2018

rayrapetyan commented Nov 14, 2018

cburgdorf commented Nov 15, 2018

rayrapetyan commented Nov 16, 2018

cburgdorf Nov 12, 2018 •

edited

Loading

Bhargavasomu commented Nov 12, 2018 •

edited

Loading

cburgdorf commented Nov 12, 2018 •

edited

Loading