Python 3 support #163

Merged
merged 55 commits into from Jul 8, 2016

Projects

None yet

4 participants

@escapewindow
Contributor

@twobraids @peterbe , I started working on this in 2015 and gave up. Since then my py3 porting skills have leveled up. I picked configman back up on a whim today and actually got everything green in py26,py27,py33,py34,py35!

However, this is a large enough change, with associated unit test changes, that I'd like someone familiar with it to really eyeball and put this version through its paces.

Let me know if you have any questions!

escapewindow added some commits Jun 25, 2015
@escapewindow escapewindow get tox green for py26,py27 6410400
@escapewindow escapewindow start porting to py3. green on py27, already dropped py26 support 7cdd996
@escapewindow escapewindow future statement through config_manager 86dec44
@escapewindow escapewindow re-add py26 support. argparse help output will need to be regexed or
|substr in str|'ed :(
aabff59
@escapewindow escapewindow works in py27, tox seems a bit busted? 5407ff0
@escapewindow escapewindow fix tox 45b4bd0
@escapewindow escapewindow move the first py3 error to dotdict 3f1c92d
@escapewindow escapewindow python 3 < 3.3 not supported 197f698
@escapewindow escapewindow move py3 error to def_sources/__init__.py 68dfb84
@escapewindow escapewindow move py3 error to value_sources/for_conf.py 8dcf054
@escapewindow escapewindow move py3 error to value_sources/for_modules.py 817350d
@escapewindow escapewindow first py3 error in test_val_for_modules.py! 7694333
@escapewindow escapewindow port a bunch more files f47ae77
@escapewindow escapewindow a lot more files 120c9b3
@escapewindow escapewindow more files, still green on py2 d31b3d2
@escapewindow escapewindow four more files to go, then more py3 fixes 38da698
@escapewindow escapewindow down to 57 errors, 1 failure 41a2f54
@escapewindow escapewindow down to 53 errors, 1 failure 105346c
@escapewindow escapewindow 47 errors, 1 failure 0042c66
@escapewindow escapewindow errors=40, failures=1 5bb3e28
@escapewindow escapewindow errors=13, failures=1 3e6c864
@escapewindow escapewindow 20 errors, 4 failures now that i fixed some broken python =\ 5698eaa
@escapewindow escapewindow all ints in py3 are long. also, 31 errors, 4 failures =\ 5f72e1e
@escapewindow escapewindow 30 errors, 4 failures 72f3510
@escapewindow escapewindow comment out ArgumentParser.version; errors-18, failures=4 8b4dc5e
@escapewindow escapewindow the sorted()/cmp and getattr hacks are killing me 81f87f5
@escapewindow escapewindow back down to errors=18, failures=4 34dd795
@escapewindow escapewindow errors=18 failures=1 c9902bd
@escapewindow escapewindow errors=15, failures=3 3e5f909
@escapewindow escapewindow errors=15, failures=1 aa44d1c
@escapewindow escapewindow errors=15, failures=0 dae6dd6
@escapewindow escapewindow i think i've moved on to other errors in the same tests? 15 errors 37d1432
@escapewindow escapewindow Merge branch 'master' into python3 7a41e46
@escapewindow escapewindow errors = 12 cb7ec8e
@escapewindow escapewindow 11 errors 2df1e57
@escapewindow escapewindow 5 errors 9246b28
@escapewindow escapewindow add py3{3,4,5} to travis 2ba6b1e
@escapewindow escapewindow get converters more sane 74e9dc9
@escapewindow escapewindow to_str ize more things, get rid of unicode_literals 56daba1
@escapewindow escapewindow finish to_str 3a52053
@escapewindow escapewindow 2 errors 2b72cdb
@escapewindow escapewindow omg, green tests py26,py27,py33,py34,py35 e2ffa05
@escapewindow escapewindow reenable commented-out test after fixing 17e8777
@escapewindow escapewindow update packaging files for py3 70e5433
@escapewindow
Contributor
escapewindow commented Jul 1, 2016 edited

Just noticed I haven't touched demo/ or docs/. fixed!

@peterbe peterbe and 1 other commented on an outdated diff Jul 1, 2016
demo/advanced_demo3.py
@@ -46,6 +47,7 @@
import random
import time
import socket
+from six.moves import range as xrange
@peterbe
peterbe Jul 1, 2016 Contributor

I wonder if we really need xrange at all. In py2, range exists and works the same as py3's range except deep inside py3's range returns an iterator. But does it matter? I doubt we really needed it to be xrange in py2.

@escapewindow
escapewindow Jul 1, 2016 Contributor

done.

@peterbe
Contributor
peterbe commented Jul 1, 2016

That. Is amazing!
@twobraids is away on a long vacation at the moment but he might be excited to have a look when he's near a computer.

I've skimmed through the whole thing and it all looks great. I really like how you solved the sorting problem with OrderableTuple and OrderableMap.

There were a couple of places where I felt we shouldn't need six if we just simplified but I guess that's hard to figure out in the same pull request. Something for the future.

I'm eager to merge this but I think we should wait for @twobraids. Also, what I will try to do is to throw this version into socorro (at least here on my laptop) and see how it goes.

THANK YOU!

@peterbe
Contributor
peterbe commented Jul 1, 2016

By the way, was it a lot of work to make it work in 2.6? Because perhaps we can drop 2.6 at some point.

@peterbe
Contributor
peterbe commented Jul 1, 2016

By the way @escapewindow,
If @twobraids or I fail to follow up in the near future (due to Lars's vacation) please ping us again here on this pull request.

@willkg willkg and 1 other commented on an outdated diff Jul 1, 2016
configman/def_sources/for_argparse.py
@@ -573,7 +575,7 @@ def parse_args(self, args=None, namespace=None):
values_source_list=self.value_source_list,
argv_source=args,
app_name=self.prog,
- app_version=self.version,
+# app_version=self.version,
@willkg
willkg Jul 1, 2016 Member

This got commented out. What was the problem here?

@escapewindow
escapewindow Jul 1, 2016 Contributor

Just uncommented and re-ran.
I got this:

======================================================================
ERROR: test_parser_setup (configman.tests.test_def_for_argparse.TestCaseForDefSourceArgparse)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/asasaki/src/releng/configman/configman/tests/test_def_for_argparse.py", line 250, in test_parser_setup
    config = parser.parse_args(args=[])
  File "/Users/asasaki/src/releng/configman/configman/def_sources/for_argparse.py", line 578, in parse_args
    app_version=self.version,
AttributeError: 'ArgumentParser' object has no attribute 'version'

Want me to add it back in if six.PY2 ?

@willkg
willkg Jul 5, 2016 Member

Mmm... I think we should be passing it in regardless of Python version. Version is helpful to know when using a cli.

I'm not sure why there is no version attribute. There should be. I looked at the Python 3.2 source and ArgumentParser.__init__ sets it on the instance based on incoming arguments. How puzzling. I'll take a look at this more after I finish reviewing.

@willkg
willkg Jul 5, 2016 Member

My bad--I didn't read the code well. The version argument for __init__ got deprecated, so that's gone after Python 3.2.

My vote is to change ArgumentParser.__init__ to pop version off the kwargs using a default of None and save it as an attribute.

@escapewindow
escapewindow Jul 5, 2016 Contributor

Done.

@escapewindow
Contributor

By the way, was it a lot of work to make it work in 2.6? Because perhaps we can drop 2.6 at some point.

It wasn't too difficult to keep 2.6 support, so feel free to keep or drop at your discretion.
(However, I've dropped 2.6 support from all of the projects I've been working on recently. In some of them I've dropped pre-3.5 support, for the async def goodness.)

There were a couple of places where I felt we shouldn't need six if we just simplified but I guess that's hard to figure out in the same pull request. Something for the future.

Sure. My code got pretty ugly with all the debugging, and I think I went off on a couple wrong tangents that I've [mostly?] cleaned up. I imagine some things will crop up sooner or later that need fixing or tweaking.

I'm eager to merge this but I think we should wait for @twobraids. Also, what I will try to do is to throw this version into socorro (at least here on my laptop) and see how it goes.

Makes sense. No need to rush.

THANK YOU!

np :)

@escapewindow
Contributor

Sorry for landing 13e99ae mid-review, but I noticed these and they were bugging me :) I think I'm done touching configman now until review comments come in.

