Modify COMPASS scripts to be PEP8 compliant

[xylar]

The four main scripts that drive the COMPASS testing infrastructure were written with tabs, long lines and many other coding conventions that are discouraged in python. This merge ensures that the scripts are compliant with the PEP8 specification, as determined by the automatic code analysis within the spyder editor.

The following mode line (for vim) has been added to the bottom of each file:

# vim: foldmethod=marker ai ts=4 sts=4 et sw=4 ft=python

All empty except: clauses have been replaced with explicit exception names. Empty except clauses are considered a bad practice in python because (among other issues) even syntax errors in the code itself will be trapped by the except (a potential debugging nightmare).

With one exception, the scripts are expected to function identically before and after this merge. Only formatting changes for PEP8 compliance have been performed.

The exception is that support for the --verbose flag has been added to the cleanup phase of the manage_regression_suite.py script for consistency with the setup phase.

Copying of the environment in subprocess calls has been removed, as this is the default behavior.

Auto-generated scripts have been made PEP8 compliant to the extent possible. (The main PEP8 violations are that lines are often too long because of long strings that can not easily be broken into substrings.)

[xylar]

This PR is intended to address #1472

[pwolfram]

@xylar, I'm wondering if we shouldn't be clear that the script should be identical in function and that the script is just a formatting change to be PEP8 compliant. This is implied but maybe we should make it explicit.

[xylar]

Testing

I tested all 4 scripts on my laptop (Ubuntu 16.04):

./list_testcases.py 

I get an identical list of 74 test cases with ocean/develop and this branch.

./list_testcases.py -c global_ocean
./list_testcases.py -c global_ocean -r QU240

The expected list of a subset of test cases is produced, as expected.

./setup_testcase.py -n 64 -m runtime_definitions/mpirun.xml -f config.ocean --work-dir ~/data/mpas/test_compass_pep8

I get the QU240/performance test case in the expected location. The test case runs successfully.

 2013  ./clean_testcase.py -n 64 --work_dir ~/data/mpas/test_compass_pep8

The test case is deleted, as expected

./setup_testcase.py -o ocean -c global_ocean -r QU240 -t default -m runtime_definitions/mpirun.xml -f config.ocean --work_dir ~/data/mpas/test_compass_pep8

I get the QU240/default test case in the expected location. The test case runs successfully.

./manage_regression_suite.py -t ocean/regression_suites/nightly.xml -f config.ocean -s -m runtime_definitions/mpirun.xml --work_dir ~/data/mpas/test_compass_pep8

I get all test in the nightly regression test suite, as expected, as well as the driver script. The driver script runs successfully and all tests pass.

runtime_definitions/mpirun.xml --work_dir ~/data/mpas/test_compass_pep8_compare -b ~/data/mpas/test_compass_pep8 

A comparison script is created and (after fixing a small error) all tests in the regression suite pass, including comparison with the previous run.

./manage_regression_suite.py -t ocean/regression_suites/nightly.xml -c -f config.ocean --work_dir ~/data/mpas/test_compass_pep8

All test cases in the test suite and the driver script are removed, as expected.

[xylar]

Note: The python files produced by the COMPASS scripts are far from being PEP8 compliant. We might want to address at least some of the more egregious violations (e.g. bad indentation; really, really long lines) in a future PR.

[xylar]

@pwolfram, this is ready to review. @mark-petersen, for safety's sake, could you also run a couple of tests on this (your standard workflow with COMPASS plus some edge cases if you can think of some)? I'd hate to break the test suite...

[mark-petersen]

Yes, I will test. @pwolfram could you rebase this PR on the head of ocean/develop after you merge #1470? There may be conflicts. I would like to test after that.

[xylar]

@pwolfram, pep8 has been renamed:

