Add Python 3.8 testing (take 2) #1940

veox · 2020-05-29T14:51:30Z

What was wrong?

Nothing; Python 3.8 has been released (half a year ago 😅).

First attempt was in PR #1885, but too many dependencies didn't have binary wheels yet.

How was it fixed?

Lots of cargo-culting to appease the linters.

Bumped dependencies and (nested d's) to versions with wheels for Python 3.8.
Added CI job definitions.
Fixed linting errors coming from:
- flake8;
- mypy.

Closes #1872 as implemented.

To-Do

Clean up commit history
Open fallout issues: Add Python 3.8 testing (take 2) #1940 (comment)

Add entry to the release notes

Cute Animal Picture

Source: Cmycherrytree, via imgur

veox · 2020-05-29T16:27:54Z

Most linting errors/warnings fixed. Reasons are in commit messages, if anywhere.

On the following:

/home/veox/src/py-evm/eth/vm/base.py:248:13: F841 local variable 'exc' is assigned to but never used
/home/veox/src/py-evm/eth/vm/base.py:371:13: F841 local variable 'exc' is assigned to but never used
/home/veox/src/py-evm/eth/consensus/clique/snapshot_manager.py:185:13: F841 local variable 'e' is assigned to but never used
/home/veox/src/py-evm/eth/consensus/clique/snapshot_manager.py:262:9: F841 local variable 'e' is assigned to but never used
/home/veox/src/py-evm/tests/json-fixtures/test_blockchain.py:300:13: F841 local variable 'err' is assigned to but never used

... someone should take a closer look: these are all cases where raise is used with an (implied) exception.

I'll push a commit shortly so the places are easier to find.

carver · 2020-05-29T16:33:44Z

... someone should take a closer look: these are all cases where raise is used with an (implied) exception.

Yup, I think the preferred option is the bare raise and just drop the as exc. The only counter-example I see is:

/home/veox/src/py-evm/eth/consensus/clique/snapshot_manager.py:262:9: F841 local variable 'e' is assigned to but never used

Since we're raising a different exception, I almost always prefer chaining the exception. In this case:

        except KeyError as e:                                                                                      
            raise SnapshotNotFound(                                                                                
                f"Can not get on-disk snapshot for {block_hash}"                                                   
            ) from e

carver · 2020-05-29T16:43:09Z

tox.ini

+    E252,
+    # W504 line break after binary operator
+    # It's either this or W503 (line break _before_ binary operator), pick your poison
+    W504


I think we'd rather ignore W503 here if it's not a huge change to this PR:

That's actually flake8's preferred solution: https://www.flake8rules.com/rules/W503.html

See a nice summary here: raiden-network/raiden#1382
(Roughly, ignoring W503 is flake8's default, but when we started ignoring E252, then flake8 stopped ignoring W503, so we need to add it back again)

Luckily, I already have a stash where I did the changes... One minute.

carver · 2020-05-29T16:45:42Z

tox.ini

+    # E252 missing whitespace around parameter equals
+    # Already used too much in the codebase at the point of introduction.
+    E252,


Maybe open a "Good First Issue" to clean all these up? I agree it probably belongs in its own PR, if it touches a lot of lines.

veox · 2020-05-29T17:47:50Z

flake8 issues fixed, going through mypy...

... A-a-and we're back to where we ended in the previous PR:

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

There is now also this (separate issue):

eth/vm/state.py:180: error: "AccountDatabaseAPI" has no attribute "lock_changes"

I see that AccountStorageDatabaseAPI does have the method. So the question is whether the ABC is lacking, a wrong class is used, or is mypy bonkers.

carver · 2020-05-29T18:14:06Z

I see that AccountStorageDatabaseAPI does have the method. So the question is whether the ABC is lacking, a wrong class is used, or is mypy bonkers.

Looks like AccountDatabaseAPI is missing the lock_changes stub.

carver · 2020-05-29T18:16:20Z

eth/vm/state.py:165: error: Incompatible return value type (got "Tuple[Hash32, JournalDBCheckpoint]", expected "Tuple[Hash32, UUID]")

Bug in the state API, should be updated to JournalDBCheckpoint (it hasn't been a UUID for a while)

... and I think that change ^ will fix the other two mypy warnings.

veox · 2020-05-29T19:57:43Z

Huh, looks like that worked.

Not sure I'm not breaking anyone's databases, but here goes.

eth/abc.py

carver

I suppose a few of those commits could be squashed. I'm not opposed to keeping them granular, though, to have a nice history of why the changes were made. So I guess let me know when you have done any squashing you want to do, and I'll go ahead and merge.

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

SQUASHED: tox: `flake8` to ignore E252, at least for now. There are many instances of: /home/veox/src/py-evm/eth/tools/_utils/normalization.py:139:45: E252 missing whitespace around parameter equals these are function parameters with a default value, like: ParamType: paramname=DefaultValue This comes from PEP8: > Don't use spaces around the = sign when used to indicate a keyword > argument, or when used to indicate a default value for an unannotated > function parameter: > ... > When combining an argument annotation with a default value, however, > do use spaces around the = sign: > ... tox: ignore W504 (line break before binary operator). Also add comments to the .ini file with verbose descriptions. tox: ignore W503 instead of W504 (line break before/after binary op). PEP8 says it's nicer to have line breaks before binary operators: > Following the tradition from mathematics usually results in > more readable code: > ...

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.

SQUASHED: lint/flake8: fix E117 (over-indented). lint/flake8: fix W605 (invalid escape sequence '\_', and the like). This comes from ASCII art in the benchmarking script. There are invalid escape sequences used in the font.

SQUASHED: lint/flake8: fix B011 (Do not call assert False...) ...since python -O removes these calls. Instead callers should raise AssertionError(). I did as the tool said, without thinking too much. lint/flake8: fix F841 (local variable is assigned to but never used). ... When used in try/except blocks, with blank `raise`. lint/flake8: re-fix F841 (local variable assigned but never used).

Copy-paste from the earlier commit (that was cherry-picked from previous PR): 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" This commit has been cherry-picked from previous PR: ethereum@07b4398

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 commit has been cherry-picked from previous PR: ethereum@851c141

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 This change has been copy-pasted from the previous PR. I would've cherry-picked it in, but since then the constructor has changed, adding in `consensus_context`, too. Anyway, here's the previous PR's commit: ethereum@97cc8db

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 This commit has been cherry-picked from the previous PR: ethereum@7350ac3

SQUASHED: lint/mypy,ABC: silence error in an AccountDatabaseAPI lacking a stub. The error is: eth/vm/state.py:180: error: "AccountDatabaseAPI" has no attribute "lock_changes" The stub comment is copied over from AccountDatabaseStorageAPI, removing the reference to "storage". ABC: improve AccountDatabaseAPI.lock_changes() heredoc. Co-authored-by: Jason Carver <[email protected]>

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

veox · 2020-05-30T12:54:43Z

Did some more squashing, grouping relevant fixes as much as possible.

veox changed the title ~~Add Python 3.8 testing (take 2)~~ [WIP] Add Python 3.8 testing (take 2) May 29, 2020

carver reviewed May 29, 2020

View reviewed changes

veox force-pushed the add-python3.8-testing-take2 branch from bd83a7d to cd5bdcb Compare May 29, 2020 17:02

This was referenced May 29, 2020

Add py3.8 tests #1872

Closed

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

Closed

veox force-pushed the add-python3.8-testing-take2 branch 3 times, most recently from 16080dc to c68f57e Compare May 29, 2020 17:17

veox mentioned this pull request May 29, 2020

Remove linting error ignores #1942

Closed

carver reviewed May 29, 2020

View reviewed changes

eth/abc.py Outdated Show resolved Hide resolved

carver approved these changes May 29, 2020

View reviewed changes

carver mentioned this pull request May 29, 2020

Add support for Python 3.8 ethereum/trinity#1679

Merged

2 tasks

veox added 10 commits May 30, 2020 15:22

veox force-pushed the add-python3.8-testing-take2 branch from 288da8e to f765a7f Compare May 30, 2020 12:32

veox changed the title ~~[WIP] Add Python 3.8 testing (take 2)~~ Add Python 3.8 testing (take 2) May 30, 2020

veox and others added 3 commits May 30, 2020 15:39

misc: Add newsfragment.

d7af77e

veox force-pushed the add-python3.8-testing-take2 branch from f765a7f to d7af77e Compare May 30, 2020 12:39

carver merged commit ad33442 into ethereum:master Jun 1, 2020

Elnaril mentioned this pull request Nov 19, 2020

Issue #1942 remove linting error ignores #1964

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Python 3.8 testing (take 2) #1940

Add Python 3.8 testing (take 2) #1940

veox commented May 29, 2020 •

edited

Loading

veox commented May 29, 2020

carver commented May 29, 2020

carver May 29, 2020

veox May 29, 2020

carver May 29, 2020

veox May 29, 2020

veox commented May 29, 2020

carver commented May 29, 2020

carver commented May 29, 2020 •

edited

Loading

veox commented May 29, 2020

carver left a comment

veox commented May 30, 2020

Add Python 3.8 testing (take 2) #1940

Add Python 3.8 testing (take 2) #1940

Conversation

veox commented May 29, 2020 • edited Loading

What was wrong?

How was it fixed?

To-Do

Cute Animal Picture

veox commented May 29, 2020

carver commented May 29, 2020

carver May 29, 2020

Choose a reason for hiding this comment

veox May 29, 2020

Choose a reason for hiding this comment

carver May 29, 2020

Choose a reason for hiding this comment

veox May 29, 2020

Choose a reason for hiding this comment

veox commented May 29, 2020

carver commented May 29, 2020

carver commented May 29, 2020 • edited Loading

veox commented May 29, 2020

carver left a comment

Choose a reason for hiding this comment

veox commented May 30, 2020

veox commented May 29, 2020 •

edited

Loading

carver commented May 29, 2020 •

edited

Loading