MPAS-Dev / MPAS

Repository for private MPAS development prior to the MPAS v6.0 release.
Other
4 stars 0 forks source link

Add timing capability for test cases #1480

Closed xylar closed 6 years ago

xylar commented 6 years ago

This merge adds timing capability for COMPASS test cases.

Support for a --verbose flag has been added to the setup phase of the manage_regression_suite.py script. If this flag is specified, the output from setting up or cleaning up individual test cases is stored in a log file in the output directory (manage_regression_suite.py.out). On successful completion of each of the setup stage, the contents of the log file are also displayed via stdout.

xylar commented 6 years ago

At @matthewhoffman's request and to simplify debugging of problems found in #1479, I am breaking this commit out into its own PR. I have also decided not to complicate things by adding the --verbose flag to the clean-up stage of the regression test suite. It didn't prove to be useful in my testing.

@pwolfram, please use this branch to debug the issue found by @matthewhoffman in #1479. Feel free to push/force push to my branch.

We will need to decide how to update ocean/develop depending on what changes are made here.

mark-petersen commented 6 years ago

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

xylar commented 6 years ago

@mark-petersen, I was assuming neither @mgduda nor @akturner would care to comment because their cores don't use COMPASS.

mark-petersen commented 6 years ago

Correct. Since Michael confirmed that by phone today, I just wanted to write it to be clear. No need to check on that for future COMPASS PRs.

matthewhoffman commented 6 years ago

I skimmed the changes and don't see anything that concerns me apart from the issues that @xylar already raised.

pwolfram commented 6 years ago

@matthewhoffman, I'd like to get this fixed up so we can merge. Regarding the error you found:

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

can you please provide some more detail on how to reproduce it so that I can fix the bug? Is this for a test that isn't currently merged or is on one of your branches? Any help reproducing this error would be really helpful. Thanks!

xylar commented 6 years ago

@pwolfram, I would guess that the starting point would be to check out landice/develop, do a test merge with this branch, and run the standard regression test suite:

landice/develop/testing_and_setup/compass/landice/regression_suites/standard_integration_test_suite.xml

@matthewhoffman, correct me if that's not correct.

pwolfram commented 6 years ago

@xylar, thanks! I made the classic mistake of not pulling from MPAS-Dev and was grabbing your land ice develop branch instead of MPAS-Dev. I'll work on this tomorrow, especially since I haven't used compass for land ice before and may have some questions for @matthewhoffman.

pwolfram commented 6 years ago

@matthewhoffman and @xylar, I believe this is fixed now. A quick verification on either of your part would be helpful. This was a fun chance to get to use the land ice core!

The bug problem was that grep was grabbing multiple lines but I really just needed the last line in the file, hence the extra split that was added as annotated in the code.

xylar commented 6 years ago

Sounds good to me. I'm glad the fix wasn't too hard. It might have been more robust to read in the file with python tools and split the last line rather than making an external call to grep but as long as it works robustly at this point, I'm fine with this fix.

xylar commented 6 years ago

@matthewhoffman, if you could re-run your regression test that caused the earlier problem and verify this fix, that would be great. Once you've approved the PR, I'll merge.

pwolfram commented 6 years ago

@xylar, I agree that it would be more robust to do file IO read operations. But, given the need and application for this I did not think it was justified to go further than pythonizing a bash one-liner for expediency. It would have produced a lot more lines of code for less value-added. If we need to revisit it in the future I'm happy to do so.

xylar commented 6 years ago

@pwolfram, differences in coding philosophy, I think. What you wrote is definitely in line with typical python coding style. I often find that several lines are preferable to one (e.g. compactness shouldn't sacrifice clarity). I'm not personally fond of performing a series of operations on a single line, but maybe I'm just old-fashioned that way.

xylar commented 6 years ago

Okay, I'm going to merge. @pwolfram and @mark-petersen have already approved this PR in previous incarnations.

xylar commented 6 years ago

Thanks @matthewhoffman for reviewing and @pwolfram for the debugging work.