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

tests: run 'cice.run' with '-nomodules' #724

Closed

Conversation

phil-blain
Copy link
Member

PR checklist

  • Short (1 sentence) summary of your PR:
    title
  • Developer(s):
    me
  • 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.
    no code changes, no test suite ran. I tested this works as intended.
  • 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:

Some compiler environments (namely newer version of Intel's oneAPI
toolkit) prevent users form sourcing their initialization script twice,
at least by default.

The CICE test infrastructure currently does this, because
env.$machine_$env is sourced by cice.test and then again by cice.run
(which is ran from cice.test). Sourcing the env file in 'cice.run', when
ran from 'cice.test', is thus unnecessary.

Most env files already support the '-nomodules' flag, which is used by
'cice.setup' when only machine variables are needed, and not a full
compiling environment. Leverage this flag by calling 'cice.run' with
'-nomodules' in 'cice.test'. As a byprodcut, this should make tests run
slightly faster since the environment setup is done only once.

Some compiler environments (namely newer version of Intel's oneAPI
toolkit) prevent users form sourcing their initialization script twice,
at least by default.

The CICE test infrastructure currently does this, because
env.$machine_$env is sourced by cice.test and then again by cice.run
(which is ran from cice.test). Sourcing the env file in 'cice.run', when
ran from 'cice.test', is thus unnecessary.

Most env files already support the '-nomodules' flag, which is used by
'cice.setup' when only machine variables are needed, and not a full
compiling environment. Leverage this flag by calling 'cice.run' with
'-nomodules' in 'cice.test'. As a byprodcut, this should make tests run
slightly faster since the environment setup is done only once.

Closes: CICE-Consortium#695
@apcraig
Copy link
Contributor

apcraig commented May 24, 2022

How does passing -nomodules to cice.run do anything without a modification to the cice.run script? I don't think cice.run uses or checks for arguments.

@phil-blain
Copy link
Member Author

The arguments are passed-through to source env.$machine_$env, so $1 in env.$machine_$env correctly refers to -nomodules from the ./cice.run -nomodules invocation.

@apcraig
Copy link
Contributor

apcraig commented May 24, 2022

That's an interesting feature. So $1, unless reset, just retains it's "last" value. When env.machine_env is sourced, if nothing it passed into it, it doesn't reset the $1, $2, $3 arguments? That seems a little dicey. What if you pass an argument into cice.run that's used in cice.run. Then pass nothing into env.machine_env and it picks up on the $1 in cice.run, which might be completely useless or incorrect? I would have expected the local $n to default to unset by default unless an argument is explicitly set in the execution. Do we really want to leverage/rely on this feature?

@phil-blain
Copy link
Member Author

