nextflow icon indicating copy to clipboard operation
nextflow copied to clipboard

Modify slurm executor to support cluster [closes #2473]

Open knservis opened this issue 2 years ago • 4 comments

Hi @pditommaso ,

I had a stab at adding the cluster directive. This is my first contact with your project and with Groovy so I hope I did not make to many mistakes.

It is important to note that this is cluster not clusters in the doc as allowing multiple would overly complicate scancel and squeue

knservis avatar Dec 03 '21 15:12 knservis

Thanks for opening this PR. It looks a good start. Not sure to understand the comment below, though

It is important to note that this is cluster not clusters in the doc as allowing multiple would overly complicate scancel and squeue

pditommaso avatar Dec 05 '21 15:12 pditommaso

Thanks for opening this PR. It looks a good start. Not sure to understand the comment below, though

It is important to note that this is cluster not clusters in the doc as allowing multiple would overly complicate scancel and squeue

So if we look at the documentation for sbatch for instance https://slurm.schedmd.com/sbatch.html we can see that -M (long format --clusters) accepts multiple clusters (e.g. zeus,magnus ) and the job gets submitted to the first cluster that has available slots. By not covering that case but just allowing a single cluster, we probably cover most cases everyone cares about without the added complexity of querying squeue to find out where the job was actually submitted.

My comment is a reminder that before merging there should be a NOTICE box in the documentation that informs users about this distinction

knservis avatar Dec 06 '21 02:12 knservis

I have to add that this PR is not yet ready as I seem to be having issues when running this. I will comment back when ready and will keep updating the commit.

knservis avatar Dec 06 '21 02:12 knservis

I have to add that this PR is not yet ready as I seem to be having issues when running this. I will comment back when ready and will keep updating the commit.

Ok @pditommaso this PR has worked for me as it is at the moment. Please have a look

Update: fixed oversight that I found in testing

knservis avatar Dec 06 '21 08:12 knservis