Repository navigation
Conversation
e7e7928 to
bd26291
Compare
de660f1 to
32a9572
Compare
roedoejet
left a comment
roedoejet
left a comment
There was a problem hiding this comment.
looks great, like the other submodules. the big comment/request for change here though is that we're still importing fns that have sys.exit and I think those need to be systematically changed in core so that only things in the cli shim are causing exits.
| **everyvoice train text-to-wav config/{TEXT_TO_WAV_CONFIG_FILENAME_PREFIX}.yaml --mode first** | ||
| """, | ||
| )(app_train) | ||
| command(app)(preprocess) |
|
|
||
| from ..ev_config import StyleTTS2Config | ||
|
|
||
| config = load_config_base_command( |
There was a problem hiding this comment.
the core load_config_base_command still uses sys.exit(1) which is still CLI-centric. I think the base command needs to raise a ValidationError and then the CLI-shim needs to handle it (sys.exit is fine here). But sys.exit in the programmatic/notebook use is not good
There was a problem hiding this comment.
Right, that's a very good point.
All those sys.exit bother me anyway. Even in the CLI that's not what we should do: we should raise typer.BadParameter instead, because Typer handles that and gives better error messages to the user. I've started doing that for synthesize in dev.ej/refactor-synthesize, and I'll do it everywhere.
I agree for the API that it's a critical issue, we cannot have an API function called from a Jupyter notebook or wherever call sys.exit, that's just not cool.
There was a problem hiding this comment.
good point about just raising typer.BadParameter and let the CLI handle it!
| Your StyleTTS2 config file is called "config/everyvoice-text-to-wav.yaml" if it | ||
| was created using the "everyvoice new-project" wizard. | ||
|
|
||
| If you need to override any values, change them in the returned config object. |
There was a problem hiding this comment.
does changing/overriding values in this way still go through validation?
There was a problem hiding this comment.
I don't think so, no. But once we return the object to the user, they're free to use it, no?
We at least partly rely on intellisense guiding the user, but that's not the same.
What would be a better behaviour?
Actually, now I hesitate: I think --config-args on the CLI also bypasses the validation, since it is applied after the object is created and therefore after Pydantic validation has run.
| overwrite (bool): if false, existing files will be kept and only new files will be generated; | ||
| if true, redo all preprocessing, even if files already exist | ||
| debug (bool): enable debugging | ||
| """ |
There was a problem hiding this comment.
FS2 has this:
for step in steps:
if step not in PREPROCESS_CATEGORIES:
raise ValueError(f"Unknown step '{step}'")
which I think is helpful - could we add it here (and also in the HiFiGAN code?)
EveryVoiceTTS/EveryVoice#885