-
Notifications
You must be signed in to change notification settings - Fork 760
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
Whisper: Support command line #746
base: main
Are you sure you want to change the base?
Conversation
I implemented it with reference to the openai whisper project and tested most of the command parameters (such as |
def optional_int(string): | ||
return None if string == "None" else int(string) | ||
|
||
|
||
def optional_float(string): | ||
return None if string == "None" else float(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I get the purpose of these. Shouldn't argparse handle the default=None
case correctly already?
def str2bool(string): | ||
str2val = {"True": True, "False": False} | ||
if string in str2val: | ||
return str2val[string] | ||
else: | ||
raise ValueError(f"Expected one of {set(str2val.keys())}, got {string}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what is the purpose of this.. why not just use type=bool
in the argparse argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really nice. I left a couple minor comments. Could you please check? After that I think we can merge it. Thanks for the addition!
help="the path to save model files, or the hugging face repo id to use", | ||
) | ||
parser.add_argument( | ||
"--output_dir", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of our CLI tools use -
instead of _
. For consistency I would suggest we do the same for the arguments here. Or did you get this from the original Whisper implementation? Maybe it is good to be consistent with that if so?
sample:
output files:
for more details ues
mlx_whisper --help