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

Updated the Sanger config to list more queues and cover multiple internal clusters #587

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

muffato
Copy link
Member

@muffato muffato commented Nov 8, 2023

Supersedes #555

This config will first query the name of the cluster, to allow defining different values in the rest of the file depending on the cluster.

  1. The list of queues has been expanded to include hugemem and teramem.
  2. I've reduced the threshold of the small queue to 1 minute, as per our HPC guidelines.
  3. The maximum resources can now be defined per cluster, and I've defined different values for tol and farm5.
  4. For farm5, I've added /data to the list of mount points.
  5. For the tol cluster, I've removed all the automatic mounting of /lustre, etc, because we want to make sure that all the inputs are properly defined in the pipelines. Pipelines that do require whole filesystems to be mounted need to define their own singularity.runOptions.

@muffato muffato requested review from maxozo and DLBPointon November 8, 2023 22:49
@muffato muffato self-assigned this Nov 8, 2023
@muffato
Copy link
Member Author

muffato commented Nov 8, 2023

ping @BioinfoTongLI who I can't add as "reviewer" for an unknown reason

@DLBPointon
Copy link
Contributor

This looks good to me @muffato, thanks for taking a look.

@DLBPointon DLBPointon mentioned this pull request Nov 9, 2023
4 tasks
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

LGTM

@muffato
Copy link
Member Author

muffato commented Nov 10, 2023

@maxozo @BioinfoTongLI could you take a look ? This will affect you

@maxozo
Copy link
Member

maxozo commented Nov 10, 2023 via email

@BioinfoTongLI
Copy link

As I am not an extensitve user of all the different queues. I am unsure if the if-else will capture all the use cases. Or is it trying to capture that at all?
I see that it is now not wrapped into withlabel and is default setting. Wouldn't it be safer to wrap this part into withlabel cpu-only?

@muffato
Copy link
Member Author

muffato commented Nov 10, 2023

As I am not an extensitve user of all the different queues. I am unsure if the if-else will capture all the use cases. Or is it trying to capture that at all?

The chain of if-else always has an else and defines a queue for all scenarios.

I see that it is now not wrapped into withlabel and is default setting. Wouldn't it be safer to wrap this part into withlabel cpu-only?

That's the way the configs work, I didn't change that. "CPU-only" is the default mode for processes and is captured by having the queue = {...} directly in the process block. Processes that need GPUs must add the gpu label and are then captured by the withLabel: gpu { ... } block which overrides the default values set above. Moving the default queue settings to a withLabel: cpu_only { ... } block would mean that all existing pipelines would instantly break as they couldn't get their queue settings. I don't want to do that 😅

@BioinfoTongLI
Copy link

thanks for the explanation @muffato! I think the override piece of inofrmation is the missing piece of my knowledge - That sounds then! All good to me!

@muffato muffato merged commit c3c9060 into master Nov 10, 2023
211 checks passed
@muffato muffato deleted the sanger_multifarm branch November 10, 2023 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants