Make COMPASS scripts PEP8 compliant

[xylar]

This merge modifies COMPASS scripts to be PEP8 compliant and to produce PEP8 compliant output (at least to the extent feasible).

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).

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.)

To accommodate the PEP8 line-length restriction, a new function print_case was added to list_testcases.py. Without this function, the level of indentation had become impractical.

[xylar]

These changes were merged to ocean/develop in PRs #1470 and #1474 but should be merged to develop instead.

[xylar]

@pwolfram and @mark-petersen, you have reviewed these changes already but a quick test merge to ocean/develop would be good to make sure no conflicts occur. I have done such a test merge myself and found no difference between the resulting COMPASS scripts in the merge branch and in ocean/develop, so I think everything is good.

[xylar]

@matthewhoffman, could you do a test merge with landice/develop and run one or both of your regression test suites? If all goes as planned, it should

• work
• be bit-for-bit with landice/develop
• show timing info for each test case in the suite

[xylar]

It doesn't look like MPAS-SeaIce uses COMPASS, so I've removed @akturner as a reviewer.

[mark-petersen]

@pwolfram I tested the nightly regression suite, so you can skip this one. You can either remove your name or give it a quick look and approve.

[matthewhoffman]

@xylar , I will test this out. I just looked things over and have a few general questions.

1. What was the primary motivation for retrofitting existing code to be PEP8 compliant? Such extensive changes obviously make traversing history pretty difficult, so I'm wondering what the specific benefit is for that. Looking at the PEP8 specification, I see this:

   Some other good reasons to ignore a particular guideline:

   3. Because the code in question predates the introduction of the guideline and there is no other reason to be modifying that code.

2. Because the changes are so extensive, it would be good if the commit messages and/or PR description indicated what types of things were changed for PEP8 compliance. Is is simply changing tabs to spaces everywhere, or are there other details? The diffs are too extensive for that to be clear.

3. Minor detail - I noticed some import lines combined from separate to lists of imports on one line. That appears to make a change from being PEP8 compliant to incompliant.

   Imports should usually be on separate lines

[matthewhoffman]

@xylar, update: I tested this with the landice core and received the following error after all the tests passed successfully:

...
 ** Running case hydro-radial restart test
      PASS
TEST RUNTIMES:
Traceback (most recent call last):
  File "./standard_integration_test_suite.py", line 133, in <module>
    ['grep', 'real', outputfile]).split()[1]))
ValueError: could not convert string to float: of

[xylar]

@matthewhoffman,

What was the primary motivation for retrofitting existing code to be PEP8 compliant? Such extensive changes obviously make traversing history pretty difficult, so I'm wondering what the specific benefit is for that.

This is definitely a legit concern. @pwolfram can perhaps comment on what he found problematic but I think the tabs were likely the primary issue.

For me, the primary motivation is that it's not very easy to fix some PEP8/formatting issues without fixing all of them. My approach was to use spyder to tell me which lines have an issue and I fix them one line at a time.

When I was going through the code, there were some significant issues related to "naked" except statement that I also felt needed to be addressed. spyder highlights these a PEP8 issues but I'm not sure.

Because the changes are so extensive, it would be good if the commit messages and/or PR description indicated what types of things were changed for PEP8 compliance. Is is simply changing tabs to spaces everywhere, or are there other details? The diffs are too extensive for that to be clear.

As I describe to some degree in the PR description, the changes go beyond just changing tabs to spaces. I agree that this isn't as clear as it needs to be, so I will break this into multiple commits. Since the tab issue affects nearly every line, that will need to be fixed first.

Breaking the work into one commit for each type of change made is not a natural way to go through the code making the changes so this is not how I did it in the first place. It is much easier to go line by line with a syntax checker and figure out why it's no PEP8 compliant than to fix a type of issue.

Minor detail - I noticed some import lines combined from separate to lists of imports on one line. That appears to make a change from being PEP8 compliant to incompliant.

Could you point these out? As far as I'm aware, I broke imports into their own lines wherever I found them and I didn't intentionally combine any.

[xylar]

@pwolfram, I believe the issue @matthewhoffman is seeing above is related to your timing code. Could you take a look? Presumably you need the contents of some log file or other but I didn't follow the details of how you did the timing parsing.

[xylar]

@pwolfram, I think it would make sense to break your commit into its own PR. If it had gone through without a hitch in @matthewhoffman's testing, bundling it might have made sense. But given the glitch above, I think we should separate it.

[xylar]

Given @matthewhoffman's concerns, I'm thinking of backing off on the PEP8 and just fixing some of the more significant issues with the scripts (e.g. tabs to spaces, removing naked excepts). @mark-petersen and @pwolfram, your thoughts? If we go down this route (or make any changes at all), we'll likely need to revert #1470 and #1474.

[matthewhoffman]

@xylar , I like the idea of breaking the tabs to spaces in its own commit, and all the other changes in a single (or multiple) commit(s). I agree that it would be more logical to move the timing changes to a separate PR, but I'm ok with it staying together for convenience if that ends up being easier.

[matthewhoffman]

@xylar , ignore my comment about multiple imports on a single line. I must have read the diff backwards. Sorry about that!

[xylar]

@matthewhoffman, I have broken this PR into many commits that should make clearer what has been changed. I have also moved the timing enhancement to its own PR, #1480.

I would say don't worry about testing this PR until we have #1480 merged. But please do take a look at the diffs in the commits as you have time to make sure you're okay with the changes.

[mark-petersen]

Just FYI, @mgduda said by phone today that we can go ahead and merge COMPASS framework changes without his approval.

[matthewhoffman]

@xylar , thanks for breaking all the changes up - it makes it very easy to see what all the changes were. This looks like a huge undertaking (both the original work and the breaking it up after the fact by type of change), so thank you for working through it all. 😸 👍

[xylar]

I'm working on retesting this now that #1480 has been merged.

[xylar]

@matthewhoffman, could you test this branch on a test merge with landice/develop when you have a chance?

[xylar]

@mark-petersen, @matthewhoffman and @pwolfram, I somehow lost track of this PR. Is it ready to merge? You have all approved but it looks like my last message was a request for @matthewhoffman to do one last test.

[mark-petersen]

I tested again. I merged into ocean/develop but copied the files testing_and_setup/compass/*.py straight from here. This command works fine:

./setup_testcase.py -f general.config.ocean_turq --work_dir /lustre/scratch3/turquoise/mpeterse/runs/t55e -n 17

all the way through. This one has a problem:

./manage_regression_suite.py -t ocean/regression_suites/nightly.xml -f general.config.ocean_turq -s  --work_dir /lustre/scratch3/turquoise/mpeterse/runs/t55d

when I run it:

gr0531:t55d$ ./nightly_ocean_test_suite.py
  File "./nightly_ocean_test_suite.py", line 193
    ['grep', 'real', outputfile]).split('\n')[-2]split()[1]))
                                                     ^
SyntaxError: invalid syntax

If it's faster to debug in my office, we can do that.

[xylar]

@mark-petersen, the change that is giving you trouble was introduced by @pwolfram here: https://github.com/MPAS-Dev/MPAS/pull/1480/files#diff-c0a6a0efccad6ab100eb7624719929beR221

So it's not actually part of this PR. It sounds like we probably need to do a bug fix either before or after this PR. If you do the same procedure but for the current develop, my guess is you'll see the same error. So I think we probably need to address that first. @pwolfram, can you please make a PR with a bug fix?

[xylar]

Sorry, what I said above is probably not correct. I probably messed things up when I reformatted that line. I'll look into it. False alarm @pwolfram

[xylar]

@mark-petersen, I believe I just fixed the syntax error. Could you do a quick re-test?

[mark-petersen]

Yes, everything works now. It's good to merge from here.

[mark-petersen]

This will be included in the next batch of small fixes for MPAS-Ocean, most likely on Feb 8. @matthewhoffman you may want to merge develop to landice/develop at the same time, though it's not required because it is not framework code.