Skip to content
This repository was archived by the owner on Oct 10, 2019. It is now read-only.

Check workdir and input files before job submission (SOFTWARE-3154) #69

Merged
merged 2 commits into from
Mar 15, 2018
Merged

Check workdir and input files before job submission (SOFTWARE-3154) #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55a09bd
Check workdir and input files before job submission
brianhlin Feb 27, 2018
fda81be
Address review comments
brianhlin Mar 13, 2018
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
15 changes: 13 additions & 2 deletions src/scripts/blah_common_submit_functions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,8 +768,10 @@ function bls_finish_job_wrapper ()
fi
}

function bls_test_working_dir ()
function bls_test_input_files ()
{
# Verify the workdir can be accessed before submitting the job. If a bogus workdir is
# given, the job is hopeless
if [ "x$bls_opt_workdir" != "x" ]; then
cd $bls_opt_workdir
elif [ "x$blah_set_default_workdir_to_home" == "xyes" ]; then
Expand All @@ -782,13 +784,22 @@ function bls_test_working_dir ()
rm -f $bls_tmp_file
exit 1
fi

# Ensure local files actually exist. When called before job submission, this prevents
# unnecessary churn on the scheduler if the files don't exist.
if ! bls_fl_test_exists inputsand ; then
Copy link

@edquist edquist Mar 13, 2018

Choose a reason for hiding this comment

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

Previously, this bls_fl_test_exists inputsand check was only in slurm_submit.sh.

Just to confirm: is it intentional that now we will do this check in the other submit scripts also? ({blah,lsf,pbs,sge}_submit.sh)

Copy link
Member Author

@brianhlin brianhlin Mar 13, 2018

Choose a reason for hiding this comment

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

Yup, we want to apply these checks across all batch system submissions

echo "Input sandbox file doesn't exist: $bls_fl_test_exists_result" >&2
echo Error # for the sake of waiting fgets in blahpd
rm -f "$bls_tmp_file"
exit 1
fi
}

function bls_add_job_wrapper ()
{
bls_test_input_files
bls_start_job_wrapper >> $bls_tmp_file
bls_finish_job_wrapper >> $bls_tmp_file
bls_test_working_dir
Copy link

@edquist edquist Mar 13, 2018

Choose a reason for hiding this comment

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

Note that previously, bls_test_working_dir was getting run after $bls_tmp_file was written to, and then deleting $bls_tmp_file upon error. Now bls_test_input_files is run beforehand, but still removing the file on error. Not that it's a problem, but does this file get created somewhere else first? Or is this spot in bls_add_job_wrapper the first?

Copy link
Member Author

@brianhlin brianhlin Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

See the description of the PR:

Use the pre-existing workdir test function, which catches permission
issues with the workdir as well as non-existence. Do this all before
we write out the job wrapper because why bother?

But yes, bls_tmp_file gets created in bls_setup_all_files, which is called long before bls_add_job_wrapper

Copy link

@edquist edquist Mar 13, 2018

Choose a reason for hiding this comment

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

I guess my main question and point (which i did not find the answer to in the PR description) are:

does this file [$bls_tmp_file] get created somewhere else first? Or is this spot in bls_add_job_wrapper the first?

Because:

  • if it gets created somewhere else first, then it still makes sense to delete it in bls_test_input_files
  • if the above two lines in bls_add_job_wrapper are the first place where $bls_tmp_file is created/written to, then it is superfluous to do rm -f $bls_tmp_file in bls_test_input_files, as it will never yet exist at that point.

Copy link

@edquist edquist Mar 13, 2018

Choose a reason for hiding this comment

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

Ok, then it makes sense as-is.

}

function bls_set_up_local_and_extra_args ()
Expand Down
16 changes: 0 additions & 16 deletions src/scripts/slurm_submit.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,6 @@ if [[ $bls_opt_mpinodes -gt 1 ]] ; then
echo "#SBATCH --cpus-per-task=$bls_opt_mpinodes" >> $bls_tmp_file
fi

# Verify the workdir exists before submitting the job. If a bogus workdir is
# given, the job is hopeless
if [[ ! -z "$bls_opt_workdir" && ! -d "$bls_opt_workdir" ]] ; then
echo "Error: Workdir doesn't exist" >&2
echo Error # for the sake of waiting fgets in blahpd
exit 1
fi

# Ensure local files actually exist before submitting job. This prevents
# unnecessary churn on the scheduler if the files don't exist.
if ! bls_fl_test_exists inputsand ; then
echo "Input sandbox file doesn't exist: $bls_fl_test_exists_result" >&2
echo Error # for the sake of waiting fgets in blahpd
exit 1
fi

# Do the local and extra args after all #SBATCH commands, otherwise slurm ignores anything
# after a non-#SBATCH command
bls_set_up_local_and_extra_args
Expand Down