Remove separate default job_kwarg `n_jobs` for sorters #2712

zm711 · 2024-04-12T12:39:05Z

Right now we aren't respecting the global n_jobs that user sets so the only way to override use all cores is to specifically pass n_jobs to the sorter. This is unintuitive if we allow the user to set this globally except in the case of sorters where it has to be set locally.

The current global default appears to be n_jobs=1 so we discussed whether that should be changed for people who don't know to set n_jobs so let me know if we want to do that in this PR too.

alejoe91 · 2024-04-12T13:30:22Z

src/spikeinterface/sorters/basesorter.py

@@ -21,11 +21,11 @@
 from .utils import SpikeSortingError, ShellScript


-default_job_kwargs = {"n_jobs": -1}
+default_job_kwargs = {}


can we just remove this?

in the default_params, we can directly use the get_global_job_kwargs

We can. Once we decide on the global I get do both changes in a new commit.

@zm711 Let's do this:

We keep n_jobs=1 by default everywhere

We remove default_job_kwargs in sorters

We add a global_job_kwargs_set flag in the core/globals.py to check if the user called the set_global_job_kwargs at least once

in the fix_job_kwargs, if global_job_kwargs_set is False and n_jobs is not in the input, we raise a warning to communicate/encourage to use more jobs

Cool. I'm in the recording suite for the next couple days. If you are trying to push this with a deadline definitely just close this (or take over this one), otherwise I will do that in a couple days/if one of my experiments ends early.

alejoe91 · 2024-04-12T13:31:04Z

The current global default appears to be n_jobs=1 so we discussed whether that should be changed for people who don't know to set n_jobs so let me know if we want to do that in this PR too.

I vote for n_jobs=0.8 :)

@samuelgarcia @h-mayorquin @chrishalcrow what do you guys think?

zm711 · 2024-04-12T16:11:22Z

I guess the one concern with changing to a default of multiprocessing is that we may we see more issues whereas with opt-in not everyone is using those. But if we decide to do a default then I think 0.7 or 0.8 seems safe enough!

h-mayorquin · 2024-04-12T16:46:46Z

I think that 0.8 cores sounds good as a default. My intuition is that we should leave one physical core free as most people multitask x D.

zm711 · 2024-04-29T12:32:31Z

@samuelgarcia now that you're back opinions on n_jobs default? I think we hard code a default for now as we wait for a jobs_kwargs optimizer in the future.

samuelgarcia · 2024-04-29T14:30:21Z

If this is for the sorter 0.8 sounds OK (because it is only to write this binary).

But we need to have in mind that on some computing nodes you can have many many cores that would lead to an enormous ram demand.

So for the main n_jobs I prefer to keep 1 so it force the user to think about it to improve speed.

zm711 · 2024-04-29T15:50:16Z

I don't like changing the n_jobs without the user knowing. So if the desired global n_jobs would be n_jobs=1 then I would prefer just to feed that in to sorters as well and the end user can decide to switch to multiprocessing for sorters. But having n_jobs >1 seems like it might take more debate eh?

zm711 · 2024-05-10T11:34:26Z

Based on #2826 and #2820. I think even for sorters we should default to n_jobs=1 because some computers aren't working with multiprocessing at all. Currently not all sorters accept n_jobs as a parameter so maybe we add that to base sorting instead?

alejoe91 · 2024-05-10T11:36:50Z

I agree. Maybe a warning at the fix_job_kwargs level that "warns" that using n_jobs=1 is not optimal?

h-mayorquin · 2024-05-10T14:06:14Z

I am slightly against the warning but I can't think on a better way to let them know : /

Two suggestions if we go with the warning:

It should not be in a very general place as some sorters don't accept that and then it becomes and irrelevant warning.
There should be a way to deactivate it that does not require you to do multiprocessing. As in, yes, I am aware but no, I don't want to use multiprocesing option.

alejoe91 · 2024-05-10T14:08:10Z

@samuelgarcia suggested a global flag global_kwargs_set that by session is set to True if the user calls the set_global_job_kwargs. I think it's a good idea :)

…n_jobs_sorter

alejoe91 · 2024-05-14T11:28:14Z

@zm711 I made the suggested changes.

Now the fix_job_kwargs warns if n_jobs=1 and it hasn't been set (either globally, or locally).
Also extended a bit the job_kwargs documentation.

zm711 · 2024-05-14T12:04:05Z

Thanks Alessio!

zm711 · 2024-05-14T12:47:40Z

src/spikeinterface/core/job_tools.py

-                Number of jobs to use. With -1 the number of jobs is the same as number of cores
+            * n_jobs: int | float
+                Number of jobs to use. With -1 the number of jobs is the same as number of cores.
+                Using a float between 0 and 1 will use that fraction of the total cores.


I'm wondering if we should also make clear that technically someone can use n_jobs > number of cores which can actually degrade performance by splitting off too many processes? I didn't actually realize this until I looked at #2817.

Can you open an issue about this?

Yep. Will do after I run to a meeting!

samuelgarcia · 2024-05-14T19:23:01Z

src/spikeinterface/core/job_tools.py

@@ -60,7 +60,7 @@


 def fix_job_kwargs(runtime_job_kwargs):
-    from .globals import get_global_job_kwargs
+    from .globals import get_global_job_kwargs, global_job_kwargs_set


Suggested change

from .globals import get_global_job_kwargs, global_job_kwargs_set

from .globals import get_global_job_kwargs, is_set_global_job_kwargs_set

The use of the function better because it ensure that it is a global.

Why? I would love to learn the difference here between checking with the function and just importing the boolean. I don't really use global myself.

I agree with Zach here. It's the same and it's super internal

because it ensure that this import is not masked in between by another variable with the same name.
globals in python are not super garanty unless you put global before.

globals corss moules is evill in python having a function interface is more secure.

Good to know. Thanks :)

done in last commit

samuelgarcia · 2024-05-14T19:23:17Z

src/spikeinterface/core/job_tools.py

@@ -99,6 +99,15 @@ def fix_job_kwargs(runtime_job_kwargs):

    job_kwargs["n_jobs"] = max(n_jobs, 1)

+    if "n_jobs" not in runtime_job_kwargs and job_kwargs["n_jobs"] == 1 and not global_job_kwargs_set:


Suggested change

if "n_jobs" not in runtime_job_kwargs and job_kwargs["n_jobs"] == 1 and not global_job_kwargs_set:

if "n_jobs" not in runtime_job_kwargs and job_kwargs["n_jobs"] == 1 and not is_set_global_job_kwargs_set():

samuelgarcia · 2024-05-14T19:28:51Z

I am OK with the logic. I made a proposal.

…n_jobs_sorter

samuelgarcia · 2024-05-15T11:50:41Z

src/spikeinterface/core/globals.py

+def is_global_job_kwargs_set() -> bool:
+    """
+    Check is the global job kwargs have been manually set.
+    """
+    global global_job_kwargs_set
+    return global_job_kwargs_set


This was already existing a fiew line afte rthis one.

Fixed @samuelgarcia. But I added type hints!

samuelgarcia · 2024-05-21T13:41:33Z

src/spikeinterface/core/job_tools.py

+    if "n_jobs" not in runtime_job_kwargs and job_kwargs["n_jobs"] == 1 and not is_set_global_job_kwargs_set():
+        warnings.warn(
+            "`n_jobs` is not set so parallel processing is disabled! "
+            "To speed up computations, it is recommended to set n_jobs either "
+            "globally (with the `spikeinterface.set_global_job_kwargs()` function) or "
+            "locally (with the `n_jobs` argument). Use `spikeinterface.set_global_job_kwargs?` "
+            "for more information about job_kwargs."
+        )
+


remove default all core for sorters for n_jobs

1cf5497

zm711 added sorters Related to sorters module concurrency Related to parallel processing labels Apr 12, 2024

alejoe91 reviewed Apr 12, 2024

View reviewed changes

alejoe91 approved these changes May 7, 2024

View reviewed changes

alejoe91 added 3 commits May 14, 2024 13:04

Remove sorter default jobs

aec9c16

Merge branch 'main' of github.com:SpikeInterface/spikeinterface into …

5f00e48

…n_jobs_sorter

Add warning for n_jobs

cb51854

zm711 commented May 14, 2024

View reviewed changes

samuelgarcia reviewed May 14, 2024

View reviewed changes

alejoe91 added 2 commits May 15, 2024 13:13

Merge branch 'main' of github.com:SpikeInterface/spikeinterface into …

b045ff6

…n_jobs_sorter

Use function instead of global variable

24b5eb2

samuelgarcia reviewed May 15, 2024

View reviewed changes

zm711 added 2 commits May 17, 2024 11:20

use original is_set_global_job_kwargs_set function

2da00a4

Merge branch 'main' into n_jobs_sorter

2ddd206

samuelgarcia reviewed May 21, 2024

View reviewed changes

samuelgarcia approved these changes May 21, 2024

View reviewed changes

samuelgarcia merged commit 72c7717 into SpikeInterface:main May 21, 2024
11 checks passed

zm711 deleted the n_jobs_sorter branch May 21, 2024 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove separate default job_kwarg `n_jobs` for sorters #2712

Remove separate default job_kwarg `n_jobs` for sorters #2712

zm711 commented Apr 12, 2024

alejoe91 Apr 12, 2024

zm711 Apr 12, 2024

alejoe91 May 13, 2024

zm711 May 13, 2024

alejoe91 commented Apr 12, 2024

zm711 commented Apr 12, 2024

h-mayorquin commented Apr 12, 2024

zm711 commented Apr 29, 2024

samuelgarcia commented Apr 29, 2024

zm711 commented Apr 29, 2024

zm711 commented May 10, 2024

alejoe91 commented May 10, 2024

h-mayorquin commented May 10, 2024

alejoe91 commented May 10, 2024

alejoe91 commented May 14, 2024

zm711 commented May 14, 2024

zm711 May 14, 2024

h-mayorquin May 17, 2024

zm711 May 17, 2024

zm711 May 17, 2024

samuelgarcia May 14, 2024

samuelgarcia May 14, 2024

zm711 May 14, 2024

alejoe91 May 14, 2024

samuelgarcia May 14, 2024

samuelgarcia May 14, 2024

zm711 May 14, 2024

alejoe91 May 15, 2024

samuelgarcia May 14, 2024

alejoe91 May 15, 2024

samuelgarcia commented May 14, 2024

samuelgarcia May 15, 2024

zm711 May 17, 2024

samuelgarcia May 21, 2024

	from .globals import get_global_job_kwargs, global_job_kwargs_set
	from .globals import get_global_job_kwargs, is_set_global_job_kwargs_set

		@@ -99,6 +99,15 @@ def fix_job_kwargs(runtime_job_kwargs):

		job_kwargs["n_jobs"] = max(n_jobs, 1)

		if "n_jobs" not in runtime_job_kwargs and job_kwargs["n_jobs"] == 1 and not global_job_kwargs_set:

Remove separate default job_kwarg n_jobs for sorters #2712

Remove separate default job_kwarg n_jobs for sorters #2712

Conversation

zm711 commented Apr 12, 2024

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

alejoe91 commented Apr 12, 2024

zm711 commented Apr 12, 2024

h-mayorquin commented Apr 12, 2024

zm711 commented Apr 29, 2024

samuelgarcia commented Apr 29, 2024

zm711 commented Apr 29, 2024

zm711 commented May 10, 2024

alejoe91 commented May 10, 2024

h-mayorquin commented May 10, 2024

alejoe91 commented May 10, 2024

alejoe91 commented May 14, 2024

zm711 commented May 14, 2024

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

samuelgarcia commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove separate default job_kwarg `n_jobs` for sorters #2712

Remove separate default job_kwarg `n_jobs` for sorters #2712