Skip to content

Refactor the preprocess CLI into core, with CLI and API shims - #885

Open
joanise wants to merge 6 commits into
mainfrom
dev.ej/fs2-prepro-v2
Open

joanise wants to merge 6 commits into
mainfrom
dev.ej/fs2-prepro-v2

Conversation

@joanise

@joanise joanise commented Sep 15, 2026

Copy link
Copy Markdown
Member

PR Goal?

I'm going to use stacked pull requests for the refactoring project. This PR primarily refactors preprocess() in all modules, as we discussed in the project to create a HuggingFace/workbook friendly API to EveryVoice. A number of other changes happened here too, and they're too intricately tied together to separate into multiple PRs, but I'm making a PR to create a "checkpoint" here for review.

  • In the base commands, load config and preprocess are now spliced so users of the API can create their config programmatically if they want, or load it once and reuse it for train/synthesize/etc. In future PRs train and synthesize will also make use of the load config base command instead of loading the config themselves.
  • We discussed removing merge_args, in order to make Intellisense happier. I discussed this with Sam and it's pointless because the API is going to get its own separate shim, so the CLI shim need not be optimized for it. I did those refactorings but then rolled them back because merge_args is a good solution for the CLI shim.
  • As I'm touching CLI things everywhere, I noticed we're repeating things too much. I figured a solution to use docstrings where the function is defined, and then depend on that as much as possible when creating the apps and sub-app groups.
  • In the same vein, if you run fs2l preprocess the help gives you examples using the fs2l command, and then for EV I use a string replacement to make it show the everyvoice command equivalent. I gave some thought on factoring those replace() commands out, but I didn't come up with a good solution yet.
  • The preprocess categories are now listed in a list[str] that the API will use as is, but the CLI dynamically constructs the Enum typer needs. That dynamic Enum is kinda ugly, but it's only for the CLI, we should not use it elsewhere, so I think it's appropriate. Maybe I should factor that block out to base_cli since it's the same 5 lines of code in 3 submodules.

Feedback sought?

This is going to be the basis of our bilat discussion Thursday. High level feedback about the approach is key at this point. Micro-level feedback about things I've changed will also be useful, as long as you keep in mind that train and synthesis functions are not done yet.

Priority?

If you can take a cursory look before we meet Thursday, great, but if not we'll look at it together on Thursday anyway.

Tests added?

no

The CLI is already well exercised. The API will need its own tests.

How to test?

The CLI should work as before.

Confidence?

medium

Version change?

yes, intermodule compatibility is going to be broken all over, so at the very least this represents a patch bump, if not a minor bump.

Related PRs?

EveryVoiceTTS/StyleTTS2#34
EveryVoiceTTS/FastSpeech2_lightning#164
EveryVoiceTTS/HiFiGAN_iSTFT_lightning#68

@joanise
joanise requested a review from roedoejet September 15, 2026 21:29
@joanise
joanise added this pull request to stack #886 September 15, 2026 21:29
@semanticdiff-com

semanticdiff-com Bot commented Sep 15, 2026

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  everyvoice/cli.py  23% smaller
  everyvoice/base_cli/helpers.py  12% smaller
  everyvoice/model/e2e/StyleTTS2_lightning  0% smaller
  everyvoice/model/feature_prediction/FastSpeech2_lightning  0% smaller
  everyvoice/model/vocoder/HiFiGAN_iSTFT_lightning  0% smaller
  everyvoice/utils/__init__.py  0% smaller

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor
CLI load time: 0:00.23
Pull Request HEAD: 4ce361dca75125fb573225e8ef4b15eeb4a5cd15

Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package
import time:      7165 |     211018 | everyvoice.cli
import time:      5191 |     115676 |   rich.markdown
import time:      4640 |     210709 | typer.rich_utils

@joanise joanise changed the title Dev.ej/fs2 prepro v2 Refactor the preprocess CLI into core, with CLI and API shims Sep 15, 2026
@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.40%. Comparing base (6b408c0) to head (4ce361d).

Files with missing lines Patch % Lines
everyvoice/base_cli/helpers.py 28.57% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #885      +/-   ##
==========================================
- Coverage   85.43%   85.40%   -0.04%     
==========================================
  Files          51       51              
  Lines        4780     4781       +1     
  Branches      710      711       +1     
==========================================
- Hits         4084     4083       -1     
- Misses        525      526       +1     
- Partials      171      172       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

== TargetTrainingTextRepresentationLevel.phonological_features
):
steps.append("pfs")
steps = [*steps, "pfs"]

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.

What's the difference here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We discussed it, but to put the answer here: it's the difference between mutating the caller's variable or not. append changes the caller's list, the alternative expansion does not. It's nitpicking, but it's cleaner design: a function ought not mutate it's caller's arguments unless it's a function whose task is to mutate the caller's arguments. And especially not if it doesn't document doing so.

@joanise
joanise force-pushed the dev.ej/fs2-prepro-v2 branch from 9e4fe65 to b7e594d Compare September 17, 2026 20:47
Base automatically changed from dev.ej/safe-refactors to main September 17, 2026 20:54
@joanise
joanise force-pushed the dev.ej/fs2-prepro-v2 branch from b7e594d to 931144b Compare September 17, 2026 20:55
In order to support a programmatic API for EveryVoice, we need
preprocess, train, synthesize to accept a Config object instead of the
name of a config file, so that a script using the API can a) create or
load the config, as desired, and then b) preprocess or train or
whatever.

This requires separating load_config_base_command from the other base
commands, to be called first in CLI shims, or separately in API shims.
@joanise
joanise force-pushed the dev.ej/fs2-prepro-v2 branch 2 times, most recently from 931144b to 08f41d5 Compare September 18, 2026 18:47

This branch has not been deployed

No deployments
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