Skip to content

Add more container dependency checks in run_sorter#3024

Merged
samuelgarcia merged 14 commits intoSpikeInterface:mainfrom
JoeZiminski:more_docker_dependency_checks
Jun 28, 2024
Merged

Add more container dependency checks in run_sorter#3024
samuelgarcia merged 14 commits intoSpikeInterface:mainfrom
JoeZiminski:more_docker_dependency_checks

Conversation

@JoeZiminski
Copy link
Copy Markdown
Collaborator

@JoeZiminski JoeZiminski commented Jun 12, 2024

This PR closes #2855 by adding in additional checks on container and GPU dependencies when running run_sorter().

It adds checks that docker or singularity are installed (if running sorter with docker or singularity respectively) and also that the corresponding python packages docker and spython are installed. It also show a warning if, on Linux, some nvidia-docker tools are not available, which are usually necessary. See #2855 for a disucssion, but these nvidia-docker dependencies are a little complex so instead of raising an error, only a warning is shown.

I tested all locally and they work okay. It is not easy to test these nvidia-docker checks in code and so have left these, they raise only a warning anyway. The tests are a little bit 'extra', in particulary testing has_docker_python() and has_spython(). This patches sys.module to pretend like these modules are not available, then checks these has_<dependency> functions return False. However, these tests are a little bit complex and add a reasonable amount of code to check what is basically a try/except function so although it's nice to test as much as possible, would be happy to remove if the maintenance burden is too high.

The other tests check that at runtime, run_sorter raises the appropriate error when the has_<dependency> functions return False. This is achieved by 'monkeypatching' these has_<dependency> functions so they return False at runtime. Then it can be checked that run_sorter is raising the correct error. I think these tests are more valuable, are less complex that the tests discussed above.

@JoeZiminski JoeZiminski changed the title More docker dependency checks Add more docker dependency checks Jun 12, 2024
@JoeZiminski JoeZiminski force-pushed the more_docker_dependency_checks branch from 22e1268 to 78ccc27 Compare June 12, 2024 20:25
@JoeZiminski JoeZiminski force-pushed the more_docker_dependency_checks branch from 0040b2a to f1438c4 Compare June 12, 2024 20:27
@JoeZiminski JoeZiminski changed the title Add more docker dependency checks Add more container dependency checks in run_sorter Jun 13, 2024
@JoeZiminski JoeZiminski marked this pull request as ready for review June 13, 2024 08:08
@alejoe91 alejoe91 added the sorters Related to sorters module label Jun 13, 2024
Comment thread src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py Outdated
Comment thread src/spikeinterface/sorters/utils/misc.py Outdated
Copy link
Copy Markdown
Member

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

couple typos.

Comment thread src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py Outdated
Comment thread src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py Outdated
Comment thread src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py Outdated
alejoe91 and others added 3 commits June 19, 2024 11:49
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
Co-authored-by: Alessio Buccino <alejoe9187@gmail.com>
@JoeZiminski
Copy link
Copy Markdown
Collaborator Author

Thanks @alejoe91 and @zm711! @alejoe91 thanks for the apptainer addition, I wanted to test this locally but I have completely KO-d my linux parition by messing around with GPU drivers and it no longer boots 😆. So I can't physically check it with singularity / apptainer installed / not installed but checked the code carefully and command looks good and I'm happy to proceed with merging. Cheers!

@alejoe91 alejoe91 added this to the 0.101.0 milestone Jun 27, 2024
@JoeZiminski JoeZiminski requested review from alejoe91 and zm711 June 27, 2024 11:33
Copy link
Copy Markdown
Member

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

This looks good to me, but since I almost never use the containers I will leave it to @alejoe91 for the review for correctness.

@samuelgarcia
Copy link
Copy Markdown
Member

This looks perfect to me.
Thanks a lot for sanitazing this zone, this will be extremenlly helpfull for user fightinh with installation.

@samuelgarcia samuelgarcia merged commit 20cb6c8 into SpikeInterface:main Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sorters Related to sorters module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Additional checks on GPU when running containerised sorting

4 participants