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.

Sign up for GitHub

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

Update usage of set -ex #179

Merged
merged 1 commit into from
Oct 25, 2023
Merged

Update usage of set -ex #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions benchcab/utils/pbs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ def render_job_script(
#PBS -m e
#PBS -l storage={'+'.join(storage_flags)}

set -ex

module purge
{module_load_lines}

set -ev
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


{benchcab_path} fluxsite-run-tasks --config={config_path} {verbose_flag}
{'' if skip_bitwise_cmp else f'''
{benchcab_path} fluxsite-bitwise-cmp --config={config_path} {verbose_flag}
Expand Down
20 changes: 10 additions & 10 deletions tests/test_pbs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ def test_default_job_script(self):
#PBS -m e
#PBS -l storage=gdata/ks32+gdata/hh5

set -ex

module purge
module load foo
module load bar
module load baz

set -ev

/absolute/path/to/benchcab fluxsite-run-tasks --config=/path/to/config.yaml

/absolute/path/to/benchcab fluxsite-bitwise-cmp --config=/path/to/config.yaml
Expand Down Expand Up @@ -60,13 +60,13 @@ def test_verbose_flag_added_to_command_line_arguments(self):
#PBS -m e
#PBS -l storage=gdata/ks32+gdata/hh5

set -ex

module purge
module load foo
module load bar
module load baz

set -ev

/absolute/path/to/benchcab fluxsite-run-tasks --config=/path/to/config.yaml -v

/absolute/path/to/benchcab fluxsite-bitwise-cmp --config=/path/to/config.yaml -v
Expand Down Expand Up @@ -94,13 +94,13 @@ def test_skip_bitwise_comparison_step(self):
#PBS -m e
#PBS -l storage=gdata/ks32+gdata/hh5

set -ex

module purge
module load foo
module load bar
module load baz

set -ev

/absolute/path/to/benchcab fluxsite-run-tasks --config=/path/to/config.yaml

"""
Expand Down Expand Up @@ -132,13 +132,13 @@ def test_pbs_config_parameters(self):
#PBS -m e
#PBS -l storage=gdata/ks32+gdata/hh5+gdata/foo

set -ex

module purge
module load foo
module load bar
module load baz

set -ev

/absolute/path/to/benchcab fluxsite-run-tasks --config=/path/to/config.yaml

"""
Expand All @@ -165,13 +165,13 @@ def test_default_pbs_config(self):
#PBS -m e
#PBS -l storage=gdata/ks32+gdata/hh5

set -ex

module purge
module load foo
module load bar
module load baz

set -ev

/absolute/path/to/benchcab fluxsite-run-tasks --config=/path/to/config.yaml

"""
Expand Down