DM-7185: Python 3 #2

timj · 2016-09-19T20:37:19Z

No description provided.

autopep8 {{package_dir}} --in-place --recursive --ignore E26,E133,E226,E228,N802,N803 --max-line-length 110

timj

Looks good. There are some issues that need resolving.

timj · 2016-09-19T20:38:58Z

bin.src/dagIdInfo.py

-    if os.path.exists(filename) == False:
-        print "file %s not found" % filename
+
+    if os.path.exists(filename) is False:


Given that os.path.exists() returns a boolean I think it would be clearer as:

if not os.path.exists(filename):

timj · 2016-09-19T20:39:40Z

bin.src/allocateNodes.py

-    UNKNOWN_PLATFORM_EXIT_CODE = 10
-    MISSING_PBS_CONFIG_EXIT_CODE = 20
+    # UNKNOWN_PLATFORM_EXIT_CODE = 10
+    # MISSING_PBS_CONFIG_EXIT_CODE = 20


Are these not being used then? Are they informational?

These are intended to be used at a later date when we have severity levels. I'm looking into this later this month.

timj · 2016-09-19T20:40:26Z

bin.src/runOrca.py

@@ -55,7 +56,7 @@ def main():
        sys.stdin.close()
        sys.stdout.close()
        sys.stderr.close()
-        if creator.isVerbose == False:
+        if creator.isVerbose is False:


if not creator.isVerbose:

?

timj · 2016-09-19T20:43:32Z

etc/scripts/generateDag.py

+try:
+    from shlex import split as cmd_split
+except ImportError:
+    from pipes import split as cmd_split


I don't understand this because shlex has the split function in python 2 and 3 and pipes never seems to have it.

I'll have to look at this again.

I'm not sure what happened there. Must have been a copy buffer paste. Thanks for catching that.

timj · 2016-09-19T20:44:38Z

etc/scripts/generateDag.py

-        # If there is no space, the dataid is something simple like a skytile id  
-        newData=myData
+        # If there is no space, the dataid is something simple like a skytile id
+        newData = myData
        visit = str(int(count / 100))


visit = str(count // 100)

?

timj · 2016-09-19T20:51:52Z

python/lsst/ctrl/execute/seqFile.py

@@ -38,7 +42,7 @@ def nextSeq(self):
        @return a sequence number
        """
        seq = 0
-        if os.path.exists(self.fileName) == False:
+        if os.path.exists(self.fileName) is False:


if not os.path.exists():

?

timj · 2016-09-19T20:54:51Z

tests/testDagIdInfo.py

@@ -37,16 +46,26 @@ def test1(self):
        filename = os.path.join("tests", "testfiles", "test.diamond.dag")

        stdout = self.executeCommand("%s %s A1 %s" % (exe, execPath, filename))
-        self.assertTrue(stdout == "run=1033 filter=r camcol=2 field=229\n")
+        stdout = bytes(stdout)


I think it would make more sense for executeCommand to use decode() to decode the output from the command and convert it to a real string. Then drop all this bytes stuff.

absolutely. don't know what I was thinking.

timj · 2016-09-19T20:55:12Z

tests/testSeqFile.py

-        filename = os.path.join("/tmp",username+"_"+str(os.getpid())+".seq")
-        if os.path.exists(filename) == True:
+        filename = os.path.join("/tmp", username+"_"+str(os.getpid())+".seq")
+        if os.path.exists(filename) is True:


Drop the is True.

replaced that test

timj · 2016-09-19T20:56:36Z

tests/testSeqFile.py

    def test1(self):
        username = pwd.getpwuid(os.geteuid()).pw_name
-        filename = os.path.join("/tmp",username+"_"+str(os.getpid())+".seq")
-        if os.path.exists(filename) == True:
+        filename = os.path.join("/tmp", username+"_"+str(os.getpid())+".seq")


Can we use lsst.utils.tests.getTempFilePath() here?

Likely so. I'll check. This code pre-dates that.

Yup. Replaced that code to use getTempFilePath().

timj · 2016-09-19T20:57:07Z

tests/testTemplateWriter.py

    def test1(self):
        pairs = {}
        pairs["TEST1"] = "Hello"
        pairs["TEST2"] = "Goodbye"
        infile = os.path.join("tests", "testfiles", "templateWriter.template")
        compare = os.path.join("tests", "testfiles", "templateWriter.txt")
        username = pwd.getpwuid(os.geteuid()).pw_name
-        outfile = os.path.join("/tmp",username+"_"+str(os.getpid())+"_template.txt")
+        outfile = os.path.join("/tmp", username+"_"+str(os.getpid())+"_template.txt")


lsst.utils.tests.getTempFilePath()?

yes. Replaced that code to use getTempFilePath()

hsinfang

About the commit messages, our Developer Guide asks us to write commit messages in the imperative tense.

hsinfang · 2016-09-19T21:33:45Z

tests/testSeqFile.py


    def test1(self):
        username = pwd.getpwuid(os.geteuid()).pw_name
        filename = os.path.join("/tmp", username+"_"+str(os.getpid())+".seq")
-        if os.path.exists(filename) == True:
+        if os.path.exists(filename) is True:


is Ture not needed

hsinfang · 2016-09-19T21:34:23Z

python/lsst/ctrl/execute/configurator.py

@@ -222,7 +221,7 @@ def createConfiguration(self, input):
        @return the newly created Orca configuration file
        """
        resolvedInputName = envString.resolve(input)
-        if self.opts.verbose == True:
+        if self.opts.verbose is True:
            print("creating configuration using ", resolvedInputName)


is True not needed

hsinfang · 2016-09-19T21:34:36Z

python/lsst/ctrl/execute/configurator.py

-        self.outputFileName = os.path.join(configDir, "%s_config.py" % (self.runid))
-        if self.opts.verbose == True:
+        self.outputFileName = os.path.join(configDir, "%s.config" % (self.runid))
+        if self.opts.verbose is True:


is True not needed

hsinfang · 2016-09-19T21:35:03Z

python/lsst/ctrl/execute/configurator.py

@@ -234,10 +233,10 @@ def createConfiguration(self, input):
        substitutes["CTRL_EXECUTE_SETUP_PACKAGES"] = self.getSetupPackages()

        configDir = os.path.join(substitutes["LOCAL_SCRATCH"], "configs")
-        if os.path.exists(configDir) == False:
+        if os.path.exists(configDir) is False:


if not os.path.exists(configDir):?

hsinfang · 2016-09-19T21:35:50Z

bin.src/dagIdInfo.py

@@ -38,7 +38,7 @@
    dagNode = sys.argv[1]
    filename = sys.argv[2]

-    if os.path.exists(filename) == False:
+    if os.path.exists(filename) is False:


if not os.path.exists(filename)?

hsinfang · 2016-09-19T21:38:34Z

tests/testAllocationConfig.py

@@ -23,30 +23,40 @@
 import unittest
 import os.path
 from lsst.ctrl.execute.allocationConfig import AllocationConfig
+import lsst.utils.tests



I assume these changes in tests/ are not done by flake8; the commit message "flake8 code cleanup" is then confusing..

hsinfang · 2016-09-19T21:40:34Z

python/lsst/ctrl/execute/condorInfoConfig.py

@@ -61,5 +61,5 @@ class CondorInfoConfig(pexConfig.Config):
    filename = envString.resolve(filename)
    config.load(filename)

-    for i in config.platform.keys():
+    for i in list(config.platform.keys()):


same as the other list()+keys()

Removed redundant references using list(keys())

srp3rd added 5 commits September 8, 2016 14:26

Ran autopep8:

13fdfec

autopep8 {{package_dir}} --in-place --recursive --ignore E26,E133,E226,E228,N802,N803 --max-line-length 110

flake8 code cleanup

9daa161

Ran futurize -2 -x division_safe -n -w .

0301e4e

Fix flake8 output after futurize -2

f047208

updating pydoc

ae0ca21

timj commented Sep 19, 2016

View reviewed changes

hsinfang reviewed Sep 19, 2016

View reviewed changes

timj mentioned this pull request Sep 19, 2016

DM7183: Port to Python 3 lsst-dm/legacy-ctrl_orca#2

Merged

Fixed extraneous "is True" and "is False" conditionals.

c7bc58c

Removed redundant references using list(keys())

srp3rd merged commit c7bc58c into master Sep 20, 2016

ktlim deleted the tickets/DM-7185 branch August 25, 2018 05:05

DM-7185: Python 3 #2

DM-7185: Python 3 #2

Conversation

timj commented Sep 19, 2016

timj 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

timj Sep 19, 2016 • edited

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

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

hsinfang 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

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

timj Sep 19, 2016 •

edited