Add skip-preview-namelist to case.submit and case_run #1471

erichlf · 2017-05-03T18:58:11Z

Added '--skip-preview-namelist' to case.build

Added '--skip-preview-namelist' to case.build. This will allow the user
to specify if the namelist should be rebuilt from user_nl_*.

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

Fixes #1276, E3SM-Project/E3SM#1466

User interface changes?: Added --skip-preview-namelist to case.build and
template.case.run

Code review: @jgfouca, @rljacob

jedwards4b

The change in template.case.run needs to be made on the cesm side as well.

jedwards4b · 2017-05-03T19:00:52Z

scripts/Tools/case.submit

-        description=description,
-        formatter_class=argparse.ArgumentDefaultsHelpFormatter
-    )
+    parser = argparse.ArgumentParser(description=description)


Why was this code removed?

jedwards4b · 2017-05-03T19:06:03Z

scripts/Tools/case.submit

    parser.add_argument("-a", "--batch-args",
                        help="Used to pass additional arguments to batch system. ")

-    args = parser.parse_args(args[1:])
+    args = parser.parse_args()


This is in conflict with master - I recommend that you rebase and update the PR.

jedwards4b · 2017-05-03T19:07:13Z

scripts/Tools/case.submit


    CIME.utils.expect(args.prereq is None, "--prereq not currently supported")

    CIME.utils.handle_standard_logging_options(args)

-    return args.caseroot, args.job, args.no_batch, args.resubmit, args.batch_args
+    return args


We prefer the old method of passing the individual fields of args not the args object itself.

"We" is overly broad here. Some of us prefer passing the args object itself :-)

I am asking for some uniformity across scripts and we have chosen to pass individual fields rather than the args object. Same issue with format - we have standardized around %s and I see no reason to change it here.

@jedwards4b I see your point about uniformity. I think what's going on here (if I can try to speak somewhat for @erichlf and somewhat for myself) is that there are certain coding standards like these that were put in place when the list of cime developers was very small (basically you and Jim F), and now, as there get to be more cime developers, we're not all in agreement about these coding standards.

Could I suggest, @erichlf - for now leave this as it was, but either open an issue for discussion or put this on the agenda of an upcoming Wednesday cime telecon to revisit our coding standards on these points (% vs. format and returning an args object vs. a big list of components).

jedwards4b · 2017-05-03T19:09:05Z

scripts/lib/CIME/XML/env_batch.py

@@ -43,7 +43,7 @@ def set_value(self, item, value, subgroup=None, ignore_type=False):
                elif value.count(":") == 2:
                    t_spec = "%H:%M:%S"
                else:
-                    expect(False, "could not interpret format for wallclock time %s"%value)
+                    expect(False, "could not interpret format for wallclock time {}".format(value))


I don't understand why these changes from %s to format were made?

It is for python3 compatibility. Yes, we aren't using python3, but it is an easy change that doesn't hurt things and will make any future switch to python3 that much easier.

I personally prefer the new-style format, because I find it more readable. That said, I'm pretty sure that both are supported by python3.

I think you are right that both are currently supported, but if I recall the plan is not to support the old style.

I do not object to changes that bring us closer to python3 compatibility - however I do object to having a mixture of %s and format and we are about to do a release. I would prefer to wait until after the release and then apply this change uniformly throughout the code.

{} and format this is already many instances where this has been implemented throughout the code and not by me.

@erichlf , I will approve these changes, and I think @jedwards4b will too, if your next PR is a global update to the new string format.

Absolutely happy to do so.

erichlf · 2017-05-04T21:03:37Z

I believe I have addressed all issues, and now things should be working with no conflicts.

jedwards4b

I'd still like to see these format changes reverted to %s

jgfouca · 2017-05-05T16:43:32Z

config/acme/machines/template.case.run


    if args.caseroot is not None:
        os.chdir(args.caseroot)

-    return args.caseroot
+    if args.skip_preview_namelist is None:
+      args.skip_preview_namelist = False


We use 4-space indents in CIME's python. You should probably configure your editor to do it this way so we stay consistent.

jgfouca · 2017-05-05T16:44:21Z

config/acme/machines/template.case.run

-    > %s
-""" % ((os.path.basename(args[0]), ) * 4),
+    > {}
+""".format(((os.path.basename(args[0]), ) * 4),


I thought one of the benefits of the new-style string formatting is that you don't have to repeat yourself when inserting the same string. Something like {1} {1} {1}.

Will fix. In this case it would be {0} since lists/arrays start at 0 in python.

jgfouca · 2017-05-05T16:45:05Z

scripts/Tools/case.submit

-    > %s
-""" % ((os.path.basename(args[0]), ) * 4),
+    > {0}
+""".format(os.path.basename(args[0], )),


Does the argument to format still need to be a tuple if it's only one thing?

jgfouca · 2017-05-05T16:48:34Z

scripts/lib/CIME/XML/env_batch.py

@@ -43,7 +43,7 @@ def set_value(self, item, value, subgroup=None, ignore_type=False):
                elif value.count(":") == 2:
                    t_spec = "%H:%M:%S"
                else:
-                    expect(False, "could not interpret format for wallclock time %s"%value)
+                    expect(False, "could not interpret format for wallclock time {}".format(value))


@erichlf , I will approve these changes, and I think @jedwards4b will too, if your next PR is a global update to the new string format.

jgfouca · 2017-05-05T17:04:48Z

scripts/lib/CIME/case_run.py

@@ -120,7 +121,7 @@ def _run_model_impl(case, lid):
                        restore_from_archive(case)

                        orig_cont = case.get_value("CONTINUE_RUN")
-                        if not orig_cont:
+                        if not (orig_cont or skip_pnl):


Maybe @jedwards4b can help me here. I don't know if skipping create_namelists here is safe if orig_cont is False. run_model can be invoked several times for certain tests and in other situations.

You still want to change the CONTINUE_RUN value in either case, so the skip_pnl should be a seperate conditional
if not orig_cont:
case.set_value("CONTINUE_RUN", True)
if not skp_pnl:
create_namelists(case)

jgfouca · 2017-05-05T20:39:43Z

scripts/lib/CIME/case_run.py

@@ -122,6 +123,7 @@ def _run_model_impl(case, lid):
                        orig_cont = case.get_value("CONTINUE_RUN")
                        if not orig_cont:
                            case.set_value("CONTINUE_RUN", True)
+                        if not skip_pnl:


This still isn't right. You've changed the original intent which was to re-run create_namelists if CONTINUE_RUN was False. For now, let's not worry about skip_pnl in this code. This code is for recovery from node failures which is a rare corner case and we don't need to worry about how it performs.

Added the '--skip-preview-namelist' option to case.submit so that we only build the namelist when needed. The default is false. However, when running scripts/tests/scripts_regression_tests.py test_save_timings fails. I am currently debugging this issue.

While converting many of the python2.7 style strings to python3 I made a mistake in a regex and have fixed those mistakes.

The unpacking of args wasn't working correctly for some strange reason, so I have now switched to packed args. Additionally, I converted '--test' to be a proper argument parsed by argparser. This is still failing `./scripts/tests/scripts_regression_tests.py L_TestSaveTimings`

@jgfouca

Added '--skip-preview-namelist' to case.build. This will allow the user to specify if the namelist should be rebuilt from user_nl_8. In this particular commit I added '--skip-preview-namelist' to the call for case.build in the testing suite, since building namelists during tests isn't done. Test suite: scripts_regression_tests.py Test baseline: Test namelist changes: Test status: bit for bit Fixes #1276, E3SM-Project/E3SM#1466 (comment) User interface changes?: Added --skip-preview-namelist to case.build and template.case.run Code review: @jgfouca, @rljacob

@jgfouca

Added the new skip-preview-namelist option to the cesm version of template.case.run. Test suite: scripts_regression_tests.py Test baseline: Test namelist changes: Test status: bit for bit Fixes #1276 User interface changes?: Adds --skip-preview-namelist Code review: @jgfouca, @rljacob

@rljacob

After a rebase from master a few issues reappeared. Most issues were typos or wrong function handle used. I have fixed these issues. Test suite: scripts_regression_tests.py Test baseline: Test namelist changes: Test status: bit for bit Fixes #1276 User interface changes?: Added skip-preview-namelist option Code review: @rljacob, @jgfouca

@jgfouca

Fixed some indentation inconsistencies, removed the tuple from usage in parser, fixed logical for skip_pnl and continue_run, and finally fixed some consistency in strings. Test suite: scripts_regression_tests.py Test baseline: Test namelist changes: Test status: bit for bit Fixes #1276, E3SM-Project/E3SM#1466 User interface changes?: Added --skip-preview-namelist to case.build and template.case.run Code review: @jgfouca, @rljacob

@jgfouca

Removed the logic for skip_pnl in the CONTINUE_RUN portion of the code as per @jgfouca Test suite: scripts_regression_tests.py Test baseline: Test namelist changes: Test status: bit for bit Fixes #1276, E3SM-Project/E3SM#1466 User interface changes?: Added --skip-preview-namelist option to case.submit Code review: @jgfouca, @rljacob

erichlf · 2017-05-09T16:49:08Z

This should be ready now.

jgfouca · 2017-05-09T16:57:02Z

@erichlf why did you assign this to @rljacob ? Usually Jim or I are the integrators.

erichlf · 2017-05-09T17:01:26Z

@jgfouca Because he was on the origin issue.

rljacob · 2017-05-09T17:03:15Z

With the new reviewing system in github, I'm not sure it matters. As long as you or Jim review it. But to get it merged quickly, probably want to re-assign it.

ndkeen · 2017-06-07T21:20:35Z

When might we see this in ACME?

rljacob · 2017-06-07T22:12:21Z

We are currently a little past alpha.10 we'd need alpha.12 to get this.

erichlf assigned rljacob May 3, 2017

erichlf requested review from rljacob and jgfouca May 3, 2017 18:58

rljacob added the in progress label May 3, 2017

erichlf changed the title ~~Erichlf/case submit/skip pnl~~ Add skip-preview-namelist to case.submit and case_run May 3, 2017

jedwards4b requested changes May 3, 2017

View reviewed changes

erichlf force-pushed the erichlf/case_submit/skip_pnl branch from 863f30f to bb6fecc Compare May 4, 2017 21:02

jedwards4b reviewed May 4, 2017

View reviewed changes

jgfouca requested changes May 5, 2017

View reviewed changes

jgfouca reviewed May 5, 2017

View reviewed changes

jedwards4b mentioned this pull request May 8, 2017

python 3 format causing pylint fails #1488

Closed

Erich L Foster added 8 commits May 9, 2017 10:45

Fixed Typos in Python3 Strings

2980c34

While converting many of the python2.7 style strings to python3 I made a mistake in a regex and have fixed those mistakes.

erichlf force-pushed the erichlf/case_submit/skip_pnl branch from f94d6fd to 5a6d393 Compare May 9, 2017 16:48

erichlf assigned jgfouca and unassigned rljacob May 9, 2017

jgfouca merged commit cdcb096 into master May 9, 2017

jgfouca deleted the erichlf/case_submit/skip_pnl branch May 9, 2017 17:06

jgfouca removed the in progress label May 9, 2017

rljacob mentioned this pull request May 18, 2017

Minimize scripts in a job run E3SM-Project/E3SM#1254

Closed

billsacks mentioned this pull request May 20, 2017

case.run should not run preview_namelist (at least by default) #1575

Closed

dqwu mentioned this pull request Jun 14, 2017

Python error on skip-preview-namelist in the run script #1690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add skip-preview-namelist to case.submit and case_run #1471

Add skip-preview-namelist to case.submit and case_run #1471

erichlf commented May 3, 2017 •

edited

Loading

jedwards4b left a comment

jedwards4b May 3, 2017

jedwards4b May 3, 2017

jedwards4b May 3, 2017

billsacks May 3, 2017

jedwards4b May 3, 2017

billsacks May 3, 2017

jedwards4b May 3, 2017

erichlf May 3, 2017

billsacks May 3, 2017

erichlf May 4, 2017

jedwards4b May 4, 2017

erichlf May 5, 2017 •

edited

Loading

jgfouca May 5, 2017

erichlf May 5, 2017

erichlf commented May 4, 2017

jedwards4b left a comment

jgfouca May 5, 2017

erichlf May 5, 2017

jgfouca May 5, 2017

erichlf May 5, 2017

jgfouca May 5, 2017

jgfouca May 5, 2017

jgfouca May 5, 2017

jedwards4b May 5, 2017

erichlf May 5, 2017

jgfouca May 5, 2017

erichlf commented May 9, 2017

jgfouca commented May 9, 2017

erichlf commented May 9, 2017

rljacob commented May 9, 2017

ndkeen commented Jun 7, 2017

rljacob commented Jun 7, 2017

Add skip-preview-namelist to case.submit and case_run #1471

Add skip-preview-namelist to case.submit and case_run #1471

Conversation

erichlf commented May 3, 2017 • edited Loading

jedwards4b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichlf May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichlf commented May 4, 2017

jedwards4b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichlf commented May 9, 2017

jgfouca commented May 9, 2017

erichlf commented May 9, 2017

rljacob commented May 9, 2017

ndkeen commented Jun 7, 2017

rljacob commented Jun 7, 2017

erichlf commented May 3, 2017 •

edited

Loading

erichlf May 5, 2017 •

edited

Loading