Add python 2 and 3 support #280

jhkennedy · 2017-11-29T00:59:33Z

This PR adds python 2 and 3 support using the module six as little as possible.

I've tried to commit like-changes in small chunks so you can easily see what was needed to change the code. Note: that means that all commits from 57dbcb9 to cb5e801 are broken. This could all be squashed for merging into master.

Status:

All tests pass in both python 2.7 and 3.5, but there are likely issues lurking in the python 2 code now with unicode vs byte strings.

Left to do:

configure ci for tox (see Addition of automatic continuous integration (CI) #40 and Adds travis continuous integration (CI) for pytest #42)
check all usages of str for cross compatibility

fixes #43

In python 2, calling just `.items()`, `.values()`, or `.keys()` returns a copy of dictionaries list of key-value pairs, values, or keys, respectively. This results in extra memory overhead when used to iterate over a dictionary and instead it was common to use the `.iteritems()` methods to generate an iterator. In python 3, `.items()` now returns an iterator-view, and the `iteritems()` method has been removed. Because python 3 is the current target, using `.items()` and taking the memory hit in python 2 is preferable over using ifs or packages like six to provide efficient iteration across both python 2 and 3.

xylar · 2017-11-29T09:54:56Z

@jhkennedy, this is great!!!

I'm going through the commits one at a time. I'll note a few things here as I do because inline comments are mostly just confusing.

xylar · 2017-11-29T09:59:50Z

Regarding commit 7a474d1, I think there are at least 3 more __init__.py files with this problem that maybe the tests don't catch. What I've seen so far are:
https://github.com/MPAS-Dev/MPAS-Analysis/blob/develop/mpas_analysis/ocean/__init__.py
https://github.com/MPAS-Dev/MPAS-Analysis/blob/develop/mpas_analysis/sea_ice/__init__.py
https://github.com/MPAS-Dev/MPAS-Analysis/blob/develop/mpas_analysis/shared/time_series/__init__.py

xylar · 2017-11-29T10:04:38Z

mpas_analysis/shared/io/mpas_reader.py

        # this is an array of date strings like 'xtime'
        # convert to string
-        timeStrings = [''.join(xtime).strip() for xtime in timeVar.values]
+        timeStrings = [''.join(xtime.astype('U')) for xtime in timeVar.values]


@jhkennedy, I don't think the .strip() should have been lost here. The xtime variable typically has trailing white space.

Ah, blast. That's why I was having problems in timekeeping. 😧

I agree, this should be there still.

This is the only thing I haven't fixed because I didn't want to edit your commit history. Once it's fixed, I'll run a few more tests but everything looks good to me so far.

xylar · 2017-11-29T10:09:21Z

mpas_analysis/shared/timekeeping/utility.py


    # change underscores to spaces so both can be supported
-    dateString = dateString.replace('_', ' ')
+    dateString = dateString.replace('_', ' ').strip()


This is fine, but I think the .strip() was previously in the calling code and you deleted it by mistake. Definitely doesn't hurt if this function handles dates with extra whitespace.

xylar

I need to test this with some actual data but so far it looks really promising. Just a few very small changes I've seen so far.

xylar · 2017-11-29T10:13:44Z

...and yet you get a giant red x from me suggesting you screwed something up. Thanks, GitHub, for making me look like a jerk.

xylar · 2017-11-29T10:56:42Z

One other thing, our workflow is typically to work on our own forks of MPAS-Analysis rather than in branches within MPAS-Dev/MPAS-Analysis. I don't think this matters much. I personally just prefer it because it keeps the main repo free of clutter and it allows anyone who wants to to contribute without needing write access to the main repo.

xylar · 2017-11-29T11:03:25Z

mpas_analysis/analysis_task_template.py

 Xylar Asay-Davis
 '''

+from __future__ import absolute_import, division, print_function, unicode_literals


These lines are all too long (more than 80 characters) to be PEP8 compliant. Could they be broken into multiple lines, please?

Can do! Are you trying to follow PEP8 exactly? I've got lots of warnings for variable naming capitalization so I turned off the PEP8 checks.

I've taken care of these already, so no need to change them again.

We're trying to follow PEP8 pretty closely but we use mixed case for variable names. This doesn't give me any trouble in spyder but I guess other checkers might be stricter about that particular issue.

We definitely allow longer lines when it doesn't make sense to break them but this would be a case where I would break up lines.

xylar · 2017-11-29T11:25:21Z

Okay, I went ahead and pushed some commits that were needed in order for my python 2.7 test to run successfully. These address all of my comments above except the missing strip(), which I think should be done as a "fixup" on that commit.

xylar · 2017-11-29T11:27:23Z

@jhkennedy, I don't think we're too worried about having every single commit work, so I don't think there's a need to squash commits later on. It could be useful to see the process here because we'll need to recreate it for other outstanding branches that have yet to be merged.

jhkennedy · 2017-11-29T14:14:47Z

mpas_analysis/ocean/index_nino34.py


        nt = len(inputData)
-        sp = (len(wgts) - 1)/2
+        sp = int((len(wgts) - 1)/2)


It's probably better to use integer division here instead of a type cast:

sp = (len(wgts) - 1) // 2

Okay, agreed. I'll edit that commit, then. In which case, I'm not done after all...

xylar · 2017-11-29T14:15:44Z

I'm about to mess with commit history so probably best not to do any committing just now. I've got tests working in both python2 and python3, so we're making rapid progress!

xylar · 2017-11-29T14:22:37Z

Okay, done with messing with the commit history. @jhkennedy, feel free to modify/add commits as you see fit.

xylar · 2017-11-29T15:05:28Z

@pwolfram, is this something you're going to have time to review?

In python 3, stdout/stderr are bytes objects, which get logged incorrectly unless they get decoded into strings.

Must be decoded back to a unicode string before being written out.

jhkennedy · 2017-11-29T15:40:39Z

Regarding commit 7a474d1, I think there are at least 3 more init.py files with this problem that maybe the tests don't catch. What I've seen so far are:
https://github.com/MPAS-Dev/MPAS-Analysis/blob/develop/mpas_analysis/ocean/__init__.py
https://github.com/MPAS-Dev/MPAS-Analysis/blob/develop/mpas_analysis/sea_ice/__init__.py
https://github.com/MPAS-Dev/MPAS-Analysis/blob/develop/mpas_analysis/shared/time_series/__init__.py

It actually might be better to just stylistically pick absolute imports instead of relative imports (following the pythonism "explicit is better than implicit").

One other thing, our workflow is typically to work on our own forks of MPAS-Analysis rather than in branches within MPAS-Dev/MPAS-Analysis. I don't think this matters much. I personally just prefer it because it keeps the main repo free of clutter and it allows anyone who wants to to contribute without needing write access to the main repo.

Oh right, my bad! I actually prefer the forking workflow as well.

xylar · 2017-11-29T15:47:02Z

It actually might be better to just stylistically pick absolute imports instead of relative imports (following the pythonism "explicit is better than implicit").

I can't honestly say why we use relative imports. I guess I just learned that way by following examples I found. @pwolfram, was this an explicit choice you made for some reason early on? Or were you following my lead?

xylar · 2017-11-29T15:47:34Z

In any case, I would say a switch to absolute imports would be fine, but we should do it as a separate clean-up PR.

Travis will now test python 2.7, 3.5, and 3.6.

xylar · 2017-11-29T17:28:25Z

Awesome! It looks like CI is working, too. We're still waiting on 3.6 but it would be a surprise to me if 3.5 worked and 3.6 didn't.

jhkennedy · 2017-11-29T17:32:56Z

Awesome! It looks like CI is working, too. We're still waiting on 3.6 but it would be a surprise to me if 3.5 worked and 3.6 didn't.

I actually had to do a fixup just a little bit ago because 3.5 didn't work, but 3.6 did (forgot to use the conda-forge channel when creating the env).

Looks like they are all working now!

xylar · 2017-11-29T18:47:02Z

Testing

I've successfully run:

QU240 test on my laptop:
- python 2.7
- python 3.6
20171102.beta3rc02_1850.ne30_oECv3_ICG.edison on edison:
- python 2.7 (from acme-unified 1.1.1)
- python 3.6 (from my own conda environment with the listed MPAS-Analysis dependencies)

Links to the output are above.

xylar · 2017-11-30T13:46:48Z

@pwolfram, unless you think you'll have time to review this today or tomorrow, I think I'm going to take you off as a reviewer and go ahead and merge this PR. Please let me know one way or another.

xylar · 2017-11-30T13:49:31Z

@jhkennedy, if you are happy with the PR now, I'd suggest taking off the "don't merge" tag. You can merge it yourself if you like (once we've heard from @pwolfram) or I'll do it, whichever you prefer.

jhkennedy · 2017-11-30T13:55:40Z

I just went through the output you linked, and it looks like we're ready to merge.

jhkennedy · 2017-11-30T13:56:53Z

I'm happy to merge it; do you want me to leave the branch for easy python 2 -> 2+3 reference for a while?

xylar · 2017-11-30T15:22:42Z

No, go ahead and delete the branch when you merge. It's easy enough to go back through the commit history as needed, especially because the commit names can give you at least a pretty good idea of what's in there.

This adds python 2 and 3 support using the module `six` (as little as possible). The code base now follows python 3 styling with the use of __future__ imports for compatibility. This means: * print is a function call instead of a statement * Strings will typically be unicode instead of byte strings now. * imports need to explicitly declare relative imports * .keys()/.items()/.values() are used instead of iter versions from python 2 (there will be more memory overhead when running in python 2) * Some imports like ConfigParser will need to be handled by six while python 2 is being supported. Travis-CI will now run the pytests for python 2.7, 3.5, and 3.6.

Joseph H Kennedy added 6 commits November 28, 2017 14:12

Update print statement to function calls for python 3

57dbcb9

Use six for ConfigParser import and xrange

e8a7b0c

Must explicitly declare relative imports in python 3

7a474d1

Add futures to make python 2 code act like python 3

1dc933d

Cast iterators to lists as needed

e0be13b

jhkennedy added clean up in progress priority labels Nov 29, 2017

jhkennedy requested review from pwolfram and xylar November 29, 2017 00:59

xylar reviewed Nov 29, 2017

View reviewed changes

xylar requested changes Nov 29, 2017

View reviewed changes

xylar reviewed Nov 29, 2017

View reviewed changes

xylar mentioned this pull request Nov 29, 2017

Fix future warning from xarray #281

Merged

jhkennedy commented Nov 29, 2017

View reviewed changes

xylar force-pushed the jhkennedy/python3-43 branch from 00c1e14 to 180289c Compare November 29, 2017 14:20

xylar force-pushed the jhkennedy/python3-43 branch from 180289c to 836b39a Compare November 29, 2017 14:25

xylar added the mpas_xarray label Nov 29, 2017

Cast byte strings to unicode strings

3257e0c

xylar added 4 commits November 29, 2017 10:17

More explicitly declared relative imports

0ee8da4

More netcdf string to unicode conversion

b8c783f

Decode stdout/stderr before sending them to logger

da4abec

In python 3, stdout/stderr are bytes objects, which get logged incorrectly unless they get decoded into strings.

fix encoding as xml

89666c4

Must be decoded back to a unicode string before being written out.

jhkennedy force-pushed the jhkennedy/python3-43 branch from 836b39a to 89666c4 Compare November 29, 2017 15:19

xylar approved these changes Nov 29, 2017

View reviewed changes

jhkennedy force-pushed the jhkennedy/python3-43 branch from ff7d315 to fb9d482 Compare November 29, 2017 17:07

Update travis for multiple python version

58bd2da

Travis will now test python 2.7, 3.5, and 3.6.

jhkennedy force-pushed the jhkennedy/python3-43 branch from fb9d482 to 58bd2da Compare November 29, 2017 17:23

xylar assigned jhkennedy Nov 30, 2017

jhkennedy removed the in progress label Nov 30, 2017

jhkennedy removed the request for review from pwolfram December 1, 2017 00:11

jhkennedy merged commit 58bd2da into develop Dec 1, 2017

jhkennedy deleted the jhkennedy/python3-43 branch December 1, 2017 00:32

jhkennedy restored the jhkennedy/python3-43 branch December 1, 2017 00:34

jhkennedy deleted the jhkennedy/python3-43 branch December 1, 2017 00:36

xylar mentioned this pull request Feb 12, 2018

Switch from relative to absolute imports #307

Merged

xylar mentioned this pull request Feb 20, 2019

python 3 support E3SM-Project/processflow#108

Closed

Add python 2 and 3 support #280

Add python 2 and 3 support #280

Uh oh!

Conversation

jhkennedy commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status:

Left to do:

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar left a comment

Choose a reason for hiding this comment

Uh oh!

xylar commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

jhkennedy Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

jhkennedy commented Nov 29, 2017

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

xylar commented Nov 29, 2017

Uh oh!

jhkennedy commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Nov 29, 2017

Testing

Uh oh!

xylar commented Nov 30, 2017

Uh oh!

xylar commented Nov 30, 2017

Uh oh!

jhkennedy commented Nov 30, 2017

Uh oh!

jhkennedy commented Nov 30, 2017

Uh oh!

xylar commented Nov 30, 2017

Uh oh!

Reviewers

Assignees

jhkennedy commented Nov 29, 2017 •

edited

Loading

xylar commented Nov 29, 2017 •

edited

Loading

xylar commented Nov 29, 2017 •

edited

Loading

jhkennedy Nov 29, 2017 •

edited

Loading

jhkennedy commented Nov 29, 2017 •

edited

Loading