Skip to content

improve verbose of ChunkRecordingExecutor#2795

Merged
alejoe91 merged 3 commits intoSpikeInterface:mainfrom
h-mayorquin:improve_verbose_in_chunk_recording_executor
May 13, 2024
Merged

improve verbose of ChunkRecordingExecutor#2795
alejoe91 merged 3 commits intoSpikeInterface:mainfrom
h-mayorquin:improve_verbose_in_chunk_recording_executor

Conversation

@h-mayorquin
Copy link
Copy Markdown
Collaborator

image

@h-mayorquin h-mayorquin added the core Changes to core module label May 2, 2024
@h-mayorquin h-mayorquin self-assigned this May 2, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review May 2, 2024 21:56
@zm711
Copy link
Copy Markdown
Member

zm711 commented May 3, 2024

Is this helpful to the end-user or to the developer? This seems more like a verbose level 2 rather than a verbose level 1 type thing to me. But since this library only has two options (nothing + everything) I guess this could work. Who do you envision using this?

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

@zm711 Users actually. I was thinking that people might want to know how much memory they will be using in total. That's personally what I want to know when I am running a task. When I developed I use a memory profiler not this. Maybe I am very off base about the need of the users, how are you thinking about it?

@zm711
Copy link
Copy Markdown
Member

zm711 commented May 3, 2024

I think I'm thinking about it from the basic biologist side. I might set memory limits in the function, but I don't really care if it is 950 vs 980 if I asked for 1GB. Even chunk_size doesn't really matter. If I request 1 sec and then it says 30000 (because that is my sampling_rate) I'm like okay cool. I think this could be useful for debugging some users when they run into multiprocessing issues, but for me my concern is how much info is too much info for the user (that's why for this thing I would prefer a verbosity level. So users that really need to know this info can opt-in to more and more information).

But I'm not against this I just think there will be a subset of users who don't really care, but still want verbose=True to make sure the program is running.

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

I don't understand, right now the verbose=True is just showing the number of jobs which is information about parallel processing parameters. Do you think this is much info? What would you have under an hypotetical verbosity level=1?

My confusion is that I don't see how getting more information about the paralleling processing parameters (n_jobs vs n_jobs, memory, chunk_duration, etc) changes things qualitatively.

As in,

But I'm not against this I just think there will be a subset of users who don't really care, but still want verbose=True to make sure the program is running.

I am not sure the current verbose=True accomplishes this. No getting an assertion and the tqdm disaplay seem to do a better job at this.

@zm711
Copy link
Copy Markdown
Member

zm711 commented May 3, 2024

I think you're right. I don't actually see the value of verbose at all for this. I think tqdm is nice, but the other info is not needed so for me my hierarchy would be

verbose=0: Nothing
verbose=1: tqdm
verbose=2: everything you've posted

But since that isn't I withdraw my question :)

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

Yeah, sometimes verbose=True implies the tqdm but not here. I had this discussion with @CodyCBakerPhD once who did not want to set verbose=False by default because he (reasonably!) wanted to have tqdm enabled by default. I don't know if his thoughts have changed on the meanwhile.

So from your perspective, here we should have verbose=False by default as it provides not useful information for most users?

@zm711
Copy link
Copy Markdown
Member

zm711 commented May 3, 2024

Yeah, I think the information here is useful for people that need it but most users don't care about the inner working of the chunking so default don't give it to them and if someone wants it make them request it.

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

That makes sense to me. It seems that is False by default indeed, right?

chunk_duration_str = convert_seconds_to_str(chunk_duration)
print(
self.job_name,
"\n"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not sure 2 lines is a good idea because with the progressbar this will be 3 lines in total.

Copy link
Copy Markdown
Collaborator Author

@h-mayorquin h-mayorquin May 6, 2024

Choose a reason for hiding this comment

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

I am fine with reducing it to two. Any preferences @alejoe91 ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reverted to one line.

image

@samuelgarcia
Copy link
Copy Markdown
Member

This is cool. I think I prefer the one line version.

@alejoe91 alejoe91 merged commit 70c47f4 into SpikeInterface:main May 13, 2024
@h-mayorquin h-mayorquin deleted the improve_verbose_in_chunk_recording_executor branch May 21, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Changes to core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants