-
Notifications
You must be signed in to change notification settings - Fork 192
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
Template: Add gpu
profile
#3272
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files☔ View full report in Codecov by Sentry. |
Co-authored-by: Sateesh_Peri <[email protected]>
Co-authored-by: Sateesh_Peri <[email protected]>
So the flow of information in the scdownstream pipeline looks as following:
This implementation is the best I could come up with so far, but not many really "senior" people have had a look at it AFAIK. Maybe we can discuss which elements of the suggested approach should be part of the template and where we can perform some improvements. |
Another thought: Should this not be an optional part of the template? |
it's teeny-tiny enough, that we might keep it in, imo, similar to |
But is the goal to only add the profile to the template? I think it would at least be nice to have a common structure for all GPU-enabled modules, even if it only means adding the |
Thank you @nictru
The I don't think we need a separate
In the scdownstream pipeline, I think we only need the process {
withLabel: 'use_gpu' {
ext.use_gpu = { params.use_gpu }
}
} At pipeline execution, the user must set the
Thank you. This is very helpful.
This is quite clever. Nice! |
Hey @GallVp, thanks for the input. Some notes: It might sound stupid, but I did not know there was a difference between I am not fully aware about what you can and can't do with
I would prefer if setting the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not approving yet, because we should first decide what the implementation should look like and add all necessary parts
No, How about we get rid of both the variable and the parameter? Instead, we modify the profile as, gpu {
process {
withLabel: process_gpu {
ext.use_gpu = true
}
}
docker.runOptions = '-u $(id -u):$(id -g) --gpus all'
apptainer.runOptions = '--nv'
singularity.runOptions = '--nv'
} The workflows can use The above solution, however, does not take care of the |
Hey, I just tested the suggested approach and it seems to work as expected. I did this using the following:
So that we don't need a config variable. But this is more a cosmetic topic and not a hill-to-die-on. |
feel free to add the changes, @nictru |
Co-authored-by: Nico Trummer <[email protected]>
I agree. This is more in line with the existing infrastructure. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy now :)
Needs opt-in functionality in the |
No description provided.