@willkg willkg self-assigned this Jul 1, 2016
@willkg
Member
willkg commented Jul 1, 2016

@peterbe is out all next week and I'm pretty sure @twobraids is, too. Given that, I'll take on the task of reviewing this.

I'm old to Python, but kind of new to configman, so I'm going to be slow and plodding on this. I plan to do the following:

  1. review the code changes and surrounding code (about 1/3 done already)
  2. review the tests and docs
  3. test everything out locally
  4. test with socorro and socorro-collector
  5. test with crontabber

Are there other things we should do to vet this change?

I'll work on this next week.

@escapewindow
Contributor

Are there other things we should do to vet this change?

I'm not sure. That sounds like a good list to me. Thanks!

@adngdb
Member
adngdb commented Jul 4, 2016

@willkg Make sure you test the back-end and front-end of Socorro when you do, we do use it in our django app, in a pretty twisted way. Other than that, I think you have covered all of our use cases.

@willkg willkg and 1 other commented on an outdated diff Jul 5, 2016
@@ -45,6 +57,9 @@ def find_tests_require():
'Programming Language :: Python',
'Programming Language :: Python :: 2.6',
'Programming Language :: Python :: 2.7',
+ 'Programming Language :: Python :: 3.3',
+ 'Programming Language :: Python :: 3.4',
+ 'Programming Language :: Python :: 3.5',
@willkg
willkg Jul 5, 2016 Member

This should include:

Programming Language :: Python :: 2

and:

Programming Language :: Python :: 3

This is mostly to be nice to some of the tools out there tracking python 2 vs. python 3 support. Otherwise I don't think it affects anything.

@escapewindow
escapewindow Jul 5, 2016 Contributor

Done.

@willkg willkg and 1 other commented on an outdated diff Jul 5, 2016
configman/value_sources/for_modules.py
+ return (self.value_str < other.value_str)
+ def __repr__(self):
+ return self.value
+
+
+#==============================================================================
+class OrderableTuple(object):
+ """Python3 can't sort non-string types implicitly.
+ """
+ def __init__(self, value, index=1):
+ self.value = value
+ self.index = index
+ def __lt__(self, other):
+ return (self.value[self.index] < other.value[self.index])
+ def __repr__(self):
+ return self.value
@willkg
willkg Jul 5, 2016 Member

We should add tests for these two things. I don't think it needs to be spectacular, but it'd be good to know that they work as advertised and that we don't break them in the future.

Also, do we need to add a functools.total_ordering decorator to these classes? If not, why not?

@escapewindow
escapewindow Jul 5, 2016 edited Contributor

functools.total_ordering isn't in python 2.6. We can skip it, add it and drop 2.6 support, or write our own total_ordering decorator that passes through in python>=2.7 and does something else in 2.6 (noop? roll our own total_ordering?)

Admittedly, I've never used functools.total_ordering, and these classes work without it. OOC, what benefit will it provide? n/m, read the source :) We seem to only use these classes internally atm, and these seem to work with sorted(). Adding total_ordering seems like a good choice, still, though.

... drop 2.6 support?

@escapewindow
escapewindow Jul 5, 2016 Contributor

Added small tests for the Orderable* objects, and found __repr__ bugs for both :)
I think we're done with review comments other than the functools.total_ordering decision.

@willkg
willkg Jul 5, 2016 edited Member

total_ordering lets you define __lt__ and __eq__ and then it adds handling for the other comparators based on those. It's a nice shorthand for creating classes that can be compared in various ways.

I spent some time looking into how Python's sort works and whether it's safe to just specify __lt__. The source code suggests that it is safe and that it's sufficient to specify only __lt__. Then I did some skulking around to see if anyone had asked this question before and whether the Python docs state it's ok and all that and bumped into this:

http://stackoverflow.com/questions/8796886/is-it-safe-to-just-implement-lt-for-a-class-that-will-be-sorted

The summary of that is that it currently works, but it's not something you should rely on working in the future and that the current best practice is to decorate with functools.total_ordering.

I'm of the "to hell with Python 2.6" variety and because of that, I think this is a good reason to drop Python 2.6 support. I'm new to configman, so I don't know what the ramifications of that are for the greater configman universe. That's an option.

Another option is to implement it or use a backported library. This is one that my wildly cursory glance suggests is "good enough":

https://pypi.python.org/pypi/total-ordering/0.1.0

An "easy" way to do that is to add something like this to setup.py:

def find_install_requires():
    reqs = [x.strip() for x in
            read('requirements.txt').splitlines()
            if x.strip() and not x.startswith('#')]

    try:
        from functools import total_ordering
    except ImportError:
        reqs.append('total-ordering==0.1')
    return reqs

I think that'd work, but I'm not 100% sure that works with wheels. We could do a release and then test it out and do a fixit release afterwards if it doesn't work.

@escapewindow
escapewindow Jul 5, 2016 Contributor

Done. If this doesn't work, let's revisit #163 (comment) .

@willkg
Member
willkg commented Jul 5, 2016

I've gone through it all. There are a few things we should figure out, but otherwise I think this looks good!

@escapewindow Can you work through those some time this week? If not, I can take over--whichever is good for you.

After that, I'll re-review and then start testing things out.

Thank you!

@escapewindow
Contributor

Hm. Not sure why travis isn't picking up the install_requires fix.
Alternatives: add total_ordering to test-requirements.txt, switch travis to use tox instead of nosetests, desupport py26, ...?

@willkg willkg and 1 other commented on an outdated diff Jul 6, 2016
@@ -33,6 +33,11 @@ def find_install_requires():
return [x.strip() for x in
read('requirements.txt').splitlines()
if x.strip() and not x.startswith('#')]
+ try:
@willkg
willkg Jul 6, 2016 Member

You need to change the previous line to:

reqs = [x.strip() for x in
        read('requirements.txt').splitlines()
        if x.strip() and not x.startswith('#')]
@escapewindow
escapewindow Jul 6, 2016 Contributor

Oops. Done.

@willkg
Member
willkg commented Jul 6, 2016

Yay! Travis is happy!

I'll go through and test this out with the various things I was thinking I'd test this out with and see how that goes. I'll try to do that this week. If I don't get to it this week, I'll get to it next next week (next week I'm on PTO). If I don't get to it this week and @peterbe wants to do the testing work, that's fine by me.

@escapewindow Thank you!

@willkg
Member
willkg commented Jul 8, 2016

I tested this out with the new socorro-collector I'm working on and didn't have any problems. I looked at socorro, socorrolib and crontabber and all are using older versions of configman than what's currently available.

Looking through the changes, I don't see anything that's fishy. I'm not sure it makes sense to spend the effort to test everything meticulously given these circumstances and if there are problems, we'll find them when we upgrade things later.

I went through the docs and other than the fact that they're Python 2-centric, they look fine.

Given that, I'm going to merge it.

@escapewindow Thank you!

@willkg willkg merged commit c09752d into mozilla:master Jul 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@willkg willkg referenced this pull request Jul 8, 2016
Closed

Add Python 3 support #125

@escapewindow
Contributor

@willkg Thank you!

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