Merge pull request #2567 from jedwards4b/smarter_xmlchange

Smarter xmlchange 
Move the flag that indicates an xml file needs to be rewritten from the case object to the individual file objects and before writing a value confirm that that value is not already the same as the file value.

Test suite: scripts_regression_tests.py (running full cesm prealpha test suite in progress)
Test baseline: cesm2_0_alpha10f
Test namelist changes: none
Test status: bit for bit
Addresses #2448

User interface changes?:

Update gh-pages html (Y/N)?:

Code review: @jgfouca

scripts/lib/CIME/XML/generic_xml.py

@@ -48,6 +48,7 @@ def __init__(self, infile=None, schema=None, root_name_override=None, root_attri
         self.locked = False
         self.read_only = read_only
         self.filename = infile
+        self.needsrewrite = False
         if infile is None:
             return
 
@@ -76,6 +77,8 @@ def read(self, infile, schema=None):
         """
         if not self.DISABLE_CACHING and infile in self._FILEMAP and self.read_only:
             logger.debug("read (cached): " + infile)
+            expect(not self.filename or not self.needsrewrite, "Reading into object marked for rewrite, file {}"
+                   .format(self.filename))
             self.tree, self.root = self._FILEMAP[infile]
         else:
             logger.debug("read: " + infile)
@@ -91,6 +94,7 @@ def read(self, infile, schema=None):
             self._FILEMAP[infile] = (self.tree, self.root)
 
     def read_fd(self, fd):
+        expect(not self.filename or not self.needsrewrite, "Reading into object marked for rewrite, file {}"               .format(self.filename))
         if self.tree:
             addroot = _Element(ET.parse(fd).getroot())
             read_only = self.read_only
@@ -143,12 +147,15 @@ def set(self, node, attrib_name, value):
         expect(not self.read_only, "locked")
         if attrib_name == "id":
             expect(not self.locked, "locked")
-        return node.xml_element.set(attrib_name, value)
+        if self.get(node, attrib_name) != value:
+            self.needsrewrite = True
+            return node.xml_element.set(attrib_name, value)
 
     def pop(self, node, attrib_name):
         expect(not self.read_only, "locked")
         if attrib_name == "id":
             expect(not self.locked, "locked")
+        self.needsrewrite = True
         return node.xml_element.attrib.pop(attrib_name)
 
     def attrib(self, node):
@@ -157,11 +164,15 @@ def attrib(self, node):
 
     def set_name(self, node, name):
         expect(not self.read_only, "locked")
-        node.xml_element.tag = name
+        if node.xml_element.tag != name:
+            self.needsrewrite = True
+            node.xml_element.tag = name
 
     def set_text(self, node, text):
         expect(not self.read_only, "locked")
-        node.xml_element.text = text
+        if node.xml_element.text != text:
+            node.xml_element.text = text
+            self.needsrewrite = True
 
     def name(self, node):
         return node.xml_element.tag
@@ -174,6 +185,7 @@ def add_child(self, node, root=None, position=None):
         Add element node to self at root
         """
         expect(not self.locked and not self.read_only, "locked")
+        self.needsrewrite = True
         root = root if root is not None else self.root
         if position is not None:
             root.xml_element.insert(position, node.xml_element)
@@ -185,12 +197,14 @@ def copy(self, node):
 
     def remove_child(self, node, root=None):
         expect(not self.locked and not self.read_only, "locked")
+        self.needsrewrite = True
         root = root if root is not None else self.root
         root.xml_element.remove(node.xml_element)
 
     def make_child(self, name, attributes=None, root=None, text=None):
         expect(not self.locked and not self.read_only, "locked")
         root = root if root is not None else self.root
+        self.needsrewrite = True
         if attributes is None:
             node = _Element(ET.SubElement(root.xml_element, name))
         else:
@@ -271,10 +285,13 @@ def get_version(self):
         version = 1.0 if version is None else float(version)
         return version
 
-    def write(self, outfile=None):
+    def write(self, outfile=None, force_write=False):
         """
         Write an xml file from data in self
         """
+        if not (self.needsrewrite or force_write):
+            return
+
         if outfile is None:
             outfile = self.filename
 
@@ -293,6 +310,7 @@ def write(self, outfile=None):
         else:
             with open(outfile,'w') as xmlout:
                 xmlout.write(xmlstr)
+        self.needsrewrite = False
 
     def scan_child(self, nodename, attributes=None, root=None):
         """

scripts/lib/CIME/case/case.py

@@ -84,7 +84,6 @@ def __init__(self, case_root=None, read_only=True):
             case_root = os.getcwd()
         self._caseroot = case_root
         logger.debug("Initializing Case.")
-        self._env_files_that_need_rewrite = set()
         self._read_only_mode = True
         self._force_read_only = read_only
         self._primary_component = None
@@ -191,16 +190,7 @@ def __exit__(self, *_):
         self._read_only_mode = True
         return False
 
-    def schedule_rewrite(self, env_file):
-        assert not self._read_only_mode, \
-            "case.py scripts error: attempted to modify an env file while in " \
-            "read-only mode"
-        self._env_files_that_need_rewrite.add(env_file)
-
     def read_xml(self):
-        if self._env_files_that_need_rewrite:
-            expect(False, "Object(s) {} seem to have newer data than the corresponding case file".format(" ".join([env_file.filename for env_file in self._env_files_that_need_rewrite])))
-
         self._env_entryid_files = []
         self._env_entryid_files.append(EnvCase(self._caseroot, components=None))
         components = self._env_entryid_files[0].get_values("COMP_CLASSES")
@@ -251,12 +241,8 @@ def flush(self, flushall=False):
         if not os.path.isdir(self._caseroot):
             # do not flush if caseroot wasnt created
             return
-        if flushall:
-            for env_file in self._files:
-                self.schedule_rewrite(env_file)
-        for env_file in self._env_files_that_need_rewrite:
-            env_file.write()
-        self._env_files_that_need_rewrite = set()
+        for env_file in self._files:
+            env_file.write(force_write=flushall)
 
     def get_values(self, item, attribute=None, resolved=True, subgroup=None):
         for env_file in self._files:
@@ -387,7 +373,6 @@ def set_value(self, item, value, subgroup=None, ignore_type=False, allow_undefin
             result = env_file.set_value(item, value, subgroup, ignore_type)
             if (result is not None):
                 logger.debug("Will rewrite file {} {}".format(env_file.filename, item))
-                self._env_files_that_need_rewrite.add(env_file)
                 return result
 
         if len(self._files) == 1:
@@ -406,7 +391,6 @@ def set_valid_values(self, item, valid_values):
             result = env_file.set_valid_values(item, valid_values)
             if (result is not None):
                 logger.debug("Will rewrite file {} {}".format(env_file.filename, item))
-                self._env_files_that_need_rewrite.add(env_file)
                 return result
 
     def set_lookup_value(self, item, value):
@@ -863,7 +847,6 @@ def configure(self, compset_name, grid_name, machine_name=None,
         # Create env_mach_specific settings from machine info.
         env_mach_specific_obj = self.get_env("mach_specific")
         env_mach_specific_obj.populate(machobj)
-        self.schedule_rewrite(env_mach_specific_obj)
 
         self._setup_mach_pes(pecount, multi_driver, ninst, machine_name, mpilib)
 
@@ -880,7 +863,6 @@ def configure(self, compset_name, grid_name, machine_name=None,
         logger.debug("archive defaults located in {}".format(infile))
         archive = Archive(infile=infile, files=files)
         archive.setup(env_archive, self._components, files=files)
-        self.schedule_rewrite(env_archive)
 
         self.set_value("COMPSET",self._compsetname)
 
@@ -967,7 +949,6 @@ def configure(self, compset_name, grid_name, machine_name=None,
             self.set_value("USER_REQUESTED_QUEUE", queue, subgroup=self.get_primary_job())
 
         env_batch.set_job_defaults(bjobs, self)
-        self.schedule_rewrite(env_batch)
 
         # Make sure that parallel IO is not specified if total_tasks==1
         if self.total_tasks == 1:
@@ -1406,11 +1387,8 @@ def update_env(self, new_object, env_file):
         elif old_object in self._env_generic_files:
             self._env_generic_files.remove(old_object)
             self._env_generic_files.append(new_object)
-        if old_object in self._env_files_that_need_rewrite:
-            self._env_files_that_need_rewrite.remove(old_object)
         self._files.remove(old_object)
         self._files.append(new_object)
-        self.schedule_rewrite(new_object)
 
     def get_latest_cpl_log(self, coupler_log_path=None):
         """

scripts/lib/CIME/case/case_setup.py

@@ -160,7 +160,6 @@ def _case_setup_impl(case, caseroot, clean=False, test_mode=False, reset=False):
             # May need to select new batch settings if pelayout changed (e.g. problem is now too big for prev-selected queue)
             env_batch = case.get_env("batch")
             env_batch.set_job_defaults([(case.get_primary_job(), {})], case)
-            case.schedule_rewrite(env_batch)
 
             # create batch files
             env_batch.make_all_batch_files(case)

scripts/tests/scripts_regression_tests.py

@@ -309,10 +309,31 @@ def test_a_createnewcase(self):
     def test_aa_no_flush_on_instantiate(self):
         testdir = os.path.join(self.__class__._testroot, 'testcreatenewcase')
         with Case(testdir, read_only=False) as case:
-            self.assertFalse(case._env_files_that_need_rewrite, msg="Instantiating a case should not trigger a flush call")
+            for env_file in case._files:
+                self.assertFalse(env_file.needsrewrite, msg="Instantiating a case should not trigger a flush call")
         with Case(testdir, read_only=False) as case:
-            case.set_value("HIST_OPTION","nsteps")
-            self.assertTrue(case._env_files_that_need_rewrite, msg="Expected flush call not triggered")
+            case.set_value("HIST_OPTION","nyears")
+            runfile = case.get_env('run')
+            self.assertTrue(runfile.needsrewrite, msg="Expected flush call not triggered")
+            for env_file in case._files:
+                if env_file != runfile:
+                    self.assertFalse(env_file.needsrewrite, msg="Unexpected flush triggered for file {}"
+                                     .format(env_file.filename))
+            # Flush the file
+            runfile.write()
+            # set it again to the same value
+            case.set_value("HIST_OPTION","nyears")
+            # now the file should not need to be flushed
+            for env_file in case._files:
+                self.assertFalse(env_file.needsrewrite, msg="Unexpected flush triggered for file {}"
+                                 .format(env_file.filename))
+
+        # Check once more with a new instance
+        with Case(testdir, read_only=False) as case:
+            case.set_value("HIST_OPTION","nyears")
+            for env_file in case._files:
+                self.assertFalse(env_file.needsrewrite, msg="Unexpected flush triggered for file {}"
+                                 .format(env_file.filename))
 
     def test_b_user_mods(self):
         cls = self.__class__