So I checked the Bash documentation (https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#Bourne-Shell-Builtins) which says:

. (a period)

. filename [arguments]

Read and execute commands from the filename argument in the current shell context. [...] If any arguments are supplied, they become the positional parameters when filename is executed. Otherwise the positional parameters are unchanged.

So for Bash it's a documented behaviour.

But we use csh. Here is what man csh (http://manpages.ubuntu.com/manpages/bionic/man1/bsd-csh.1.html) has to say about the source builtin:

       source name
       source -h name
               The shell reads commands from name.  source commands may be nested; if they
               are nested too deeply the shell may run out of file descriptors.  An error in
               a source at any level terminates all nested source commands.  Normally input
               during source commands is not placed on the history list; the -h option causes
               the commands to be placed on the history list without being executed.

So it is not specified what happens to existing positional arguments, but I understand it to mean that they are implicitely passed. Note that the original csh does not allow any arguments to be passed explicitely to source.

tcsh (http://manpages.ubuntu.com/manpages/bionic/man1/tcsh.1.html) does not add that much info, but it does allow positional arguments to be explicitely passed:

   source [-h] name [args ...]
           The shell reads and executes commands from name.  The commands are not  placed  on
           the  history  list.   If  any args are given, they are placed in argv.  (+) source
           commands may be nested; if they are nested too deeply the shell  may  run  out  of
           file  descriptors.  An error in a source at any level terminates all nested source
           commands.  With -h, commands are placed on  the  history  list  instead  of  being
           executed, much like `history -L'.

So I think we can safely use that feature.

@apcraig
Copy link
Contributor

apcraig commented May 26, 2022

Good to know this feature is likely to be robust. I guess another question is do we really want to leverage this feature. It seems to lack transparency and may cause confusion later on. I would almost prefer if cice.run had an explicit check if -nomodules was passed and if so, that it would be passed to "source env.machine_env" explicitly. Thoughts?

@phil-blain
Copy link
Member Author

phil-blain commented May 26, 2022

It depends if we want to drop support for the original csh and just claim outright that our scripts need tcsh to work, because the "original" csh does not accept additional arguments to its source builtin (as the man page excerpts above mention).

I would not be opposed to that as along as all shebangs are changed to #!/usr/bin/tcsh or maybe better #!/usr/bin/env tcsh, but then we might need #!/usr/bin/env -S tcsh -fif we want to keep -f, and then we need env from GNU coreutils 8.30 or newer (EDIT because that's where the -S flag` which allows passing additional arguments to the program name was added).

@apcraig
Copy link
Contributor

apcraig commented May 26, 2022

OK, so one question is whether we are even allowed to pass arguments with csh. What we're doing now should not work. Our scripts are #!/bin/csh but we pass arguments into them. It's working only because it's either using tcsh under the covers or because the implementation of csh under the covers allows it. Or maybe the argument is ignored and we can't tell.

How much risk is there to requiring tcsh in terms of portability? Since the current "csh" seems to be working OK, should we fix this?

A separate question still seems to be whether we want to explicitly or implicitly pass the arguments down the calling tree. I do think I prefer explicit. That would mean adding something like the following to cice.run

set nomodules = ""
if ($#argv == 1) then
  if ($1 == "-nomodules") then
    set nomodules = $1
  endif
endif

.....

source ./env.\${ICE_MACHCOMP} ${nomodules} || exit 2

or similar. Thoughts on both issues?

@phil-blain
Copy link
Member Author

phil-blain commented May 26, 2022

Passing arguments to scripts does work in csh, it's passing additional arguments to the source builtin that only works in tcsh i.e.

# this works in csh and tcsh
source some_file
# this only works in tcsh
source some_file -some_argument

@apcraig
Copy link
Contributor

apcraig commented Jun 3, 2022

So all our scripts are basically setup as csh, but may be running tcsh under the covers. The -nomodules option is mainly useful to speed up running large test suites. I don't think most users do that.

Let me circle back again. Is this to help performance? What does "Some compiler environments (namely newer version of Intel's oneAPI toolkit) prevent users form sourcing their initialization script twice, at least by default" mean? Is there a failure or problem resetting the env with Intel oneAPI?

@phil-blain
Copy link
Member Author

What does "Some compiler environments (namely newer version of Intel's oneAPI toolkit) prevent users form sourcing their initialization script twice, at least by default" mean? Is there a failure or problem resetting the env with Intel oneAPI?

It means that the Intel initialization script detects it is being sourced a second time, and aborts, yes. You can force it to do it with a special flag though.

But since I opened this PR, our CS departement has added additional methods to load the compiler, so these changes are not needed for me anymore. So we can drop it if you prefer.

@apcraig
Copy link
Contributor

apcraig commented Jun 13, 2022

Thanks @phil-blain. At this point, if the feature is not needed, I guess I prefer we just close this PR. I think there is a bigger issue about scripting language and overall implementation. @phil-blain, do you think we should switch to bash or some other language/tool? Is it time to review the current scripts and do some refactoring? Maybe we need a new issue where we can accumulate additional script ideas and then find a time to update? This could include a variety of things including #674, for instance.

@phil-blain
Copy link
Member Author

I'm ambivalent about switching tools / languages. It might be quite an involved process...

@phil-blain
Copy link
Member Author

I'll close this PR.

@phil-blain phil-blain closed this Jun 13, 2022
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.

2 participants