Skip to content

Commit 1ea48f3

Browse files
shaypal5claude
andauthored
fix(labels,qa): #109 — drop hardcoded tts_engine, derive single_backend from generation_metadata.tts_backend (#112)
* fix(labels,qa): #109 — drop hardcoded tts_engine, derive single_backend from generation_metadata.tts_backend ClipMetadata.tts_engine was always written as the literal string "azure_he_IL" regardless of which TTS provider actually rendered the clip. This mis-labeled every google-rendered clip and caused qa-report's run-level `single_backend` warning to fire as a false positive on mixed-provider runs (see delivery-003 in avdp-synth-corpus#4, where 20 mixed-backend clips were tagged `"clips_by_tts_engine": {"azure_he_IL": 20}` and triggered `single_backend`). Per the issue's recommended option (a): drop `tts_engine` as the primary signal. The structured per-speaker `tts_backend` map already lives in `generation_metadata` and carries richer information; the flat field was dead weight. Changes: - synthbanshee/labels/schema.py: removed `tts_engine` field from `ClipMetadata`; removed from the ascii-safe `field_validator`. Old corpus JSON still parses cleanly (Pydantic ignores unknown fields). - synthbanshee/labels/generator.py: dropped the hardcoded `tts_engine="azure_he_IL"` line in `generate_clip_metadata`. - synthbanshee/package/qa.py: backend diversity now derived from `metadata.generation_metadata.tts_backend.values()`. `RunSummary` field renamed `clips_by_tts_engine` → `clips_by_tts_backend` (a clip with mixed providers contributes one count to each backend). - synthbanshee/cli.py: qa-report renderer + JSON output updated for the renamed field. - docs/spec.md §5.1: removed `"tts_engine": "azure_he_IL"` from the example JSON; added a field note pointing readers to `generation_metadata.tts_backend`. Old corpus snapshots still carrying the field should be tolerated and ignored. - AGENTS.md (= CLAUDE.md): removed `tts_engine` from the ascii-safe validator list. - tests/unit/test_qa.py: helper `_write_valid_clip` replaced `tts_engine` kwarg with `tts_backend` (per-speaker map); every clip now emits `generation_metadata` so qa derivation has a source of truth. Added two new tests under TestRunQARunSummary: `single_backend` does NOT fire when two clips use different backends; `single_backend` DOES fire when both clips are azure. Existing `test_run_summary_backend_diversity` updated for the new field name. - tests/unit/test_labels.py: added `test_tts_engine_field_removed` asserting the attribute is absent on freshly-built ClipMetadata and that legacy JSON carrying the old field still parses cleanly. - tests/unit/{test_cli,test_manifest,test_generation_metadata}.py: dropped `tts_engine` from minimal-clip JSON fixtures. Fixes #109. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(labels,qa): #109 — bucket legacy clips as "unknown", harden tests, document break Self-review caught five gaps in the first cut. This commit addresses them: 1. Pre-#109 clips (no ``generation_metadata``) now bucket under the ``"unknown"`` backend key in ``RunSummary.clips_by_tts_backend`` instead of silently contributing zero. Previously, a corpus mixing pre- and post-#109 clips would lose the legacy clips from ``backend_count``, making ``single_backend`` flip the wrong way. 2. Updated ``RunSummary.clips_by_tts_backend`` docstring to: - flag the ``sum(values) >= total_clips`` invariant change (mixed- backend clips contribute +1 to each backend, so the pre-#109 sum-equals-total invariant no longer holds); - document the ``"unknown"`` bucket for missing generation_metadata. 3. ``test_tts_engine_field_removed`` now explicitly exercises ``with pytest.raises(AttributeError): m.tts_engine`` for both fresh models and ones parsed from legacy JSON. The previous version only asserted ``not hasattr(...)``, which would silently pass if Pydantic ever shipped ``extra="allow"`` semantics by accident. The new test makes the breaking-change contract explicit. 4. New ``test_pipeline_output_feeds_qa_backend_derivation`` integration test runs the full mocked ``_run_generate_pipeline`` end-to-end and then feeds the resulting clip directory to ``run_qa(run_summary=True)``, asserting ``"azure"`` lands in ``clips_by_tts_backend`` and ``"unknown"`` does NOT. Catches the regression where the generator's ``_backends_map`` shape diverges from qa.py's expectation — both unit-tested components could pass in isolation while the integration silently breaks. 5. New ``test_run_summary_clips_without_generation_metadata_bucket_as_unknown`` and ``test_run_summary_all_legacy_clips_fire_single_backend`` pin the unknown-bucket contract: legacy clips remain visible to diversity metrics, and an all-legacy corpus still correctly fires ``single_backend``. Full unit suite: 1702/1702 passing (up from 1699 — three new tests). ruff + mypy clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 58b047f commit 1ea48f3

