2to3 1st attempt on independent commands #2286

wxtim · 2019-01-30T11:59:46Z

Work in progress for Rose Python3 upgrade.

Test	Status	Notes
docs	✔️
Rosie Apps (Deliberatly not fixed: waiting on @sadielbartholomew)
rosa-db-create	✖️	PR #2288 - @sadielbartholomew
rosa-svn-post-commit	✖️	PR #2288 - @sadielbartholomew
rosa-svn-pre-commit	✖️	#2304
rose-ana	✖️	#2304
rosie-checkout	✔️
rosie-create	✔️
rosie-delete	✔️
rosie-disco	✖️	PR #2288 - @sadielbartholomew
rosie-graph	✖️	PR #2288 - @sadielbartholomew
rosie-hello	✔️
rosie-id	✔️
rosie-lookup	✖️	#2304
rosie-ls	✖️	partly ✔️, but the tests are reliant on rosie-disco
rosie.db	✖️	PR #2288 - @sadielbartholomew
Rose Standalone Apps
rose-app-run	✔️
rose-app-upgrade	✔️
rose-cli-bash-completion	✔️
rose-config	✔️
rose-config-diff	✔️
rose-config-dump	✔️
rose-config-edit	✔️
rose-date	✖️	issue #2303
rose-env-cat	✔️
rose-help	✔️
rose-host-select	✔️
rose-macro	✔️
rose-metadata-check	✔️
rose-metadata-graph	❓	#2301
rose-metadata-gen	✔️
rose-mpi-launch	✔️
rose-namelist-dump	✔️
rose-stem	✔️
rose.c3	✔️
rose.config_tree	✔️
rose.env	✔️
rose.popen	✔️
rose.variable	✔️
Rose GUI Apps (removed from codebase - to be re-implemented later)
rose-bush	removed 🗑️
Rose-Suite & Task Apps (waiting on rose-suite-run to be sorted)
rose-task-env	✖️	#2304
rose-task-run	✖️	#2304
rose-suite-clean	✖️	#2304
rose-suite-cmp-vc	✔️	#2304
rose-suite-log	✔️	#2304
rose-suite-restart	✖️	#2304
rose-suite-run	✖️	#2304
rose-suite-shutdown	✔️	#2304

❄️ Flakey Tests

rosa-svn-pre-commit/03-meta.t "test 02 password modify bad owner" fails when run in a sub shell.
Many tests that look at output fail when dictionary order changes and are therefore flaky. rose-app-run/05-file.t:line82 contains scripts for dealing with this, but perhaps there should be
** Consideration of Re-factoring this out into an addition to the test header.
** Consideration of where else output should be sorted to prevent changes in the code breaking tests incorrectly.

Tests Needed? 🔬

The following cases of non Python3 compatible code were found using grep, rather than the Tests:

cmp()

rose.git/lib/python>  grep -rin "cmp(" * --color

rose/apps/rose_arch.py:85: lambda s1, s2: cmp(s1.name, s2.name)):
rose/config.py:1614: return cmp(setting_1, setting_2)
rose/suite_clean.py:155: ret = cmp(item1, item2)

Unittests Needed

I have identified places where I have used cmp_to_key as a quick, hackky solution. These should probably be unit-tested in pyhton2 then upgraded again.

Automated testing issues

Codacy and codecov do not pass. 😢
The codacy failures are as follows:

`sphinx/ext/auto_cli_doc.py:426`:

stdout = check_output(cmd)
# was changed to:
stdout = check_output(cmd).decode()

This failure exists in master, and is only highlighted because it's been changed slightly. I could make it go away thus:

stdout = check_output(cmd)
stdout = stdout.decode

but feel this to be silly.

`lib/python/isodatetime/tests/test_time_point.py:101&120`:

Codacy says
"Standard pseudo-random generators are not suitable for security/cryptographic purposes."
which is good, becuase they aren't.

Python 3.6 to 3.7 conversions.

Most of these were to remove 3.8 deprecation warnings:

`collections`

-import collections
+import collections.abc
...
-if isinstance(x, collections.Callable):
+if isinstance(x.collections.abc.Callable):

`imp`

-import imp
+from importlib.machinery import SourceFileLoader
...
-modules.append(imp.load_source(as_name, meta_file))
+modules.append(SourceFileLoader(as_name, meta_file).load_module())

`inspect.getargspec`

-inspect.getargspec(...
+inspect.getfullargspec(...

bin/rose-check-software

wxtim · 2019-02-06T10:02:28Z

Possible refactoring

There are a number of places where the code from https://gist.github.com/wxtim/cc3f4e653d37616ff24714bfbc32444b might be useful/faster. This checks whether and object to write and object to be written are both expecting bytes of string types, converting the object to write to the appropriate type.

lib/python/rose/config.py

sbin/rosa-svn-pre-commit

oliver-sanders

Haven't had the chance for an in-depth pass but these changes look sensible. Now just a matter of fixing tests one at a time 🤕 !

lib/python/rose/checksum.py

lib/python/rose/macros/__init__.py

lib/python/rose/stem.py

lib/python/rosie/svn_post_commit.py

sadielbartholomew

Great work so far! Overall this looks sound. For now I'll be diving in with some comments as I spot them whilst cross-referencing this for my own porting work.

lib/bash/rose_init

bin/rose-make-docs

kinow

Cheers @wxtim

rose is a project that I am not really familiar with (both code and features). Nor have a development environment with a working version of rose to test it. So my review is a bit superficial here, sorry.
Several files appear marked as "deleted" in GitHub. Is it an issue with GitHub UI, and these items were actually moved elsewhere? Or, for Python 3, we actually have to remove these files?
Touching tests while refactoring can be dangerous. Some tests appear as deleted too, while in others it looks like we are now skipping the tests. Wouldn't it be better to avoid touching those tests, and instead use them as the target? And then work to fix the tests and consider work done once everything works. Just in case we are running the risk of accidentally changing the test in such way that it will give the green flag for the new code, even though it was supposed to fail.
Ignored tests as I don't really know much about the test harness used
Ignored isodatetime as I believe these files were copied from the latest release in PYPI
Ignored files where the change was just the shebang

Great work! Hope that helps!
Bruno

bin/rose-check-software

sbin/rosa-svn-pre-commit

lib/python/rose/macros/value.py

lib/python/rose/config_processors/fileinstall.py

wxtim · 2019-02-20T10:53:58Z

Thanks Bruno - useful to have your thoughts on it.

Questionable `list(???)` statments

2to3 was rather enthusiastic with actaulizing iterators (adding list(iterator)) so I excluded the converters that do this. As a result, where list(some_iterator) does occur, I have added it manually after seeing the RuntimeError: dictionary changed size during iteration in the traceback. Mostly this is caused by a .pop() inside the iterator:

>>> _dict_= {'a': 1, 'b': 2}
>>> for item in _dict_.keys():
...  _dict_.pop(item)
... 
1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: dictionary changed size during iteration

Although there were other cases where something inside the iterator caused this issue.

Still useful to have your views...

* rose is a project that I am not really familiar with (both code and features). Nor have a development environment with a working version of rose to test it. So my review is a bit superficial here, sorry.

Deleted files

* Several files appear marked as "deleted" in GitHub. Is it an issue with GitHub UI, and these items were actually moved elsewhere? Or, for Python 3, we actually have to remove these files?

Lots of files were deleted. All rose programmes with a GUI should have gone. One of my concerns for reviewers is lingering references to these programmes.

Danger - Test Changes

* Touching tests while refactoring can be dangerous. Some tests appear as deleted too, while in others it looks like we are now skipping the tests. Wouldn't it be better to avoid touching those tests, and instead use them as the target? And then work to fix the tests and consider work done once everything works. Just in case we are running the risk of accidentally changing the test in such way that it will give the green flag for the new code, even though it was supposed to fail.

Changed tests

Yes, it is. I don't like it. Where I have changed tests this has usually been becuase they are flakey as a result of different versions of Python handling dictionary ordering differently. This can, and possibly should (although the fact that dicts are now ordered means it is probably not a priority) be addressed by greater use of sorting tests, having first agreed that the order of the test output is not important. If it's come from a dict in Python 2.x it better not have been. 😄

Removed tests

Tests have been removed where app to be tested is gone.

Skipped tests

Occur where app will remain in framework, but is not yet passing and is known to be beyond the scope of the ticket, either because it or its test have dependency on rose suite-run.

kinow · 2019-02-20T20:14:51Z

@wxtim I think 2to3 cannot guess whether we will modify the iterator or not, so it just plays safe and puts list(iterator) everywhere. Which should work, but then later we may want to revisit this and remove it for performance (mainly memory). So fixing a few of these if possible is fine IMO.

Happy with all your comments. I think there is one or two comments pending to be resolved. But both minor. Happy to approve once these two are resolved 👍

And once again, really detailed and nice work! Although it is a pull request with lots of changes, your work is well organised, so it helps reviewers. Cheers

lib/python/rose/config_processors/fileinstall.py

lib/python/rose/job_runner.py

lib/python/rose/loc_handlers/namelist.py

lib/python/rose/loc_handlers/rsync.py

oliver-sanders

OK, I think this is my final pass. I think we are pretty much there with this one:

A few bytes vs strings comments which require clarification
A potential import issue
One potentially suspect test which requires investigation

t/rose-app-upgrade/08-category-is-package.t

lib/python/rosie/ws_client_auth.py

lib/python/rosie/svn_post_commit.py

lib/python/rose/suite_engine_procs/cylc.py

lib/python/rose/popen.py

t/rose-app-upgrade/05-cwd.t

wxtim · 2019-03-11T16:17:10Z

@oliver-sanders wrote RE: t/rose-app-upgrade/05-cwd.t

I've now been through the TB changes, I think this is the only suspect one.
Has the order of events actually changed or is it just the order the log messages are printed in that's changed (if so can we change it back)?

I've reverted this change having hopefully dealt with the cause of the change in order by using a sys.std.flush() statement at lib/python/rose/upgrade.py:781

fix broken hyperlink squashlater: documetation squashlater: address OS concern RE: test output order

matthewrmshin

I have given this PR a good 👀 with 👓 . I have added some comments. Hopefully should not take too long to fix. Thanks for the big effort!

bin/rose-check-software

bin/rose-make-docs

etc/rose-docs-env.yaml

lib/python/rose/reporter.py

lib/python/rose/run.py

t/rose-app-upgrade/08-category-is-package.t

t/rose-metadata-check/lib/custom_widget.py

wxtim · 2019-03-13T15:40:27Z

I have responded to all @matthewrmshin 's comments, either by fixing things or creating new tickets. 🎉

matthewrmshin

Fantastic first attempt. Thank you very much.

matthewrmshin · 2019-03-13T15:47:10Z

I am done. @oliver-sanders @sadielbartholomew please review 2.

oliver-sanders

Happy now t/rose-app-upgrade/05-cwd.t is sorted. Let's move onto follow up work.

matthewrmshin · 2019-03-14T11:32:33Z

Doubly approved. Merge!

MetRonnie · 2021-01-22T16:17:34Z

lib/python/rose/reporter.py

-            return self.handle.write(message.encode("utf-8"))
-        except UnicodeDecodeError:
+            return self.handle.buffer.write(message.encode("utf-8"))
+        except TypeError:
            return self.handle.write(message)
+        except AttributeError:
+            return self.handle.write(message.encode('UTF-8'))


This could be why the stderr is coming at the end of all stdout in rosa db-create @dpmatthews

Jamming in a self.handle.flush() after the write might to the trick.

wxtim commented Jan 30, 2019

View reviewed changes

bin/rose-check-software Show resolved Hide resolved

matthewrmshin added this to the next-feature milestone Jan 30, 2019

matthewrmshin changed the title ~~Python3 fork tidy copy~~ 2to3 1st attempt on independent commands Jan 30, 2019

wxtim requested review from matthewrmshin and oliver-sanders February 1, 2019 10:08

wxtim commented Feb 6, 2019

View reviewed changes

lib/python/rose/config.py Show resolved Hide resolved

oliver-sanders assigned wxtim Feb 7, 2019

wxtim commented Feb 7, 2019

View reviewed changes

sbin/rosa-svn-pre-commit Show resolved Hide resolved

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from 6ac9dc0 to 3b981e1 Compare February 8, 2019 11:25

oliver-sanders reviewed Feb 8, 2019

View reviewed changes

lib/python/rose/checksum.py Outdated Show resolved Hide resolved

lib/python/rose/macros/__init__.py Show resolved Hide resolved

lib/python/rose/stem.py Outdated Show resolved Hide resolved

lib/python/rosie/svn_post_commit.py Outdated Show resolved Hide resolved

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from ec34c95 to 578d543 Compare February 11, 2019 10:14

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from 98ec1d0 to 6869a8c Compare February 18, 2019 15:28

wxtim requested review from sadielbartholomew, hjoliver and kinow February 18, 2019 16:30

sadielbartholomew reviewed Feb 19, 2019

View reviewed changes

lib/bash/rose_init Outdated Show resolved Hide resolved

bin/rose-make-docs Show resolved Hide resolved

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from a933e88 to a82ee82 Compare February 19, 2019 09:44

kinow reviewed Feb 20, 2019

View reviewed changes

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from 4c4dc91 to 7e4b0d2 Compare February 20, 2019 15:52

wxtim force-pushed the PYTHON3FORK_tidy_copy branch 2 times, most recently from 1847c34 to 955e699 Compare February 21, 2019 08:33

oliver-sanders reviewed Feb 21, 2019

View reviewed changes

lib/python/rose/config_processors/fileinstall.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Feb 21, 2019

View reviewed changes

lib/python/rose/job_runner.py Show resolved Hide resolved

oliver-sanders reviewed Feb 21, 2019

View reviewed changes

lib/python/rose/loc_handlers/namelist.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Feb 21, 2019

View reviewed changes

lib/python/rose/loc_handlers/rsync.py Show resolved Hide resolved

oliver-sanders reviewed Mar 7, 2019

View reviewed changes

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from ae4082d to a9fc4e0 Compare March 8, 2019 08:54

Response to review

701be1e

fix broken hyperlink squashlater: documetation squashlater: address OS concern RE: test output order

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from f83364b to 701be1e Compare March 12, 2019 10:02

wxtim added 4 commits March 12, 2019 10:32

Modified travis.yml

9471bd8

add pyzmq to travis.yml

6e4c108

colorama in travis.yml

eb830b3

remove pyssl in travis.yml

33b84d3

matthewrmshin reviewed Mar 12, 2019

View reviewed changes

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from d8523a6 to 3194371 Compare March 13, 2019 12:46

Review Changes: Based on Matts comments

6e1b731

wxtim force-pushed the PYTHON3FORK_tidy_copy branch from e81a039 to 6e1b731 Compare March 13, 2019 12:49

oliver-sanders mentioned this pull request Mar 13, 2019

2to3 tutorial #2310

Merged

matthewrmshin approved these changes Mar 13, 2019

View reviewed changes

oliver-sanders approved these changes Mar 14, 2019

View reviewed changes

matthewrmshin merged commit b8e6433 into metomi:master Mar 14, 2019

sadielbartholomew added a commit to sadielbartholomew/rose that referenced this pull request Mar 14, 2019

Tidy up reversion of collections.abc.Callable from metomi#2286 merge

da10d1c

sadielbartholomew added a commit to sadielbartholomew/rose that referenced this pull request Mar 14, 2019

Tidy up reversion of collections.abc.Callable from metomi#2286 merge

103463e

sadielbartholomew mentioned this pull request Mar 14, 2019

rosie & rosa: migrate to Python 3(.6-.7) & Tornado #2288

Merged

10 tasks

matthewrmshin modified the milestones: next-feature, next-feature-alpha Mar 14, 2019

matthewrmshin mentioned this pull request Mar 14, 2019

rose.config: python 3 compatibility #2221

Closed

sadielbartholomew added a commit to sadielbartholomew/rose that referenced this pull request Apr 9, 2019

Tidy up reversion of collections.abc.Callable from metomi#2286 merge

54b4382

MetRonnie reviewed Jan 22, 2021

View reviewed changes

wxtim deleted the PYTHON3FORK_tidy_copy branch July 25, 2023 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2to3 1st attempt on independent commands #2286

2to3 1st attempt on independent commands #2286

wxtim commented Jan 30, 2019 •

edited

Loading

wxtim commented Feb 6, 2019

oliver-sanders left a comment

sadielbartholomew left a comment

kinow left a comment

wxtim commented Feb 20, 2019 •

edited

Loading

kinow commented Feb 20, 2019

oliver-sanders left a comment

wxtim commented Mar 11, 2019

matthewrmshin left a comment

wxtim commented Mar 13, 2019

matthewrmshin left a comment

matthewrmshin commented Mar 13, 2019

oliver-sanders left a comment

matthewrmshin commented Mar 14, 2019

MetRonnie Jan 22, 2021

oliver-sanders Jan 22, 2021 •

edited

Loading

2to3 1st attempt on independent commands #2286

2to3 1st attempt on independent commands #2286

Conversation

wxtim commented Jan 30, 2019 • edited Loading

❄️ Flakey Tests

Tests Needed? 🔬

cmp()

Unittests Needed

Automated testing issues

sphinx/ext/auto_cli_doc.py:426:

lib/python/isodatetime/tests/test_time_point.py:101&120:

Python 3.6 to 3.7 conversions.

collections

imp

inspect.getargspec

wxtim commented Feb 6, 2019

Possible refactoring

oliver-sanders left a comment

Choose a reason for hiding this comment

sadielbartholomew left a comment

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

wxtim commented Feb 20, 2019 • edited Loading

Questionable list(???) statments

Still useful to have your views...

Deleted files

Danger - Test Changes

Changed tests

Removed tests

Skipped tests

kinow commented Feb 20, 2019

oliver-sanders left a comment

Choose a reason for hiding this comment

wxtim commented Mar 11, 2019

matthewrmshin left a comment

Choose a reason for hiding this comment

wxtim commented Mar 13, 2019

matthewrmshin left a comment

Choose a reason for hiding this comment

matthewrmshin commented Mar 13, 2019

oliver-sanders left a comment

Choose a reason for hiding this comment

matthewrmshin commented Mar 14, 2019

MetRonnie Jan 22, 2021

Choose a reason for hiding this comment

oliver-sanders Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

wxtim commented Jan 30, 2019 •

edited

Loading

`sphinx/ext/auto_cli_doc.py:426`:

`lib/python/isodatetime/tests/test_time_point.py:101&120`:

`collections`

`imp`

`inspect.getargspec`

wxtim commented Feb 20, 2019 •

edited

Loading

Questionable `list(???)` statments

oliver-sanders Jan 22, 2021 •

edited

Loading