update to run_sorter_jobs() and slurm #3105

MarinManuel · 2024-06-29T20:03:17Z

I am trying to run spikeinterface sorters on a HPC cluster and needed to pass specific parameters to slurm/sbatch.

I added the option to pass arbitrary arguments to sbatch in the engine_kwargs.

Let me know if that's causing any issues

…er_jobs`

# Conflicts: # src/spikeinterface/sorters/launcher.py

alejoe91 · 2024-06-30T07:21:00Z

@MarinManuel thank you! This is great!

The SLURM deployment has been unexplored.on our side, so we are very glad that it works for you.

Would you mind sharing the script that you are using? I'd love to add an How To page on how to deploy multiple spike sorting jobs on SLURM

zm711

@alejoe91 / @samuelgarcia know the launcher stuff better, is popping the best strategy or could you just pull out the kwargs without changing the dict as you go?

Otherwise, I just added some fixes for our docstring style :)

zm711 · 2024-06-30T19:14:06Z

src/spikeinterface/sorters/launcher.py

-
- return_output : bool, dfault False
+ In the case of engine="slum", possible kwargs are:
+ - tmp_script_folder: str, default None


Suggested change

- tmp_script_folder: str, default None

- tmp_script_folder : str, default: None

zm711 · 2024-06-30T19:14:37Z

src/spikeinterface/sorters/launcher.py

- return_output : bool, dfault False
+ In the case of engine="slum", possible kwargs are:
+ - tmp_script_folder: str, default None
+ the folder in which the job scripts are created. Default: directory created by


Suggested change

the folder in which the job scripts are created. Default: directory created by

the folder in which the job scripts are created. If None, it will be the directory created by

zm711 · 2024-06-30T19:14:56Z

src/spikeinterface/sorters/launcher.py

+ - tmp_script_folder: str, default None
+ the folder in which the job scripts are created. Default: directory created by
+ the `tempfile` library
+ - sbatch_executable_path: str, default 'sbatch'


Suggested change

- sbatch_executable_path: str, default 'sbatch'

- sbatch_executable_path : str, default: 'sbatch'

zm711 · 2024-06-30T19:15:19Z

src/spikeinterface/sorters/launcher.py

+ - other kwargs are interpreted as arguments to sbatch, and are translated to the --args to be passed to sbatch.
+ see the [documentation for `sbatch`](https://slurm.schedmd.com/sbatch.html) for a list of possible arguments
+
+ return_output : bool, default False


Suggested change

return_output : bool, default False

return_output : bool, default: False

Happy to follow whatever guidelines you recommend. The advantage of pop-ing out the few arguments that we want to use for something else is to not have to specify which aruments to sbatch we accept. That way, if sbatch were to add an additional argument, for instance, then we do not need to update spikeinterface to match.

Since I don't use slurm I'm not sure. Some times there are json dump of some params for provenance, so I would wait to see whatever Sam and Alessio say regarding that.

JoeZiminski

This is very cool! really opens up the SLURM options here. I have a couple of comments now, and will try it out on our HPC ASAP!

JoeZiminski · 2024-07-02T09:09:41Z

src/spikeinterface/sorters/launcher.py

 )


 _implemented_engine = list(_default_engine_kwargs.keys())


-def run_sorter_jobs(job_list, engine="loop", engine_kwargs={}, return_output=False):
+def run_sorter_jobs(job_list, engine="loop", engine_kwargs=None, return_output=False):


JoeZiminski · 2024-07-02T09:26:37Z

src/spikeinterface/sorters/launcher.py

@@ -26,14 +26,14 @@
 joblib=dict(n_jobs=-1, backend="loky"),
 processpoolexecutor=dict(max_workers=2, mp_context=None),
 dask=dict(client=None),
- slurm=dict(tmp_script_folder=None, cpus_per_task=1, mem="1G"),
+ slurm={"tmp_script_folder": None, "sbatch_executable_path": "sbatch", "cpus-per-task": 1, "mem": "1G"},


I would lean towards having something like 'sbatch_kwargs' as a sub-dict, with all kwargs that are passed directly to sbatch held there. The reason is that later engine_kwargs is iterated over an all parameters passed to sbatch. In future when editing people may miss this and not pop new arguments before engine_kwargs is iterated over, so accidentally pass unrelated arguments to sbatch. So could make sense to have "sbatch_kwargs" as a sub-dict to make the separation very clear then iterate only over engine_kwargs["sbatch_kwargs"].

This was what I had originally done, but then I thought it made the kwargs pretty cluttered and changed my mind. But I can see how that would help avoid mistakes in the future, I will change the code.

JoeZiminski · 2024-07-02T09:28:34Z

src/spikeinterface/sorters/launcher.py

+ - tmp_script_folder: str, default None
+ the folder in which the job scripts are created. Default: directory created by
+ the `tempfile` library
+ - sbatch_executable_path: str, default 'sbatch'


This is cool, out of interest what is the use case? will sbatch alone not always work?

Just in case sbatch is not in the PATH. Unlikely but why not ?

I can see why this would be useful but I think in terms of codebase maintainability it would be better that the user handles this on their end by adding sbatch to PATH, given that it is (I think?) a real edge case. Otherwise, if this is a general pattern that should be adopted it will require adding such arguments everywhere (e.g. if a sorter uses matlab, it will have to expose a path to the matlab executable, similar for anything that calls a system executable).

That's fair

JoeZiminski · 2024-07-02T09:28:52Z

src/spikeinterface/sorters/launcher.py

 Return a sortings or None.
- This also overwrite kwargs in  in run_sorter(with_sorting=True/False)
+ This also overwrite kwargs in run_sorter(with_sorting=True/False)


Suggested change

This also overwrite kwargs in run_sorter(with_sorting=True/False)

This also overwrites kwargs in run_sorter(with_sorting=True/False)

JoeZiminski · 2024-07-02T09:34:55Z

src/spikeinterface/sorters/launcher.py

@@ -146,12 +155,18 @@ def run_sorter_jobs(job_list, engine="loop", engine_kwargs={}, return_output=Fal

 elif engine == "slurm":
 # generate python script for slurm
- tmp_script_folder = engine_kwargs["tmp_script_folder"]
+ tmp_script_folder = engine_kwargs.pop("tmp_script_folder")


Here the popping is done so these arguments are not passed to sbatch. If using a subdict (comment above) I would revert these just to passing the variable so the code-reader does not have to track the state of engine_kwargs when reading.

JoeZiminski · 2024-07-02T09:35:25Z

src/spikeinterface/sorters/launcher.py

+
+ # for backward compatibility with previous version
+ if "cpus_per_task" in engine_kwargs:
+ warnings.warn("cpus_per_task is deprecated, use cpus-per-task instead", DeprecationWarning)


Nice, I guess this is to match the syntax of SLURM? 👍

JoeZiminski · 2024-07-02T09:36:50Z

src/spikeinterface/sorters/launcher.py

+ if "cpus_per_task" in engine_kwargs:
+ warnings.warn("cpus_per_task is deprecated, use cpus-per-task instead", DeprecationWarning)
+ cpus_per_task = engine_kwargs.pop("cpus_per_task")
+ if "cpus-per-task" not in engine_kwargs:


I wonder if better (though a bit more verbose) to raise an error here, if the user has accidentally passed two versions of the same kwarg it would be better to let them know I think.

JoeZiminski · 2024-07-02T09:37:27Z

src/spikeinterface/sorters/launcher.py

+ progr.append(f"--{k}")
+ progr.append(f"{v}")
+ progr.append(str(script_name.absolute()))
+ p = subprocess.run(progr, capture_output=True, text=True)


will subprocess.run() mean this is no longer run asynchronously vs. subprocess.Popen()?

Sbatch returns immediately after submitting the job to the queue, so it's still going to be asynchronous. The run() syntax is just a simpler way to interact with the subprocess module and the one I have more experience with, that's all

Okay nice ofc sbatch will just return, agree run is the preferred way to use subprocess. Nice!

…irectly into engine_kwargs

for more information, see https://pre-commit.ci

# Conflicts: # src/spikeinterface/sorters/launcher.py

for more information, see https://pre-commit.ci

Marin Manuel and others added 6 commits June 29, 2024 11:22

added option to pass extra arguments to sbatch when using `run_sort…

efa0751

…er_jobs`

added option to pass extra arguments to sbatch when using `run_sort…

4bdf244

…er_jobs`

Merge remote-tracking branch 'origin/main'

2d0a83b

# Conflicts: # src/spikeinterface/sorters/launcher.py

mistake in Popen

ce87e68

Merge remote-tracking branch 'origin/main'

ebabfec

# Conflicts: # src/spikeinterface/sorters/launcher.py

cleaned up code

4e59f23

zm711 reviewed Jun 30, 2024

View reviewed changes

alejoe91 added the sorters Related to sorters module label Jul 1, 2024

JoeZiminski reviewed Jul 2, 2024

View reviewed changes

MarinManuel and others added 7 commits July 2, 2024 14:23

Merge branch 'SpikeInterface:main' into slurm_updates

407c488

updated to use sbatch_kwargs instead of putting all slurm arguments d…

d204642

…irectly into engine_kwargs

[pre-commit.ci] auto fixes from pre-commit.com hooks

8fedd95

for more information, see https://pre-commit.ci

removed sbatch_executable from the list of kwargs

0733589

Merge remote-tracking branch 'origin/slurm_updates' into slurm_updates

54a7b8f

# Conflicts: # src/spikeinterface/sorters/launcher.py

removed sbatch_executable from the list of kwargs

9c3ff1d

[pre-commit.ci] auto fixes from pre-commit.com hooks

c86f3b2

for more information, see https://pre-commit.ci

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update to run_sorter_jobs() and slurm #3105

update to run_sorter_jobs() and slurm #3105

MarinManuel commented Jun 29, 2024

alejoe91 commented Jun 30, 2024

zm711 left a comment

zm711 Jun 30, 2024

zm711 Jun 30, 2024

zm711 Jun 30, 2024

zm711 Jun 30, 2024

MarinManuel Jul 1, 2024

zm711 Jul 1, 2024

JoeZiminski left a comment

JoeZiminski Jul 2, 2024

JoeZiminski Jul 2, 2024

MarinManuel Jul 2, 2024

JoeZiminski Jul 2, 2024

MarinManuel Jul 2, 2024

JoeZiminski Jul 3, 2024

MarinManuel Jul 3, 2024

JoeZiminski Jul 2, 2024

JoeZiminski Jul 2, 2024

JoeZiminski Jul 2, 2024

JoeZiminski Jul 2, 2024

JoeZiminski Jul 2, 2024

MarinManuel Jul 2, 2024

JoeZiminski Jul 3, 2024

	- tmp_script_folder: str, default None
	- tmp_script_folder : str, default: None

	the folder in which the job scripts are created. Default: directory created by
	the folder in which the job scripts are created. If None, it will be the directory created by

	- sbatch_executable_path: str, default 'sbatch'
	- sbatch_executable_path : str, default: 'sbatch'

	return_output : bool, default False
	return_output : bool, default: False

	This also overwrite kwargs in run_sorter(with_sorting=True/False)
	This also overwrites kwargs in run_sorter(with_sorting=True/False)

update to run_sorter_jobs() and slurm #3105

Are you sure you want to change the base?

update to run_sorter_jobs() and slurm #3105

Conversation

MarinManuel commented Jun 29, 2024

alejoe91 commented Jun 30, 2024

zm711 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeZiminski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment