Python3 Rose suite run

[wxtim]

fix: #2304
Current travis test failure exists in master

[matthewrmshin]

First pass through my 👀 👓. Mostly OK. Some test files were symbolic links so one file could do several tests. You might have changed them to regular files, which is fine, but you may want to prune some of the logic for handling the multiple tests.

[sadielbartholomew]

I am still testing the functionality.

A few minor comments, including stylistic ones that do not have to be addressed in this PR (they will not block my approval) but I will register now, possibly to address later, since I have noticed them. An umbrella comment, there are (as you seem to be aware) a lot of instances of:

if isinstance(<VAR>, bytes):
    <VAR> = <VAR>.decode()
if isinstance(<VAR>, bytes):
    <VAR> = <VAR>.decode()

As per your comment @TODO fast fix in the Python3 conversion. Working > perfect) they are not ideal, but in fact I think if we have these cases whereby we need to test if a variable is bytes or not, we have not been careful enough in the encoding, & when we eventually get round to cleaning things up, we could easily end up changing most or all of the de- & en-coding calls. So if possible can we try to handle the change in processing of strings & bytes with no need to check types.

[wxtim]

This is failing on the same thing as master. :(

[oliver-sanders]

Note:

In #2310 the tutorial suite tests (t/docs/02-tutorial-suites.t) are re-instated but the rose suite-run tests are skipped.

[matthewrmshin]

Why are all your earlier changesets in pairs?

[wxtim]

Why are all your earlier changesets in pairs?

I have no idea. Weird, isn't it!

[oliver-sanders]

Suggested change

	.. _documentation: https://docs.python.org/3.7/library/itertools.html
	.. _itertools documentation: https://docs.python.org/3/library/itertools.html

[oliver-sanders]

Documentation test failure may be related to the latest Sphinx release. I would suggest ignoring this for now as the only docs change is fine.

[sadielbartholomew]

Thanks for addressing that final point. The only Travis CI failure is also failing on master (with feasible origin given as above), so that can be considered as a pass for the code changes here.

Just running my local test battery on the final state & I expect that will pass, then we can merge. Great work!

[sadielbartholomew]

In general, passes a "sanity check" but (sorry) there are a few more issues emerging from some functionality testing. They shouldn't take long to fix, however:

1. Consistently for rose suite-run commands some lines towards the end of the STDOUT appear to have not been decoded properly, so for example the 'CYLC' ASCII letters get mangled:

   [INFO] delete: suite.rc
   [INFO] install: suite.rc
   [INFO] b'REGISTERED subsuite-app-domingo -> /home/h06/sbarth/cylc-run/subsuite-app-domingo\n'
   [INFO] chdir: log/
   [INFO] b"            ._.                                                       \n            | |                 The Cylc Suite Engine [8.0a0]         \n._____._. ._| |_____.           Copyright (C) 2008-2019 NIWA          \n| .___| | | | | .___|   & British Crown (Met Office) & Contributors.  \n| !___| !_! | | !___.  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _\n!_____!___. |_!_____!  This program comes with ABSOLUTELY NO WARRANTY.\n      .___! |              It is free software, you are welcome to    \n      !_____!             redistribute it under certain conditions;   \n\n*** listening on tcp://vld456.cmpd1.metoffice.gov.uk:43016/ ***\n\nTo view suite server program contact information:\n $ cylc get-suite-contact subsuite-app-domingo\n\nOther ways to see if the suite is still running:\n $ cylc scan -n 'subsuite-app-domingo' vld456.cmpd1.metoffice.gov.uk\n $ cylc ping -v --host=vld456.cmpd1.metoffice.gov.uk subsuite-app-domingo\n $ ps -opid,args 92618  # on vld456.cmpd1.metoffice.gov.uk\n\n"
   

2. I get a repeated error from many of the test battery tests for the security auto-message which should just get ignored. Again it looks like decoding is not quite right (or present) somewhere:

    ssh -oBatchMode=yes -oConnectTimeout=10 -n xcel00 env\ ROSE_VERSION=2019.01.0-104-gf0d6583d\ CYLC_VERSION=8.0a0\ bash\ -l\ -c\ \'\"$0\"\ \"$@\"\'\ rose\ suite-run\ -vv\ -n\ rose-test-battery.40kEqx\ --new\ --run=run\ --remote=uuid=f11fb24a-a15c-487e-b23d-55147b393332,now-str=20190603T142808Z # return-code=126, stderr=
   [FAIL] b'  WARNING: This computer is provided for the processing of official\n  information. Unauthorised access may constitute a criminal\n  offence. By continuing to use this facility you agree to abide by\n  the Met Office Your Security Commitments. All activity on this\n  system is liable to monitoring.\n\n/opt/ukmo/supported/bin/rose: line 24: /home/d04/sbarth/rose/bin/rose: No such file or directory\n/opt/ukmo/supported/bin/rose: line 24: exec: /home/d04/sbarth/rose/bin/rose: cannot execute: No such file or directory\n'
   

[oliver-sanders]

it looks like decoding is not quite right (or present) somewhere:

Looks like a subprocess call isn't getting wrapped with decode as desired but that it's output is then getting repred (because it appears as b'...').

[wxtim]

it looks like decoding is not quite right (or present) somewhere:

Looks like a subprocess call isn't getting wrapped with decode as desired but that it's output is then getting repred (because it appears as b'...').

I've put an extra isinstance(message, bytes) into reporter.py. This should act as a catch all for any more of these.

[oliver-sanders]

Have tested rose suite-run, things seem to be working as expected.

I've just found some decode issues:

[edit] tested as fixed:

• Stuff returned rose suite-run when invoked remotely via ssh.
• lib/python/rose/suite_run.py#424 TypeError: a bytes-like object is required, not 'str'

[wxtim]

I think that I've not done the python3 conversion as well as it could have been done, and that there should be a decode in rose.popen in method run for stderr and stdout. This will be safer in the long run because it will decode everything at source, but will require removal of a bunch of decodes put in elsewhere.

I have created issue #2344 to cover this, but it's not as much of a priority IMO as rose-packaging.

[sadielbartholomew]

[Note: awaiting a final second review from myself before merging as the PR has changed greatly since @matthewrmshin's approval.]

[sadielbartholomew]

Previous feedback addressed.

Passes a sanity check overall, though with the caveat that we are unable to properly evaluate the local test battery remote job tests (not run on Travis CI) due to the dependency on the newly-packaged cylc-flow.

Great! Another big step towards a Python 3 Rose 🌄.

[matthewrmshin]

🍾