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 mypy type hinting check #1166

Merged
merged 24 commits into from
Jun 18, 2019
Merged

Add mypy type hinting check #1166

merged 24 commits into from
Jun 18, 2019

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Jun 11, 2019

  1. Fix Type hint linting #1002
  2. Refactor hash cache
  3. Note: since mypy doesn't support X = NewType('X', NONE-BASE-TYPE) (Fine-grained: Inconsistent behavior for nested NewType python/mypy#4615), I changed uint64 to class to make the check pass. I'll benchmark it to see if it increased too much overhead.
    • Also the BytesN custom types family

TODOs:

  • Make phase 0 CI pass
  • phase 1
  • Clean up
  • Benchmark

for challenge in state.custody_bit_challenge_records:
if get_current_epoch(state) > challenge.inclusion_epoch + CUSTODY_RESPONSE_DEADLINE:
slash_validator(state, challenge.responder_index, challenge.challenger_index)
for custody_chunk_challenge in state.custody_chunk_challenge_records:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workaround for mypy issue of redefining variable in the different scopes (python/mypy#5750).

@hwwhww
Copy link
Contributor Author

hwwhww commented Jun 13, 2019

Lazy benchmarking on running make test:
Before this PR: 106 passed in 145.90 seconds
With this PR: 106 passed in 162.60 seconds 😬

@hwwhww
Copy link
Contributor Author

hwwhww commented Jun 15, 2019

Re: benchmark
Run make ./eth2.0-spec-tests/tests/sanity for benchmarking
Before this PR: 106 passed in 15m 38s
With this PR: took 29m 1s 😨
/cc @protolambda

@@ -1140,15 +1140,15 @@ def slash_validator(state: BeaconState,
current_epoch = get_current_epoch(state)
initiate_validator_exit(state, slashed_index)
state.validator_registry[slashed_index].slashed = True
state.validator_registry[slashed_index].withdrawable_epoch = current_epoch + LATEST_SLASHED_EXIT_LENGTH
state.validator_registry[slashed_index].withdrawable_epoch = Epoch(current_epoch + LATEST_SLASHED_EXIT_LENGTH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If current_epoch and LATEST_SLASHED_EXIT_LENGTH are both of type Epoch do we need to wrap the sum with Epoch( ... )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Epoch is defined as "EpochNumber/EpochID" and LATEST_SLASHED_EXIT_LENGTH is defined as "diff between Epochs".

I'm slightly in favor of the status quo but I'm open to it; for Epoch and Slot, we can change it to the more flexible definition if we want. :)

@hwwhww
Copy link
Contributor Author

hwwhww commented Jun 15, 2019

I'm not so happy with the benchmark results but it's ready to get the first review.

There are some other uint64 in the specs, IMO we can also create custom types for some of them.

  • phase 0: genesis_time -> Timestamp
  • phase 1:
    • chunk_index -> ChunkIndex
    • challenge_index -> ChallengeIndex

/cc @JustinDrake what would you say?

@hwwhww hwwhww changed the title [WIP] Add mypy type hinting check Add mypy type hinting check Jun 15, 2019
slot: uint64
shard: uint64
slot: Slot
shard: Shard
beacon_chain_root: Bytes32
parent_root: Bytes32
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the hashes can be of type Hash (see PR1156 where I introduce the type).

@hwwhww
Copy link
Contributor Author

hwwhww commented Jun 17, 2019

@JustinDrake up-to-date now with #1156

scripts/build_spec.py Outdated Show resolved Hide resolved
scripts/build_spec.py Outdated Show resolved Hide resolved
scripts/build_spec.py Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
@hwwhww hwwhww merged commit 145e54b into dev Jun 18, 2019
@protolambda protolambda deleted the mypy branch June 22, 2019 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type hint linting
4 participants