fix(cli): split Kokoro text on token overflow - #3041
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at head f318ff9b611a8fdbccf8caec47af73eadb33b81f.
Strengths
- The escaping across the TS -> Python boundary is correct in both directions, which is where embedded-script changes usually break.
\\datpackages/cli/src/tts/synthesize.ts:45emits\dinside a Python raw string, and\\nin the boundary class at:55emits a real newline inside the Python literal. I extracted the emittedSYNTH_SCRIPTand ranpy_compileon it - clean. - The catch is genuinely narrow (
synthesize.ts:43-48,:60-63): only anIndexErrorwhose message matches the 510-entry axis overflow is retried, and a piece shorter than two characters re-raises. synthesize.ts:52-57cannot produce an empty half.indexis constrained to0 < index < len-1, sosplit_atlands in[2, len-1]and both sides come out non-empty and strictly shorter. Recursion terminates for every input, including one with no boundary characters at all.
I exercised the emitted script against a stub model that raises the real NumPy message above a token budget. Eight properties hold: order preservation on Japanese and Western text, splitting exactly at the 。 boundaries, termination with no boundaries at all, termination at budget=1 (32 pieces), the original IndexError re-raised for text that can never fit rather than spinning, foreign IndexErrors propagating untouched, and the sample-rate guard at :69 firing on mismatch.
important - the new test asserts the source text of the file it is testing, so it cannot fail for any behavioral regression
synthesize.test.ts:127-135 reads synthesize.ts off disk and asserts four substrings are present. That passes if and only if the diff is present. None of the properties above are covered: a regex that stops matching (upstream changes size 510, or an escape regresses), a split_for_retry that returns an empty half, a concatenation that drops or reverses the right half, and the sample-rate guard are all invisible to it.
It also makes the body's "regression test RED before fix, GREEN after fix" claim true only in the trivial sense - it went red because the strings were not in the file yet.
The file already has the harness to do better: execFileSyncMock / getCapturedArgv at synthesize.test.ts:13-38 captures the argv handed to Python. Two cheap additions would carry real weight:
py_compilethe emittedSYNTH_SCRIPT. This catches the template-literal escaping class of bug, which is the failure that breaks synthesis for every user rather than only the overflow path.- Exec the emitted
split_for_retry/synthesize_textagainst a stub model and assert order preservation and termination.
Not gating the change - I verified the behaviour by hand and it is right - but the coverage as written will not hold that for the next person to touch this script.
nit - synthesize.test.ts:134 pins the cache filename
toContain('const SCRIPT_PATH = join(SCRIPT_DIR, "synth-v3.py")') couples this test to the script version. The suffix exists precisely so the body can change, so the next bump to synth-v4.py breaks an assertion that is not about versioning. Drop that line, or move it into a test that is about the cache upgrade.
nit - boundary-free text splits mid-word
When boundaries is empty, synthesize.ts:57 falls back to the raw character midpoint, so a 510+-token unbroken run (a long URL, unpunctuated CJK) is cut mid-word and the two fragments are synthesized independently, leaving an audible seam at the join. Rare, since whitespace already counts as a boundary - worth one line of comment recording that it is accepted.
Checked, not a finding
- Old cached scripts are not orphaned by the version bump.
ensureSynthScriptalready sweeps^synth(-v\d+)?\.py$and unlinks anything but the current name (synthesize.ts:106-117). That predates this PR; the bump inherits it. samples = list(left_samples)then.extend(...)returns a Python list where the direct path returns a NumPy array. Both downstream uses -sf.writeandlen(samples) / sample_rate- accept either, so the shape difference is inert.min(boundaries, ...)takes the leftmost boundary on an equidistant tie, so the split point is deterministic.
Verdict: APPROVE
Reasoning: The production change is correct and narrowly scoped - termination, order preservation and the narrowness of the catch all hold against the emitted script rather than against the body's description of it. The new test contributes no behavioural coverage, which is worth fixing but is not a defect in the change itself.
— Rames Jusso
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed at exact head. Kokoro's 510-entry voice token limit now triggers an adaptive binary split on sentence/clause boundaries, recursively re-synthesizing each half and concatenating the audio. The split selects the boundary closest to the midpoint, the rate consistency check prevents silent corruption, and the script version bumps to v3 so older installs auto-upgrade. Test verifies the key Python functions are present in the embedded script source.
— Miga
fixes reported:1785890373.136199:kokoro; reported:1785890373.136199:ffmpeg remains blocked
Source claim: https://slack.com/archives/C0BGC335AQY/p1785890373136199
Kokoro synthesis now catches only its 510-entry voice-token overflow, splits text near the midpoint at Japanese/Western sentence boundaries, synthesizes recursively, and concatenates the audio. Other IndexError failures still propagate. The cached bridge is bumped to synth-v3.py so existing installations receive the update.
Verification:
The separate Windows FFmpeg diagnostic claim is not changed by this PR and remains blocked on a Windows low-memory reproduction.