Python 3 Upgrade #160

fanpu · 2019-12-27T16:48:17Z

Plan:

Unfortunately, we have almost no test coverage so regressions must be detected by hand. Our goal is to port Tango to Python 3 while still maintaining Python 2 compatibility.

Follow Futurize porting guide (http://python-future.org/automatic_conversion.html) with manual inspection. For each stage, create PRs that merge into this PR so it is clearer which changes are being introduced at which stage for accountability.
Test Tango CLI on Python 3 manually (maybe write some tests?)
Test Tango on Python 3 with Autolab
Test Tango CLI on Python 2 manually
Test Tango on Python 2 with Autolab

Tested with all CLI commands given in https://autolab.github.io/docs/tango-cli/

This is now ready for review!

fanpu · 2020-02-29T15:07:29Z

I have looked through the stage 1 fixes applied by futurize and they LGTM, mostly very safe

victorhuangwq

Tested Tango by grading various assignments through Autolab, and it works as expected.
Overall the changes looks good, and I think this is a good patch (and improvement).

Some minor issues should be addressed before merging, but other than that it should be okay!

jobManager.py

Dockerfile

jobQueue.py

victorhuangwq · 2020-07-04T14:39:07Z

preallocator.py

@@ -29,7 +31,7 @@ def __init__(self, vmms):
    def poolSize(self, vmName):
        """ poolSize - returns the size of the vmName pool, for external callers
        """
-        if vmName not in self.machines.keys():
+        if vmName not in list(self.machines.keys()):


same comment with regards to the key situation in jobqueue

self.machines here is also either a TangoNativeDictionary or TangoRemoteDictionary

refer to comment below

tangoObjects.py

vmms/distDocker.py

…ents

fanpu · 2020-07-06T22:37:13Z

@victorhuangwq Thanks for your comments! I've rectified the issues you raised, do look at it again when you're available :D

victorhuangwq

There are still some issues with regards to the usage of TangoRemoteDictionary. Other than that it looks good.

victorhuangwq · 2020-07-08T12:36:56Z

tangoObjects.py

            return self.dict[str(id)]
        else:
            return None

    def keys(self):
-        return self.dict.keys()
+        return list(self.dict.keys())


doesn't it return a list here?

Wouldn't calling list again on keys() be redundant in the other lines I have highlighted above? Such as in
if vmName not in list(self.machines.keys()):

victorhuangwq · 2020-07-08T12:40:57Z

tangoObjects.py

            unpickled_obj = self.r.hget(self.hash_name, str(id))
            obj = pickle.loads(unpickled_obj)
            return obj
        else:
            return None

    def keys(self):
-        return self.r.hkeys(self.hash_name)
+        keys = map(lambda key : key.decode(), self.r.hkeys(self.hash_name))
+        return list(keys)


It seems to be returning a list when keys are called on the TangoRemoteDictionary.

victorhuangwq · 2020-07-08T12:53:42Z

preallocator.py

@@ -29,7 +31,7 @@ def __init__(self, vmms):
    def poolSize(self, vmName):
        """ poolSize - returns the size of the vmName pool, for external callers
        """
-        if vmName not in self.machines.keys():
+        if vmName not in list(self.machines.keys()):


refer to comment below

cg2v · 2020-08-15T12:46:43Z

This should not block the upgrade, since I believe the current code to be correct, but most if not all of the keys() calls in the code do not require the list form of the keys (you only need to listify the keys if you either want to use [] to access elements of the key list, or if you will alter the dict while iterating over the keys). Furthermore, a more readable way to do
if vmName in self.machines.keys():

would be to implement __contains__ in the Tango*Dictionary classes themselves and then call

if vmName in self.machines:

instead. TangoRemoteDictionary could use HEXISTS to implement __contains__ and not have to fetch everything.

cg2v · 2020-08-15T12:55:30Z

jobManager.py

@@ -117,7 +129,7 @@ def __manage(self):
        tango = TangoServer()
        tango.log.debug("Resetting Tango VMs")
        tango.resetTango(tango.preallocator.vmms)
-        for key in tango.preallocator.machines.keys():
+        for key in list(tango.preallocator.machines.keys()):


Another unnecessary list()

cg2v · 2020-08-15T12:56:04Z

preallocator.py

@@ -44,7 +46,7 @@ def update(self, vm, num):
        of machines as necessary.
        """
        self.lock.acquire()
-        if vm.name not in self.machines.keys():
+        if vm.name not in list(self.machines.keys()):


another unnecessary list()

fanpu · 2020-08-15T15:08:56Z

This should not block the upgrade, since I believe the current code to be correct, but most if not all of the keys() calls in the code do not require the list form of the keys (you only need to listify the keys if you either want to use [] to access elements of the key list, or if you will alter the dict while iterating over the keys). Furthermore, a more readable way to do
if vmName in self.machines.keys():

would be to implement __contains__ in the Tango*Dictionary classes themselves and then call

if vmName in self.machines:

instead. TangoRemoteDictionary could use HEXISTS to implement __contains__ and not have to fetch everything.

@cg2v This is a great suggestion, I will add it as a future enhancement instead of adding that to this PR to avoid making this PR bigger than it already is.

Thank you and really appreciate the spotting of other redundant wrappers of list as well!

victorhuangwq

Testing: Tested Tango3 branch by grading various assignments through Autolab, and it works as expected.

I have verified that:

Patch from Autograding. Error message: 795: unexpected token at '---' #153 has been applied
Unnecessary lists, as mentioned by both @cg2v and I, has been removed
Dockerfile is now back on 18.04 (change removed)

This pull request LGTM :)

victorhuangwq · 2020-08-19T10:15:29Z

@fanpu Before you merge in however, please create an alternate Python 2 branch, preferably called master-python-2 to hold the old code.

At the same time, here are the list of things we should also do alongside this PR. I can help with it as well.

Update the description on the readme under the section Python 3 Upgrade. I do believe the wording that was used in the Autolab readme regarding rails 4 support is good.
Add a new release, marking this as a new version of Tango. (v2.0.0 maybe?)

fanpu · 2020-08-19T15:36:50Z

Created master-python2 branch with updated README.
Updated the README on this branch to refer to master-python2 branch for legacy purposes.

I will draft a new release after I merge this to master. I think other TODO items would include update installation instructions on our docs too.

fanpu requested a review from droh April 17, 2020 00:06

victorhuangwq requested a review from TheodorJ April 17, 2020 00:09

fanpu removed the request for review from TheodorJ May 25, 2020 20:28

victorhuangwq requested changes Jul 4, 2020

View reviewed changes

fanpu added 25 commits July 6, 2020 18:03

Stage 1 fixes for restful-tango/*.py

b9f8faa

Stage1 fix for tests/*.py

f49a24d

Stage1 fix for jobManager

2fede95

Stage1 fix for tango-cli.py

de0f03b

Remove accidentally committed .bak files

296c5ea

dictionary fixes

1ea7326

map fixes

53dcf46

remapping for builtins semantics

b4fafcb

use standard library alises for refactored modules

d8b9da1

urllib renamed modules

3c8513d

pasteurize newstyle for obj

877c527

update packages

5f95779

check and apply fix_unicode_keep_u fix

1fd770d

xrange fixes

e2e6476

downgrade tornado to be capped below 6

0fa7c3a

update tango dict to use new iter dict syntax

da74689

downgrade to tornado 4.x because 5.x has some new event loop requirem…

38cdda8

…ents

convert shell output to utf8

d270552

convert subprocess.check_output results to utf8

da9d68b

better error and unicode fixes

16a5f89

rollback asyncio event loop

812f47f

get better perf by using hexists and to avoid getting bytes back

78520bf

remove accidental dbg

4e71932

specify append bytes for worker

b75150f

convert redis keys output from bytes to string

7fd640b

fanpu added 2 commits July 6, 2020 18:03

convert request response to text

28ddf89

Skip over jobs that are None

6398c7d

fanpu force-pushed the python3-upgrade branch from ef20680 to 6398c7d Compare July 6, 2020 22:04

fanpu added 2 commits July 6, 2020 18:28

More efficient native dict key lookup

9352903

Issue #153 fixes by changing &> to 2>&1

b3462fb

victorhuangwq requested changes Jul 8, 2020

View reviewed changes

victorhuangwq added Status: In Progress Type: Enhancement labels Jul 8, 2020

This was linked to issues Jul 8, 2020

Autograding. Error message: 795: unexpected token at '---' #153

Closed

NoneType Exception When Queuing Jobs #138

Closed

fanpu added 4 commits August 14, 2020 17:31

apply correct fixes

76d4215

ignore more ide files

86dbca8

list fix

a8f986b

remove more usages of unnecessary list

0b13783

fanpu changed the title ~~Python 3 Upgrade (WIP)~~ Python 3 Upgrade Aug 14, 2020

cg2v reviewed Aug 15, 2020

View reviewed changes

remove more unnecessary list wraps

4e630a9

fanpu mentioned this pull request Aug 15, 2020

Code improvement opportunities #171

Closed

victorhuangwq approved these changes Aug 19, 2020

View reviewed changes

Update README

9feb851

fanpu merged commit 88fcf61 into master Aug 19, 2020

fanpu removed the Status: In Progress label Aug 19, 2020

oliverli deleted the python3-upgrade branch February 18, 2021 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python 3 Upgrade #160

Python 3 Upgrade #160

fanpu commented Dec 27, 2019 •

edited

Loading

fanpu commented Feb 29, 2020 •

edited

Loading

victorhuangwq left a comment

victorhuangwq Jul 4, 2020

fanpu Jul 6, 2020

victorhuangwq Jul 8, 2020

fanpu commented Jul 6, 2020

victorhuangwq left a comment

victorhuangwq Jul 8, 2020

victorhuangwq Jul 8, 2020

victorhuangwq Jul 8, 2020

cg2v commented Aug 15, 2020 •

edited

Loading

cg2v Aug 15, 2020

cg2v Aug 15, 2020

fanpu commented Aug 15, 2020

victorhuangwq left a comment

victorhuangwq commented Aug 19, 2020 •

edited

Loading

fanpu commented Aug 19, 2020

Python 3 Upgrade #160

Python 3 Upgrade #160

Conversation

fanpu commented Dec 27, 2019 • edited Loading

Plan:

fanpu commented Feb 29, 2020 • edited Loading

victorhuangwq 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

fanpu commented Jul 6, 2020

victorhuangwq 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

cg2v commented Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanpu commented Aug 15, 2020

victorhuangwq left a comment

Choose a reason for hiding this comment

victorhuangwq commented Aug 19, 2020 • edited Loading

fanpu commented Aug 19, 2020

fanpu commented Dec 27, 2019 •

edited

Loading

fanpu commented Feb 29, 2020 •

edited

Loading

cg2v commented Aug 15, 2020 •

edited

Loading

victorhuangwq commented Aug 19, 2020 •

edited

Loading