[WIP] [BLOCKED] Add Python 3.8 testing #1885

veox · 2019-11-28T16:06:54Z

Superseded by PR #1940.

Blocked by pyflakes; see #1885 (comment).

What was wrong?

Nothing; Python 3.8 has been released.

How was it fixed?

See #1872 for even more verbose noise.

Note: Many lint fixes are 99.99% cargo-culting, especially those relating to low-level APIs, e.g. AccountDatabaseAPI, OpcodeAPI, VirtualMachineAPI. I don't understand the structuring of the codebase to make an informed decision on what has to be chiseled, the underlying ABCs or the consumers. So my principle is/was "less LOCs changed == better".

Now installing libleveldb-dev on all CI jobs, so we can get it on Python 3.8 jobs, so it can build a plyvel wheel locally (otherwise not yet available).
Updated coincurve dep to latest: that has a Python 3.8 wheel: fixes installation on Arch Linux.
Updated pyflakes dep to latest (so it has Python 3.8 support).
Updated mypy due to nested dependency (typed-ast) local wheel build fail.
- Fixed what newer mypy found to be broken (lint jobs across all Pythons).
Updated hypothesis to latest (not much consideration given).
- Fixed introduced failure (core jobs across all Pythons).

Closes #1872 as implemented.

To-Do

Clean up commit history.
Add entry to the release notes.
Update docs.
Try running trinity and see if API interfaces are now broken.

Cute Animal Picture

Source: Cmycherrytree, via imgur

veox · 2019-12-02T18:36:54Z

tests/core/code-stream/test_code_stream.py

@@ -203,7 +203,10 @@ def test_new_vs_reference_code_stream_iter(bytecode):
    assert latest.program_counter == reference.program_counter


-@given(read_len=st.integers(min_value=0), bytecode=st.binary(max_size=128))
+@given(
+    read_len=st.integers(min_value=0, max_value=2048),


This (here and in test below) certainly requires review.

max_value=2**63-1 works just fine, too. 2**63 (gets passed implicitly if max_value not present) is what breaks the test.

See also commit message, copied below.

A failure happens because the underlying io.BytesIO.read(size) call gets a size=9223372036854775808. That's (2^63), but it seems that the biggest size that BytesIO can read is (2^63)-1.

The error surfaced after an update of package hypothesis from v3 to v4 (version that is latest ATM), and is possibly an issue from upstream. Or it could be ours.

Anyway, limiting read_len fixes the issue. Could use (2^63)-1, too, but considering that bytecode max_size is limited, I don't see the point.

Likely a limitation of CPython; I'd guess the read index is an int64 on 64-bit machines (since -1 is a valid value). Also possible that hypothesis doesn't know.

This seems fine to me.

veox · 2019-12-05T14:34:38Z

Remaining lint failures (after commit 7ffeef7) are actual inconsistencies in the codebase.

voith · 2019-12-05T16:34:36Z

You might also want to bump the minimum supported eth-utils version to v1.8.0 as Python3.8 support was added in that version.

veox · 2019-12-06T10:52:56Z

eth/abc.py

@@ -775,7 +775,7 @@ def __call__(self, computation: 'ComputationAPI') -> None:
    def as_opcode(cls: Type[T],
                  logic_fn: Callable[['ComputationAPI'], None],
                  mnemonic: str,
-                  gas_cost: int) -> Type[T]:
+                  gas_cost: int) -> T:


ABC made same as the consumer:

py-evm/eth/vm/opcode.py

Lines 39 to 43 in 3f33ac8

@classmethod

def as_opcode(cls: Type[T],

logic_fn: Callable[..., Any],

mnemonic: str,

gas_cost: int) -> T:

Before the change, mypy said:

eth/vm/opcode.py:40: error: Return type "Opcode" of "as_opcode" incompatible with return type "Type[Opcode]" in supertype "OpcodeAPI"

Yeah, I agree with your call here, too. The implementation is clearly returning an Opcode instance instead of class, and would require a lot of refactoring to change (if we even wanted to).

The only thing I'd add it that the ABC docs should update, too.

veox · 2019-12-06T11:10:55Z

eth/vm/state.py

@@ -51,7 +51,7 @@ def __init__(
            self,
            db: AtomicDatabaseAPI,
            execution_context: ExecutionContextAPI,
-            state_root: bytes) -> None:
+            state_root: Hash32) -> None:


@carver This is the example of cargo-culting I mentioned on gitter: consumer changed to fit the ABC; previous commit went the other way (ABC changed to appease the consumer).

mypy said (before the change):

eth/vm/state.py:57: error: Argument 2 to "AccountDatabaseAPI" has incompatible type "bytes"; expected "Hash32"

... so I gave it exactly what it asked. But I don't know if it's right or not.

Yeah, in this case, it looks like the right thing to do to go with Hash32, since it's logically the hash of the root node. 👍

Should has_root() of AccountDB (and ABC AccountDatabaseAPI) have a state_root argument that's Hash32 also?..

py-evm/eth/db/account.py

Lines 135 to 136 in 0d79bb6

def has_root(self, state_root: bytes) -> bool:

return state_root in self._batchtrie

This doesn't produce a linting error; just wondering if this would be right to change now, too.

eth/abc.py

veox · 2019-12-10T19:28:14Z

Remaining errors in py36-lint seem related:

eth/vm/state.py:164: error: Incompatible return value type (got "Tuple[Hash32, JournalDBCheckpoint]", expected "Tuple[Hash32, UUID]")
eth/vm/state.py:172: error: Argument 1 to "discard" of "AccountDatabaseAPI" has incompatible type "UUID"; expected "JournalDBCheckpoint"
eth/vm/state.py:176: error: Argument 1 to "commit" of "AccountDatabaseAPI" has incompatible type "UUID"; expected "JournalDBCheckpoint"

This has me scratching my head.

veox · 2019-12-10T19:45:34Z

More importantly to subj, py38-lint fails with:

  File "/home/circleci/repo/.tox/py38-lint/lib/python3.8/site-packages/pyflakes/checker.py", line 696, in getNodeHandler
    self._nodeHandlers[node_class] = handler = getattr(self, nodeType)
AttributeError: 'FlakesChecker' object has no attribute 'CONSTANT'

This comes from pyflakes, a nested dependency; the issue has already been reported upstream and fixed. It's not in a tagged release, however - therefore marking PR blocked.

While waiting for pyflakes, I decided to review how much these changes impact trinity. Things are not looking good.

There are nested dependencies that have similar pinnings as py-evm has currently. libp2p (with its coincurve pinned in the latest tagged release) jumps out immediately, as that'll produce a pip installation error.

Downstream projects with py-evm as a dependency will get stuck on old versions of py-evm if this gets merged, until a time when other dependencies get their dependencies updated...

"The hell you make is the one you deserve."

Minimal set of dependencies that otherwise break the virtualenv, detailed here: ethereum#1872 (comment)

That version added support for Python 3.8 internally: https://github.com/ethereum/eth-utils/blob/master/docs/releases.rst#eth-utils-180-2019-11-04 This is also what's being installed according to `pip freeze`.

Python package `plyvel` doesn't have pre-built wheels for Python 3.8 yet, so the CI machines need to build the wheel themselves. They need LevelDB header files for that. This change installs the headers system-wide. SQUASHED: CircleCI YAML can't handle complex map merging via anchors. So, do the blunt thing, and install the dependency on _all_ CI runs, even ones that don't need it.

... and CodeStream, of course - it's just that SlowCodeStream comes first, so the test fails on it. A failure happens because the underlying io.BytesIO.read(size) call gets a size=9223372036854775808. That's (2^63), but it seems that the biggest size that BytesIO can read is (2^63)-1. The error surfaced after an update of package `hypothesis` from v3 to v4 (version that is latest ATM), and is possibly an issue from upstream. Or it could be ours. Anyway, limiting read_len fixes the issue. Could use (2^63)-1, too, but considering that `bytecode` max_size is limited, I don't see the point.

The messages silenced in this commit are all: On Python 3 '{}'.format(b'abc') produces "b'abc'"; use !r if this is a desired behavior Sadly, `mypy` doesn't say which particular fields are problematic; I've tried my best to single them out - hopefully, nothing extraneous got in.

99.99% cargo-culting. I don't understand the purpose of this structuring, but the function signatures don't match, and this is the only permutation I could find between OpcodeAPI, Opcode and FRONTIER_OPCODES that stops producing an error when running `mypy`: eth/vm/opcode.py:40: error: Return type "Opcode" of "as_opcode" incompatible with return type "Type[Opcode]" in supertype "OpcodeAPI"

Again, 99.99% cargo-culting. This just silences a `mypy` error: eth/vm/state.py:57: error: Argument 2 to "AccountDatabaseAPI" has incompatible type "bytes"; expected "Hash32" A valid state root will definitely be a 32-byte-long hash. Question is if this is the right place to make sure of it; and whether that should be made more explicit in code than being picky about a type.

This silences `mypy` error: eth/chains/base.py:250: error: Unexpected keyword argument "chain_context" for "VirtualMachineAPI" eth/abc.py:2270: note: "VirtualMachineAPI" defined here

AccountDB (from eth/db/account.py) has a setter for the `state_root` property, and `BaseState` (from eth/vm/state.py) forcibly sets the state root in its revert() function. This helps silence `mypy` error: eth/vm/state.py:170: error: Property "state_root" defined in "AccountDatabaseAPI" is read-only

veox · 2020-04-26T16:06:33Z

Just noticed that a new pyflakes has been released - will try to revisit next week.

veox · 2020-05-29T14:46:57Z

Well, it's been a while. I see trinity has Python 3.8 already...

Enough has changed in dependencies to start a fresh PR, closing.

v1.9.0 is installed now anyway, but this has been suggested in ethereum#1885 (comment)

Minimal set of dependencies that otherwise break the virtualenv, detailed here: ethereum#1872 (comment) SQUASHED: setup: bump `plyvel` version so pre-built wheel can be used: ethereum#1872 (comment) setup: bump minimum eth-utils to v1.8.0 (has Python 3.8 support). v1.9.0 is installed now anyway, but this has been suggested in ethereum#1885 (comment) setup: bump `pyflake` nested dependency to that with Python 3.8 support. Get around: AttributeError: 'FlakesChecker' object has no attribute 'CONSTANT' `pyflakes` has proper support from v2.2.0, which in turn was added to `flake8` as a dependency in the v3.8.x branch: https://gitlab.com/pycqa/flake8/-/blob/3.8.0/setup.cfg Also bump `flake8-bugbear` to latest version while we're at it, even though its dependency on `flake8` is rather lax: https://github.com/PyCQA/flake8-bugbear/blob/20.1.4/setup.py#L41

veox force-pushed the add-python3.8-testing branch 2 times, most recently from 2000193 to 3513f69 Compare December 2, 2019 18:33

veox commented Dec 2, 2019

View reviewed changes

veox force-pushed the add-python3.8-testing branch from b5a2ba4 to 7ffeef7 Compare December 5, 2019 14:38

veox commented Dec 6, 2019

View reviewed changes

carver reviewed Dec 6, 2019

View reviewed changes

eth/abc.py Outdated Show resolved Hide resolved

veox force-pushed the add-python3.8-testing branch 3 times, most recently from 3e9ae1d to a46b2b2 Compare December 10, 2019 18:50

veox changed the title ~~[WIP] Add Python 3.8 testing~~ [BLOCKED] [WIP] Add Python 3.8 testing Dec 10, 2019

veox changed the title ~~[BLOCKED] [WIP] Add Python 3.8 testing~~ [WIP] [BLOCKED] Add Python 3.8 testing Dec 10, 2019

