Update the EntryPoint class to provide user feedback on required positional arguments (#922)

* Update the entry point class to provide the settings file as an required
position argument when a user requests the help menu. Additionally,
add a try/except around adding a new argument from case settings to
handle duplicate entries more robustly rather than requiring exclusion
of settings like `verbosity` and `branchVerbosity`.

* Suppress the printing of all possible settings in clone CLI

* There are 18 entry points!

* Make changes to checkInputs CLI and tests

Having both a settings file and patterns arg is both redundant and causes
unreliable use issues. Since only patterns was used in invoke, decided
to remove the settingsArgument as optional. I also removed the
sys.exit(), since there is no need that I can think of to do this. The
armi unit test settings file, e.g., fails this check (with can start =
UNKNOWN).

* Move settings arg processing to not be first (nargs='*' behavior means this can't be first in the list)

* Minor docstring add

* Remove reloadDBName setting from armiRun.yaml

This is only needed for 3 tests, and it was causing some annoying
failures on the check-inputs entry point test (although I fixed it
separately by rejecting the notion that there needed to be a
sys.exit()).

* Update warning to error for checkInputs CLI

* Remove unused CLI args from checkInputs (finishing what #754 started)

* Remove update to real file because that ain't necessary

* Release Notes

Co-authored-by: Arrielle Opotowsky <[email protected]>

Author: jakehader

armi/bookkeeping/db/tests/test_comparedb3.py

@@ -86,7 +86,9 @@ def test_diffResultsBasic(self):
     def test_compareDatabaseDuplicate(self):
         """end-to-end test of compareDatabases() on a photocopy database"""
         # build two super-simple H5 files for testing
-        o, r = test_reactors.loadTestReactor(TEST_ROOT)
+        o, r = test_reactors.loadTestReactor(
+            TEST_ROOT, customSettings={"reloadDBName": "reloadingDB.h5"}
+        )
 
         # create two DBs, identical but for file names
         dbs = []
@@ -115,7 +117,9 @@ def test_compareDatabaseDuplicate(self):
     def test_compareDatabaseSim(self):
         """end-to-end test of compareDatabases() on very simlar databases"""
         # build two super-simple H5 files for testing
-        o, r = test_reactors.loadTestReactor(TEST_ROOT)
+        o, r = test_reactors.loadTestReactor(
+            TEST_ROOT, customSettings={"reloadDBName": "reloadingDB.h5"}
+        )
 
         # create two DBs, identical but for file names
         dbs = []

armi/bookkeeping/db/tests/test_database3.py

@@ -38,7 +38,9 @@ class TestDatabase3(unittest.TestCase):
     def setUp(self):
         self.td = TemporaryDirectoryChanger()
         self.td.__enter__()
-        self.o, self.r = test_reactors.loadTestReactor(TEST_ROOT)
+        self.o, self.r = test_reactors.loadTestReactor(
+            TEST_ROOT, customSettings={"reloadDBName": "reloadingDB.h5"}
+        )
 
         self.dbi = database3.DatabaseInterface(self.r, self.o.cs)
         self.dbi.initDB(fName=self._testMethodName + ".h5")
@@ -180,8 +182,11 @@ def test_prepRestartRun(self):
         created here for this test.
         """
         # first successfully call to prepRestartRun
-        o, r = test_reactors.loadTestReactor(TEST_ROOT)
+        o, r = test_reactors.loadTestReactor(
+            TEST_ROOT, customSettings={"reloadDBName": "reloadingDB.h5"}
+        )
         cs = o.cs
+
         ratedPower = cs["power"]
         startCycle = cs["startCycle"]
         startNode = cs["startNode"]

armi/cli/checkInputs.py

@@ -60,7 +60,6 @@ class CheckInputEntryPoint(EntryPoint):
     """
 
     name = "check-input"
-    settingsArgument = "optional"
 
     def addOptions(self):
         self.parser.add_argument(
@@ -70,19 +69,6 @@ def addOptions(self):
             default=False,
             help="Generate a report to summarize the inputs",
         )
-        self.parser.add_argument(
-            "--full-core-map",
-            "-m",
-            action="store_true",
-            default=False,
-            help="Generate the full core reactor map in the design report",
-        )
-        self.parser.add_argument(
-            "--disable-block-axial-mesh",
-            action="store_true",
-            default=False,
-            help="Remove the additional block axial mesh points on the assembly type figure(s)",
-        )
         self.parser.add_argument(
             "--recursive",
             "-r",
@@ -118,16 +104,15 @@ def invoke(self):
             if not self.args.skip_checks:
                 hasIssues = "PASSED" if case.checkInputs() else "HAS ISSUES"
 
-            try:
-                if self.args.generate_design_summary:
+            canStart = "UNKNOWN"
+            if self.args.generate_design_summary:
+                try:
                     case.summarizeDesign()
                     canStart = "PASSED"
-                else:
-                    canStart = "UNKNOWN"
-            except Exception:
-                runLog.error("Failed to initialize/summarize {}".format(case))
-                runLog.error(traceback.format_exc())
-                canStart = "FAILED"
+                except Exception:
+                    runLog.error("Failed to initialize/summarize {}".format(case))
+                    runLog.error(traceback.format_exc())
+                    canStart = "FAILED"
 
             table.append((case.cs.path, case.title, canStart, hasIssues))
 
@@ -139,5 +124,8 @@ def invoke(self):
             )
         )
 
-        if any(t[2] != "PASSED" or t[3] != "PASSED" for t in table):
-            sys.exit(-1)
+        if any(t[3] == "HAS ISSUES" for t in table):
+            runLog.error("The case is not self consistent")
+
+        if any(t[2] == "FAILED" for t in table):
+            runLog.error("The case can not start")

armi/cli/clone.py

@@ -28,17 +28,16 @@ class CloneArmiRunCommandBatch(EntryPoint):
     settingsArgument = "required"
 
     def addOptions(self):
-        for settingName in self.cs.keys():
-            # verbosity and branchVerbosity already have command line options in the default parser
-            # adding them again would result in an error from argparse.
-            if settingName not in ["verbosity", "branchVerbosity"]:
-                self.createOptionFromSetting(settingName)
         self.parser.add_argument(
             "--additional-files",
             nargs="*",
             default=[],
             help="Additional files from the source directory to copy into the target directory",
         )
+        # somehow running `armi clone-batch -h` on the command line requires this to
+        # not be first?
+        for settingName in self.cs.keys():
+            self.createOptionFromSetting(settingName, suppressHelp=True)
 
     def invoke(self):
         # get the case title.
@@ -65,10 +64,8 @@ class CloneSuiteCommand(EntryPoint):
 
     def addOptions(self):
         for settingName in self.cs.environmentSettings:
-            # verbosity and branchVerbosity already have command line options in the default parser
-            # adding them again would result in an error from argparse.
-            if settingName not in {"verbosity", "branchVerbosity"}:
-                self.createOptionFromSetting(settingName)
+            self.createOptionFromSetting(settingName)
+
         self.parser.add_argument(
             "--directory",
             "-d",

armi/cli/entryPoint.py

@@ -90,18 +90,53 @@ class EntryPoint:
     def __init__(self):
         if self.name is None:
             raise AttributeError(
-                "Subclasses of EntryPoint must define a `name` class attribute"
+                f"Subclasses of EntryPoint must define a `name` class attribute"
             )
 
+        self.cs = self._initSettings()
+        settings.setMasterCs(self.cs)
+
         self.parser = argparse.ArgumentParser(
             prog="{} {}".format(context.APP_NAME, self.name),
             description=self.description or self.__doc__,
         )
+        if self.settingsArgument is not None:
+            if self.settingsArgument not in ["required", "optional"]:
+                raise AttributeError(
+                    f"Subclasses of EntryPoint must specify if the a case settings file is `required` or `optional`"
+                )
+            if self.settingsArgument == "optional":
+                self.parser.add_argument(
+                    "settings_file",
+                    nargs="?",
+                    action=loadSettings(self.cs),
+                    help="path to the settings file to load.",
+                )
+            elif self.settingsArgument == "required":
+                self.parser.add_argument(
+                    "settings_file",
+                    action=loadSettings(self.cs),
+                    help="path to the settings file to load.",
+                )
 
-        self.args = argparse.Namespace()
+        # optional arguments
+        self.parser.add_argument(
+            "--caseTitle",
+            type=str,
+            nargs=None,
+            action=setCaseTitle(self.cs),
+            help="update the case title of the run.",
+        )
+        self.parser.add_argument(
+            "--batch",
+            action="store_true",
+            default=False,
+            help="Run in batch mode even on TTY, silencing all queries.",
+        )
+        self.createOptionFromSetting("verbosity", "-v")
+        self.createOptionFromSetting("branchVerbosity", "-V")
 
-        self.cs = self._initSettings()
-        settings.setMasterCs(self.cs)
+        self.args = argparse.Namespace()
         self.settingsProvidedOnCommandLine = []
 
     @staticmethod
@@ -129,7 +164,6 @@ def addOptions(self):
 
         argparse.ArgumentParser.add_argument : Often called from here using
             ``self.parser.add_argument`` to add custom argparse arguments.
-
         """
 
     def parse_args(self, args):
@@ -140,37 +174,6 @@ def parse(self, args):
         """
         Parse the command line arguments, with the command specific arguments.
         """
-
-        if self.settingsArgument == "optional":
-            self.parser.add_argument(
-                "settings_file",
-                nargs="?",
-                action=loadSettings(self.cs),
-                help="path to the settings file to load.",
-            )
-        elif self.settingsArgument == "required":
-            self.parser.add_argument(
-                "settings_file",
-                action=loadSettings(self.cs),
-                help="path to the settings file to load.",
-            )
-        # optional arguments
-        self.parser.add_argument(
-            "--caseTitle",
-            type=str,
-            nargs=None,
-            action=setCaseTitle(self.cs),
-            help="update the case title of the run.",
-        )
-        self.parser.add_argument(
-            "--batch",
-            action="store_true",
-            default=False,
-            help="Run in batch mode even on TTY, silencing all queries.",
-        )
-        self.createOptionFromSetting("verbosity", "-v")
-        self.createOptionFromSetting("branchVerbosity", "-V")
-
         self.addOptions()
         self.parse_args(args)
 
@@ -233,15 +236,21 @@ def createOptionFromSetting(
 
             isListType = settingsInstance.underlyingType == list
 
-            self.parser.add_argument(
-                *aliases,
-                type=str,  # types are properly converted by _SetSettingAction
-                nargs="*" if isListType else None,
-                action=setSetting(self),
-                default=settingsInstance.default,
-                choices=choices,
-                help=helpMessage
-            )
+            try:
+                self.parser.add_argument(
+                    *aliases,
+                    type=str,  # types are properly converted by _SetSettingAction
+                    nargs="*" if isListType else None,
+                    action=setSetting(self),
+                    default=settingsInstance.default,
+                    choices=choices,
+                    help=helpMessage,
+                )
+            # Capture an argument error here to prevent errors when duplicate options are attempting
+            # to be added. This may also be captured by exploring the parser's `_actions` list as well
+            # but this avoid accessing a private attribute.
+            except argparse.ArgumentError:
+                pass
 
     def _createToggleFromSetting(self, settingName, helpMessage, additionalAlias=None):
         aliases = ["--" + settingName]

armi/cli/modify.py

@@ -76,11 +76,7 @@ def addOptions(self):
             help="Pattern(s) to use to find match file names (e.g. *.yaml)",
         )
         for settingName in self.cs.keys():
-            # verbosity and branchVerbosity already have command line options in the default parser
-            # adding them again would result in an error from argparse.
-            if settingName not in ["verbosity", "branchVerbosity"]:
-                # can't modify case title, just use clone
-                self.createOptionFromSetting(settingName, suppressHelp=True)
+            self.createOptionFromSetting(settingName, suppressHelp=True)
 
     def invoke(self):
         csInstances = settings.recursivelyLoadSettingsFiles(

armi/cli/tests/test_runEntryPoint.py

@@ -21,6 +21,7 @@
 import unittest
 
 from armi.__main__ import main
+from armi.cli.entryPoint import EntryPoint
 from armi.bookkeeping.visualization.entryPoint import VisFileEntryPoint
 from armi.cli.checkInputs import CheckInputEntryPoint, ExpandBlueprints
 from armi.cli.clone import CloneArmiRunCommandBatch, CloneSuiteCommand
@@ -32,38 +33,71 @@
 from armi.cli.run import RunEntryPoint
 from armi.cli.runSuite import RunSuiteCommand
 from armi.physics.neutronics.diffIsotxs import CompareIsotxsLibraries
-from armi.tests import mockRunLogs, TEST_ROOT
+from armi.tests import mockRunLogs, TEST_ROOT, ARMI_RUN_PATH
 from armi.utils.directoryChangers import TemporaryDirectoryChanger
+from armi.utils.dynamicImporter import getEntireFamilyTree
+
+
+class TestInitializationEntryPoints(unittest.TestCase):
+    def test_entryPointInitialization(self):
+        """Tests the initialization of all subclasses of `EntryPoint`."""
+        entryPoints = getEntireFamilyTree(EntryPoint)
+
+        # Note that this is a hard coded number that should be incremented
+        # if a new ARMI framework entry point is added. This is a bit hacky,
+        # but will help demonstrate that entry point classes can be initialized
+        # and the `addOptions` and public API is tested.
+        self.assertEqual(len(entryPoints), 18)
+
+        for e in entryPoints:
+            entryPoint = e()
+            entryPoint.addOptions()
+            settingsArg = None
+            if entryPoint.settingsArgument is not None:
+                for a in entryPoint.parser._actions:
+                    if "settings_file" in a.dest:
+                        settingsArg = a
+                        break
+                self.assertIsNotNone(
+                    settingsArg,
+                    msg=(
+                        f"A settings file argument was expected for {entryPoint}, "
+                        f"but does not exist. This is a error in the EntryPoint "
+                        f"implementation."
+                    ),
+                )
 
 
 class TestCheckInputEntryPoint(unittest.TestCase):
     def test_checkInputEntryPointBasics(self):
         ci = CheckInputEntryPoint()
         ci.addOptions()
-        ci.parse_args(["/path/to/fake.yaml"])
+        ci.parse_args(["/path/to/fake.yaml", "-C"])
 
         self.assertEqual(ci.name, "check-input")
-        self.assertEqual(ci.settingsArgument, "optional")
+        self.assertEqual(ci.args.patterns, ["/path/to/fake.yaml"])
+        self.assertEqual(ci.args.skip_checks, True)
+        self.assertEqual(ci.args.generate_design_summary, False)
 
     def test_checkInputEntryPointInvoke(self):
         ci = CheckInputEntryPoint()
         ci.addOptions()
-        ci.parse_args(["/path/to/fake.yaml"])
+        ci.parse_args([ARMI_RUN_PATH])
 
         with mockRunLogs.BufferLog() as mock:
             self.assertEqual("", mock._outputStream)
 
             ci.invoke()
 
-            self.assertIn("/path/to/fake.yaml", mock._outputStream)
+            self.assertIn(ARMI_RUN_PATH, mock._outputStream)
             self.assertIn("input is self consistent", mock._outputStream)
 
 
 class TestCloneArmiRunCommandBatch(unittest.TestCase):
     def test_cloneArmiRunCommandBatchBasics(self):
         ca = CloneArmiRunCommandBatch()
         ca.addOptions()
-        ca.parse_args(["--additional-files", "test"])
+        ca.parse_args([ARMI_RUN_PATH, "--additional-files", "test"])
 
         self.assertEqual(ca.name, "clone-batch")
         self.assertEqual(ca.settingsArgument, "required")
@@ -237,7 +271,7 @@ class TestRunEntryPoint(unittest.TestCase):
     def test_runEntryPointBasics(self):
         rep = RunEntryPoint()
         rep.addOptions()
-        rep.parse_args([])
+        rep.parse_args([ARMI_RUN_PATH])
 
         self.assertEqual(rep.name, "run")
         self.assertEqual(rep.settingsArgument, "required")

armi/reactor/tests/test_reactors.py

@@ -136,10 +136,10 @@ def loadTestReactor(
 
     Parameters
     ----------
-    inputFilePath : str
+    inputFilePath : str, default=TEST_ROOT
         Path to the directory of the input file.
 
-    customSettings : dict with str keys and values of any type
+    customSettings : dict with str keys and values of any type, default=None
         For each key in customSettings, the cs which is loaded from the
         armiRun.yaml will be overwritten to the value given in customSettings
         for that key.