Repository: terrapower/armi
Pull request #1244 (2023-04-14 00:51:53)

Update settings file when user agrees to corrective query settings
## Description
Seeks to resolve https://github.com/terrapower/armi/issues/1221. 

When a corrective query is resolved per the discretion of the use, the inspector now prompts the user whether to preserve the original settings. If yes, then a copy of the original settings is saved into the same filename but with an additional suffix: `_old.yaml`. If no, then the settings file is overwritten with the new settings. If an `_old.yaml` file exists in the repository, then the inspector will prompt the user if the `_old.yaml` file should be overwritten.

Testing has been done to make sure that the settings file is overwritten with new changes and the original settings are preserved correctly in the `_old.yaml` file.



<!-- Mandatory: Please include a summary of the change and link to any related GitHub Issues.-->

---

## Checklist

<!--
    You (the pull requester) should put an `x` in the boxes below you have completed.
    If you're unsure about any of them, don't hesitate to ask. We're here to help!
    Learn what a "good PR" looks like here: https://terrapower.github.io/armi/developer/tooling.html#good-pull-requests
-->

- [x] This PR has only one purpose or idea.
- [x] Tests have been added/updated to verify that the new/changed code works.

<!-- Check the code quality -->

- [x] The code style follows [good practices](https://terrapower.github.io/armi/developer/standards_and_practices.html).
- [x] The commit message(s) follow [good practices](https://terrapower.github.io/armi/developer/tooling.html).

<!-- Check the project-level cruft -->

- [x] The [release notes](https://terrapower.github.io/armi/release/index.html) (location `doc/release/0.X.rst`) are up-to-date with any bug fixes or new features.
- [x] The documentation is still up-to-date in the `doc` folder.
- [x] The dependencies are still up-to-date in `setup.py`.



john-science: Are we at all worried that we are over-writing the original setting file, and that is unrecoverable? 

For instance, if the settings file is on a share folder, and multiple people are using it for different purposes, then this gives someone the power to change the settings file out from underneath another user.

Just asking.

HunterPSmith: > Are we at all worried that we are over-writing the original setting file, and that is unrecoverable?
> 
> For instance, if the settings file is on a share folder, and multiple people are using it for different purposes, then this gives someone the power to change the settings file out from underneath another user.
> 
> Just asking.


It's a good question. I have thought about it, but from my experience, if someone is helping me out with trouble shooting or if I'm sharing it with someone who is interested in the case I ran, they usually just copy all the input files including the settings file over into their own folder and then run it or make changes.  I can't really think of a situation where multiple people would need to point to the same settings file. 

Also, if multiple people are working from the same settings file, the user trying to run the settings is being asked by the inspector if they want to change the settings, so maybe we leave that up to them to decide if it is appropriate or if they should notify the other users. Currently, when the inspector prompts the user, it implies that it is fixing the files for you, so the settings get changed in memory but not in the settings file which is problematic because then you have an output that doesn't actually reflect the "true" settings file. 
HunterPSmith: Ok, this looks a bit different now. I updated the PR description. But now I think it now handles it in a way that makes it very clear to the user what is going on.

Patch:
diff --git a/armi/operators/settingsValidation.py b/armi/operators/settingsValidation.py
index 674ff876..c1c845cd 100644
--- a/armi/operators/settingsValidation.py
+++ b/armi/operators/settingsValidation.py
@@ -23,6 +23,8 @@ is impossible. Would you like to switch to ___?"
 """
 import re
 import os
+import shutil
+import itertools
 
 from armi import context
 from armi import getPluginManagerOrFail
@@ -219,6 +221,24 @@ class Inspector:
                 )
             runLog.debug("{} has finished querying.".format(self.__class__.__name__))
 
+        if correctionsMade:
+            # find unused file path to store original settings as to avoid overwrite
+            strSkeleton = "{}_old".format(self.cs.path.split(".yaml")[0])
+            for num in itertools.count():
+                if num == 0:
+                    renamePath = f"{strSkeleton}.yaml"
+                else:
+                    renamePath = f"{strSkeleton}{num}.yaml"
+                if not self._csRelativePathExists(renamePath):
+                    break
+            # preserve old file before saving settings file
+            runLog.important(
+                f"Preserving original settings file by renaming `{renamePath}`"
+            )
+            shutil.copy(self.cs.path, renamePath)
+            # save settings file
+            self.cs.writeToYamlFile(self.cs.path)
+
         return correctionsMade
 
     def addQuery(self, condition, statement, question, correction):


Test patch:
diff --git a/armi/operators/tests/test_inspectors.py b/armi/operators/tests/test_inspectors.py
index 9f0b0ae6..ba9a5943 100644
--- a/armi/operators/tests/test_inspectors.py
+++ b/armi/operators/tests/test_inspectors.py
@@ -16,21 +16,35 @@
 Tests for settings validation system.
 """
 import unittest
+import os
 
 from armi import settings
 from armi import operators
 from armi.operators.settingsValidation import createQueryRevertBadPathToDefault
+from armi.operators import settingsValidation
+from armi.utils import directoryChangers
+from armi import context
 
 
 class TestInspector(unittest.TestCase):
     """Test case"""
 
     def setUp(self):
+        self.td = directoryChangers.TemporaryDirectoryChanger()
+        self.td.__enter__()
+        self.init_mode = context.CURRENT_MODE
         self.cs = settings.Settings()
         self.inspector = operators.getOperatorClassFromSettings(self.cs).inspector(
             self.cs
         )
         self.inspector.queries = []  # clear out the auto-generated ones
+        self.filepathYaml = os.path.join(
+            os.getcwd(), self._testMethodName + "_test_setting_io.yaml"
+        )
+
+    def tearDown(self):
+        context.Mode.setMode(self.init_mode)
+        self.td.__exit__(None, None, None)
 
     def test_query(self):
         buh = {1: 2, 3: 4}
@@ -52,6 +66,54 @@ class TestInspector(unittest.TestCase):
 
         self.assertEqual(str(query), "<Query: beepbopboopbeep>")
 
+    def test_overwriteSettingsCorrectiveQuery(self):
+        """
+        Tests the case where a corrective query is resolved.
+        Checks to make sure the settings file is overwritten with the resolved setting.
+        """
+        # load settings from test settings file
+        self.cs["cycleLength"] = 300.0
+        self.cs.writeToYamlFile(self.filepathYaml)
+        self.cs.loadFromInputFile(self.filepathYaml)
+        self.assertEqual(self.cs["cycleLength"], 300.0)
+
+        # define corrective query
+        def csChange(x, y, z):
+            x[y] = z
+
+        self.inspector.addQuery(
+            lambda: self.inspector.cs["cycleLength"] == 300.0,
+            "Changing `cycleLength` from 300.0 to 666",
+            ":D",
+            lambda: csChange(self.cs, "cycleLength", 666),
+        )
+
+        # redefine prompt function in order to circumvent need for user input
+        def fakePrompt(*inputs):
+            return True
+
+        nominalPromptFunction = settingsValidation.prompt
+        settingsValidation.prompt = fakePrompt
+
+        try:
+            # run inspector
+            self.inspector.run()
+
+            # check to see if file was overwritten correctly
+            self.cs.loadFromInputFile(self.filepathYaml)
+            self.assertEqual(self.cs["cycleLength"], 666)
+
+            # check to see if original settings were saved in "_old.yaml" file
+            oldFilePath = "{}_old.yaml".format(self.filepathYaml.split(".yaml")[0])
+            self.assertTrue(os.path.exists(oldFilePath) and os.path.isfile(oldFilePath))
+            self.csOriginal = settings.Settings()
+            self.csOriginal.loadFromInputFile(oldFilePath)
+            self.assertEqual(self.csOriginal["cycleLength"], 300.0)
+
+        finally:
+            # reset prompt function to nominal
+            settingsValidation.prompt = nominalPromptFunction
+
     def test_changeOfCS(self):
         self.inspector.addQuery(
             lambda: self.inspector.cs["runType"] == "banane",