veox added 10 commits December 11, 2019 11:52

setup: update to package versions that work with Python 3.8.

8120716

Minimal set of dependencies that otherwise break the virtualenv, detailed here: ethereum#1872 (comment)

setup: bump eth-utils to >=1.8.0.

ead4e57

That version added support for Python 3.8 internally: https://github.com/ethereum/eth-utils/blob/master/docs/releases.rst#eth-utils-180-2019-11-04 This is also what's being installed according to `pip freeze`.

circleci/tox: add Python 3.8 environments.

de0049f

ABC: add missing chain_context to VirtualMachineAPI's constructor.

97cc8db

This silences `mypy` error: eth/chains/base.py:250: error: Unexpected keyword argument "chain_context" for "VirtualMachineAPI" eth/abc.py:2270: note: "VirtualMachineAPI" defined here

veox force-pushed the add-python3.8-testing branch from 0d79bb6 to 7350ac3 Compare December 11, 2019 09:56

veox mentioned this pull request Dec 11, 2019

Add py3.8 tests #1872

Closed

veox closed this May 29, 2020

veox added a commit to veox/py-evm that referenced this pull request May 29, 2020

setup: bump minimum eth-utils to v1.8.0 (has Python 3.8 support).

46055ab

v1.9.0 is installed now anyway, but this has been suggested in ethereum#1885 (comment)

veox mentioned this pull request May 29, 2020

Add Python 3.8 testing (take 2) #1940

Merged

8 tasks

veox added a commit to veox/py-evm that referenced this pull request May 29, 2020

setup: bump minimum eth-utils to v1.8.0 (has Python 3.8 support).

83ca0ba

v1.9.0 is installed now anyway, but this has been suggested in ethereum#1885 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] [BLOCKED] Add Python 3.8 testing #1885

[WIP] [BLOCKED] Add Python 3.8 testing #1885

veox commented Nov 28, 2019 •

edited

Loading

veox Dec 2, 2019 •

edited

Loading

pipermerriam Dec 3, 2019

veox commented Dec 5, 2019 •

edited

Loading

voith commented Dec 5, 2019

veox Dec 6, 2019 •

edited

Loading

carver Dec 6, 2019

veox Dec 6, 2019 •

edited

Loading

carver Dec 6, 2019

veox Dec 10, 2019 •

edited

Loading

veox commented Dec 10, 2019

veox commented Dec 10, 2019 •

edited

Loading

veox commented Apr 26, 2020

veox commented May 29, 2020

	@classmethod
	def as_opcode(cls: Type[T],
	logic_fn: Callable[..., Any],
	mnemonic: str,
	gas_cost: int) -> T:

	def has_root(self, state_root: bytes) -> bool:
	return state_root in self._batchtrie

[WIP] [BLOCKED] Add Python 3.8 testing #1885

[WIP] [BLOCKED] Add Python 3.8 testing #1885

Conversation

veox commented Nov 28, 2019 • edited Loading

What was wrong?

How was it fixed?

To-Do

Cute Animal Picture

veox Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

pipermerriam Dec 3, 2019

Choose a reason for hiding this comment

veox commented Dec 5, 2019 • edited Loading

voith commented Dec 5, 2019

veox Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

carver Dec 6, 2019

Choose a reason for hiding this comment

veox Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

carver Dec 6, 2019

Choose a reason for hiding this comment

veox Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

veox commented Dec 10, 2019

veox commented Dec 10, 2019 • edited Loading

veox commented Apr 26, 2020

veox commented May 29, 2020

veox commented Nov 28, 2019 •

edited

Loading

veox Dec 2, 2019 •

edited

Loading

veox commented Dec 5, 2019 •

edited

Loading

veox Dec 6, 2019 •

edited

Loading

veox Dec 6, 2019 •

edited

Loading

veox Dec 10, 2019 •

edited

Loading

veox commented Dec 10, 2019 •

edited

Loading