Sta fix

[jedwards4b]

Fix logic in st_archive wrt DOUT_S_SAVE_INTERIM_RESTART_FILES
Test suite: scripts_regression_tests.py - several runs of create_newcase with various settings
of DOUT_S_SAVE_INTERIM_RESTART_FILES and REST_N
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1572

User interface changes?:

Code review:

[billsacks]

I'm basically fine with this, but see my note / question about case.get_value("TEST") part of the conditional.

[billsacks]

Do we definitely need case.get_value("TEST") here? I'm wondering if that was added as a quick way to get the tests passing with the previous incorrect logic? It seems dangerous to have too many instances where logic differs depending on whether you're running a test, because it makes it more likely that we'll miss problems in testing. If this is needed, I'm fine keeping it, but let's remove it if possible.

[jedwards4b]

I've refined this to only have logic for the tests which explicitly call st_archive internally.

[billsacks]

It still makes me uncomfortable to have logic about whether we're running a test inside production code. I'll trust you if you say it's truly needed, but can you please add a comment saying why this logic is needed when we're running an ERI or SSP test?

And actually, if it's needed by certain tests that call st_archive internally, then it feels to me like a cleaner solution would be to have an extra argument to the st_archive call like force_restart_directory_creation, which could be False by default by set to True by those tests. Then it's more apparent when reading the tests that they are operating differently from the typical behavior.

[jedwards4b]

Retested after refactoring.

[billsacks]

I'm fine with this code, but I'll ask again: Can you please document why we need to operate differently if we're in a test? If you don't know the answer, but just know that tests fail if we don't do that, that's okay. But if you know the answer, it would be really good to get this unintuitive behavior documented.

[jedwards4b]

The ERI test needs this because it writes history files before the end of the ref2 run that it will use
in the branch start of the base run. If you are happy with this please remove the 'changes requested' flag in your review

[billsacks]

I'm pretty sure that you don't need to force the creation of archive_restdir when running a test: you can rely on the existing logic for the production case (if datename_is_last or case.get_value('DOUT_S_SAVE_INTERIM_RESTART_FILES')).

I have determined this by:

1. Running ERI.T31_g37_rx1.A.yellowstone_intel with

--- a/scripts/lib/CIME/SystemTests/system_tests_common.py
+++ b/scripts/lib/CIME/SystemTests/system_tests_common.py
@@ -221,7 +221,7 @@ class SystemTestsCommon(object):
             self._component_compare_copy(suffix)

         if st_archive:
-            case_st_archive(self._case, test=True)
+            case_st_archive(self._case, test=False)

     def _coupler_log_indicates_run_complete(self):
         newestcpllogfile = self._get_latest_cpl_log()

2. Tracing uses of archive_restdir in the _archive_restarts routine (and the _archive_rpointer_files routine that it calls): archive_restdir is only used if either (a) datename_is_last is true, or (b) DOUT_S_SAVE_INTERIM_RESTART_FILES is true. I can't see any code path that would require creating archive_restdir if both of those are false.

Sorry to be nit-picky here: I just find it confusing and error-prone to have test-specific logic in production code unless there's really no way around it.

So unless I'm missing something, can you please remove this test check?

[jedwards4b]

Why don't you just update the process with what you think you want.

…

On May 19, 2017 8:17 PM, "Bill Sacks" ***@***.***> wrote: I'm pretty sure that you don't need to force the creation of archive_restdir when running a test: you can rely on the existing logic for the production case (if datename_is_last or case.get_value('DOUT_S_SAVE_ INTERIM_RESTART_FILES')). I have determined this by: 1. Running ERI.T31_g37_rx1.A.yellowstone_intel with --- a/scripts/lib/CIME/SystemTests/system_tests_common.py+++ b/scripts/lib/CIME/SystemTests/system_tests_common.py@@ -221,7 +221,7 @@ class SystemTestsCommon(object): self._component_compare_copy(suffix) if st_archive:- case_st_archive(self._case, test=True)+ case_st_archive(self._case, test=False) def _coupler_log_indicates_run_complete(self): newestcpllogfile = self._get_latest_cpl_log() 1. Tracing uses of archive_restdir in the _archive_restarts routine (and the _archive_rpointer_files routine that it calls): archive_restdir is only used if either (a) datename_is_last is true, or (b) DOUT_S_SAVE_INTERIM_RESTART_FILES is true. I can't see any code path that would require creating archive_restdir if both of those are false. Sorry to be nit-picky here: I just find it confusing and error-prone to have test-specific logic in production code unless there's really no way around it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF16GKhjBbaZjn3kBr9XegYLXNWS_cPGks5r7k0cgaJpZM4Ng4rd> .

[billsacks]

@jedwards4b - I have pushed a commit that backs out the test-specific code. I can't see any reasons why this would cause problems, but feel free to correct me if you feel differently. I have run scripts_regression_tests plus ERI.T31_g37_rx1.A.yellowstone_intel. I'll merge this if you approve and if @mfdeakin-sandia approves.

[billsacks]

@jedwards4b and @mfdeakin-sandia - my updated changes are awaiting your "okay". @mfdeakin-sandia , if we don't hear from you soon, we plan to merge this, since it addresses a critical issue. So please let us know if you'd like a chance to review it.

[jedwards4b]

I'm okay with this change, I ran a ERI.f09_g16.B1850.cheyenne_intel and it also passed.