11 files changed

Lines changed: 205 additions & 28 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ docs/ tests/unit/ tests/integration/
2727
- **Filenames:** strictly lowercase `[a-z0-9_-]`
2828
- **JSON string fields validated by `ClipMetadata`:** no characters above U+00A1
2929
- Hebrew text goes in `.j2` templates or `.txt` transcripts only
30-
- `ClipMetadata` enforces the string-field rule via `@field_validator` on `clip_id`, `project`, `tts_engine`, `violence_typology`, `generator_version`
30+
- `ClipMetadata` enforces the string-field rule via `@field_validator` on `clip_id`, `project`, `violence_typology`, `generator_version`
3131

3232
## Audio format (hard constraints)
3333

‎docs/spec.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,6 @@ Every clip has a companion `{clip_id}.json` file with this schema:
284284
"generation_date": "2026-04-10",
285285
"generator_version": "0.1.0",
286286
"is_synthetic": true,
287-
"tts_engine": "azure_he_IL",
288287
"acoustic_scene": {
289288
"room_type": "apartment_kitchen",
290289
"device": "phone_in_pocket",
@@ -345,6 +344,7 @@ Every clip has a companion `{clip_id}.json` file with this schema:
345344
**Field notes**
346345

347346
- `preprocessing_applied.normalized_dbfs` is the **measured** post-preprocess peak (pair with `generation_metadata.loudness_target_peak_dbfs` to compute drift from target — see `labels/schema.py` for the docstring that pins this split).
347+
- `tts_engine` was **removed in #109**. The TTS provider is now recorded per-speaker in `generation_metadata.tts_backend` (e.g. `{"AGG_M_30-45_001": "azure", "VIC_F_25-40_002": "google"}`); read backend diversity from the structured map. Pre-#109 corpus snapshots still carry the field — consumers should tolerate but ignore it.
348348
- `generation_metadata` is **optional**: a JSON object when the generator recorded pipeline provenance, `null` otherwise. Treat absence as "unknown", not as failure. `generator_version` alone is not a reliable presence signal.
349349
- `speakers[].voice_family` is **optional**: a stable family handle (e.g. `"Avri"`) when the speaker YAML overrides it, omitted otherwise. Consumers should fall back to `tts_voice_id`.
350350
- `weak_label.has_violence` is **derived**, not asserted: `any(e.tier1_category != "NONE" for e in events)` — see `synthbanshee/labels/generator.py`. Corollaries: empty `events` → `False`; `NEG` typology clips are `False` (every event lands `tier1_category: "NONE"` by §4.1); `violence_typology` and `has_violence` may disagree (e.g. `SV` with `False` if no violent tier1 fired). The events are the ground truth; the flag is convenience. **External docs and downstream code must mirror this rule** — re-deriving from typology or intensity alone produces disagreement on every NEG row.

‎synthbanshee/cli.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1702,8 +1702,8 @@ def qa_report(
17021702
else str(rs.backend_count)
17031703
)
17041704
t_run.add_row("TTS backends", be_label)
1705-
for engine, count in sorted(rs.clips_by_tts_engine.items()):
1706-
t_run.add_row(f" {engine}", str(count))
1705+
for backend, count in sorted(rs.clips_by_tts_backend.items()):
1706+
t_run.add_row(f" {backend}", str(count))
17071707

17081708
# Overlap and emotion-downgrade ratios
17091709
ovr_label = (
@@ -1806,7 +1806,7 @@ def qa_report(
18061806
],
18071807
"voices_by_gender": rs.voices_by_gender,
18081808
"backend_count": rs.backend_count,
1809-
"clips_by_tts_engine": rs.clips_by_tts_engine,
1809+
"clips_by_tts_backend": rs.clips_by_tts_backend,
18101810
"overlap_ratio": rs.overlap_ratio,
18111811
"clips_with_i4_plus": rs.clips_with_i4_plus,
18121812
"emotion_downgrade_ratio": rs.emotion_downgrade_ratio,

‎synthbanshee/labels/generator.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,8 @@ def generate_clip_metadata(
261261
generation_date=datetime.date.today().isoformat(),
262262
generator_version=self.generator_version,
263263
is_synthetic=True,
264-
tts_engine="azure_he_IL",
264+
# #109: tts_engine removed; provider is recorded per-speaker in
265+
# generation_metadata.tts_backend.
265266
acoustic_scene=acoustic_scene or ClipAcousticScene(),
266267
speakers=speakers or [],
267268
weak_label=WeakLabel(

‎synthbanshee/labels/schema.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,12 @@ class ClipMetadata(BaseModel):
210210
generation_date: str
211211
generator_version: str
212212
is_synthetic: Literal[True] = True
213-
tts_engine: str = "azure_he_IL"
213+
# #109: tts_engine removed — it was a per-clip flat string hardcoded to
214+
# "azure_he_IL" regardless of the actual provider, which mis-labeled
215+
# google-rendered clips and made qa-report's single_backend warning a
216+
# false positive. The structured per-speaker source of truth is
217+
# ``generation_metadata.tts_backend``; downstream tools read backend
218+
# diversity from there.
214219
acoustic_scene: ClipAcousticScene = Field(default_factory=ClipAcousticScene)
215220
speakers: list[SpeakerInfo] = Field(default_factory=list)
216221
weak_label: WeakLabel
@@ -252,7 +257,7 @@ def valid_quality_flags(cls, v: list[str]) -> list[str]:
252257
raise ValueError(f"Unknown quality_flags: {bad}. Valid: {sorted(_QUALITY_FLAGS)}")
253258
return v
254259

255-
@field_validator("clip_id", "project", "tts_engine", "violence_typology", "generator_version")
260+
@field_validator("clip_id", "project", "violence_typology", "generator_version")
256261
@classmethod
257262
def ascii_safe_string(cls, v: str, info) -> str:
258263
return _assert_ascii_safe(v, info.field_name)

‎synthbanshee/package/qa.py‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,17 @@ class RunSummary:
127127
# Voice and backend diversity
128128
voices_by_gender: dict[str, int] = field(default_factory=dict)
129129
backend_count: int = 0
130-
clips_by_tts_engine: dict[str, int] = field(default_factory=dict)
130+
# #109: histogram of (clip, distinct backend used) pairs, derived from
131+
# generation_metadata.tts_backend.values() rather than the old hardcoded
132+
# ClipMetadata.tts_engine field. A clip with two speakers on different
133+
# providers contributes one count to each backend, so
134+
# ``sum(clips_by_tts_backend.values()) >= total_clips`` — the pre-#109
135+
# sum invariant no longer holds. Pre-#109 corpus clips (which lack
136+
# ``generation_metadata`` entirely) are bucketed under the
137+
# ``"unknown"`` backend key so they still appear in the histogram and
138+
# in ``backend_count``; otherwise they would silently vanish from
139+
# diversity metrics whenever an old + new corpus is mixed.
140+
clips_by_tts_backend: dict[str, int] = field(default_factory=dict)
131141

132142
# Overlap and emotion-downgrade ratios
133143
overlap_ratio: float = 0.0
@@ -366,8 +376,8 @@ def run_qa(
366376
# M10b accumulators
367377
_all_turns: list[TurnMetrics] = []
368378
_voices_by_gender: dict[str, set[str]] = defaultdict(set)
369-
_tts_engines: set[str] = set()
370-
_clips_by_engine: dict[str, int] = defaultdict(int)
379+
_tts_backends: set[str] = set()
380+
_clips_by_backend: dict[str, int] = defaultdict(int)
371381
_clips_with_overlap: int = 0
372382
_clips_with_emotion_downgrade: int = 0
373383
_clips_with_i4_plus: int = 0 # denominator for overlap ratio
@@ -421,10 +431,22 @@ def run_qa(
421431
if any("Strong labels JSONL missing" in w for w in validation.warnings):
422432
stats.clips_missing_strong_labels += 1
423433

424-
# M10b: track voice and backend diversity
434+
# M10b: track voice and backend diversity (#109 — derive backend
435+
# from generation_metadata.tts_backend per-speaker map, not the
436+
# removed flat tts_engine field). Clips that lack
437+
# generation_metadata (pre-#109 corpus snapshots) bucket under
438+
# "unknown" so they remain visible in the histogram rather than
439+
# silently vanishing.
425440
if run_summary:
426-
_tts_engines.add(metadata.tts_engine)
427-
_clips_by_engine[metadata.tts_engine] += 1
441+
if metadata.generation_metadata is not None and (
442+
metadata.generation_metadata.tts_backend
443+
):
444+
clip_backends = set(metadata.generation_metadata.tts_backend.values())
445+
else:
446+
clip_backends = {"unknown"}
447+
_tts_backends.update(clip_backends)
448+
for backend in clip_backends:
449+
_clips_by_backend[backend] += 1
428450
for spk in metadata.speakers:
429451
_voices_by_gender[spk.gender].add(spk.tts_voice_id)
430452

@@ -520,8 +542,8 @@ def run_qa(
520542
summary = RunSummary(
521543
role_intensity_stats=ri_stats,
522544
voices_by_gender={g: len(v) for g, v in _voices_by_gender.items()},
523-
backend_count=len(_tts_engines),
524-
clips_by_tts_engine=dict(_clips_by_engine),
545+
backend_count=len(_tts_backends),
546+
clips_by_tts_backend=dict(_clips_by_backend),
525547
overlap_ratio=(
526548
_clips_with_overlap / _clips_with_i4_plus if _clips_with_i4_plus > 0 else 0.0
527549
),

‎tests/unit/test_cli.py‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,6 @@ def _write_valid_clip(tmp_path: Path, clip_id: str = "test_clip_01") -> Path:
121121
"generation_date": datetime.date.today().isoformat(),
122122
"generator_version": "0.1.0",
123123
"is_synthetic": True,
124-
"tts_engine": "azure_he_IL",
125124
"acoustic_scene": {},
126125
"speakers": [],
127126
"weak_label": {
@@ -1172,6 +1171,46 @@ def test_metadata_paths_fall_back_to_absolute_when_outside_root(self, tmp_path):
11721171
# Out-of-root → absolute fallback. Not great, but not a crash.
11731172
assert Path(meta_json["transcript_path"]).is_absolute()
11741173

1174+
def test_pipeline_output_feeds_qa_backend_derivation(self, tmp_path):
1175+
"""End-to-end #109 contract: a clip produced by ``_run_generate_pipeline``
1176+
carries ``generation_metadata.tts_backend`` whose values land in
1177+
``RunSummary.clips_by_tts_backend`` when fed to ``run_qa``.
1178+
1179+
Catches the regression where the generator's ``_backends_map``
1180+
shape diverges from qa.py's expectation — both unit-tested
1181+
components could pass in isolation while the integration breaks.
1182+
"""
1183+
from synthbanshee.package.qa import run_qa
1184+
1185+
turns = _make_dialogue_turns(n=1)
1186+
mixed = _make_mixed_scene(n_turns=1)
1187+
with (
1188+
patch("synthbanshee.script.generator.ScriptGenerator") as MockGen,
1189+
patch("synthbanshee.tts.renderer.TTSRenderer") as MockRenderer,
1190+
):
1191+
MockGen.return_value.generate.return_value = turns
1192+
MockRenderer.return_value.render_scene.return_value = mixed
1193+
wav, _ = _run_generate_pipeline(
1194+
SCENES_DIR / "test_scene_001.yaml",
1195+
tmp_path / "out",
1196+
tmp_path / "cache",
1197+
tmp_path / "dirty",
1198+
tmp_path / "scripts",
1199+
)
1200+
1201+
assert wav is not None
1202+
report = run_qa(tmp_path / "out", run_summary=True)
1203+
rs = report.run_summary
1204+
assert rs is not None, "run_summary should be populated when flag is set"
1205+
# The example speaker (AGG_M_30-45_001) is azure-backed in the
1206+
# example YAML; the generator writes that into tts_backend, and
1207+
# qa derives the histogram from there.
1208+
assert "azure" in rs.clips_by_tts_backend, f"expected 'azure' in {rs.clips_by_tts_backend}"
1209+
assert "unknown" not in rs.clips_by_tts_backend, (
1210+
"fresh clips should not bucket as 'unknown' — the generator "
1211+
"must write generation_metadata.tts_backend per #109"
1212+
)
1213+
11751214

11761215
# ---------------------------------------------------------------------------
11771216
# generate command — additional failure and warning branches

‎tests/unit/test_generation_metadata.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ def _minimal_clip_metadata(**overrides) -> dict:
2525
"generation_date": "2026-05-01",
2626
"generator_version": "0.1.0",
2727
"is_synthetic": True,
28-
"tts_engine": "azure_he_IL",
2928
"weak_label": {
3029
"has_violence": False,
3130
"violence_categories": [],

‎tests/unit/test_labels.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
from __future__ import annotations
44

5+
import json
6+
57
import numpy as np
68
import pytest
79
from pydantic import ValidationError
@@ -169,6 +171,36 @@ def test_ascii_safe_clip_id(self):
169171
with pytest.raises(ValidationError):
170172
_make_metadata(clip_id="\u05e9\u05dc\u05d5\u05dd")
171173

174+
def test_tts_engine_field_removed(self):
175+
"""#109: the hardcoded ``tts_engine`` field was dropped \u2014 the
176+
per-speaker ``generation_metadata.tts_backend`` is the source of
177+
truth. Old corpus JSON carrying the field still parses (Pydantic
178+
ignores unknown fields by default); the attribute is no longer
179+
exposed on the model.
180+
181+
This test pins the **breaking-change** half of the contract:
182+
downstream code that touches ``meta.tts_engine`` must be
183+
updated, and an ``AttributeError`` is what they will hit. The
184+
test would silently pass even if Pydantic re-introduced the
185+
field with ``extra="allow"`` semantics \u2014 we exercise the
186+
attribute access explicitly to catch that drift.
187+
"""
188+
m = _make_metadata()
189+
assert not hasattr(m, "tts_engine")
190+
# Touching the attribute must raise AttributeError \u2014 this is
191+
# the silent break a senior reviewer asked be made explicit.
192+
with pytest.raises(AttributeError):
193+
_ = m.tts_engine # type: ignore[attr-defined]
194+
195+
# Old clip JSON with the legacy field still parses cleanly
196+
# (Pydantic v2's default extra="ignore" drops the unknown field).
197+
legacy_json = json.loads(m.model_dump_json())
198+
legacy_json["tts_engine"] = "azure_he_IL"
199+
m2 = ClipMetadata.model_validate_json(json.dumps(legacy_json))
200+
assert m2.clip_id == m.clip_id
201+
with pytest.raises(AttributeError):
202+
_ = m2.tts_engine # type: ignore[attr-defined]
203+
172204

173205
# ---------------------------------------------------------------------------
174206
# LabelGenerator tests

‎tests/unit/test_manifest.py‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ def _write_valid_clip(
5858
"generation_date": datetime.date.today().isoformat(),
5959
"generator_version": "0.1.0",
6060
"is_synthetic": True,
61-
"tts_engine": "azure_he_IL",
6261
"acoustic_scene": {},
6362
"speakers": [
6463
{
@@ -360,7 +359,6 @@ def test_json_without_sibling_wav_is_skipped(self, tmp_path):
360359
"generation_date": datetime.date.today().isoformat(),
361360
"generator_version": "0.1.0",
362361
"is_synthetic": True,
363-
"tts_engine": "azure_he_IL",
364362
"acoustic_scene": {},
365363
"speakers": [
366364
{

0 commit comments

Comments
 (0)