avoid a problem with pio_typename in clone with keepexe #1305

jedwards4b · 2017-04-05T20:17:54Z

Print a warning if create_clone is run before case1 is built. Add code to system_tests_compare_two to update pio_typename in case2 after case1 build is complete.

Test suite: scripts_regression_tests
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1304

User interface changes?:

Code review:

billsacks

Looks good, Jim. I have one inline comment suggesting being more explicit in the warning.

It seems like, in theory, we could use the same mechanism in system_tests_compare_two for all clones - i.e., in the particular circumstances where this problem arises (a clone with --keep-exe?), the clone case could query its origin case for the *PIO_TYPENAME* values and update its own values appropriately. But I imagine that's more work than is warranted right now, so I'm fine with your current solution.

billsacks · 2017-04-05T20:57:05Z

scripts/lib/CIME/case.py

@@ -1006,6 +1006,10 @@ def create_clone(self, newcase, keepexe=False, mach_dir=None, project=None, cime
            orig_exeroot = self.get_value("EXEROOT")
            newcase.set_value("EXEROOT", orig_exeroot)
            newcase.set_value("BUILD_COMPLETE","TRUE")
+            orig_bld_complete = self.get_value("BUILD_COMPLETE")
+            if not orig_bld_complete:
+                logger.warn("\nWARNING: Creating a clone before building the original case may cause PIO_TYPENAME to be invalid in the clone")


My understanding from #1304 is that this is only a problem if the clone uses --keep-exe. Is that true? If so, I think the warning should say this explicitly, so that it's more clear when you do and do not have to worry about this warning.

The warning is only printed if you use --keepexe

I've updated the warning to include --keepexe

Ah, sorry I missed that (the conditional was just above github's context - now I see that). Anyhow, I like your new wording - thanks.

gold2718

Two problems.

In the clone, the valid_values is not updated so a user could easily set the PIO_TYPENAME_ to a non-valid value. This should be fixed.
(and this may be a bigger problem) The valid_values is not enforced for PIO_TYPENAME. With a valid_values of only netcdf, ./xmlchange happily let me change the value to pnetcdf. I may open a separate issue for this.

jedwards4b · 2017-04-10T14:50:01Z

@gold2718: I have fixed the issue of checking against valid_values.
On the second issue, if the clone is created after case.build then the clone valid_values are set correctly however if the clone is created prior to building the first case I can't think of any way
to update the valid_values in the clone.

gold2718 · 2017-04-10T21:00:25Z

The best solution would be to set valid_values to be correct during create_newcase since the compiler (and in principle it's limitations) are known then. That is, while having buildlib.pio act as a check, I feel we should have this information in config_compilers.xml.
For instance, for Hobart, there is no PNETCDF_PATH tag. This should equate to pnetcdf not showing up in valid_values. Similarly, it would be good to have a tag that specified NetCDF supported types (e.g., netcdf4p).

jedwards4b · 2017-04-10T21:30:32Z

Whether the user has supplied the proper netcdf and pnetcdf libraries is not a compiler limitation and is determined during the pio build phase. I would ask that you accept this PR as an improvement and open another issue if you feel that this issue has not been fully addressed.

gold2718 · 2017-04-10T21:38:54Z

Maybe it's not strictly a compiler limitation (bad phrasing on my part) but it is a feature of the CIME support for a machine (e.g., NETCDF_PATH and PNETCDF_PATH).
My preference is that if we are not going to fully address this issue (and I would like feedback from others, e.g, @mvertens) that we do not allow ./create_clone --keepexe if the case is not built.
My justification is that --keepexe makes no sense if there is no exe to keep.
@mvertens? @billsacks?

jedwards4b · 2017-04-10T21:41:26Z

I did consider that but it would require rewriting the system_test_compare_two.

billsacks · 2017-04-10T22:46:17Z

Hmmm, tricky. I sympathize with @gold2718 's point here, but @jedwards4b raises a good point that @gold2718 's suggestion:

we do not allow ./create_clone --keepexe if the case is not built

will require a massive overhaul of system_test_compare_two. I could also see this interfering with some people's desired workflow (maybe).

So my conclusion would be: For now, I'd say the current warning is sufficient; if this seems to bite people in practice, then we should revisit it.

mvertens · 2017-04-11T19:57:03Z

@gold2718 @jedwards4b @billsacks

At this point I agree with Bill. I think that an additional statement in create_clone --help stating that for the --keepexe option you MUST have built the reference executable first would be really helpful. Could we please add that new documentation to this PR and then go ahead and merge it.

gold2718 · 2017-04-11T20:06:51Z

Sorry, I keep getting interrupted.
I will create an issue with my current thinking on how to avoid leaving this hole open.

jedwards4b · 2017-04-11T20:40:37Z

Rerunning scripts regression tests now, will merge upon completion.

Allow the case.st_archive script to work with mpaso and mpascice history and restart files. Also should work with mpasli but not tested. From the case directory, executing ./case.st_archive should move all history and restart files to the short term archive for all ACME components. Fixes #1305 S2-131 #close [BFB] * rljacob/cime/fix-mpas-starchive: fix mpas pattern matching so only interim restart files are deleted Add ability to archive MPAS land ice files Add ability to handle mpas files Change regex for mpaso and mpascice files

Short Term Archiving Features This implements features to the short term archiver to enable running it while the model is without obviously breaking things (see #1503 for potential issues with the --last-date option). Other options added include --copy-only, which copies the files to be archived instead of moving them; and --no-incomplete-logs, which ignores logs which are not gzipped, and thus not complete Fixes #1305 Passes scripts_regression_tests BFB * origin/mfdeakin-sandia/in_run_archive: Adds the --force-move option and implies --copy-only when --last-date is specified without --force-move Adds a warning when using the --last-date option and to its help Implement the copy_only option for short term archiving. This copies files rather than moving them Implemented most of the machinery for testing with "incomplete" log files Fix code format issue - replace unused variable with _ Update template.st_archive Adds options to the st_archive to specify the last date (--last-date) to archive, and whether to disable archiving incomplete log files (--no-incomplete-logs)

Allow the case.st_archive script to work with mpaso and mpascice history and restart files. Also should work with mpasli but not tested. From the case directory, executing ./case.st_archive should move all history and restart files to the short term archive for all ACME components. Fixes #1305 S2-131 #close [BFB] * rljacob/cime/fix-mpas-starchive: fix mpas pattern matching so only interim restart files are deleted Add ability to archive MPAS land ice files Add ability to handle mpas files Change regex for mpaso and mpascice files

Short Term Archiving Features This implements features to the short term archiver to enable running it while the model is without obviously breaking things (see #1503 for potential issues with the --last-date option). Other options added include --copy-only, which copies the files to be archived instead of moving them; and --no-incomplete-logs, which ignores logs which are not gzipped, and thus not complete Fixes #1305 Passes scripts_regression_tests BFB * origin/mfdeakin-sandia/in_run_archive: Adds the --force-move option and implies --copy-only when --last-date is specified without --force-move Adds a warning when using the --last-date option and to its help Implement the copy_only option for short term archiving. This copies files rather than moving them Implemented most of the machinery for testing with "incomplete" log files Fix code format issue - replace unused variable with _ Update template.st_archive Adds options to the st_archive to specify the last date (--last-date) to archive, and whether to disable archiving incomplete log files (--no-incomplete-logs)

Allow the case.st_archive script to work with mpaso and mpascice history and restart files. Also should work with mpasli but not tested. From the case directory, executing ./case.st_archive should move all history and restart files to the short term archive for all ACME components. Fixes #1305 S2-131 #close [BFB] * rljacob/cime/fix-mpas-starchive: fix mpas pattern matching so only interim restart files are deleted Add ability to archive MPAS land ice files Add ability to handle mpas files Change regex for mpaso and mpascice files

Short Term Archiving Features This implements features to the short term archiver to enable running it while the model is without obviously breaking things (see #1503 for potential issues with the --last-date option). Other options added include --copy-only, which copies the files to be archived instead of moving them; and --no-incomplete-logs, which ignores logs which are not gzipped, and thus not complete Fixes #1305 Passes scripts_regression_tests BFB * origin/mfdeakin-sandia/in_run_archive: Adds the --force-move option and implies --copy-only when --last-date is specified without --force-move Adds a warning when using the --last-date option and to its help Implement the copy_only option for short term archiving. This copies files rather than moving them Implemented most of the machinery for testing with "incomplete" log files Fix code format issue - replace unused variable with _ Update template.st_archive Adds options to the st_archive to specify the last date (--last-date) to archive, and whether to disable archiving incomplete log files (--no-incomplete-logs)

jedwards4b self-assigned this Apr 5, 2017

jedwards4b requested review from gold2718 and billsacks April 5, 2017 20:17

jedwards4b added the in progress label Apr 5, 2017

billsacks requested changes Apr 5, 2017

View reviewed changes

billsacks approved these changes Apr 5, 2017

View reviewed changes

gold2718 requested changes Apr 9, 2017

View reviewed changes

jedwards4b added 3 commits April 10, 2017 08:00

avoid a problem with pio_typename in clone with keepexe

db23ad5

update warning

b7de299

enforce valid_values for compvars

a6b4e2a

jedwards4b force-pushed the clone_keepexe_and_pio_typename branch from cc80e02 to a6b4e2a Compare April 10, 2017 14:44

gold2718 approved these changes Apr 11, 2017

View reviewed changes

update create_clone help message

6ecf62e

jedwards4b merged commit 82b6181 into ESMCI:master Apr 11, 2017

jedwards4b deleted the clone_keepexe_and_pio_typename branch April 11, 2017 21:44

jedwards4b removed the in progress label Apr 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid a problem with pio_typename in clone with keepexe #1305

avoid a problem with pio_typename in clone with keepexe #1305

jedwards4b commented Apr 5, 2017

billsacks left a comment •

edited

Loading

billsacks Apr 5, 2017

jedwards4b Apr 5, 2017

jedwards4b Apr 5, 2017

billsacks Apr 5, 2017

gold2718 left a comment

jedwards4b commented Apr 10, 2017

gold2718 commented Apr 10, 2017

jedwards4b commented Apr 10, 2017

gold2718 commented Apr 10, 2017

jedwards4b commented Apr 10, 2017

billsacks commented Apr 10, 2017

mvertens commented Apr 11, 2017

gold2718 commented Apr 11, 2017

jedwards4b commented Apr 11, 2017

avoid a problem with pio_typename in clone with keepexe #1305

avoid a problem with pio_typename in clone with keepexe #1305

Conversation

jedwards4b commented Apr 5, 2017

billsacks left a comment • edited Loading

Choose a reason for hiding this comment

billsacks Apr 5, 2017

Choose a reason for hiding this comment

jedwards4b Apr 5, 2017

Choose a reason for hiding this comment

jedwards4b Apr 5, 2017

Choose a reason for hiding this comment

billsacks Apr 5, 2017

Choose a reason for hiding this comment

gold2718 left a comment

Choose a reason for hiding this comment

jedwards4b commented Apr 10, 2017

gold2718 commented Apr 10, 2017

jedwards4b commented Apr 10, 2017

gold2718 commented Apr 10, 2017

jedwards4b commented Apr 10, 2017

billsacks commented Apr 10, 2017

mvertens commented Apr 11, 2017

gold2718 commented Apr 11, 2017

jedwards4b commented Apr 11, 2017

billsacks left a comment •

edited

Loading