pep8 has been renamed to pycodestyle (GitHub issue #466)
Use of the pep8 tool will be removed in a future release.
Please install and use `pycodestyle` instead.

$ pip install pycodestyle
$ pycodestyle ...

When I run pycodestyle on the 4 scripts, I get no output (which I think means they pass). This is probably no surprise because I think this is used internally by spyder.

[xylar]

@mark-petersen, I will do the rebase after @pwolfram has merged. That's easier than making @pwolfram do it :-)

[mark-petersen]

Agreed. Thanks, that makes more sense.

[xylar]

@mark-petersen and @pwolfram, I went ahead and rebased on to @pwolfram's branch for #1470. I wasn't able to merge @pwolfram's changes because the formatting of the file has changed so much, so I had to manually re-add the changes.

@pwolfram, could you make sure they look correct?

One small thing, I added support for the --verbose flag in the clean phase as well as the setup phase. I hope no one minds.

[xylar]

Here's the output of the regression test on my laptop after the rebase:

 ** Running case Global Ocean 240km - Performance Test
      PASS
 ** Running case Global Ocean 240km - Restart Test
      PASS
 ** Running case Global Ocean 240km - SE Blocks Test
      PASS
 ** Running case Global Ocean 240km - RK4 Blocks Test
      PASS
 ** Running case Global Ocean 240km - Analysis Test
      PASS
 ** Running case ZISO 20km - Smoke Test
      PASS
 ** Running case ZISO 20km - Smoke Test with frazil
      PASS
 ** Running case Baroclinic Channel 10km - Thread Test
      PASS
 ** Running case Baroclinic Channel 10km - Decomp Test
      PASS
 ** Running case Baroclinic Channel 10km - Restart Test
      PASS
 ** Running case Global Ocean 240km - Smoke Test with land ice
      PASS
 ** Running case sub-ice-shelf 2D - restart test
      PASS
 ** Running case Periodic Planar 20km - LIGHT particle region reset test
      PASS
 ** Running case Periodic Planar 20km - LIGHT particle time reset test
      PASS
TEST RUNTIMES:
00:06 Baroclinic_Channel_10km_-_Decomp_Test
00:05 Baroclinic_Channel_10km_-_Restart_Test
00:06 Baroclinic_Channel_10km_-_Thread_Test
00:58 Global_Ocean_240km_-_Analysis_Test
00:51 Global_Ocean_240km_-_Performance_Test
00:58 Global_Ocean_240km_-_RK4_Blocks_Test
00:49 Global_Ocean_240km_-_Restart_Test
00:46 Global_Ocean_240km_-_SE_Blocks_Test
01:00 Global_Ocean_240km_-_Smoke_Test_with_land_ice
00:28 Periodic_Planar_20km_-_LIGHT_particle_region_reset_test
00:49 Periodic_Planar_20km_-_LIGHT_particle_time_reset_test
00:24 ZISO_20km_-_Smoke_Test
00:10 ZISO_20km_-_Smoke_Test_with_frazil
00:08 sub-ice-shelf_2D_-_restart_test
Total runtime 07:38

As far as I can tell, things are working as expected. I'll run the comparison test now.

[xylar]

I have not been able to complete the comparison test on my laptop because I ran out of disk space. I saw PIO errors but I think they are related to running out of space.

[pwolfram]

@xylar, I did a quick local rebase-- no errors. I will test now.

[pwolfram]

@xylar, I took a look at chances in #1470 and verified that they were included and converted into correct formatting here.

[pwolfram]

@xylar, just to be clear, can you please highlight this change or set of changes so we know which ones you referenced?

One small thing, I added support for the --verbose flag in the clean phase as well as the setup phase. I hope no one minds.

The inclusion in manage_regression_suite.py is great and I'm glad you caught that for me because I initially missed it. I'm happy to take a look at other changes too, but this was the one I primarily noticed. I didn't see others via inspection diffing the code but just wanted to double-check.

[pwolfram]

The print_case change is greatly appreciated. Arguably, there are probably other places that need functions too but I don't think we should go down that path right now.

[pwolfram]

Testing:

Found a minor bug:

┌─[pwolfram][weaklynonlinear][~/Documents/MPAS_development/MPAS/testing_and_setup/compass][14:15][±][compass_pep8_rebased ✗]
└─▪ rm -rf tmp_light; ./manage_regression_suite.py -t ocean/regression_suites/light.xml -s -c --work_dir=tmp_light -f general.config.ocean.pn1704857 -v 
WARNING: No model runtime specified. Using the default of /Users/pwolfram/Documents/MPAS_development/MPAS/testing_and_setup/compass/runtime_definitions/mpirun.xml
Cleaning Test Cases:
Traceback (most recent call last):
  File "./manage_regression_suite.py", line 482, in <module>
    clean_suite(suite_root, args.work_dir, args.verbose)
  File "./manage_regression_suite.py", line 312, in clean_suite
    process_test_clean(child, work_dir, regression_script, verbose)
  File "./manage_regression_suite.py", line 150, in process_test_clean
    stdout = open(work_dir + '/manage_regression_suite.py.out', 'a')
IOError: [Errno 2] No such file or directory: '/Users/pwolfram/Documents/MPAS_development/MPAS/testing_and_setup/compass/tmp_light/manage_regression_suite.py.out'

The solution is easy-- add the following right after parsing the arguments but before # Parse regression_suite file:

# create work directory                     
if not os.path.exists(args.work_dir):
    os.makedirs(args.work_dir)

[pwolfram]

Following that minor bug fix I verified that the nightly test suite worked using ocean/develop to build a baseline and then this branch to compare against that baseline. Once we address the bug (fixed in pwolfram/ocean/compass_pep8_rebased) I'm ok with approving a merge.

[xylar]

Found a minor bug

@pwolfram, nice catch with this bug. I don't think that's the desired behavior in the case you discovered

if not os.path.exists(args.work_dir): os.makedirs(args.work_dir)

I don't think that's the desired behavior when you're cleaning a directory and it doesn't yet exist. Instead, I am making the clean script return immediately without doing anything if the directory doesn't exist (the log would be empty in any case).

[xylar]

Okay, I believe I have addressed @pwolfram's comments.

@mark-petersen, if you could re-test with the new version now that I have rebased onto ocean/develop, that would be very helpful.

[xylar]

I tested both setting up and cleaning test cases using manage_regression_suite.py. I also confirmed that the bug @pwolfram pointed out no longer happens when trying to clean a nonexistent directory.

I also can confirm that all tests in the nightly regression suite pass, including a comparison with another run of the suite. I haven't yet confirmed that results are bit-for-bit identical to ocean/develop.

[xylar]

@pwolfram and @mark-petersen, I apologize that this has been something of a moving target but I decided to do my best to address #1475 as well as #1472.

The latest commit is my best effort to make the output scripts PEP8 compliant. Breaking long strings into multiple lines is simply too challenging to be worth the trouble in my view. I have broken long lines into multiple lines where appropriate without breaking up long strings. I have also not worried about the fact that there are extra import commands in many of the autogenerated scripts because it is simpler to overdo it than to figure out which scripts need which imports (given that this varies from test case to test case).

[xylar]

Re-Testing

I retested with my latest changes by comparing the nightly test suite against ocean/develop (once again). All tests passed and the timing output looks similar to previous runs.

[pwolfram]

@xylar, I'm taking one more quick look and will get back to you shortly...

[xylar]

Thanks @mark-petersen. Still waiting for the thumbs up from @pwolfram. Then, I'll merge.

[pwolfram]

@xylar, thanks for making this much cleaner... this is great! Please merge when ready.

[xylar]

@mark-petersen and @pwolfram, I just realized in the process of updating my JIRA tasks that this should have been a framework PR, not an ocean PR. I think we probably need to do a PR to revert it in ocean/develop and another to merge it into develop instead. Please let me know if you agree.

[mark-petersen]

@xylar thanks for catching that. I should have seen it too. It's even more complicated, because #1470 changed framework files too.

The good news is that we are the only ones using COMPASS right now, so I think the others don't care. Rather than revert these two, could you make a PR into develop for the current state of these files here? Then on the next develop -> ocean/develop merge these files will just remain the same - and that might take a few weeks. We just won't tell Doug :).

[xylar]

@mark-petersen, yes, I can do that in the next couple of days as I have time. I agree, that's not a crazy solution. It would prove a bit confusing if one wanted to find the source of these changes in the future.

[xylar]

1479 is intended to add the script changes to framework

[pwolfram]

I think github's cross-referencing compensates for this type of issue but do agree that conceptually this was not done in optimal order. Thanks for bringing this to my attention for the future @xylar