Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Add support for Python 3.8 #1679

Merged
merged 4 commits into from
May 27, 2020
Merged

Add support for Python 3.8 #1679

merged 4 commits into from
May 27, 2020

Conversation

cburgdorf
Copy link
Contributor

@cburgdorf cburgdorf commented Apr 17, 2020 •
edited
Loading

What was wrong?

When we get rid of Python 3.6 support we should make sure we support Python 3.8 by testing against it in CI.

How was it fixed?

Add CI jobs for Python 3.8

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@cburgdorf cburgdorf force-pushed the christoph/feat/py38 branch 3 times, most recently from 5fea2a6 to 6d3b3ee Compare April 21, 2020 12:51
pipermerriam
pipermerriam reviewed Apr 21, 2020
View reviewed changes
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Doesn't have to happen in this PR (and might be a great "Good First Issues") but we should look into using the native contextlib.asynccontextmanager and any other 3.7 things that we've had to shim back into 3.6.

@cburgdorf cburgdorf force-pushed the christoph/feat/py38 branch 2 times, most recently from 188c310 to 395f953 Compare April 22, 2020 14:39
@cburgdorf
Copy link
Contributor Author

@ChihChengLiang do you have any idea what's preventing the milagro-bls-binding from getting properly installed under a Python 3.8 environment?

@cburgdorf
Copy link
Contributor Author

@pipermerriam No you're right, there was a huge chunk of things that we could get rid off. I updated the PR.

@ChihChengLiang
Copy link
Contributor

Hi @cburgdorf, I think I missed some release of milagro-bls-binding binding for 3.8 version. Just fixed it now, would you like to give it another try?

@cburgdorf
Copy link
Contributor Author

@ChihChengLiang Thanks for fixing the Python 3.8 builds! Unfortunately, it seems like the API has changed substantially enough to throw me off 😅 For example, sign has no domain anymore, and verify_multiple is gone. Would you mind handling the upgrade to the latest version in a standalone PR?

@ChihChengLiang
Copy link
Contributor

@cburgdorf I'm terribly sorry for saying it's ready for the upgrade and I'm so sorry for your time on trying to upgrade that 🙈, I totally forgot the breaking API change. The API change would require a lengthy PR because it touches eth2 code and eth2-fixture tests.

Here are some band-aids for that.

  • The quickest possible way is to drop the eth2-extra requirement in CI, the tests would fall back to using py-ecc as the bls library.
  • Meanwhile, I'll try to release an old API compatible py 3.8 release of milagro-bls-binding, under version 0.1.x. If it works, it makes existing Trinity codebase works. I'll yank out the 0.2.0, 0.3.0, and 0.4.0, as those versions are using the new API. The new API will be released under 2.0.x. I'll update in this thread when it's done.

I'm not able to commit to the upgrade of Trinity codebase to the new API yet, due to the other ongoing tasks. But happy to do it a while later if no one else is on it.

@ChihChengLiang
Copy link
Contributor

@cburgdorf I just released milagro-bls-binding==0.1.4 for py3.8 and old API support. The installation in Trinity looks fine and CI tests depend on that pass here. Hope this time works 😂.

@cburgdorf cburgdorf force-pushed the christoph/feat/py38 branch 2 times, most recently from 6d5b05e to e600f60 Compare April 27, 2020 08:18
@cburgdorf
Copy link
Contributor Author

@ChihChengLiang Awesome! That fixes it. Also, don't worry too much, I haven't wasted much time on the issue and it isn't urgent. Thank you very much for cutting a new release 🙏

@cburgdorf cburgdorf force-pushed the christoph/feat/py38 branch from e600f60 to 483539b Compare April 28, 2020 14:38
@cburgdorf cburgdorf requested a review from carver April 28, 2020 15:05
@cburgdorf
Copy link
Contributor Author

@carver When you feel ready to ditch Python 3.6 we can merge this to trade it against Python 3.8 support

@cburgdorf cburgdorf force-pushed the christoph/feat/py38 branch from 483539b to f4f82ec Compare May 25, 2020 13:42
@cburgdorf
Copy link
Contributor Author

@carver Are we good to trade Python 3.6 against Python 3.8 support by now?

@carver
Copy link
Contributor

carver commented May 26, 2020

@carver Are we good to trade Python 3.6 against Python 3.8 support by now?

What is the urgency to drop 3.6 again? Can't we add 3.8 support without dropping 3.6?

@cburgdorf
Copy link
Contributor Author

cburgdorf commented May 26, 2020 •
edited
Loading

  1. We can get rid of a bunch of Python 3.6 shims. I also suspect there may be some other features we could use that do not have shims available.

  2. Python 3.6 does not support pickleing of generic classes (Something that our pending eth/66 implementation uses but which I suspect might also be useful for various lahja events e.g. eventbus.stream(Something[PayloadX]))

  3. Adding Python 3.8 support next to Python 3.6 would also increase our CI times.

carver
carver approved these changes May 26, 2020
View reviewed changes
Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

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

Okay, I guess it's been long enough since the LTS came out. I still haven't upgraded but 3.7 is of course available through the package manager.

setup.py Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@cburgdorf cburgdorf force-pushed the christoph/feat/py38 branch 2 times, most recently from 598fe68 to 61fd1c1 Compare May 27, 2020 08:42
@cburgdorf cburgdorf force-pushed the christoph/feat/py38 branch from 61fd1c1 to 873619f Compare May 27, 2020 08:56
@cburgdorf cburgdorf mentioned this pull request May 27, 2020
Closed
@cburgdorf cburgdorf merged commit 1f31660 into ethereum:master May 27, 2020
carver
carver reviewed May 27, 2020
View reviewed changes
tox.ini
py{37,38}-{eth1-core,p2p,p2p-trio,integration,lightchain_integration,eth2-core,eth2-fixtures,eth2-integration,eth1-components,eth2-utils,eth2-trio}
py37-long_run_integration
py37-rpc-blockchain
py38-rpc-state-{frontier,homestead,tangerine_whistle,spurious_dragon,byzantium,constantinople,petersburg,istanbul}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this one got missed, I expected: py37

@veox veox mentioned this pull request May 29, 2020
Closed
11 tasks
@veox
Copy link
Contributor

veox commented May 29, 2020

So this added proper Python 3.8 support (for running the node, etc.), or does it only make CI pass?

Because if the former, then the newsfragments doesn't reflect it, and the fact will not be in the release notes.

@cburgdorf
Copy link
Contributor Author

@veox oh you're right. I somehow missed to mention Python 3.8 support. I should work but afaik there are still some quirks with Python 3.8 that need to be resolved.

@carver
Copy link
Contributor

carver commented May 29, 2020

We can add the release note when we upgrade to a py-evm version that includes ethereum/py-evm#1940

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carver carver carver approved these changes

@pipermerriam pipermerriam pipermerriam left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cburgdorf @ChihChengLiang @carver @veox @pipermerriam