Inconsistencies in the way global_job_kwargs can be defined: porting a logger in SI

[yger]

Currently, there are some inconsistencies in SI with the way global job params are handled. For example, verbose can be set in such a global dict, but then, it is conflicting with the signatures of some internal functions (like ChunkRecording), that are explicitly expecting a verbose argument. For example, the following code is crahsing

import spikeinterface.full as si
si.set_global_job_kwargs(verbose=True, n_jobs=0.8)
rec, sorting = si.generate_ground_truth_recording()
rec.save_to_memory()

Because of verbose. This patches the issue, but this is just an example, and a more systematic approach should be decided. Are we allowing job_kwargs to also store output/display arguments (such as verbose, progress_bar, ...). If so, then we should clean all signatures relying on job_kwargs to make sure this is working. If not, then verbose should be forbidden from the global job kwargs. Let's decide @samuelgarcia @alejoe91

[samuelgarcia]

I really prefer to remove verbose from job_kwargs rather than your solution!!
For me verbose in job_kwargs is a mistake.

[h-mayorquin]

Yeah, I agree with this:

This patches the issue, but this is just an example, and a more systematic approach should be decided. Are we allowing job_kwargs to also store output/display arguments (such as verbose, progress_bar, ...). If so, then we should clean all signatures relying on job_kwargs to make sure this is working.

Agree that it would be good to have a better more systematic design.

For provenance , here it was decided that "job_name" (a display variable) should not be in "job_kwargs"
#1986

Maybe verbose and progress_bar should go as well? No idea. People love progress_bars x D

[yger]

Then maybe the option is to have a global verbose_kwargs dict, similar to the job_kwargs one, that will regroup all the output of si? The good point by doing so it that it could also be a log file maybe, with the logging module, that will handle everything properly with controlled level of verbosity. This means that params such as verbose, progress_bar, job_name (?) could go there. We just need to ensure that all internal functions will thus work with job_kwargs and verbose_kwargs. What do you think?

[samuelgarcia]

Lets keep progress_bar because it is very link to the chunkrecording executor.
lets keep it simple.
Just remove verbose that should not have been in the list.

[zm711]

We may want to add a warning since there will likely be a chunk of users doing this (I think some of the tutorials also mention including verbose in the job_kwargs--so we will need to double check and fix those). If this change is happening I would suggest at least a 1 version warning saying verbose has been moved into individual functions or whatever solution is agreed upon.

[DradeAW]

I understand the idea about removing verbose from the job_kwargs,

However, I would really like a way to control the verbosity at a global level,
Dealing with multiple analyses (10+ a lot of time), it can be really annoying to have a lot of logs. But specifying verbose=False all the time is really annoying.

So I would like it if we propose an alternative solution to control the verbosity at a global level (or keep it in the job_kwargs)

[yger]

I have a proposal, but would need approval before trying to implement it everywhere. Why not using the python logging module? I've made a test here in the branch, and it could help us to have a fine grained control on all logs of spikeinterface. Similarly to what have been done with set_global_job_kwargs() we could have a set_global_logger_kwargs(). This can help to specify if we want to log to file/screen/which level of log do we want (info/warning/debugs).
Internally, all functions that would need to display/log something would do

from .globals import get_global_logger logger = get_global_logger() logger.info('Something')

I'fve started to implement the solution and this is working. Of course, this would require to go through all functions that currently have a verbose argument in their API signature to remove that, and favour this global logger instead. Similarly, all functions that have print() calls should be tracked, to turn these print() calls into logging calls. What do you think guys @alejoe91 @samuelgarcia @h-mayorquin @DradeAW

[alejoe91]

I'm in favor of using logging everywhere! Thanks for bringing this up @yger

[yger]

Ok cool, let's wait for some more feedback before I'll do the big dive.

The plus:

• it will help to structure all logs
• no extra dependencies
• have a fine grained control and ease the tracking of errors/infos
• decide where and what do we want to log.

The Down:

• slightly heavier
• we need to set a global level of logging once for all at the beginning of a script/notebook
• ?

[yger]

Other options would be to allow having multiple loggers, and instead of removing the "verbose" argument, provide some way for all function requesting logging to select which logger to use. For example, a "logger" argument. If None, then the default logger is used, otherwise a custom logger can be selected. This could allow users to log some info (for examples on runing the sorters) into a file, and then other info (for example to compute metrics, ...) into a different logger (and possibly file)

[DradeAW]

I quite like the idea of using a logger!

I don't know if using only a global logger makes sense.
As you pointed out, you might want different behaviour for running sorters or for other functions :)

We need to make sure it can't interfere with other libraries using logger btw (but I'm guessing if you create a logger object like you did, then it won't)

we need to set a global level of logging once for all at the beginning of a script/notebook

I don't understand this. Why can't there be one by default in SpikeInterface?

[yger]

Yes, of course there will be a default one in SI. Up to us to decide if we will log to screen/file, and what is the logging level. The problem with using multiple loggers is that it will complexify the syntax of all functions, and we can still offer some filtering options if we have a single logger, so I don't know. open to discussion