Repository: terrapower/armi (python)

Issue:
Weird behavior with case clone, copied inputs, and setting expecting a list of files
I saw some weird behavior recently that I'm going to try to explain as best I could.

We have an input file that has a setting that expects list of files. When we have armi clone the case, it
1. removed the list behavior on the setting, and
2. mangled the setting contents when writing the new settings file.

Initially, we had something like

```yaml
specialFiles:
  - first_file.txt
```

And following the clone, we got
```yaml
specialFiles:
  - f
  - i
  - r
  - s
  - t
  - _
  - f
  - i
  - l
  - e
  - .
  - t
  - x
  - t
```

After some inspection, I found this line in `copyInterfaceInputs`, the function responsible for moving ancillary files found in the input to the location of the cloned suite
https://github.com/terrapower/armi/blob/3882d739cc4f13a77e04938299c47ecb16cacc57/armi/cases/case.py#L982-L988

The `first_file.txt` was correctly moved. But this line set the `specialFiles` setting not to be a list of a single element, but a string with the file. Then, logic that writes the new settings file still sees an iterable of strings for `specialFiles` and writes out each character as if it were a new setting.

This caused the run to crash because the code that works with `specialFiles` cannot access the file `f`.

Pull request description:
Fixing issue in copyInterfaceInputs with one file
## What is the change? Why is it being made?

If a setting has a list with a single file in it, `copyInterfaceInputs()` will correctly copy the file, but mangle the setting when it writes the settings file back out. This PR fixes that problem.

close #2097


## SCR Information

<!-- MANDATORY: uncomment one-and-only-one of these -->
Change Type: fixes

<!-- MANDATORY: Describe the change in one sentence -->
One-Sentence Description: This fixes an edge case in `copyInterfaceInputs()` where there is a list of files to copy, but the list is only one element long.

<!-- MANDATORY: Describe any impact on the requirements, all on one line -->
One-line Impact on Requirements: NA

---

## Checklist

- [x] This PR has only [one purpose or idea](https://terrapower.github.io/armi/developer/tooling.html#one-idea-one-pr).
- [x] [Tests](https://terrapower.github.io/armi/developer/tooling.html#test-it) have been added/updated to verify any new/changed code.
- [x] The [documentation](https://terrapower.github.io/armi/developer/tooling.html#document-it) is still up-to-date in the `doc` folder.
- [x] The code style follows [good practices](https://terrapower.github.io/armi/developer/standards_and_practices.html).
- [x] The dependencies are still up-to-date in `pyproject.toml`.


Patch:
diff --git a/armi/cases/case.py b/armi/cases/case.py
index 4104ff74..4b419b85 100644
--- a/armi/cases/case.py
+++ b/armi/cases/case.py
@@ -380,10 +380,9 @@ class Case:
                 debug=["dataio"],
             )
             if context.MPI_SIZE > 1:
-                # interestingly, you cannot set the parallel flag in the constructor
-                # without auto-specifying the data suffix. This should enable
-                # parallel coverage with auto-generated data file suffixes and
-                # combinations.
+                # interestingly, you cannot set the parallel flag in the constructor without
+                # auto-specifying the data suffix. This should enable parallel coverage with
+                # auto-generated data file suffixes and combinations.
                 cov.config.parallel = True
             cov.start()
 
@@ -391,8 +390,7 @@ class Case:
 
     @staticmethod
     def _endCoverage(userCovFile, cov=None):
-        """Helper to the Case.run(): stop and report code coverage,
-        if the Settings file says to.
+        """Helper to the Case.run(): stop and report code coverage, if the Settings file says to.
 
         Parameters
         ----------
@@ -454,8 +452,7 @@ class Case:
         return os.path.join(covRcDir, "pyproject.toml")
 
     def _startProfiling(self):
-        """Helper to the Case.run(): start the Python profiling,
-        if the Settings file says to.
+        """Helper to the Case.run(): start the Python profiling, if the Settings file says to.
 
         Returns
         -------
@@ -752,9 +749,8 @@ class Case:
         """
         Write the inputs to disk.
 
-        This allows input objects that have been modified in memory (e.g.
-        for a parameter sweep or migration) to be written out as input
-        for a forthcoming case.
+        This allows input objects that have been modified in memory (e.g. for a parameter sweep or
+        migration) to be written out as input for a forthcoming case.
 
         Parameters
         ----------
@@ -767,8 +763,8 @@ class Case:
 
         Notes
         -----
-        This will rename the ``loadingFile`` and ``geomFile`` to be ``title-blueprints + '.yaml'`` and
-        ``title + '-geom.yaml'`` respectively.
+        This will rename the ``loadingFile`` and ``geomFile`` to be ``title-blueprints + '.yaml'``
+        and ``title + '-geom.yaml'`` respectively.
 
         See Also
         --------
@@ -862,23 +858,22 @@ def copyInterfaceInputs(
     cs, destination: str, sourceDir: Optional[str] = None
 ) -> Dict[str, Union[str, list]]:
     """
-    Ping active interfaces to determine which files are considered "input". This
-    enables developers to add new inputs in a plugin-dependent/ modular way.
+    Ping active interfaces to determine which files are considered "input". This enables developers
+    to add new inputs in a plugin-dependent/ modular way.
 
     This function should now be able to handle the updating of:
 
       - a single file (relative or absolute)
       - a list of files (relative or absolute)
-      - a file entry that has a wildcard processing into multiple files.
-        Glob is used to offer support for wildcards.
+      - a file entry that has a wildcard processing into multiple files. Glob is used to offer
+        support for wildcards.
       - a directory and its contents
 
     If the file paths are absolute, do nothing. The case will be able to find the file.
 
-    In case suites or parameter sweeps, these files often have a sourceDir associated
-    with them that is different from the cs.inputDirectory. So, if relative or wildcard,
-    update the file paths to be absolute in the case settings and copy the file to the
-    destination directory.
+    In case suites or parameter sweeps, these files often have a sourceDir associated with them that
+    is different from the cs.inputDirectory. So, if relative or wildcard, update the file paths to
+    be absolute in the case settings and copy the file to the destination directory.
 
     Parameters
     ----------
@@ -892,16 +887,15 @@ def copyInterfaceInputs(
     Returns
     -------
     dict
-        A new settings object that contains settings for the keys and values that are
-        either an absolute file path, a list of absolute file paths, or the original
-        file path if absolute paths could not be resolved
+        A new settings object that contains settings for the keys and values that are either an
+        absolute file path, a list of absolute file paths, or the original file path if absolute
+        paths could not be resolved.
 
     Notes
     -----
-    Regarding the handling of relative file paths: In the future this could be
-    simplified by adding a concept for a suite root directory, below which it is safe
-    to copy files without needing to update settings that point with a relative path
-    to files that are below it.
+    Regarding the handling of relative file paths: In the future this could be simplified by adding
+    a concept for a suite root directory, below which it is safe to copy files without needing to
+    update settings that point with a relative path to files that are below it.
     """
     activeInterfaces = interfaces.getActiveInterfaceInfo(cs)
     sourceDir = sourceDir or cs.inputDirectory
@@ -935,20 +929,20 @@ def copyInterfaceInputs(
                 if "*" in f:
                     WILDCARD = True
                 if not f:
-                    # beware: pathlib.path("") returns "." which can be bad news, so we handle empty strings as
-                    # their own category
+                    # beware: pathlib.path("") returns "." which can be bad news, so we handle empty
+                    # strings as their own category
                     EMPTY = True
                 path = pathlib.Path(f)
                 if not EMPTY and path.is_absolute():
                     ABSOLUTE = True
 
                 # Attempt to construct an absolute file path
-                sourceFullPath = os.path.join(sourceDirPath, f)
+                srcFullPath = os.path.join(sourceDirPath, f)
                 destFilePath = None
                 if WILDCARD:
                     globFilePaths = [
                         pathlib.Path(os.path.join(sourceDirPath, g))
-                        for g in glob.glob(sourceFullPath)
+                        for g in glob.glob(srcFullPath)
                     ]
                     if len(globFilePaths) == 0:
                         destFilePath = f
@@ -967,21 +961,24 @@ def copyInterfaceInputs(
                         newFiles.append(path)
                 else:
                     # treat as a relative path
-                    destFilePath = _copyInputsHelper(
-                        label, sourceFullPath, destination, f
-                    )
+                    destFilePath = _copyInputsHelper(label, srcFullPath, destination, f)
                     newFiles.append(str(destFilePath))
 
                 if destFilePath == f:
                     runLog.debug(
-                        f"No input files for `{label}` could be resolved with the "
-                        f"following path: `{sourceFullPath}`. Will not update `{label}`."
+                        f"No input files for `{label}` could be resolved with the following path: "
+                        f"`{srcFullPath}`. Will not update `{label}`."
                     )
 
             # Some settings are a single filename. Others are lists of files. Make
             # sure we are returning what the setting expects
             if isSetting and len(newFiles):
-                if len(files) == 1 and not WILDCARD:
+                if (
+                    len(files) == 1
+                    and not WILDCARD
+                    and key.name in cs
+                    and not isinstance(cs[key.name], list)
+                ):
                     newSettings[label] = newFiles[0]
                 else:
                     newSettings[label] = newFiles
diff --git a/armi/interfaces.py b/armi/interfaces.py
index 329dfab4..f22b2d5a 100644
--- a/armi/interfaces.py
+++ b/armi/interfaces.py
@@ -619,7 +619,6 @@ class Interface:
         paths, or glob patterns that will be interpolated relative to the input directory. Absolute
         paths will not be copied anywhere.
 
-
         The returned dictionary will enable the source Settings object to be updated to the new file
         location. While the dictionary keys are recommended to be Setting objects, the name of the
         setting as a string, e.g., "shuffleLogic", is still interpreted. If the string name does not


Test patch:
diff --git a/armi/cases/tests/test_cases.py b/armi/cases/tests/test_cases.py
index 23883008..f7f23b99 100644
--- a/armi/cases/tests/test_cases.py
+++ b/armi/cases/tests/test_cases.py
@@ -481,8 +481,8 @@ class TestExtraInputWriting(unittest.TestCase):
             case = baseCase.clone()
             case.writeInputs()
             self.assertTrue(os.path.exists(cs[CONF_SHUFFLE_LOGIC]))
-            # Availability factor is in the original settings file but since it is a
-            # default value, gets removed for the write-out
+            # Availability factor is in the original settings file but since it is a default value,
+            # gets removed for the write-out
             txt = open("armiRun.yaml", "r").read()
             self.assertNotIn("availabilityFactor", txt)
             self.assertIn("armiRun-blueprints.yaml", txt)
@@ -490,16 +490,16 @@ class TestExtraInputWriting(unittest.TestCase):
         with directoryChangers.TemporaryDirectoryChanger():
             case = baseCase.clone(writeStyle="medium")
             case.writeInputs(writeStyle="medium")
-            # Availability factor is in the original settings file and it is a default
-            # value. While "short" (default writing style) removes, "medium" should not
+            # Availability factor is in the original settings file and it is a default value. While
+            # "short" (default writing style) removes, "medium" should not
             txt = open("armiRun.yaml", "r").read()
             self.assertIn("availabilityFactor", txt)
 
 
 class MultiFilesInterfaces(interfaces.Interface):
     """
-    A little test interface that adds a setting that we need to test copyInterfaceInputs
-    with multiple files.
+    A little test interface that adds a setting that we need to test copyInterfaceInputs with
+    multiple files.
     """
 
     name = "MultiFilesInterfaces"
@@ -589,8 +589,7 @@ class TestCopyInterfaceInputs(unittest.TestCase):
             self.assertTrue(os.path.exists(newFilePath))
             self.assertEqual(destFilePath, os.path.basename(newFilePath))
 
-        # test with bad file path, should return original file
-        # ensure we are not in TEST_ROOT
+        # test with bad file path, should return original file ensure we are not in TEST_ROOT
         with directoryChangers.TemporaryDirectoryChanger() as newDir:
             destFilePath = cases.case._copyInputsHelper(
                 testSetting,
@@ -601,7 +600,7 @@ class TestCopyInterfaceInputs(unittest.TestCase):
             self.assertFalse(os.path.exists(destFilePath))
             self.assertEqual(destFilePath, shuffleFile)
 
-    def test_copyInterfaceInputs_singleFile(self):
+    def test_copyInterfaceInputsSingleFile(self):
         testSetting = CONF_SHUFFLE_LOGIC
         cs = settings.Settings(ARMI_RUN_PATH)
         shuffleFile = cs[testSetting]
@@ -615,7 +614,7 @@ class TestCopyInterfaceInputs(unittest.TestCase):
             self.assertTrue(os.path.exists(newFilePath))
             self.assertEqual(newSettings[testSetting], os.path.basename(newFilePath))
 
-    def test_copyInterfaceInputs_nonFilePath(self):
+    def test_copyInterfaceInputsNonFilePath(self):
         testSetting = CONF_SHUFFLE_LOGIC
         cs = settings.Settings(ARMI_RUN_PATH)
         fakeShuffle = "fakeFile.py"
@@ -653,7 +652,7 @@ class TestCopyInterfaceInputs(unittest.TestCase):
         with self.assertRaises(ValueError):
             _ = settings.Settings(ARMI_RUN_PATH)
 
-    def test_copyInterfaceInputs_multipleFiles(self):
+    def test_copyInterfaceInputsMultipleFiles(self):
         # register the new Plugin
         app = getApp()
         app.pluginManager.register(TestPluginForCopyInterfacesMultipleFiles)
@@ -682,7 +681,36 @@ class TestCopyInterfaceInputs(unittest.TestCase):
                 self.assertTrue(os.path.exists(newFilePath))
             self.assertEqual(newSettings[testSetting], settingFiles)
 
-    def test_copyInterfaceInputs_wildcardFile(self):
+    def test_copyInterfaceInputsOneFile(self):
+        # register the new Plugin
+        app = getApp()
+        app.pluginManager.register(TestPluginForCopyInterfacesMultipleFiles)
+
+        pluginPath = (
+            "armi.cases.tests.test_cases.TestPluginForCopyInterfacesMultipleFiles"
+        )
+        settingFiles = ["COMPXS.ascii"]
+        testName = "test_copyInterfaceInputsOneFile"
+        testSetting = "multipleFilesSetting"
+
+        cs = settings.Settings(ARMI_RUN_PATH)
+        cs = cs.modified(
+            caseTitle=testName,
+            newSettings={testName: [pluginPath]},
+        )
+        cs = cs.modified(newSettings={testSetting: settingFiles})
+
+        # ensure we are not in TEST_ROOT
+        with directoryChangers.TemporaryDirectoryChanger() as newDir:
+            newSettings = cases.case.copyInterfaceInputs(
+                cs, destination=newDir.destination
+            )
+            newFilePaths = [os.path.join(newDir.destination, f) for f in settingFiles]
+            for newFilePath in newFilePaths:
+                self.assertTrue(os.path.exists(newFilePath))
+            self.assertEqual(newSettings[testSetting], settingFiles)
+
+    def test_copyInterfaceInputsWildcardFile(self):
         testSetting = CONF_SHUFFLE_LOGIC
         cs = settings.Settings(ARMI_RUN_PATH)
         # Use something that isn't the shuffle logic file in the case settings
@@ -710,7 +738,7 @@ class TestCopyInterfaceInputs(unittest.TestCase):
             self.assertFalse(os.path.exists(newSettings[testSetting][0]))
             self.assertEqual(newSettings[testSetting], [wcFile])
 
-    def test_copyInterfaceInputs_relPath(self):
+    def test_copyInterfaceInputsRelPath(self):
         testSetting = CONF_SHUFFLE_LOGIC
         cs = settings.Settings(ARMI_RUN_PATH)
         shuffleFile = cs[testSetting]
@@ -726,7 +754,7 @@ class TestCopyInterfaceInputs(unittest.TestCase):
             self.assertTrue(os.path.exists(newFilePath))
             self.assertEqual(newSettings[testSetting], os.path.basename(newFilePath))
 
-    def test_copyInterfaceInputs_absPath(self):
+    def test_copyInterfaceInputsAbsPath(self):
         testSetting = CONF_SHUFFLE_LOGIC
         cs = settings.Settings(ARMI_RUN_PATH)
         shuffleFile = cs[testSetting]