Skip to content

Add flexibility to cice.setup for test suites#357

Closed
phil-blain wants to merge 3 commits into
CICE-Consortium:masterfrom
phil-blain:suite-no-submit
Closed

Add flexibility to cice.setup for test suites#357
phil-blain wants to merge 3 commits into
CICE-Consortium:masterfrom
phil-blain:suite-no-submit

Conversation

@phil-blain
Copy link
Copy Markdown
Member

@phil-blain phil-blain commented Aug 29, 2019

PR checklist

  • Short (1 sentence) summary of your PR:
    Add --no-submit and --no-build options to cice.setup
  • Developer(s):
    Philippe Blain
  • Suggest PR reviewers from list in the column to the right. @apcraig
  • Please copy the PR test results link or provide a summary of testing completed below.
    No changes to code, so I did not perform the full base_suite. I tested to check that the new options work as they should
  • 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/.)
  • Please provide any additional information or relevant details below:

I added two options to cice.setup (for test suites) that I considered useful for my workflow.

  1. --no-submit : this creates the test case folders and compiles the executables but does not submit the jobs. This is useful when doing both a baseline + test (regression test) : I can use ./cice.setup to setup, build and submit the 'baseline' base_suite and then ./cice.setup --no-submit to setup and build the 'test' base_suite (I don't want to submit it immediately since the 'baseline' base_suite might not be finished running, so some tests in the 'test' base_suite might run before the corresponding ones in the 'baseline' suite, which would make them fail). The 'test' suite can then be manually submitted with the existing suite.submit script.
  2. --no-build : this only creates the test case folders. The tests can then be built with the new script suite.build and then submitted with suite.submit.

Do you think that the options would be better named --build-only (instead of --no-submit) and --setup-only (instead of --no-build) ?

Copy link
Copy Markdown
Contributor

@mattdturner mattdturner left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I like the additional capability of only creating the case directories and also the option to not submit to the queue. Generally I would prefer --setup-only and --build-only, but that's just personal preference.

@apcraig apcraig self-requested a review August 30, 2019 18:56
@apcraig
Copy link
Copy Markdown
Contributor

apcraig commented Sep 2, 2019

I very much like this new feature. I think I also prefer --setup-only and --build-only as the options.

But I would like to propose a slightly different strategy. With the current implementation, we are generating suite.build, suite.run. and suite.submit scripts. These scripts mostly overlap. What if we created a single script that could handle the different options. What I envision is a single suite.submit script that would either have a few command line options or variables set at the top of the script that would be implemented something like as follows

set dobuild=true
set dosubmit=true
set dorun=true

if (dosubmit == 'true' && dorun == 'true') exit -2

echo "-------test--------------"
echo "conrad_intel_restart_gx3_4x2_debug_diag1_run5day.xs01"
cd conrad_intel_restart_gx3_4x2_debug_diag1_run5day.xs01
source ./cice.settings
if (dobuild == 'true') then
  set ciceexe = "../ciceexe.${ICE_COMPILER}.${ICE_COMMDIR}.${ICE_BLDDEBUG}.${ICE_THREADED}.${ICE_IOTYPE}"
  ./cice.build ${ciceexe}
  if !(-e ${ciceexe}) cp -p ${ICE_RUNDIR}/cice ${ciceexe}
endif
if (dorun == "true") then
  ./cice.test
endif
if (dosubmit == 'true') then
  ./cice.submit | tee -a ../suite.jobs
endif
cd ..

There are a few benefits. First, we avoid having to deal with the buildincremental issues. Rather than having each case have to turn on incremental builds for the suite to behave in a reasonable way, the suite.submit script would cleanly handle this and the incremental (or not) build would be purely up to the user whether they want to use it or not for other reasons. Second, we would merge 3 scripts to 1 without the redundancy between scripts. Third, it may provide easier future extensions to have everything in one script.

The cice.setup would still take --no-build --no-submit options which would then be converted to defaults in the script and then the suite.submit script would be run with the flags set appropriately.

I have suggested and prefer that we use variables in the script to set the flags rather than command line arguments to suite.submit just because the implementation is easier. I also think these features are not going to be used all that much. But I am open to further discussion. Any thoughts on this proposal?

@phil-blain
Copy link
Copy Markdown
Member Author

phil-blain commented Sep 4, 2019

I opted for the easiest implementation. Regarding the fact that we have 3 scripts: since they are not version controlled but simply created by cice.setup, I don't think it creates much redundancy...
Also, I think that having 3 scripts is easier from a user's point of view since each script is named after its purpose, and mimic the scripts in a case directory.
I think it's easier and makes more sense from a UI/UX perspective to run suite.build to compile rather than editing suite.setup to turn on the corresponding variable and then run suite.submit to compile, then later edit it again to submit...
Also, since as you say these features are not going to be used all that much I think the fact that the buildincremental preset has to be chosen is not much of an issue.

@apcraig
Copy link
Copy Markdown
Contributor

apcraig commented Sep 4, 2019

@phil-blain, that makes some sense. But one issue I have is how the buildincremental needs to be turned on for all this to work OK. I think that's a complicating factor. Without using that flag, these new scripts don't provide much use.

As an alternative, If we had a script that just built, just submitted, or did both, that would remove the buildincremental factor. But then there is also the ability to run interactively or run in batch which you also seem to want to have. Again, I think very few users are going to use any of this which is why I also think it makes sense for it to be somewhat hidden in one big script. If it's not hidden in one script, then I think we need to probably have separate .build (just build), .submit (just submit), .run (just run), .build_run (build and run), and .build_submit (build and submit). I think this is a little over the top. The point of having the build_run and build_submit is so you can build and run/submit interleaved which is how you want to do it in practice if you are doing it together. You don't want to build everything then submit everything as that slows the runs down quite a bit. If it's all in one script, we can support any kinds of modes we want easier.

I can see advantages and disadvantages in both approaches. I also understand that these scripts are generated so the redundancy is less of an issue. But that redundancy has to be maintained in the generation of the scripts, so there is some cost there. I still prefer the idea that everything is in one script in large part because I think most people will just run the test suites fully automated and never use the other scripts. But again, that might argue that most folks will never see the scripts anyway.

@phil-blain
Copy link
Copy Markdown
Member Author

I see your point. I just don't link having to hand-edit the script to change it's behavior. If we don't want to do argument-handling in this script, would just using environment variables work for you ? for example depending on the options chosen when calling cice.setup we would have
cice.setup :

setenv SUITE_BUILD=${dobuild}
setenv SUITE_RUN=${dorun}
setenv SUITE_SUBMIT=${dosubmit}
./suite.submit

and then suite.submit would just reference these variables and when we want to use the script in other ways we set these variables on the command line:
SUITE_BUILD=true ./suite.submit
or something link that.
The defaults would be SUITE_BUILD=true, SUITE_RUN=false, SUITE_SUBMIT=true.

Or go with arguments handling...

@apcraig
Copy link
Copy Markdown
Contributor

apcraig commented Sep 5, 2019

I think env variables is a great idea. So, just to suggest a couple other changes.

  • How about if we rename the script to suite_script.csh or something like it?
  • The script defaults will always be the same. If a user invokes --no-build or --no-submit then those flags would be converted to an internal variable in cice.setup and then a setenv command is executed before the suite_script is run. My suggestion for implementation would be to have the defaults in cice.setup be the same as the script and just always invoke it with the env variables.
  • The script needs to check for existance of the env variables before using them like
  set dobuild = "true"
  if ($?SUITE_BUILD) set dobuild = "${SUITE_BUILD}"
  • We should echo dobuild, dorun, and dosubmit at the top of suite_script.csh
  • I think it's fine to keep the implementation simple such that "true" is the only matched value. Anything else (TRUE, false, tru, xyz, etc) will not match "true" in the script. If anyone else thinks we need some checks (like that true and false are the only valid values), I would be open to something like that too, but am not convinced it's critical. I prefer to avoid a bunch of code to support some superset of valid values at this point. If we find that usability is an issue in the community, we can extend it in the future.

@apcraig
Copy link
Copy Markdown
Contributor

apcraig commented Jan 15, 2020

See #395, closing now.

@apcraig apcraig closed this Jan 15, 2020
@phil-blain phil-blain deleted the suite-no-submit branch February 12, 2020 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants