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

Make GPU usage for containers more flexible #674

Merged

Conversation

alejoe91
Copy link
Member

@alejoe91 alejoe91 commented Jun 8, 2022

For some sorters (Ironclust, Kilosort1), GPU usage is optional.
This PR allows one to override the default GPU requirements using sorter parameters. Additionally, it checks if an nvidia gpu is available when needed (and if not it throws an interpretable error)

@alejoe91
Copy link
Member Author

alejoe91 commented Jun 8, 2022

@chyumin can you check this? I tried to make the GPU requirement/usage a bit more flexible

@chyumin
Copy link
Contributor

chyumin commented Jun 8, 2022

@alejoe91 Yes, I'll take a look. Great idea!

@chyumin
Copy link
Contributor

chyumin commented Jun 8, 2022

@alejoe91
Particularly I found this code little confusing, took me a while to understand

requires_gpu looks likes a flag name, but the code it's not using as True/False. Instead, the possible values are None and nvidia string
Also requires_gpu stands for really required. But for some sorters it's actually optional

What do you think about changing this property name to: gpu_capability with three possible values:

  • None
  • 'nvidia-optional'
  • 'nvidia-required'

And adapt the rest of the code of course

Also it'd be good to warn the user when a sorter is gpu-optional but the flag is enabled when raising the Exception message

@alejoe91
Copy link
Member Author

alejoe91 commented Jun 9, 2022

@chyumin thanks for the suggestion! I implemented them, let me know what you think :)

Co-authored-by: Chuang Yu Min <chg.yumin@gmail.com>
Co-authored-by: Garcia Samuel <sam.garcia.die@gmail.com>
@samuelgarcia samuelgarcia merged commit 8143a0d into SpikeInterface:master Jun 10, 2022
@chyumin chyumin mentioned this pull request Jun 14, 2022
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.

None yet

3 participants