Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: transcribe verbosity #140

Merged
merged 1 commit into from
Sep 26, 2022
Merged

fix: transcribe verbosity #140

merged 1 commit into from
Sep 26, 2022

Conversation

nick-konovalchuk
Copy link
Contributor

@nick-konovalchuk nick-konovalchuk commented Sep 26, 2022

I've hidden "Detected langauge: " massage behind verbose flag.
Also fixed the flag usage with tqdm

@jongwook
Copy link
Collaborator

I actually intended the two to be displayed even when --verbose is False, thinking it'd help the user notice any language detection errors and show the progress/ETA without flooding the console.

@nick-konovalchuk
Copy link
Contributor Author

nick-konovalchuk commented Sep 26, 2022

@jongwook well, ETA is disabled when verbose=True, which is misleading as it seem to me. Also, the language tag is returned as a part of the result. Do you really need to print it if you return it?

@jongwook
Copy link
Collaborator

jongwook commented Sep 26, 2022

The language tag is returned in the result, but for those calling whisper in the command line (as it seems to be a popular use case) the detail is lost.

Regarding ETA: that's true; it's not the most intuitively named options but I thought both formats have merits (like below) and thought it'd be nice to have these verbose/concise output options, but probably better not mix up those two.

image

We can maybe make it an integer level for verbosity if a totally silent output is desired by many ..

@nick-konovalchuk
Copy link
Contributor Author

nick-konovalchuk commented Sep 26, 2022

@jongwook
How about str verbosity like "full", "minimal" and "silent"? I might add it real quick

  • full being equivalent to verbosity=True
  • minimal being equivalent to verbosity=False
  • silent being equivalent to verbosity=False from current PR

@jongwook
Copy link
Collaborator

jongwook commented Sep 26, 2022

I'd like to avoid magic string if possible. It's a bit unorthodox but I'm inclined to making it an Optional[bool] and use None/False/True so verbose=None (i.e. silent) is the default in transcribe() but from the command line it selects either True or False.

I can make these edits on this PR if you'd like/don't mind.

@nick-konovalchuk
Copy link
Contributor Author

Don't bother yourself, I have nothing to do at the moment

@@ -170,7 +172,7 @@ def add_segment(
num_frames = mel.shape[-1]
previous_seek_value = seek

with tqdm.tqdm(total=num_frames, unit='frames', disable=verbose) as pbar:
with tqdm.tqdm(total=num_frames, unit='frames', disable=verbose is False) as pbar:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you make this verbose is not False? Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jongwook jongwook merged commit b4308c4 into openai:main Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants