Skip to content

first commit - minerva hpc profile for icahn school of medicine at mo…#876

Merged
ctastad merged 7 commits intonf-core:masterfrom
ctastad:master
May 2, 2025
Merged

first commit - minerva hpc profile for icahn school of medicine at mo…#876
ctastad merged 7 commits intonf-core:masterfrom
ctastad:master

Conversation

@ctastad
Copy link
Copy Markdown
Contributor

@ctastad ctastad commented Apr 3, 2025

first commit - minerva hpc profile for icahn school of medicine at mount sinai


name: New Config - MSSM
about: Minerva HPC at Icahn School of Medicine at Mount Sinai

Please follow these steps before submitting your PR:

  • If your PR is a work in progress, include [WIP] in its title
  • [ x] Your PR targets the master branch
  • You've included links to relevant issues, if any

Steps for adding a new config profile:

  • [ x] Add your custom config file to the conf/ directory
  • [ x] Add your documentation file to the docs/ directory
  • [ x] Add your custom profile to the nfcore_custom.config file in the top-level directory
  • [ x] Add your custom profile to the README.md file in the top-level directory
  • [ x] Add your profile name to the profile: scope in .github/workflows/main.yml
  • OPTIONAL: Add your custom profile path and GitHub user name to .github/CODEOWNERS (**/<custom-profile>** @<github-username>)

Copy link
Copy Markdown
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Wow really clean PR! Nothing jumps out at me.

My only comment is on the process_low_gpu process label, I've not seen that before - but I've not handled gpu modules. Have you got a ln example nf-core module with that (just so I know)?

@ctastad
Copy link
Copy Markdown
Contributor Author

ctastad commented Apr 3, 2025

My only comment is on the process_low_gpu process label, I've not seen that before - but I've not handled gpu modules. Have you got a ln example nf-core module with that (just so I know)?

I made an assumption about the naming convention there, but this is also a product of our hardware and queue situation on this machine. We have a disparity between a large number of very new cards along side a smattering of old ones. As a result, the available resources between the main and express gpu queues are quite disproportionate. As much work should be sent to the main queue as possible, but some workflows need the hardware compatibility of the "low" routing (e.g. cellbender in scrnaseq v4.0). I've found having a hook like this can make it easier to supplement a custom.config.

If there's a recommended alternative to this, I would be open to that.

@jfy133
Copy link
Copy Markdown
Member

jfy133 commented Apr 4, 2025

My only comment is on the process_low_gpu process label, I've not seen that before - but I've not handled gpu modules. Have you got a ln example nf-core module with that (just so I know)?

I made an assumption about the naming convention there, but this is also a product of our hardware and queue situation on this machine. We have a disparity between a large number of very new cards along side a smattering of old ones. As a result, the available resources between the main and express gpu queues are quite disproportionate. As much work should be sent to the main queue as possible, but some workflows need the hardware compatibility of the "low" routing (e.g. cellbender in scrnaseq v4.0). I've found having a hook like this can make it easier to supplement a custom.config.

If there's a recommended alternative to this, I would be open to that.

You can see the valid labels here: https://github.com/nf-core/tools/blob/52e810986e382972ffad0aab28e94f828ffd509b/nf_core/pipeline-template/conf/base.config#L29-L54

To my knowledge, Nextflow specifies GPU nodes via the 'accelerator' directive. I think you can just specify task.accelerator && task.time <= 12.h for example to select tasks that are 1. requesting GPU nodes and 2. has a time less than 12.h , for example.

But we are actually increasing the number of GPU modules in nf-core, so I will bring this up so we come up with a standardised way (probably with GPU specific labels, but we need to specify this)

@ctastad
Copy link
Copy Markdown
Contributor Author

ctastad commented Apr 7, 2025

Thanks @jfy133 for the help and feedback on this. Also, I'm not sure if the merge process is automated or if another review is needed. Are there any other actions on my part to complete the PR?

thank you,
chris

@maxulysse
Copy link
Copy Markdown
Member

Thanks @jfy133 for the help and feedback on this. Also, I'm not sure if the merge process is automated or if another review is needed. Are there any other actions on my part to complete the PR?

thank you, chris

Once you get the ✔️ you're free to merge.

@jfy133
Copy link
Copy Markdown
Member

jfy133 commented Apr 8, 2025

You can merge when ready, but like I said - currently the process_low_gpu labels are currently useless:

      } else if (task.label && task.label.toString().contains('process_low_gpu')) {
            return 'gpuexpress'
        } else if (task.label && task.label.toString().contains('gpu')) {
            return 'gpu'
            ```

Comment thread conf/mssm.config Outdated
Comment thread conf/mssm.config Outdated
Copy link
Copy Markdown
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Just FYI we have settled on for process_gpu as an initial label (now merged into the upcoming nf-core pipeline tempalte) which may be extended to process_gpu_single process_gpu_low process_gpu_medium etc for the future.

So I think your .contains('gpu') is a good system - however the withLabel definition maybe reudundant in the future as this definiton is also in the dev version of the nf-core template (but fine now)

One thing I now see that is missing is the resourceLimits scope and max_*
parameters

See this section: https://nf-co.re/docs/tutorials/use_nf-core_pipelines/writing_institutional_profiles#process-scope

And the note in the section above for teh max_params

Comment thread conf/mssm.config
Comment thread conf/mssm.config Outdated
@ctastad
Copy link
Copy Markdown
Contributor Author

ctastad commented May 1, 2025

@jfy133 Do you have the authority to approve this github org membership request? I'm seeing in the docs that this is necessary for me to complete the merge.

https://nfcore.slack.com/archives/CEB982K2T/p1746031064368849

@ctastad ctastad merged commit eda67d8 into nf-core:master May 2, 2025
150 checks passed
@jfy133
Copy link
Copy Markdown
Member

jfy133 commented May 2, 2025

Sorry! Was on holiday, I see you got it in!

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.

3 participants