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

jakehader · 2022-10-05T00:25:37Z

Description

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.

@opotowsky update:

In getting the unit tests to work, a few more changes were made.

the reloadDBName setting was removed from armiRun.yaml. This ended up not being necessary after other fixes were made, but it is only used for 3 tests (in bookkeeping/db).
I altered the above-mentioned test setup there to have custom settings to handle the deletion. Side note: ARMI should be able to handle settings that are files that do not yet exist, but it can't right now. Seemed ideal to keep the base case for all unit tests simple.
the CLI for checkInputs needed some love. There are minor code updates, deletions, and some small updates to the tests.

Checklist

This PR has only one purpose or idea.
Tests have been added/updated to verify that the new/changed code works.

The code style follows good practices.
The commit message(s) follow good practices.

The release notes (location doc/release/0.X.rst) are up-to-date with any bug fixes or new features.
The documentation is still up-to-date in the doc folder.
The dependencies are still up-to-date in setup.py.

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`.

opotowsky · 2022-10-05T21:39:42Z

For the life of me, I couldn't invoke any of the clone commands in test_runEntryPoint.py

This update allows me to.

THANK YOU!!!!!!!!!!!!!!!!

opotowsky · 2022-10-05T21:51:34Z

I'll push up some unit test fixes to this branch

armi/cli/tests/test_runEntryPoint.py

armi/cli/clone.py

Having both a settings file and patterns arg is both redunant 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).

For some reason, this makes parse_args go bonkers (on my machine, at least). So this seemed like an easy resolution, even if I don't understand it.

armi/cli/checkInputs.py

armi/cli/clone.py

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()).

…oint

john-science · 2022-10-07T19:58:57Z

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"}


armi/cli/tests/test_runEntryPoint.py

opotowsky · 2022-10-07T20:24:43Z

armi/cli/tests/test_runEntryPoint.py

            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"])


Alternatively, this one needs to be a real file path

opotowsky · 2022-10-07T20:26:37Z

armi/cli/tests/test_runEntryPoint.py

@@ -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])


Same here, needs to be real since the settingsArgument is required here too

opotowsky · 2022-10-07T20:45:03Z

~~The coverage drop looks like a fluke to me....~~

Nevermind....

john-science · 2022-10-07T21:46:36Z

@opotowsky But I see you fixed the coverage issue! Awesome!

…tional arguments (terrapower#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 terrapower#754 started) * Remove update to real file because that ain't necessary * Release Notes Co-authored-by: Arrielle Opotowsky <c-aopotowsky@terrapower.com>

jakehader requested a review from john-science October 5, 2022 00:25

jakehader mentioned this pull request Oct 5, 2022

Get all options from entry points without parsing arguments #910

Closed

opotowsky added 2 commits October 5, 2022 17:22

Suppress the printing of all possible settings

0b8e18b

Tests pass but one line is commented out

d450fde

opotowsky reviewed Oct 5, 2022

View reviewed changes

armi/cli/tests/test_runEntryPoint.py Outdated Show resolved Hide resolved

opotowsky reviewed Oct 5, 2022

View reviewed changes

armi/cli/tests/test_runEntryPoint.py Outdated Show resolved Hide resolved

opotowsky reviewed Oct 5, 2022

View reviewed changes

armi/cli/tests/test_runEntryPoint.py Show resolved Hide resolved

There are 18 entry points!

cb0144f

john-science reviewed Oct 6, 2022

View reviewed changes

armi/cli/clone.py Show resolved Hide resolved

opotowsky linked an issue Oct 6, 2022 that may be closed by this pull request

Get all options from entry points without parsing arguments #910

Closed

opotowsky added the enhancement New feature or request label Oct 6, 2022

opotowsky added 3 commits October 6, 2022 16:39

Move settings arg processing to not be first

d0db0b5

For some reason, this makes parse_args go bonkers (on my machine, at least). So this seemed like an easy resolution, even if I don't understand it.

Minor docstring add

e51f3c2

opotowsky reviewed Oct 6, 2022

View reviewed changes

armi/cli/checkInputs.py Outdated Show resolved Hide resolved

opotowsky reviewed Oct 6, 2022

View reviewed changes

armi/cli/clone.py Show resolved Hide resolved

opotowsky requested a review from john-science October 6, 2022 22:35

opotowsky added 5 commits October 6, 2022 17:52

Remove reloadDBName setting from armiRun.yaml

249a736

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()).

Merge branch 'main' of https://github.com/terrapower/armi into entryp…

5dbf670

…oint

Update warning to error for checkInputs CLI

57ed9cd

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

ff4496c

Merge branch 'main' of https://github.com/terrapower/armi into entryp…

588b219

…oint

john-science reviewed Oct 7, 2022

View reviewed changes

armi/cli/tests/test_runEntryPoint.py Show resolved Hide resolved

opotowsky added 2 commits October 7, 2022 15:07

Remove update to real file because that ain't necessary

dc0f7cf

Release Notes

5b11868

opotowsky reviewed Oct 7, 2022

View reviewed changes

john-science approved these changes Oct 7, 2022

View reviewed changes

opotowsky merged commit f4bb14b into main Oct 10, 2022

opotowsky deleted the entrypoint branch October 10, 2022 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jakehader commented Oct 5, 2022 •

edited by opotowsky

Loading

opotowsky commented Oct 5, 2022

opotowsky commented Oct 5, 2022

john-science Oct 7, 2022

opotowsky Oct 7, 2022

opotowsky Oct 7, 2022

opotowsky commented Oct 7, 2022 •

edited

Loading

john-science commented Oct 7, 2022

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

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

Conversation

jakehader commented Oct 5, 2022 • edited by opotowsky Loading

Description

Checklist

opotowsky commented Oct 5, 2022

opotowsky commented Oct 5, 2022

john-science Oct 7, 2022

Choose a reason for hiding this comment

opotowsky Oct 7, 2022

Choose a reason for hiding this comment

opotowsky Oct 7, 2022

Choose a reason for hiding this comment

opotowsky commented Oct 7, 2022 • edited Loading

john-science commented Oct 7, 2022

jakehader commented Oct 5, 2022 •

edited by opotowsky

Loading

opotowsky commented Oct 7, 2022 •

edited

Loading