From cf5041062a73b1c61d8f15200ade73ea1f1d8bae Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 12 Jun 2024 19:14:51 +0100 Subject: [PATCH 01/14] Run checks for singularity, docker and related python module installations. --- src/spikeinterface/sorters/runsorter.py | 18 ++++++++++++++ src/spikeinterface/sorters/utils/misc.py | 31 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/spikeinterface/sorters/runsorter.py b/src/spikeinterface/sorters/runsorter.py index baec6aaac3..44a08a34a7 100644 --- a/src/spikeinterface/sorters/runsorter.py +++ b/src/spikeinterface/sorters/runsorter.py @@ -169,6 +169,15 @@ def run_sorter( container_image = None else: container_image = docker_image + + if not has_docker(): + raise RuntimeError("Docker is not installed. Install docker " + "on this machine to run sorting with docker.") + + if not has_docker_python(): + raise RuntimeError("The python `docker` package must be installed." + "Install with `pip install docker`") + else: mode = "singularity" assert not docker_image @@ -176,6 +185,15 @@ def run_sorter( container_image = None else: container_image = singularity_image + + if not has_singularity(): + raise RuntimeError("Singularity is not installed. Install singularity " + "on this machine to run sorting with singularity.") + + if not has_spython(): + raise RuntimeError("The python singularity package must be installed." + "Install with `pip install spython`") + return run_sorter_container( container_image=container_image, mode=mode, diff --git a/src/spikeinterface/sorters/utils/misc.py b/src/spikeinterface/sorters/utils/misc.py index 0a6b4a986c..a1cf34f059 100644 --- a/src/spikeinterface/sorters/utils/misc.py +++ b/src/spikeinterface/sorters/utils/misc.py @@ -1,6 +1,7 @@ from __future__ import annotations from pathlib import Path +import subprocess # TODO: decide best format for this from subprocess import check_output, CalledProcessError from typing import List, Union @@ -80,3 +81,33 @@ def has_nvidia(): return device_count > 0 except RuntimeError: # Failed to dlopen libcuda.so return False + +def _run_subprocess_silently(command): + output = subprocess.run( + command, shell=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL + ) + return output + + +def has_docker(): + return self._run_subprocess_silently("docker --version").returncode == 0 + + +def has_singularity(): + return self._run_subprocess_silently("singularity --version").returncode == 0 + + +def has_docker_python(): + try: + import docker + return True + except ImportError: + return False + + +def has_spython(): + try: + import spython + return True + except ImportError: + return False From e49521939f2023c50943afad21a663c3d7822011 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 12 Jun 2024 20:09:03 +0100 Subject: [PATCH 02/14] Add nvidia dependency checks, tidy up. --- src/spikeinterface/sorters/runsorter.py | 17 +++++++++++---- src/spikeinterface/sorters/utils/__init__.py | 2 +- src/spikeinterface/sorters/utils/misc.py | 22 +++++++++++++++++--- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/spikeinterface/sorters/runsorter.py b/src/spikeinterface/sorters/runsorter.py index 44a08a34a7..884cba590f 100644 --- a/src/spikeinterface/sorters/runsorter.py +++ b/src/spikeinterface/sorters/runsorter.py @@ -19,7 +19,7 @@ from ..core import BaseRecording, NumpySorting, load_extractor from ..core.core_tools import check_json, is_editable_mode from .sorterlist import sorter_dict -from .utils import SpikeSortingError, has_nvidia +from .utils import SpikeSortingError, has_nvidia, has_docker, has_docker_python, has_singularity, has_spython, has_docker_nvidia_installed, get_nvidia_docker_dependecies from .container_tools import ( find_recording_folders, path_to_unix, @@ -175,7 +175,7 @@ def run_sorter( "on this machine to run sorting with docker.") if not has_docker_python(): - raise RuntimeError("The python `docker` package must be installed." + raise RuntimeError("The python `docker` package must be installed. " "Install with `pip install docker`") else: @@ -191,8 +191,8 @@ def run_sorter( "on this machine to run sorting with singularity.") if not has_spython(): - raise RuntimeError("The python singularity package must be installed." - "Install with `pip install spython`") + raise RuntimeError("The python `spython` package must be installed to " + "run singularity. Install with `pip install spython`") return run_sorter_container( container_image=container_image, @@ -480,6 +480,15 @@ def run_sorter_container( if gpu_capability == "nvidia-required": assert has_nvidia(), "The container requires a NVIDIA GPU capability, but it is not available" extra_kwargs["container_requires_gpu"] = True + + if platform.system() == "Linux" and has_docker_nvidia_installed(): + warn( + f"nvidia-required but none of \n{get_nvidia_docker_dependecies()}\n were found. " + f"This may result in an error being raised during sorting. Try " + "installing `nvidia-container-toolkit`, including setting the " + "configuration steps, if running into errors." + ) + elif gpu_capability == "nvidia-optional": if has_nvidia(): extra_kwargs["container_requires_gpu"] = True diff --git a/src/spikeinterface/sorters/utils/__init__.py b/src/spikeinterface/sorters/utils/__init__.py index 6cad10b211..7f6f3089d4 100644 --- a/src/spikeinterface/sorters/utils/__init__.py +++ b/src/spikeinterface/sorters/utils/__init__.py @@ -1,2 +1,2 @@ from .shellscript import ShellScript -from .misc import SpikeSortingError, get_git_commit, has_nvidia, get_matlab_shell_name, get_bash_path +from .misc import SpikeSortingError, get_git_commit, has_nvidia, get_matlab_shell_name, get_bash_path, has_docker, has_docker_python, has_singularity, has_spython, has_docker_nvidia_installed, get_nvidia_docker_dependecies diff --git a/src/spikeinterface/sorters/utils/misc.py b/src/spikeinterface/sorters/utils/misc.py index a1cf34f059..4a900f4485 100644 --- a/src/spikeinterface/sorters/utils/misc.py +++ b/src/spikeinterface/sorters/utils/misc.py @@ -82,6 +82,7 @@ def has_nvidia(): except RuntimeError: # Failed to dlopen libcuda.so return False + def _run_subprocess_silently(command): output = subprocess.run( command, shell=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL @@ -90,12 +91,27 @@ def _run_subprocess_silently(command): def has_docker(): - return self._run_subprocess_silently("docker --version").returncode == 0 + return _run_subprocess_silently("docker --version").returncode == 0 def has_singularity(): - return self._run_subprocess_silently("singularity --version").returncode == 0 - + return _run_subprocess_silently("singularity --version").returncode == 0 + +def get_nvidia_docker_dependecies(): + return [ + "nvidia-docker", + "nvidia-docker2", + "nvidia-container-toolkit", + ] + +def has_docker_nvidia_installed(): + all_dependencies = get_nvidia_docker_dependecies() + has_dep = [] + for dep in all_dependencies: + has_dep.append( + _run_subprocess_silently(f"{dep} --version").returncode == 0 + ) + return not any(has_dep) def has_docker_python(): try: From e0656bb86901127c8b1c0f708e4970584e79a40d Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 12 Jun 2024 20:15:48 +0100 Subject: [PATCH 03/14] Add docstrings. --- src/spikeinterface/sorters/utils/misc.py | 44 ++++++++++++++++++------ 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/src/spikeinterface/sorters/utils/misc.py b/src/spikeinterface/sorters/utils/misc.py index 4a900f4485..66744fbab1 100644 --- a/src/spikeinterface/sorters/utils/misc.py +++ b/src/spikeinterface/sorters/utils/misc.py @@ -84,9 +84,10 @@ def has_nvidia(): def _run_subprocess_silently(command): - output = subprocess.run( - command, shell=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL - ) + """ + Run a subprocess command without outputting to stderr or stdout. + """ + output = subprocess.run(command, shell=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) return output @@ -97,25 +98,45 @@ def has_docker(): def has_singularity(): return _run_subprocess_silently("singularity --version").returncode == 0 + +def has_docker_nvidia_installed(): + """ + On Linux, nvidia has a set of container dependencies + that are required for running GPU in docker. This is a little + complex and is described in more detail in the links below. + To summarise breifly, at least one of the `get_nvidia_docker_dependecies()` + is almost certainly required to run docker with GPU. + + https://github.com/NVIDIA/nvidia-docker/issues/1268 + https://www.howtogeek.com/devops/how-to-use-an-nvidia-gpu-with-docker-containers/ + + Returns + ------- + Whether at least one of the dependencies listed in + `get_nvidia_docker_dependecies()` is installed. + """ + all_dependencies = get_nvidia_docker_dependecies() + has_dep = [] + for dep in all_dependencies: + has_dep.append(_run_subprocess_silently(f"{dep} --version").returncode == 0) + return not any(has_dep) + + def get_nvidia_docker_dependecies(): + """ + See `has_docker_nvidia_installed()` + """ return [ "nvidia-docker", "nvidia-docker2", "nvidia-container-toolkit", ] -def has_docker_nvidia_installed(): - all_dependencies = get_nvidia_docker_dependecies() - has_dep = [] - for dep in all_dependencies: - has_dep.append( - _run_subprocess_silently(f"{dep} --version").returncode == 0 - ) - return not any(has_dep) def has_docker_python(): try: import docker + return True except ImportError: return False @@ -124,6 +145,7 @@ def has_docker_python(): def has_spython(): try: import spython + return True except ImportError: return False From b145b04ac31a8de3d9c9fbfc56b4a9974ce0eb3a Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 12 Jun 2024 21:21:51 +0100 Subject: [PATCH 04/14] Add tests for runsorter dependencies. --- src/spikeinterface/sorters/runsorter.py | 35 +++-- .../tests/test_runsorter_dependency_checks.py | 144 ++++++++++++++++++ 2 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py diff --git a/src/spikeinterface/sorters/runsorter.py b/src/spikeinterface/sorters/runsorter.py index 884cba590f..5b2e80b83d 100644 --- a/src/spikeinterface/sorters/runsorter.py +++ b/src/spikeinterface/sorters/runsorter.py @@ -19,7 +19,18 @@ from ..core import BaseRecording, NumpySorting, load_extractor from ..core.core_tools import check_json, is_editable_mode from .sorterlist import sorter_dict -from .utils import SpikeSortingError, has_nvidia, has_docker, has_docker_python, has_singularity, has_spython, has_docker_nvidia_installed, get_nvidia_docker_dependecies + +# full import required for monkeypatch testing. +from spikeinterface.sorters.utils import ( + SpikeSortingError, + has_nvidia, + has_docker, + has_docker_python, + has_singularity, + has_spython, + has_docker_nvidia_installed, + get_nvidia_docker_dependecies, +) from .container_tools import ( find_recording_folders, path_to_unix, @@ -171,12 +182,14 @@ def run_sorter( container_image = docker_image if not has_docker(): - raise RuntimeError("Docker is not installed. Install docker " - "on this machine to run sorting with docker.") + raise RuntimeError( + "Docker is not installed. Install docker " "on this machine to run sorting with docker." + ) if not has_docker_python(): - raise RuntimeError("The python `docker` package must be installed. " - "Install with `pip install docker`") + raise RuntimeError( + "The python `docker` package must be installed. " "Install with `pip install docker`" + ) else: mode = "singularity" @@ -187,12 +200,16 @@ def run_sorter( container_image = singularity_image if not has_singularity(): - raise RuntimeError("Singularity is not installed. Install singularity " - "on this machine to run sorting with singularity.") + raise RuntimeError( + "Singularity is not installed. Install singularity " + "on this machine to run sorting with singularity." + ) if not has_spython(): - raise RuntimeError("The python `spython` package must be installed to " - "run singularity. Install with `pip install spython`") + raise RuntimeError( + "The python `spython` package must be installed to " + "run singularity. Install with `pip install spython`" + ) return run_sorter_container( container_image=container_image, diff --git a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py new file mode 100644 index 0000000000..8dbb1b20f6 --- /dev/null +++ b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py @@ -0,0 +1,144 @@ +import os +import pytest +from pathlib import Path +import shutil +import platform +from spikeinterface import generate_ground_truth_recording +from spikeinterface.sorters.utils import has_spython, has_docker_python +from spikeinterface.sorters import run_sorter +import subprocess +import sys +import copy + + +def _monkeypatch_return_false(): + return False + + +class TestRunersorterDependencyChecks: + """ + This class performs tests to check whether expected + dependency checks prior to sorting are run. The + run_sorter function should raise an error if: + - singularity is not installed + - spython is not installed (python package) + - docker is not installed + - docker is not installed (python package) + when running singularity / docker respectively. + + Two separate checks should be run. First, that the + relevant `has_` function (indicating if the dependency + is installed) is working. Unfortunately it is not possible to + easily test this core singularity and docker installs, so this is not done. + `uninstall_python_dependency()` allows a test to check if the + `has_spython()` and `has_docker_dependency()` return `False` as expected + when these python modules are not installed. + + Second, the `run_sorters()` function should return the appropriate error + when these functions return that the dependency is not available. This is + easier to test as these `has_` reporting functions can be + monkeypatched to return False at runtime. This is done for these 4 + dependency checks, and tests check the expected error is raised. + + Notes + ---- + `has_nvidia()` and `has_docker_nvidia_installed()` are not tested + as these are complex GPU-related dependencies which are difficult to mock. + """ + + @pytest.fixture(scope="function") + def uninstall_python_dependency(self, request): + """ + This python fixture mocks python modules not been importable + by setting the relevant `sys.modules` dict entry to `None`. + It uses `yeild` so that the function can tear-down the test + (even if it failed) and replace the patched `sys.module` entry. + + This function uses an `indirect` parameterisation, meaning the + `request.param` is passed to the fixture at the start of the + test function. This is used to reuse code for nearly identical + `spython` and `docker` python dependency tests. + """ + dep_name = request.param + assert dep_name in ["spython", "docker"] + + try: + if dep_name == "spython": + import spython + else: + import docker + dependency_installed = True + except: + dependency_installed = False + + if dependency_installed: + copy_import = sys.modules[dep_name] + sys.modules[dep_name] = None + yield + if dependency_installed: + sys.modules[dep_name] = copy_import + + @pytest.fixture(scope="session") + def recording(self): + """ + Make a small recording to have something to pass to the sorter. + """ + recording, _ = generate_ground_truth_recording(durations=[10]) + return recording + + @pytest.mark.skipif(platform.system() != "Linux", reason="spython install only for Linux.") + @pytest.mark.parametrize("uninstall_python_dependency", ["spython"], indirect=True) + def test_has_spython(self, recording, uninstall_python_dependency): + """ + Test the `has_spython()` function, see class docstring and + `uninstall_python_dependency()` for details. + """ + assert has_spython() is False + + @pytest.mark.parametrize("uninstall_python_dependency", ["docker"], indirect=True) + def test_has_docker_python(self, recording, uninstall_python_dependency): + """ + Test the `has_docker_python()` function, see class docstring and + `uninstall_python_dependency()` for details. + """ + assert has_docker_python() is False + + @pytest.mark.parametrize("dependency", ["singularity", "spython"]) + def test_has_singularity_and_spython(self, recording, monkeypatch, dependency): + """ + When running a sorting, if singularity dependencies (singularity + itself or the `spython` package`) are not installed, an error is raised. + Beacause it is hard to actually uninstall these dependencies, the + `has_` functions that let `run_sorter` know if the dependency + are installed are monkeypatched. This is done so at runtime these always + return False. Then, test the expected error is raised when the dependency + is not found. + """ + test_func = f"has_{dependency}" + + monkeypatch.setattr(f"spikeinterface.sorters.runsorter.{test_func}", _monkeypatch_return_false) + with pytest.raises(RuntimeError) as e: + run_sorter("kilosort2_5", recording, singularity_image=True) + + if dependency == "spython": + assert "The python `spython` package must be installed" in str(e) + else: + assert "Singularity is not installed." in str(e) + + @pytest.mark.parametrize("dependency", ["docker", "docker_python"]) + def test_has_docker_and_docker_python(self, recording, monkeypatch, dependency): + """ + See `test_has_singularity_and_spython()` for details. This test + is almost identical, but with some key changes for Docker. + """ + test_func = f"has_{dependency}" + + monkeypatch.setattr(f"spikeinterface.sorters.runsorter.{test_func}", _monkeypatch_return_false) + + with pytest.raises(RuntimeError) as e: + run_sorter("kilosort2_5", recording, docker_image=True) + + if dependency == "docker_python": + assert "The python `docker` package must be installed" in str(e) + else: + assert "Docker is not installed." in str(e) From 78ccc2719676b238dbd92d2ad5384786ca0724e0 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 12 Jun 2024 21:24:29 +0100 Subject: [PATCH 05/14] Remove unnecessary non-relative import. --- src/spikeinterface/sorters/runsorter.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/spikeinterface/sorters/runsorter.py b/src/spikeinterface/sorters/runsorter.py index 5b2e80b83d..c16435cdb5 100644 --- a/src/spikeinterface/sorters/runsorter.py +++ b/src/spikeinterface/sorters/runsorter.py @@ -19,9 +19,7 @@ from ..core import BaseRecording, NumpySorting, load_extractor from ..core.core_tools import check_json, is_editable_mode from .sorterlist import sorter_dict - -# full import required for monkeypatch testing. -from spikeinterface.sorters.utils import ( +from .utils import ( SpikeSortingError, has_nvidia, has_docker, From f1438c4ce20bbd7ae3c910b793f92ebb4d723253 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 12 Jun 2024 21:27:03 +0100 Subject: [PATCH 06/14] Fix some string formatting, add docstring to monkeypatch function. --- src/spikeinterface/sorters/runsorter.py | 6 ++---- .../sorters/tests/test_runsorter_dependency_checks.py | 4 ++++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/spikeinterface/sorters/runsorter.py b/src/spikeinterface/sorters/runsorter.py index c16435cdb5..f9994dd38d 100644 --- a/src/spikeinterface/sorters/runsorter.py +++ b/src/spikeinterface/sorters/runsorter.py @@ -181,13 +181,11 @@ def run_sorter( if not has_docker(): raise RuntimeError( - "Docker is not installed. Install docker " "on this machine to run sorting with docker." + "Docker is not installed. Install docker on this machine to run sorting with docker." ) if not has_docker_python(): - raise RuntimeError( - "The python `docker` package must be installed. " "Install with `pip install docker`" - ) + raise RuntimeError("The python `docker` package must be installed. Install with `pip install docker`") else: mode = "singularity" diff --git a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py index 8dbb1b20f6..c81593b7db 100644 --- a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py +++ b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py @@ -12,6 +12,10 @@ def _monkeypatch_return_false(): + """ + A function to monkeypatch the `has_` functions, + ensuring the always return `False` at runtime. + """ return False From fd4406e0826f80329614e3b59388e9640c00fe3e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 12 Jun 2024 20:27:36 +0000 Subject: [PATCH 07/14] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/spikeinterface/sorters/utils/__init__.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/spikeinterface/sorters/utils/__init__.py b/src/spikeinterface/sorters/utils/__init__.py index 7f6f3089d4..62317be6f2 100644 --- a/src/spikeinterface/sorters/utils/__init__.py +++ b/src/spikeinterface/sorters/utils/__init__.py @@ -1,2 +1,14 @@ from .shellscript import ShellScript -from .misc import SpikeSortingError, get_git_commit, has_nvidia, get_matlab_shell_name, get_bash_path, has_docker, has_docker_python, has_singularity, has_spython, has_docker_nvidia_installed, get_nvidia_docker_dependecies +from .misc import ( + SpikeSortingError, + get_git_commit, + has_nvidia, + get_matlab_shell_name, + get_bash_path, + has_docker, + has_docker_python, + has_singularity, + has_spython, + has_docker_nvidia_installed, + get_nvidia_docker_dependecies, +) From 7af611ba289e220c4bf36f4b62ae26efe94f93b1 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 12 Jun 2024 21:42:26 +0100 Subject: [PATCH 08/14] Mock all has functions to ensure tests do not depend on actual dependencies. --- .../tests/test_runsorter_dependency_checks.py | 58 +++++++++++++------ 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py index c81593b7db..a248033089 100644 --- a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py +++ b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py @@ -4,7 +4,7 @@ import shutil import platform from spikeinterface import generate_ground_truth_recording -from spikeinterface.sorters.utils import has_spython, has_docker_python +from spikeinterface.sorters.utils import has_spython, has_docker_python, has_docker, has_singularity from spikeinterface.sorters import run_sorter import subprocess import sys @@ -19,6 +19,10 @@ def _monkeypatch_return_false(): return False +def _monkeypatch_return_true(): + return True + + class TestRunersorterDependencyChecks: """ This class performs tests to check whether expected @@ -91,6 +95,7 @@ def recording(self): return recording @pytest.mark.skipif(platform.system() != "Linux", reason="spython install only for Linux.") + @pytest.mark.skipif(not has_singularity(), reason="singularity required for this test.") @pytest.mark.parametrize("uninstall_python_dependency", ["spython"], indirect=True) def test_has_spython(self, recording, uninstall_python_dependency): """ @@ -100,6 +105,7 @@ def test_has_spython(self, recording, uninstall_python_dependency): assert has_spython() is False @pytest.mark.parametrize("uninstall_python_dependency", ["docker"], indirect=True) + @pytest.mark.skipif(not has_docker(), reason="docker required for this test.") def test_has_docker_python(self, recording, uninstall_python_dependency): """ Test the `has_docker_python()` function, see class docstring and @@ -107,8 +113,7 @@ def test_has_docker_python(self, recording, uninstall_python_dependency): """ assert has_docker_python() is False - @pytest.mark.parametrize("dependency", ["singularity", "spython"]) - def test_has_singularity_and_spython(self, recording, monkeypatch, dependency): + def test_no_singularity_error_raised(self, recording, monkeypatch): """ When running a sorting, if singularity dependencies (singularity itself or the `spython` package`) are not installed, an error is raised. @@ -118,31 +123,46 @@ def test_has_singularity_and_spython(self, recording, monkeypatch, dependency): return False. Then, test the expected error is raised when the dependency is not found. """ - test_func = f"has_{dependency}" + monkeypatch.setattr(f"spikeinterface.sorters.runsorter.has_singularity", _monkeypatch_return_false) - monkeypatch.setattr(f"spikeinterface.sorters.runsorter.{test_func}", _monkeypatch_return_false) with pytest.raises(RuntimeError) as e: run_sorter("kilosort2_5", recording, singularity_image=True) - if dependency == "spython": - assert "The python `spython` package must be installed" in str(e) - else: - assert "Singularity is not installed." in str(e) + assert "Singularity is not installed." in str(e) - @pytest.mark.parametrize("dependency", ["docker", "docker_python"]) - def test_has_docker_and_docker_python(self, recording, monkeypatch, dependency): + def test_no_spython_error_raised(self, recording, monkeypatch): """ - See `test_has_singularity_and_spython()` for details. This test - is almost identical, but with some key changes for Docker. + See `test_no_singularity_error_raised()`. """ - test_func = f"has_{dependency}" + # make sure singularity test returns true as that comes first + monkeypatch.setattr(f"spikeinterface.sorters.runsorter.has_singularity", _monkeypatch_return_true) + monkeypatch.setattr(f"spikeinterface.sorters.runsorter.has_spython", _monkeypatch_return_false) + + with pytest.raises(RuntimeError) as e: + run_sorter("kilosort2_5", recording, singularity_image=True) + + assert "The python `spython` package must be installed" in str(e) - monkeypatch.setattr(f"spikeinterface.sorters.runsorter.{test_func}", _monkeypatch_return_false) + def test_no_docker_error_raised(self, recording, monkeypatch): + """ + See `test_no_singularity_error_raised()`. + """ + monkeypatch.setattr(f"spikeinterface.sorters.runsorter.has_docker", _monkeypatch_return_false) + + with pytest.raises(RuntimeError) as e: + run_sorter("kilosort2_5", recording, docker_image=True) + + assert "Docker is not installed." in str(e) + + def test_as_no_docker_python_error_raised(self, recording, monkeypatch): + """ + See `test_no_singularity_error_raised()`. + """ + # make sure docker test returns true as that comes first + monkeypatch.setattr(f"spikeinterface.sorters.runsorter.has_docker", _monkeypatch_return_true) + monkeypatch.setattr(f"spikeinterface.sorters.runsorter.has_docker_python", _monkeypatch_return_false) with pytest.raises(RuntimeError) as e: run_sorter("kilosort2_5", recording, docker_image=True) - if dependency == "docker_python": - assert "The python `docker` package must be installed" in str(e) - else: - assert "Docker is not installed." in str(e) + assert "The python `docker` package must be installed" in str(e) From 0c0b1f908d8e356b9a58cacd4524ace871ff93b3 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 12 Jun 2024 21:43:10 +0100 Subject: [PATCH 09/14] Remove unecessary skips. --- .../sorters/tests/test_runsorter_dependency_checks.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py index a248033089..741fe4ae0e 100644 --- a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py +++ b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py @@ -95,7 +95,6 @@ def recording(self): return recording @pytest.mark.skipif(platform.system() != "Linux", reason="spython install only for Linux.") - @pytest.mark.skipif(not has_singularity(), reason="singularity required for this test.") @pytest.mark.parametrize("uninstall_python_dependency", ["spython"], indirect=True) def test_has_spython(self, recording, uninstall_python_dependency): """ @@ -105,7 +104,6 @@ def test_has_spython(self, recording, uninstall_python_dependency): assert has_spython() is False @pytest.mark.parametrize("uninstall_python_dependency", ["docker"], indirect=True) - @pytest.mark.skipif(not has_docker(), reason="docker required for this test.") def test_has_docker_python(self, recording, uninstall_python_dependency): """ Test the `has_docker_python()` function, see class docstring and From 1be1dbd39a339ff56c0803ff7a59e5650d95b781 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Thu, 13 Jun 2024 09:04:04 +0100 Subject: [PATCH 10/14] Update docstrings. --- .../sorters/tests/test_runsorter_dependency_checks.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py index 741fe4ae0e..c4beaba072 100644 --- a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py +++ b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py @@ -20,14 +20,18 @@ def _monkeypatch_return_false(): def _monkeypatch_return_true(): + """ + Monkeypatch for some `has_` functions to + return `True` so functions that are later in the + `runsorter` code can be checked. + """ return True class TestRunersorterDependencyChecks: """ - This class performs tests to check whether expected - dependency checks prior to sorting are run. The - run_sorter function should raise an error if: + This class tests whether expected dependency checks prior to sorting are run. + The run_sorter function should raise an error if: - singularity is not installed - spython is not installed (python package) - docker is not installed From 00663080b03f7933d37ba4ff2ee32e3402aa200e Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Thu, 13 Jun 2024 09:10:30 +0100 Subject: [PATCH 11/14] Swap return bool for to match function name. --- src/spikeinterface/sorters/runsorter.py | 2 +- src/spikeinterface/sorters/utils/misc.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/spikeinterface/sorters/runsorter.py b/src/spikeinterface/sorters/runsorter.py index f9994dd38d..80608f8973 100644 --- a/src/spikeinterface/sorters/runsorter.py +++ b/src/spikeinterface/sorters/runsorter.py @@ -494,7 +494,7 @@ def run_sorter_container( assert has_nvidia(), "The container requires a NVIDIA GPU capability, but it is not available" extra_kwargs["container_requires_gpu"] = True - if platform.system() == "Linux" and has_docker_nvidia_installed(): + if platform.system() == "Linux" and not has_docker_nvidia_installed(): warn( f"nvidia-required but none of \n{get_nvidia_docker_dependecies()}\n were found. " f"This may result in an error being raised during sorting. Try " diff --git a/src/spikeinterface/sorters/utils/misc.py b/src/spikeinterface/sorters/utils/misc.py index 66744fbab1..1e01b9c052 100644 --- a/src/spikeinterface/sorters/utils/misc.py +++ b/src/spikeinterface/sorters/utils/misc.py @@ -119,7 +119,7 @@ def has_docker_nvidia_installed(): has_dep = [] for dep in all_dependencies: has_dep.append(_run_subprocess_silently(f"{dep} --version").returncode == 0) - return not any(has_dep) + return any(has_dep) def get_nvidia_docker_dependecies(): From 9664f69c4bcdd24e20584f601bcbd6a9ae79e174 Mon Sep 17 00:00:00 2001 From: Alessio Buccino Date: Wed, 19 Jun 2024 11:49:32 +0200 Subject: [PATCH 12/14] Apply suggestions from code review Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com> --- .../sorters/tests/test_runsorter_dependency_checks.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py index c4beaba072..83d6ec3161 100644 --- a/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py +++ b/src/spikeinterface/sorters/tests/test_runsorter_dependency_checks.py @@ -13,7 +13,7 @@ def _monkeypatch_return_false(): """ - A function to monkeypatch the `has_` functions, + A function to monkeypatch the `has_` functions, ensuring the always return `False` at runtime. """ return False @@ -61,12 +61,12 @@ class TestRunersorterDependencyChecks: @pytest.fixture(scope="function") def uninstall_python_dependency(self, request): """ - This python fixture mocks python modules not been importable + This python fixture mocks python modules not being importable by setting the relevant `sys.modules` dict entry to `None`. - It uses `yeild` so that the function can tear-down the test + It uses `yield` so that the function can tear-down the test (even if it failed) and replace the patched `sys.module` entry. - This function uses an `indirect` parameterisation, meaning the + This function uses an `indirect` parameterization, meaning the `request.param` is passed to the fixture at the start of the test function. This is used to reuse code for nearly identical `spython` and `docker` python dependency tests. From 543cc8f2a67719e4ae8b5b64a198a6c7256406e4 Mon Sep 17 00:00:00 2001 From: Joe Ziminski <55797454+JoeZiminski@users.noreply.github.com> Date: Wed, 19 Jun 2024 18:12:31 +0100 Subject: [PATCH 13/14] Add apptainer case to 'has_singularity()' Co-authored-by: Alessio Buccino --- src/spikeinterface/sorters/utils/misc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spikeinterface/sorters/utils/misc.py b/src/spikeinterface/sorters/utils/misc.py index 1e01b9c052..82480ffe0a 100644 --- a/src/spikeinterface/sorters/utils/misc.py +++ b/src/spikeinterface/sorters/utils/misc.py @@ -96,7 +96,7 @@ def has_docker(): def has_singularity(): - return _run_subprocess_silently("singularity --version").returncode == 0 + return _run_subprocess_silently("singularity --version").returncode == 0 or _run_subprocess_silently("apptainer --version").returncode == 0 def has_docker_nvidia_installed(): From dceb08070af9954b25c99c82ed2df314ef924aa7 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 19 Jun 2024 17:12:51 +0000 Subject: [PATCH 14/14] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/spikeinterface/sorters/utils/misc.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/spikeinterface/sorters/utils/misc.py b/src/spikeinterface/sorters/utils/misc.py index 82480ffe0a..9c8c3bba89 100644 --- a/src/spikeinterface/sorters/utils/misc.py +++ b/src/spikeinterface/sorters/utils/misc.py @@ -96,7 +96,10 @@ def has_docker(): def has_singularity(): - return _run_subprocess_silently("singularity --version").returncode == 0 or _run_subprocess_silently("apptainer --version").returncode == 0 + return ( + _run_subprocess_silently("singularity --version").returncode == 0 + or _run_subprocess_silently("apptainer --version").returncode == 0 + ) def has_docker_nvidia_installed():