Whisper: Support command line - #746
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.
I'm not sure I get the purpose of these. Shouldn't argparse handle the default=None case correctly already?
There was a problem hiding this comment.
argparse does handle the default=None case, but some fields do not default to None, such as the best-of field which defaults to 5, and others I have adjusted to be of type int.
| 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.
Also what is the purpose of this.. why not just use type=bool in the argparse argument?
There was a problem hiding this comment.
argparse currently does not support the type=bool type very well, so the type received during definition is still the string type.
awni
left a comment
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
I filed in a pull request fixing the current mismatch for other options in the help lines. #1390
|
@awni Sorry for the late reply. The main reason for the issue you raised is that I used I'm not sure if this processing meets your expectations, please let me know if there's anything inappropriate. |
awni
left a comment
There was a problem hiding this comment.
Thanks for the addition and sorry for the extremely delayed review!
sample:
output files:
for more details ues
mlx_whisper --help