Skip to content

Experimental python3 support - #118

Merged
sylvinus merged 4 commits into
pricingassistant:masterfrom
tume:master
Dec 9, 2016
Merged

Experimental python3 support#118
sylvinus merged 4 commits into
pricingassistant:masterfrom
tume:master

Conversation

@tume

@tume tume commented Apr 23, 2016

Copy link
Copy Markdown
Contributor

First try on implementing Python3 support.
This was done using futurize (http://python-future.org/) and then manually fixing a few issues.

Changes made:

Adds python-future as dependency which only supports python >=2.6.
Most of the small changes should be pretty straightforward e.g. python3 style print function and no .iteritems() in python3

Probably the most problematic area is strings. Unicodes were converted to use str from python-future library and now that python3 has separate bytes type there are few places where those are converted.
There is also few places where it is explicitly checking if python version is 3 and doing different stuff e.g. logger just skips encode/decode in python3 and probably there is better way to do it.

Redis is now using decode_responses flag which may or may not be the good solution but at least it allowed me to get it working w/o decoding all the responses manually.

Also changed urllib2 to use urllib trough python-future. And needed to add monkey.patch_all(subprocess=False) to mrq_worker to get that running fine. Didn't really dig too deep in this monkeypatching stuff so not sure if this is a problem.

There are few test cases failing but couldn't get them to pass even with master. There is also no automated test runner for python3 ( I was locally testing with modified Dockerfile ).

requirements.txt also now contains different packages based on python version so installing with recent enough pip works but no idea about setuptools(?)

Any feedback on how I could improve this would be great.

( I have been running this with some "real" python3 jobs which seem to work but there is most likely some corner cases which I might be missing and also didn't run any benchmarks so no idea about the performance )

@tume tume mentioned this pull request Apr 23, 2016
@AlJohri

AlJohri commented Apr 23, 2016

Copy link
Copy Markdown

Does mrq/mrq/context.py also need from builtins import str ?

@tume

tume commented Apr 23, 2016

Copy link
Copy Markdown
Contributor Author

Good catch, should be there

@sylvinus

Copy link
Copy Markdown
Contributor

Wow, amazing!! We're going to review this carefully and merge it as soon as possible :)

@AlJohri

AlJohri commented Apr 23, 2016

Copy link
Copy Markdown

Anywhere that's performance critical might want to use the dict builtin from futures instead of just switching to .items. I'm not sure what places this is important.

http://python-future.org/what_else.html#dict


Also in a few places it seems you used list(d.keys()) and list(d.items()) while iterating over the dict's keys or items. I think the list is not necessary when iterating.

$ source activate py27
$ python2
Python 2.7.11 |Anaconda 4.0.0 (x86_64)| (default, Dec  6 2015, 18:57:58)
[GCC 4.2.1 (Apple Inc. build 5577)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
Anaconda is brought to you by Continuum Analytics.
Please check out: http://continuum.io/thanks and https://anaconda.org
>>> d = {1:1, 2:2, 3:3}
>>> [x for x in d.items()]
[(1, 1), (2, 2), (3, 3)]
>>> [x for x in d.keys()]
[1, 2, 3]
>>> [x for x in d.values()]
[1, 2, 3]

$ source deactivate
$ python3
Python 3.5.1 |Anaconda 2.5.0 (x86_64)| (default, Dec  7 2015, 11:24:55)
[GCC 4.2.1 (Apple Inc. build 5577)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> d = {1:1, 2:2, 3:3}
>>> [x for x in d.items()]
[(1, 1), (2, 2), (3, 3)]
>>> [x for x in d.keys()]
[1, 2, 3]
>>> [x for x in d.values()]
[1, 2, 3]

@sylvinus

Copy link
Copy Markdown
Contributor

If you want to add six as a dependency to deal with many of those issues, it would be okay!

@AlJohri

AlJohri commented Apr 23, 2016

Copy link
Copy Markdown

@sylvinus I think future accomplishes the same. http://python-future.org/faq.html#what-is-the-relationship-between-future-and-six

python-future is a higher-level compatibility layer than six that includes more backported functionality from Python 3, more forward-ported functionality from Python 2, and supports cleaner code, but requires more modern Python versions to run.

@sylvinus

Copy link
Copy Markdown
Contributor

Ah, right! I was confused because http://python-future.org/compatible_idioms.html also mentions six :-) Great then!

@sylvinus

Copy link
Copy Markdown
Contributor

To make testing easier I think we should switch to Travis to have a build matrix and test both 2 and 3. Maybe we can do that ourselves in master and merge it here afterwards?

@ggueret

ggueret commented Apr 24, 2016

Copy link
Copy Markdown
Contributor

Yeah that's very nice! You can already consider that the switch to Travis will soon be done to run py 2/3 tests.

@tume

tume commented Apr 24, 2016

Copy link
Copy Markdown
Contributor Author

Removed extra list calls when iterating. There seems to be couple of files which are missing str import from python-future so those probably need still fixing.

@tume

tume commented Apr 26, 2016

Copy link
Copy Markdown
Contributor Author

Changed remaining files which were using str straight. Only mrq/queue.py is still using it because bytes.fromhex is expecting string to have a decode method.

Still couple of tests failing with python2 so might need a bit advice what to do with those.

@tume

tume commented Jul 21, 2016

Copy link
Copy Markdown
Contributor Author

Have been a bit busy with other stuff but could now invest some more time on this... any feedback how I could push this forward?

@sylvinus

Copy link
Copy Markdown
Contributor

Sorry @tume for the lag and thanks again for the work!

I'm definitely willing to help. First order of business is switching to Travis and adding python3 to the Dockerfile. Let me do that in master so you can integrate those commits here. I'll provide further feedback once this is done!

@sylvinus

Copy link
Copy Markdown
Contributor

@tume: OK the tests are running on Travis!
https://travis-ci.org/pricingassistant/mrq/jobs/148468754

You should maybe merge master in your branch or rebase from it, then we can go over the remaining tests!

@sylvinus

Copy link
Copy Markdown
Contributor

@tume did you have some time to review this?

@tume

tume commented Sep 5, 2016

Copy link
Copy Markdown
Contributor Author

Sorry that took so long to reply but I was on holiday for a month.

Rebased this with master.

@AlJohri

AlJohri commented Sep 5, 2016

Copy link
Copy Markdown

@tume travis build fails on this line in mrq/config.py

change to:

config_keys = set(list(default_config.keys()) + list(from_file.keys()))

@sylvinus

sylvinus commented Sep 6, 2016

Copy link
Copy Markdown
Contributor

Good catch @AlJohri !

@tume glad to have you back! Let us know if you need any help, seems we're not far now!

@tume

tume commented Sep 12, 2016

Copy link
Copy Markdown
Contributor Author

Fixed

@AlJohri

AlJohri commented Oct 22, 2016

Copy link
Copy Markdown

will #122 fix the failing travis build? seems to be related to the UUID thing

@achauve

achauve commented Dec 7, 2016

Copy link
Copy Markdown
Contributor

Is help needed here? I'd be glad to if I can. I'd like to test my stack on python 3 and mrq is the last package to prevent it.

By the way as travis tests are not green on master either, I'm not sure what's still need to be done for this branch to be ok for merge.

Last question: flask version is frozen to 0.10 in requirements-dashboard.txt and thus in setup.py, which blocks upgrade to flask 0.11 to anything depending on MRQ. Is there a reason for this freeze?

Thanks!

@sylvinus

sylvinus commented Dec 8, 2016

Copy link
Copy Markdown
Contributor

@achauve yes let's do it!!

no reason for freezing flask, we can remove it.

there are some long-standing flaky tests, I'm going to fix or skip them short term so we can get back to green and merge this!

@sylvinus
sylvinus merged commit f9f6b33 into pricingassistant:master Dec 9, 2016
@sylvinus

sylvinus commented Dec 9, 2016

Copy link
Copy Markdown
Contributor

Congrats! Thank you all :)

@sylvinus

sylvinus commented Dec 9, 2016

Copy link
Copy Markdown
Contributor

@achauve let me know how your tests go!

@achauve

achauve commented Dec 9, 2016

Copy link
Copy Markdown
Contributor

@sylvinus Yes I will! Thanks a lot for the cleanings and the merge :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants