Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cice.setup: allow command line to override suite options #745

Merged

Conversation

phil-blain
Copy link
Member

PR checklist

  • Short (1 sentence) summary of your PR:
    title
  • Developer(s):
    PB
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    I ran a few test suites (decomp, nothread) with additional command line options (-s dynpicard) and verified that all test cases were correctly set to use kdyn=3, and that the bfbcomp cases were comparing against the correct test name.
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

This changes the behaviour of cice.setup to give higher priority to command-line options. I made these changes to support my testing of the VP solver and thought it made sense to submit a PR, since I think in general it's common UX for command line tools to give precedence to the command line over other configuration.

If we do not want these changes no problem, I'll keep them locally for when I need them.

Full commit messages follow:
b19149d: cice.setup: allow command line to override suite options

Options chosen on the 'cice.setup' command line (using the '-s' flag) are
added to the options defined for each test in the test suite definition
file, when running a test suite.

This is implemented by appending the options from the test suite (in
variable 'sets_tmp') to the options from the command line ('sets_base')
in the variable 'sets', which is ultimately (via the variable 'setsx')
looped through to apply each option.

Since 'sets_tmp' is appended to 'sets_base', options on the command line
can't override options from the test suite file, which means one can't
e.g. run a test suite with 'kdyn=3' and expect all tests to use this
option if any option specified in the test suite set 'kdyn' to some
other value.

To allow options from the command line to override options from the test
suite, reverse the order in which 'sets_tmp' and 'sets_base' are used to
define 'sets'. This is in line with the common behaviour of the command
line taking precedence.

Adjust the documentation accordingly, fixing a typo along the way.

66ec2d1: cice.setup: name test suite cases less ambiguously

In the previous commit, we allowed options from the command line to
override options from the test suite definition file. However, test case
directories are still named using a sorted list of all active options,
both from command line and the suite definition file (variable
'setsarray'). This is nonoptimal since it is not clear from looking at
the test directory name which options have precedence in case of
conflict.

Change the naming logic so that options from the command line are last
in the test directory name, in a "last-one-wins" fashion. To do that,
let 'setsarray' be defined from the test suite options ('sets_tmp') and
add a second loop for the command line options ('sets_base').

Note that we do not check if the same option is mentioned both in the
test suite and the command line, in order not to complicate the code
further.

This also allows greatly simplifying the logic used to adjust 'bfbcomp'
test names to include command line options. This part of the code is
checking if the options for the test aginst which to compare contain
duplicates and 'none', which is unnecessary since these options come
directly from the test suite definition file.

Options chosen on the 'cice.setup' command line (using the '-s' flag) are
added to the options defined for each test in the test suite definition
file, when running a test suite.

This is implemented by appending the options from the test suite (in
variable 'sets_tmp') to the options from the command line ('sets_base')
in the variable 'sets', which is ultimately (via the variable 'setsx')
looped through to apply each option.

Since 'sets_tmp' is appended to 'sets_base', options on the command line
can't override options from the test suite file, which means one can't
e.g. run a test suite with 'kdyn=3' and expect all tests to use this
option if any option specified in the test suite set 'kdyn' to some
other value.

To allow options from the command line to override options from the test
suite, reverse the order in which 'sets_tmp' and 'sets_base' are used to
define 'sets'. This is in line with the common behaviour of the command
line taking precedence.

Adjust the documentation accordingly, fixing a typo along the way.
In the previous commit, we allowed options from the command line to
override options from the test suite definition file. However, test case
directories are still named using a sorted list of all active options,
both from command line and the suite definition file (variable
'setsarray'). This is nonoptimal since it is not clear from looking at
the test directory name which options have precedence in case of
conflict.

Change the naming logic so that options from the command line are last
in the test directory name, in a "last-one-wins" fashion. To do that,
let 'setsarray' be defined from the test suite options ('sets_tmp') and
add a second loop for the command line options ('sets_base').

Note that we do not check if the same option is mentioned both in the
test suite and the command line, in order not to complicate the code
further.

This also allows greatly simplifying the logic used to adjust 'bfbcomp'
test names to include command line options. This part of the code is
checking if the options for the test aginst which to compare contain
duplicates and 'none', which is unnecessary since these options come
directly from the test suite definition file.
Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems fine to me, but I'll defer to @apcraig for the review.

@apcraig
Copy link
Contributor

apcraig commented Aug 2, 2022

This looks good to me. I think it's a better way to handle the priority of the options overall. And I like the idea of separating the options in the suite files and options on the command line cleanly. This will create unique case names when there is an overlap in the ts and -s setup. Thanks @phil-blain.

@apcraig apcraig merged commit 26db2c3 into CICE-Consortium:main Aug 2, 2022
@phil-blain
Copy link
Member Author

Great! thanks for the quick review and merge.

@phil-blain phil-blain deleted the cice-setup-cmdline-override branch August 8, 2022 13:36
dabail10 pushed a commit to ESCOMP/CICE that referenced this pull request Oct 4, 2022
…tium#745)

* cice.setup: allow command line to override suite options

Options chosen on the 'cice.setup' command line (using the '-s' flag) are
added to the options defined for each test in the test suite definition
file, when running a test suite.

This is implemented by appending the options from the test suite (in
variable 'sets_tmp') to the options from the command line ('sets_base')
in the variable 'sets', which is ultimately (via the variable 'setsx')
looped through to apply each option.

Since 'sets_tmp' is appended to 'sets_base', options on the command line
can't override options from the test suite file, which means one can't
e.g. run a test suite with 'kdyn=3' and expect all tests to use this
option if any option specified in the test suite set 'kdyn' to some
other value.

To allow options from the command line to override options from the test
suite, reverse the order in which 'sets_tmp' and 'sets_base' are used to
define 'sets'. This is in line with the common behaviour of the command
line taking precedence.

Adjust the documentation accordingly, fixing a typo along the way.

* cice.setup: name test suite cases less ambiguously

In the previous commit, we allowed options from the command line to
override options from the test suite definition file. However, test case
directories are still named using a sorted list of all active options,
both from command line and the suite definition file (variable
'setsarray'). This is nonoptimal since it is not clear from looking at
the test directory name which options have precedence in case of
conflict.

Change the naming logic so that options from the command line are last
in the test directory name, in a "last-one-wins" fashion. To do that,
let 'setsarray' be defined from the test suite options ('sets_tmp') and
add a second loop for the command line options ('sets_base').

Note that we do not check if the same option is mentioned both in the
test suite and the command line, in order not to complicate the code
further.

This also allows greatly simplifying the logic used to adjust 'bfbcomp'
test names to include command line options. This part of the code is
checking if the options for the test aginst which to compare contain
duplicates and 'none', which is unnecessary since these options come
directly from the test suite definition file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants