Skip to content
This repository was archived by the owner on May 23, 2023. It is now read-only.

Piper/python3 support #357

Merged
merged 20 commits into from
Jun 21, 2016

Conversation

pipermerriam
Copy link
Member

@pipermerriam pipermerriam commented Jun 15, 2016

#132

What was wrong?

This project did not support python3

How was it fixed.

  • Expanded Travis CI testing to include 2.7, 3.4 and 3.5
  • Converted division operators to the floored division operator //
  • Prefixed many strings with b to make them byte strings.
  • Convert use of print statements to python3 compatable function calls.
  • Change import statements to use absolute import syntax.
  • Fix a few tests that were non-deterministic and intermittently failing.

Cute animal picture

badger web version tim matthews

@pipermerriam
Copy link
Member Author

Depends on ethereum/serpent#102

@heikoheiko
Copy link
Member

hi @pipermerriam thank you for this effort.
do you have an eye on the performance impacts?
i'm a bit afraid of py2/py3 logic overhead in inner loops.

@pipermerriam
Copy link
Member Author

@heikoheiko I'm working on getting a fully passing test suite first. Then I'll deal with any performance problems.

@heikoheiko
Copy link
Member

Then I'll deal with any performance problems.
ok, so comparing the performance could work by testing different branches / tags.

@@ -3,4 +3,4 @@ coveralls
pytest>=2.9.0
pytest-catchlog==1.2.2
pytest-timeout==1.0.0
https://github.com/ethereum/serpent/tarball/develop
Copy link
Member Author

Choose a reason for hiding this comment

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

This should NOT be merged. This is just here to get the python3 tests running until I can get ethereum/serpent#102 merged.

@pipermerriam
Copy link
Member Author

ethereum/solidity#651 is causing some of the current test failures within the test_solidity.py tests. Doesn't seem fixable until a new solc release is made.

@pipermerriam
Copy link
Member Author

@heikoheiko here is some profiling data from the ethereum/tests/profile_vm.py script.

https://gist.github.com/pipermerriam/3d12e18097c0d6be72956bd19a24eac7

@pipermerriam
Copy link
Member Author

Alrighty!

The CI failures are all related to ethereum/solidity#651.

This is ready for review and theoretically merging if people are ok with it.

This depends on ethereum/serpent#102 being reviewed and merged. Alternatively I can remove the code that references my fork/branch which will cause CI to start failing in more places but will allow this to be merged independently.

@pipermerriam pipermerriam force-pushed the piper/python3-support branch from 011f315 to 6361eb4 Compare June 16, 2016 18:10
@pipermerriam
Copy link
Member Author

This should close #132, #238, #254, #329, and #347

@pipermerriam pipermerriam force-pushed the piper/python3-support branch from 6361eb4 to 65ee002 Compare June 16, 2016 18:35
@pipermerriam pipermerriam mentioned this pull request Jun 16, 2016
@pipermerriam
Copy link
Member Author

pipermerriam commented Jun 17, 2016

ok, All of the test failures are related to the solidity stdin bug (see ethereum/solidity#651). You can view these under the travis run for d3e61e1 which can be found https://travis-ci.org/ethereum/pyethereum/builds/138391919 (note this commit was rebased away but you can still see the test run)

I've just pushed 49e570f which reverts the serpent dependency back to the official repo. It was previously pointed at the branch represented by ethereum/serpent#102 which implements python3 compatability. These two repositories have a circular dependency so my feeling is that we should do the following.

Soliciting feedback on this plan and hoping to move it forward in a timely fashion.

@pipermerriam
Copy link
Member Author

pipermerriam commented Jun 20, 2016

Benchmark Data

# Function Calls Time % Change
Python 2 (with py3 support) 3668001 3.590 -10%
Python 3 2261856 2.113 n/a
Baseline (develop branch) 3668001 3.227

@heikoheiko
Copy link
Member

Thank you. Looks way better than what I expected.

udpf = self.config['UNCLE_DEPTH_PENALTY_FACTOR']
un = uncle.number
bn = self.number

Copy link
Member

@ulope ulope Jun 20, 2016

Choose a reason for hiding this comment

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

Those assignments (except uncle.number) could probably be pulled out of the loop (assuming self.delta_balance() doesn't modify self.number).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. Fixed

@ulope
Copy link
Member

ulope commented Jun 20, 2016

Looks very good to me (apart from the minor nit picks above).

One thing I would like to change a bit is the travis and tox configuration.
I've put the config I'd suggest into a gist.

There are a few reasons for doing it this way:

  • We retain the ability to locally use tox to run only specific tests (through the use of {posargs}). This is very useful while working on a specific issue for example.
  • Coverage is measured across all runs on all supported Python versions. This way we don't get any false positives due to version specific code.

@pipermerriam
Copy link
Member Author

@ulope I've tried to adapt your examples in a manner that tries to maximize the ability to use tox to runt he tests locally and on travis while still getting full coverage reports. See 5ca58a6

I'm still probably gonna have to futz with it a bit to get it running smoothly but I'd be curious to get your feedback on whether this is inline with what you were thinking.

@pipermerriam pipermerriam force-pushed the piper/python3-support branch from 474952f to 2c16c37 Compare June 20, 2016 19:05
@pipermerriam
Copy link
Member Author

@ulope I actually undid the bytearray_to_bytestr change. There are some places where it doesn't actually get passed an actual bytearray but rather an array of integer byte values. bytes(value) doesn't have the same behavior in this case across python2/3.

@pipermerriam pipermerriam force-pushed the piper/python3-support branch from 4f67ea8 to 818a91d Compare June 20, 2016 19:58
@pipermerriam
Copy link
Member Author

Additional benchmark results from python ethereum/tests/stress_test.py:

  • Baseline (python2, no support for python 3) - 9.238459
  • Python 2 (with py3 support) - 9.285555
  • Python 3 - 22.872115

@ulope
Copy link
Member

ulope commented Jun 21, 2016

@pipermerriam Yeah, ok in that case we don't have a choice. Bummer. Regarding the tox/travis config, I don't think it's critical to this PR. If it works ok just leave it for now and then we can have another look and possibly optimize it later.

@konradkonrad
Copy link
Contributor

konradkonrad commented Jun 21, 2016

Baseline (python2, no support for python 3) - 9.238459
Python 2 (with py3 support) - 9.285555

thanks @pipermerriam!
also thanks @ulope for your review !
Overall this looks good to me! -- the failing solidity tests make me sad though..
Any open questions before the merge @ulope @heikoheiko?

@konradkonrad konradkonrad merged commit 9281d4e into ethereum:develop Jun 21, 2016
konradkonrad added a commit that referenced this pull request Jun 22, 2016
There is sort of a deadlock for getting py3 support between
`ethereum/pyethereum` and `ethereum/serpent`, due to a circular
test dependency. In order to break the circle, this can be used
to create a release, which in turn can be used by `serpent`s build.

see #357
konradkonrad added a commit that referenced this pull request Jul 4, 2016
There is sort of a deadlock for getting py3 support between
`ethereum/pyethereum` and `ethereum/serpent`, due to a circular
test dependency. In order to break the circle, this can be used
to create a release, which in turn can be used by `serpent`s build.

see #357
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants