Upgrade Modules to Python 3 #1686

amarkpayne · 2019-08-12T21:11:28Z

Changes

PEP-8 reformatting
Renaming local variables following PEP-8
Futurize
Fix relative imports
Fix iterators

amarkpayne · 2019-08-12T21:12:27Z

Not complete yet, but I want to see what happens with the tests and Codacy checks

rwest · 2019-08-12T21:31:10Z

Looks like a great start! Please can you post somewhere a list of steps taken to do similar on other unmerged pull requests, other branches, and other projects that build on or use RMG?

amarkpayne · 2019-08-12T22:06:41Z

Looks like a great start! Please can you post somewhere a list of steps taken to do similar on other unmerged pull requests, other branches, and other projects that build on or use RMG?

Certainly! First off, I recommend taking a look at the google doc that @mliu49 sent out in the shared Slack channel, there are a lot of good details in there. The general workflow I am taking so far is as follows (@mliu49, @mjohnson541, @alongd, and @xiaoruiDong feel free to add anything or correct any mistakes I make):

PEP-8 reformatting (optional but highly encouraged. It has made a lot of the files easier to parse through already). As Max showed me I highly recommend using PyCharm's Reformat Code tool (Code > Reformat Code) and then going through the file to fix up any mistakes or changes that still need to be made. This should focus just on whitespace and formatting issues.
Renaming local variables following PEP-8 (optional, but some of us think that it is worthwhile). Again, PyCharm's refactor feature is great for this. DO NOT rename any public variables (functions/methods, classes, etc.)
Futurize (required) Get the futurize script conda install future and then call it on a single file with futurize -0 -wn file_name. The zero flag performs both stage 1 and stage 2 improvements to the code. The w flag actually writes changes to the file, and the n flag prevents creating a backup file (this is what git is for of course). I haven't run into as many of the issues with this as Max and Matt, but one issue that comes up is that if the code has any division statements it will try to import old style division and place it everywhere in your code. If this happens I recommend reverting the file back, placing a from __future__ import division statement at the top of the import block, and running the futurize script again. Thanks to Max for figuring out that this prevents the script from using old style division (make sure to unit test that old style division wasn't needed anywhere).
Replace relative import statements with absolute import statements (required). The futurize scripts will fix the relative import statements, but we want to get rid of them completely. For example, no from molecule import Molecule, replace this with from rmgpy.molecule import Molecule.
Replace iter[keys, values, items] with [keys, values, items] (required). Searching the files for iter is helpful here. If an iterator won't work and a list is need be sure to convert with list()
Unit Test one more time (required)
Squash commits so that there is one commit for each of steps 1-5. (required)
Open PR to py3 branch or master (if this is after the initial transition or an existing PR)

One final note: the order here is important. In particular, do the style changes before the futurize changes so that the futurize changes are not masked in the git blame.

codecov · 2019-08-12T22:13:46Z

Codecov Report

Merging #1686 into py3 will decrease coverage by 0.06%.
The diff coverage is 34.29%.

@@            Coverage Diff             @@
##              py3    #1686      +/-   ##
==========================================
- Coverage   41.82%   41.75%   -0.07%     
==========================================
  Files         167      167              
  Lines       29684    29725      +41     
  Branches     6203     6222      +19     
==========================================
- Hits        12415    12413       -2     
- Misses      16348    16385      +37     
- Partials      921      927       +6

Impacted Files	Coverage Δ
rmgpy/rmg/__init__.py	`100% <ø> (ø)`	⬆️
rmgpy/rmg/main.py	`21.74% <ø> (-0.11%)`	⬇️
rmgpy/rmg/model.py	`40.98% <ø> (-0.1%)`	⬇️
rmgpy/rmg/listener.py	`100% <100%> (ø)`	⬆️
rmgpy/rmg/react.py	`86.53% <100%> (ø)`	⬆️
rmgpy/rmg/input.py	`34.31% <29.29%> (-1.67%)`	⬇️
rmgpy/display.py	`50% <50%> (ø)`	⬆️
rmgpy/rmg/output.py	`53.36% <60.78%> (ø)`	⬆️
rmgpy/rmg/settings.py	`82.22% <83.33%> (ø)`	⬆️
rmgpy/rmg/pdep.py	`12.35% <9.33%> (+0.02%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3fa250...2913cb7. Read the comment docs.

mjohnson541 · 2019-08-12T22:30:37Z

It should be noted that you shouldn't rename local variables because if you do so it will make rebasing on RMG an absolute nightmare.

amarkpayne · 2019-08-15T20:31:52Z

I have altered the author dates on these commits so that they show up in the easiest order to review them in. Note that in the "Futurize" commit I introduce the absolute import feature, but this is always removed in the next commit "replace relative imports".

amarkpayne · 2019-08-16T14:27:31Z

Please note that all of the steps (PEP-8 reformatting, renaming local variables, futurize, replace relative imports) have been done for all files list here and with my name on them in the google doc. If you don't see a commit about it, it just means that the step was unnecessary for that file.

I have also separated the commits out by groups of files to make it easier to review. @alongd feel free to do what you want with the Arkane commits at the end of this PR.

alongd · 2019-08-16T15:17:24Z

Thanks! The Arkane commits were cherry-picked and rebased on top of the Arkane Py3 PR

mliu49 · 2019-08-16T23:58:46Z

@alongd, can you confirm that it's safe to remove the last 5 commits from this branch?

alongd · 2019-08-17T10:58:41Z

I confirm. They were implemented appropriately in the Arkane branch, split by content and squashed into existing commits.

mliu49

I made a couple of comments. Two things which you could consider doing in all of these files (if applicable) are

Replacing star imports (my approach has been to delete it and wait for PyCharm to flag unknown names)
Replacing import numpy with import numpy as np for consistency

rmg.py

mliu49 · 2019-08-20T16:47:32Z

rmgpy/rmgobjectTest.py

@@ -273,12 +273,12 @@ def setUp(self):
        self.scalar_dict = {'class': 'ScalarQuantity', 'value': 500.0, 'units': 'K'}

        # Abbreviate name
-        PRO = PseudoRMGObject
+        pro = PseudoRMGObject


This is technically aliasing a class, so should it be CamelCase? (Bringing up for discussion)

Thanks for the catch! I am the one who wrote this, so I guess past me got it right then.

https://stackoverflow.com/questions/13765980/what-is-the-naming-convention-for-python-class-references

Also, I just realized that this abbreviation is not necessary, as without it this code will still be within the character limit. I will get rid of it entirely then.

mliu49 · 2019-08-20T18:11:16Z

rmgpy/rmg/outputTest.py

-from .model import CoreEdgeReactionModel, ReactionModel
-from .output import *
+from rmgpy.rmg.model import CoreEdgeReactionModel, ReactionModel
+from rmgpy.rmg.output import *


I think we would also like to replace star imports if possible.

Done, thanks!

rmgpy/__init__.py

environment_linux.yml

amarkpayne · 2019-08-20T21:08:55Z

Thanks for all of the comments @mliu49! You were right about removing the * imports, we were getting imports in very strange ways. I have rebased this branch on the current py3 branch, let me know what you think.

mliu49 · 2019-08-20T21:33:47Z

rmgpy/rmg/inputTest.py


-from rmgpy.rmg.main import RMG
 from rmgpy.rmg import input as inp


What would be the difference between this and import rmgpy.rmg.input as inp?

After doing some searching and some playing around in python, there is no difference between these two statements, and it is purely up to us as to which one we prefer. It is worth noting that if it wasn't for the as inp statements in each that there would be a slight difference, as import rmgpy.rmg.input would put rmgpy.rmg into the namespace, whereas from rmgpy.rmg import input would not. There is an even more interesting but subtle effect if we were trying to import a module level variable (see the second from the top response here), but this does not apply in this case.

Given this, which one do we prefer?

Good to know!

I have a slight preference for import rmgpy.rmg.input as inp since it's cleaner and seems a bit more straightforward. I always figured that from imports were mainly if you wanted something from within a file. Up to you whether or not to change it though.

mliu49

Thanks! I added a couple minor comments.

Are you planning to squash commits together?

mliu49 · 2019-08-20T21:35:46Z

rmgpy/rmg/main.py

    def loadInput(self, path=None):
        """
        Load an RMG job from the input file located at `inputFile`, or
        from the `inputFile` attribute if not given as a parameter.
        """
-        from input import readInputFile
+        from .input import readInputFile


Replace with absolute import.

Thanks for the catch! I forgot to check import statements nested in the code. I did some further searching using regex in PyCharm and I didn't find any further instances in the files covered by this PR, so I think we have it all now.

mliu49 · 2019-08-20T21:35:57Z

rmgpy/rmg/main.py

    def loadThermoInput(self, path=None):
        """
        Load an Thermo Estimation job from a thermo input file located at `inputFile`, or
        from the `inputFile` attribute if not given as a parameter.
        """
-        from input import readThermoInputFile
-        if path is None: path = self.inputFile
+        from .input import readThermoInputFile


mliu49 · 2019-08-20T21:36:16Z

rmgpy/rmg/main.py

    def saveInput(self, path=None):
        """
        Save an RMG job to the input file located at `path`, or
        from the `outputFile` attribute if not given as a parameter.
        """
-        from input import saveInputFile
-        if path is None: path = self.outputFile
+        from .input import saveInputFile


mliu49 · 2019-08-20T21:41:35Z

rmgpy/rmg/model.py

+        self.newSurfaceRxnsLoss = self.newSurfaceRxnsLoss | lost_surface_rxns
+
+        return not (
+                    self.newSurfaceRxnsAdd != set() or self.newSurfaceRxnsLoss != set() or self.newSurfaceSpcsLoss != set() or self.newSurfaceSpcsAdd != set())


I think this should either be one line or split after each or.

Done, I agree this is much better

amarkpayne

Thanks for the comments, I have made the fixes (including an error I made in the last rebase that caused the tests to fail, but I have made sure that everything is back to what it should be).

Before I push the commits, I can squash them if you would prefer. I wasn't sure if we were doing that before each merge into py3 or if we were going to squash the py3 branch before merging into master

amarkpayne · 2019-08-21T01:20:22Z

rmgpy/rmg/inputTest.py


-from rmgpy.rmg.main import RMG
 from rmgpy.rmg import input as inp


After doing some searching and some playing around in python, there is no difference between these two statements, and it is purely up to us as to which one we prefer. It is worth noting that if it wasn't for the as inp statements in each that there would be a slight difference, as import rmgpy.rmg.input would put rmgpy.rmg into the namespace, whereas from rmgpy.rmg import input would not. There is an even more interesting but subtle effect if we were trying to import a module level variable (see the second from the top response here), but this does not apply in this case.

Given this, which one do we prefer?

amarkpayne · 2019-08-21T01:22:30Z

rmgpy/rmg/main.py

    def loadInput(self, path=None):
        """
        Load an RMG job from the input file located at `inputFile`, or
        from the `inputFile` attribute if not given as a parameter.
        """
-        from input import readInputFile
+        from .input import readInputFile


Thanks for the catch! I forgot to check import statements nested in the code. I did some further searching using regex in PyCharm and I didn't find any further instances in the files covered by this PR, so I think we have it all now.

amarkpayne · 2019-08-21T01:22:38Z

rmgpy/rmg/main.py

    def loadThermoInput(self, path=None):
        """
        Load an Thermo Estimation job from a thermo input file located at `inputFile`, or
        from the `inputFile` attribute if not given as a parameter.
        """
-        from input import readThermoInputFile
-        if path is None: path = self.inputFile
+        from .input import readThermoInputFile


amarkpayne · 2019-08-21T01:22:46Z

rmgpy/rmg/main.py

    def saveInput(self, path=None):
        """
        Save an RMG job to the input file located at `path`, or
        from the `outputFile` attribute if not given as a parameter.
        """
-        from input import saveInputFile
-        if path is None: path = self.outputFile
+        from .input import saveInputFile


amarkpayne · 2019-08-21T01:25:48Z

rmgpy/rmg/model.py

+        self.newSurfaceRxnsLoss = self.newSurfaceRxnsLoss | lost_surface_rxns
+
+        return not (
+                    self.newSurfaceRxnsAdd != set() or self.newSurfaceRxnsLoss != set() or self.newSurfaceSpcsLoss != set() or self.newSurfaceSpcsAdd != set())


Done, I agree this is much better

amarkpayne · 2019-08-21T02:06:22Z

Final note: the commit Add missing InputError import in arkane/util.py is no longer needed and will be dropped.

mliu49 · 2019-08-21T02:25:17Z

I'm still a bit undecided on the best way to handle commits for this process. Perhaps we can discuss tomorrow.

Only covers the following files: chemkinTest.py constants*.py constraints*.py display.py exceptions.py quantityTest.py rmgObjectTest.py

Before we were getting it from a dependency of a dependency

@mliu49

Previous wildcard imports masked this issue. Thanks to @mliu49 for catching this and making this fix.

amarkpayne · 2019-08-21T15:21:37Z

I have squashed the commits and rebased on the current py3 branch

amarkpayne self-assigned this Aug 12, 2019

amarkpayne added Py3 Status: Blocked This PR should not be merged labels Aug 12, 2019

amarkpayne force-pushed the amp_py3_pr branch 4 times, most recently from 9720913 to cbc2329 Compare August 14, 2019 15:55

amarkpayne marked this pull request as ready for review August 14, 2019 15:55

amarkpayne requested a review from mliu49 August 14, 2019 15:56

amarkpayne force-pushed the amp_py3_pr branch 2 times, most recently from 60d0fe9 to b42bd3b Compare August 14, 2019 20:47

amarkpayne added Status: Ready for Review PR is complete and ready to be reviewed and removed Status: Blocked This PR should not be merged labels Aug 15, 2019

amarkpayne mentioned this pull request Aug 15, 2019

Prevent file locking for HDF5 files #1688

Closed

amarkpayne force-pushed the amp_py3_pr branch from d7275e8 to c7d5f7a Compare August 15, 2019 20:30

amarkpayne mentioned this pull request Aug 16, 2019

Futurize Arkane to Py3 #1693

Merged

amarkpayne force-pushed the amp_py3_pr branch 2 times, most recently from 70dd9fe to 764147c Compare August 16, 2019 14:24

mliu49 added a commit that referenced this pull request Aug 17, 2019

TEMP FIX: This results in merge conflicts with #1686

04a0a4d

amarkpayne force-pushed the amp_py3_pr branch from 764147c to d8125a1 Compare August 19, 2019 11:33

mliu49 reviewed Aug 20, 2019

View reviewed changes

amarkpayne force-pushed the amp_py3_pr branch from 39a5d97 to 682f446 Compare August 20, 2019 21:07

mliu49 reviewed Aug 20, 2019

View reviewed changes

amarkpayne commented Aug 21, 2019

View reviewed changes

amarkpayne force-pushed the amp_py3_pr branch from 682f446 to 01355b6 Compare August 21, 2019 02:01

mliu49 force-pushed the py3 branch from c659139 to b3fa250 Compare August 21, 2019 14:54

amarkpayne added 9 commits August 21, 2019 11:04

Upgrade external wip to Python 3

7e206f3

Upgrade rmg.py to Python 3

8423581

Upgrade rmgpy/*.py to Python 3

57edf9a

Only covers the following files: chemkinTest.py constants*.py constraints*.py display.py exceptions.py quantityTest.py rmgObjectTest.py

Upgrade rmgpy/rmg/*.py to Python 3

d74b859

Upgrade scripts to Python 3

1701e94

Correct path issue so that outputTest.py runs from any location

abe3d06

Prevent file locking for HDF5 files

22af8bb

Explicitly add h5py dependency

2194bf8

Before we were getting it from a dependency of a dependency

Bug fix: Import conformer from statmech instead of pdep

2913cb7

Previous wildcard imports masked this issue. Thanks to @mliu49 for catching this and making this fix.

amarkpayne force-pushed the amp_py3_pr branch from 01355b6 to 2913cb7 Compare August 21, 2019 15:08

mliu49 merged commit 81ed4c6 into py3 Aug 21, 2019

mliu49 deleted the amp_py3_pr branch August 21, 2019 18:55

amarkpayne mentioned this pull request Aug 26, 2019

h5py locking of HDF5 files causes issues on NFS mounted file systems #1708

Closed


		from rmgpy.rmg.main import RMG
		from rmgpy.rmg import input as inp

Upgrade Modules to Python 3 #1686

Upgrade Modules to Python 3 #1686

Conversation

amarkpayne commented Aug 12, 2019 • edited Loading

Changes

amarkpayne commented Aug 12, 2019

rwest commented Aug 12, 2019

amarkpayne commented Aug 12, 2019 • edited Loading

codecov bot commented Aug 12, 2019 • edited Loading

Codecov Report

mjohnson541 commented Aug 12, 2019

amarkpayne commented Aug 15, 2019

amarkpayne commented Aug 16, 2019

alongd commented Aug 16, 2019

mliu49 commented Aug 16, 2019

alongd commented Aug 17, 2019

mliu49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarkpayne commented Aug 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mliu49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarkpayne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarkpayne commented Aug 21, 2019

mliu49 commented Aug 21, 2019

amarkpayne commented Aug 21, 2019

amarkpayne commented Aug 12, 2019 •

edited

Loading

amarkpayne commented Aug 12, 2019 •

edited

Loading

codecov bot commented Aug 12, 2019 •

edited

Loading