Update usage of set -ex

[SeanBryan51]

The PBS job script produces a lot of excessive output from the module purge and module load <module> commands. This can be fixed easily by enabling the set options after the module commands.

Replace xtrace mode -x with verbose mode -v as the job script is not complex.

Fixes #174

[codecov]

Codecov Report

Merging #179 (d1192c9) into main (b8a92f9) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #179   +/-   ##
=======================================
  Coverage   86.74%   86.74%           
=======================================
  Files          27       27           
  Lines        1396     1396           
=======================================
  Hits         1211     1211           
  Misses        185      185           

Files	Coverage Δ
benchcab/utils/pbs.py	100.00% <ø> (ø)
tests/test_pbs.py	100.00% <ø> (ø)

[bschroeter]

I think we should have a bit more of a discussion around this and decide as a group how much verbosity is acceptable.

[bschroeter]

Are we sure we want to do this? To my understanding, set -v only outputs commands as they are read by the interpreter, not as they are executed as with set -x. The level of tractability in errors will likely be reduced by switching these flags...

Just throwing that wrench in there for discussion.

[SeanBryan51]

I think set -x is useful when debugging complex bash scripts but I thought it was a bit overkill for it to be enabled by default for our use case (the only difference I see is -x prints the $PS4 variable, and -v does not).

[ccarouge]

I agree for now it looks like the difference between set -x and set -v is only the $PS4 variable. @bschroeter do you have an example to show if -x is better than -v. At the same time, if the difference is that small, I don't care which one is used.

But I am wondering if we should keep set -e at the start of the script. Otherwise, if a module load fails, it will keep going through and possibly never fail (if it fails when running CABLE, currently it does not fail) or only fail quite some time afterwards. We could move set -v/set -x only before the benchcab commands.

[bschroeter]

@ccarouge, I'll see if I can find a test that compares the two. I'm also of the opinion that if the difference is negligible then it doesn't really matter that much.

[bschroeter]

Internet gives us the following answer:

-v
Print shell input lines as they are read.

-x
Print a trace of simple commands, for commands, case commands, select commands, and arithmetic for commands and their arguments or associated word lists after they are expanded and before they are executed. The value of the PS4 variable is expanded and the resultant value is printed before the command and its expanded arguments.

Personally I prefer to get in the habit of setting -ex purely to get the exact place and stack trace of the error, with more information regarding the nature of the failure. We used to use this (and pipefail) excessively at BNOC. However, I accept that it might not be necessary here and is adding to noise.

I'll leave it up to your discretion on which way to go. I'm happy either way.

[SeanBryan51]

But I am wondering if we should keep set -e at the start of the script. Otherwise, if a module load fails, it will keep going through and possibly never fail (if it fails when running CABLE, currently it does not fail) or only fail quite some time afterwards. We could move set -v/set -x only before the benchcab commands.

I don't see this as too big of a deal as we can let benchcab crash if the module load step fails. I'm happy to keep the strict error checking for executing benchcab specific commands and not for external dependencies like environment modules.

[ccarouge]

Sorry for the delay in coming back to this.

Let's start with what is currently in the code. We can change later if we have a use case for it.