Skip to content

Refactor train CLI into core, with CLI and API shims - #39

Open
joanise wants to merge 4 commits into
dev.ej/fs2-prepro-v2from
dev.ej/refactor-train
Open

joanise wants to merge 4 commits into
dev.ej/fs2-prepro-v2from
dev.ej/refactor-train

Conversation

@joanise

@joanise joanise commented Sep 23, 2026

Copy link
Copy Markdown
Member

As I prepare the refactor cli.train to core.train with cli and api
shims in every submodule, I also want to bring StyleTTS2 a little closer
to our standard where that makes sense.

@roedoejet roedoejet left a comment

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.

Generally looks good, same vein as the preprocess refactor. I think the model dump needs to be addressed and a couple other minor things. Also, my comment on the preprocess PR about sys.exit also applies here.

Comment thread styletts2/core/train.py
shutil.copy(config_file_s, os.path.join(log_dir, config_file_s))
# Log the full in-memory config object as really used, including any
# --config-args overrides on the CLI or programmatic changes to config
with open(

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.

I don't think we can yaml.dump here because that will miss the pydantic logic we have for exporting functions and paths correctly which is why we use config.model_dump_json() instead of model_dump(). Is there a reason not to use the base_cli.helpers.save_configuration_to_log_dir though? I think it would be good to use that fn across all train commands for all models.

Comment thread styletts2/core/train.py
os.makedirs(log_dir, exist_ok=True)
if config_file is not None:
config_file_s = str(config_file)
shutil.copy(config_file_s, os.path.join(log_dir, config_file_s))

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.

Hm, shouldn't this be os.path.join(log_dir, Path(config_file).name) or something similar? The old code was:

shutil.copy(str(config_file), os.path.join(log_dir, config_file.name))

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