-
Notifications
You must be signed in to change notification settings - Fork 6.3k
test: add evaluation coverage and pre-commit tooling #1033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| bunx lint-staged | ||
| bun run typecheck | ||
| python3 -m pytest backend/tests/test_fixture_contract.py -q | ||
| ruff check backend/tests/conftest.py backend/tests/test_generation_unload.py backend/tests/test_mcp_mount_slashes.py backend/tests/test_fixture_contract.py backend/tests/test_tts_vram_churn.py backend/tests/test_whisper_long_audio_e2e.py |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "*": "prettier --ignore-unknown --write" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "useTabs": false, | ||
| "tabWidth": 2, | ||
| "printWidth": 80, | ||
| "singleQuote": false, | ||
| "trailingComma": "es5", | ||
| "semi": true, | ||
| "arrowParens": "always" | ||
| } |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -296,7 +296,13 @@ def _load_model_sync(self, model_size: str): | |
| logger.info("Loading Whisper model %s on %s...", model_size, self.device) | ||
|
|
||
| self.processor = WhisperProcessor.from_pretrained(model_name) | ||
| self.model = WhisperForConditionalGeneration.from_pretrained(model_name) | ||
| # Usar bfloat16 en lugar de float32 para ahorrar VRAM en GPUs | ||
| # modernas (RTX 40/50, Ada/Blackwell) sin incurrir en el bug de | ||
| # dtype de float16 (c10::Half) que se da en esta arquitectura. | ||
| self.model = WhisperForConditionalGeneration.from_pretrained( | ||
| model_name, | ||
| torch_dtype=torch.bfloat16, | ||
| ) | ||
|
|
||
| self.model.to(self.device) | ||
| self.model_size = model_size | ||
|
|
@@ -342,37 +348,59 @@ def _transcribe_sync(): | |
| # state — forcing offline here (issue #462) broke online users | ||
| # whose `get_decoder_prompt_ids` / tokenizer calls issue | ||
| # legitimate metadata lookups. | ||
| # Process audio | ||
| inputs = self.processor( | ||
| audio, | ||
| sampling_rate=16000, | ||
| return_tensors="pt", | ||
| ) | ||
| inputs = inputs.to(self.device) | ||
| # Whisper's encoder only accepts 30s of audio per pass (3000 mel | ||
| # frames); feeding it a longer clip silently transcribes just the | ||
| # first 30 seconds. Chunk into 30s windows and join the results. | ||
| chunk_samples = 30 * 16000 | ||
| chunks = [ | ||
| audio[i : i + chunk_samples] | ||
| for i in range(0, len(audio), chunk_samples) | ||
| ] or [audio] | ||
|
|
||
| # Generate transcription | ||
| # If language is provided, force it; otherwise let Whisper auto-detect | ||
| generate_kwargs = {} | ||
| generate_kwargs = { | ||
| # Sin timestamps, Whisper a veces emite <|endoftext|> tras la | ||
| # primera frase de una ventana y descarta el resto del audio. | ||
| # Forzar timestamps obliga al modelo a recorrer la ventana | ||
| # completa en cada chunk. | ||
| "return_timestamps": True, | ||
| } | ||
| if language: | ||
| forced_decoder_ids = self.processor.get_decoder_prompt_ids( | ||
| language=language, | ||
| task="transcribe", | ||
| ) | ||
| generate_kwargs["forced_decoder_ids"] = forced_decoder_ids | ||
|
|
||
| with torch.no_grad(): | ||
| predicted_ids = self.model.generate( | ||
| inputs["input_features"], | ||
| **generate_kwargs, | ||
| transcriptions = [] | ||
| for chunk in chunks: | ||
| # Process audio | ||
| inputs = self.processor( | ||
| chunk, | ||
| sampling_rate=16000, | ||
| return_tensors="pt", | ||
| ) | ||
| # Mover al device y convertir al mismo dtype que el modelo para | ||
| # evitar el error "Input type (float) and bias type (...) should be | ||
| # the same" en GPUs como la RTX 5060. | ||
| inputs = inputs.to(device=self.device, dtype=self.model.dtype) | ||
|
|
||
| with torch.no_grad(): | ||
| predicted_ids = self.model.generate( | ||
| inputs["input_features"], | ||
| **generate_kwargs, | ||
| ) | ||
|
|
||
| # Decode | ||
| transcriptions.append( | ||
| self.processor.batch_decode( | ||
| predicted_ids, | ||
| skip_special_tokens=True, | ||
| )[0] | ||
| ) | ||
|
|
||
| # Decode | ||
| transcription = self.processor.batch_decode( | ||
| predicted_ids, | ||
| skip_special_tokens=True, | ||
| )[0] | ||
|
|
||
| return transcription.strip() | ||
| return " ".join(t.strip() for t in transcriptions).strip() | ||
|
Comment on lines
+376
to
+403
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift Serialize model unload with active transcription. The loop repeatedly reads Protect model load, unload, and inference with one shared lifecycle guard. Make Based on learnings: serialize model swap with in-flight operations because reload/unload can race inference references. 🤖 Prompt for AI AgentsSource: Learnings |
||
|
|
||
| # Run blocking transcription in thread pool | ||
| return await asyncio.to_thread(_transcribe_sync) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,11 +9,12 @@ | |
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| from contextlib import AsyncExitStack, asynccontextmanager | ||
| from collections.abc import Callable | ||
| from contextlib import AsyncExitStack, asynccontextmanager | ||
|
|
||
| from fastapi import FastAPI | ||
| from fastmcp import FastMCP | ||
| from starlette.types import ASGIApp, Receive, Scope, Send | ||
|
|
||
| from .context import ClientIdMiddleware | ||
| from .tools import register_tools | ||
|
|
@@ -22,6 +23,31 @@ | |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class MountRootSlashRewrite: | ||
| """ASGI wrapper that maps the bare mount root onto FastMCP's ``/`` route. | ||
|
|
||
| Starlette's ``Mount`` strips the ``/mcp`` prefix, so ``POST /mcp`` (no | ||
| trailing slash) arrives inside the sub-application with ``path == ""`` | ||
| and FastMCP's router — which only knows ``/`` — answers 405. Most MCP | ||
| clients (Claude Code, Cursor, …) point at the bare ``/mcp`` URL and do | ||
| not follow 307 redirects on POST, so we rewrite the empty path to ``/`` | ||
| instead of redirecting. | ||
| """ | ||
|
|
||
| def __init__(self, app: ASGIApp) -> None: | ||
| self.app = app | ||
|
|
||
| async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | ||
| if scope["type"] == "http" and scope.get("path") == "": | ||
| scope = dict(scope) | ||
| scope["path"] = "/" | ||
| # Keep path and raw_path coherent for ASGI routers that inspect | ||
| # both fields (Starlette normally supplies raw_path from the | ||
| # original request, e.g. b"/mcp"). | ||
| scope["raw_path"] = b"/" | ||
|
Comment on lines
+41
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== file excerpt =="
if [ -f backend/mcp_server/server.py ]; then
cat -n backend/mcp_server/server.py | sed -n '1,120p'
else
echo "backend/mcp_server/server.py not found"
fi
echo
echo "== ASGI path/raw_path usages =="
rg -n --hidden --glob '!*.pyc' 'raw_path|scope\["path"\]|scope\["raw_path"\]|\["raw_path"\]' backend || true
echo
echo "== repository dependencies mentioning starlette/fastmcp/ASGI (if present) =="
for f in pyproject.toml backend/pyproject.toml requirements.txt backend/requirements.txt uv.lock backend/uv.lock poetry.lock backend/poetry.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n "starlette|fastmcp|asgi" "$f" || true
fi
done
echo
echo "== current git status/diff summary =="
git diff --stat || true
git diff -- backend/mcp_server/server.py | sed -n '1,220p' || trueRepository: jamiepine/voicebox Length of output: 5976 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
import re
path = pathlib.Path("backend/mcp_server/server.py")
if not path.exists():
print("backend/mcp_server/server.py missing")
raise SystemExit
text = path.read_text()
m = re.search(r'def MountRootSlashRewrite\(app\):.*?(?=\ndef |\Z)', text, re.S)
print("MountRootSlashRewrite function present:", bool(m))
if m:
body = m.group(0)
print("contains scope copy:", bool(re.search(r'scope\s*=\s*dict\(scope\)', body)))
print("contains path rewrite:", bool(re.search(r'scope\["path"\]\s*=\s*"/"', body)))
print("contains raw_path rewrite:", bool(re.search(r'scope\["raw_path"\]\s*=\s*b"/"', body)))
# Deterministic probe of the exact transformation for a bare mounted request.
initial = {"type": "http", "path": "", "raw_path": b"/mcp"}
if initial["type"] == "http" and initial.get("path") == "":
scope = dict(initial)
scope["path"] = "/"
scope["raw_path"] = b"/"
else:
scope = initial
print("initial raw_path:", initial["raw_path"])
print("transformed raw_path:", scope["raw_path"])
print("child_match_path:", scope["path"])
PYRepository: jamiepine/voicebox Length of output: 275 🌐 Web query:
💡 Result: In the ASGI specification, raw_path is an optional field within the connection scope [1][2]. It is defined as a byte string representing the original HTTP path component exactly as it was received by the web server, without modification, unquoting, or percent-decoding [1][3]. Key characteristics of raw_path include: 1. Data Type: It is provided as a byte string, distinct from the path field, which is a Unicode string that has been fully decoded from both UTF-8 and percent-encoded sequences [1][2]. 2. Scope: It excludes the query string [1][3]. 3. Optionality: Because some web server implementations may be unable to provide the original raw bytes or may perform their own path normalization before the application layer is reached, raw_path is optional [1][4]. If a server cannot provide it, the field may be missing or set to None [1][3]. The addition of raw_path to the ASGI specification was intended to allow applications to access the path without the information loss that occurs when a server automatically performs percent-decoding on the standard path field [4][5]. However, developers should note that because intermediate proxies or the underlying web server might modify the path before it reaches the ASGI interface, the availability and exact content of raw_path can depend on the specific server implementation [1][5]. Citations:
Preserve the original ASGI For a bare mount request, 🤖 Prompt for AI Agents |
||
| await self.app(scope, receive, send) | ||
|
|
||
|
|
||
| def build_mcp_server() -> FastMCP: | ||
| """Create the FastMCP instance with Voicebox tools registered.""" | ||
| mcp = FastMCP( | ||
|
|
@@ -55,7 +81,7 @@ def mount_into( | |
| # by the time tool handlers execute. Starlette composes middlewares | ||
| # outermost-first, so adding here on the parent app is correct. | ||
| app.add_middleware(ClientIdMiddleware) | ||
| app.mount("/mcp", mcp_app) | ||
| app.mount("/mcp", MountRootSlashRewrite(mcp_app)) | ||
| app.state.mcp_lifespan = mcp_app.router.lifespan_context | ||
| logger.info("MCP: mounted at /mcp (FastMCP %s)", getattr(mcp, "version", "")) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import logging | ||
| import traceback | ||
| from typing import Literal, Optional | ||
|
|
||
|
|
@@ -25,6 +26,26 @@ | |
| from ..database import get_db | ||
| from ..utils.tasks import get_task_manager | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| #: tada degenerates on long inputs (produces ~1s of audio or hallucinated | ||
| #: text), so its chunks are capped well below the global 800-char default. | ||
| TADA_MAX_CHUNK_CHARS = 250 | ||
|
|
||
|
|
||
| def effective_max_chunk_chars(engine: str, requested: Optional[int]) -> Optional[int]: | ||
| """Resolve the chunk size used for long-text TTS splitting. | ||
|
|
||
| ``GenerationRequest.max_chunk_chars`` defaults to 800, so callers almost | ||
| never pass ``None``. For tada we force :data:`TADA_MAX_CHUNK_CHARS` | ||
| unless the caller explicitly asked for an even smaller chunk. | ||
|
|
||
| Returns the requested value unchanged for other engines. | ||
| """ | ||
| if engine == "tada" and (requested is None or requested > TADA_MAX_CHUNK_CHARS): | ||
| return TADA_MAX_CHUNK_CHARS | ||
| return requested | ||
|
|
||
|
|
||
| async def run_generation( | ||
| *, | ||
|
|
@@ -55,6 +76,8 @@ async def run_generation( | |
| load_engine_model, | ||
| ) | ||
| from ..utils.chunked_tts import generate_chunked | ||
|
|
||
| max_chunk_chars = effective_max_chunk_chars(engine, max_chunk_chars) | ||
| from ..utils.audio import has_tts_runaway, normalize_audio, save_audio, trim_tts_output | ||
|
|
||
| task_manager = get_task_manager() | ||
|
|
@@ -156,6 +179,14 @@ async def run_generation( | |
| finally: | ||
| task_manager.complete_generation(generation_id) | ||
| bg_db.close() | ||
| # Liberar TODA la VRAM tras cada generación (TTS + Whisper + LLM). | ||
| # Liberar solo el motor usado dejaba Whisper cargado y la GPU de 8 GB | ||
| # acababa en CUDA OOM al encadenar varias generaciones. | ||
| try: | ||
| from ..backends import unload_all_models | ||
| unload_all_models() | ||
| except Exception: | ||
| logger.warning("Failed to unload models after generation", exc_info=True) | ||
|
|
||
|
|
||
| def _notify_speak_end(generation_id: str, *, status: str) -> None: | ||
|
|
@@ -281,6 +312,8 @@ async def generate_audio_sync( | |
| load_engine_model, | ||
| ) | ||
| from ..utils.chunked_tts import generate_chunked | ||
|
|
||
| max_chunk_chars = effective_max_chunk_chars(engine, max_chunk_chars) | ||
| from ..utils.audio import has_tts_runaway, normalize_audio, trim_tts_output | ||
| from . import tts | ||
|
|
||
|
|
@@ -297,6 +330,13 @@ async def generate_audio_sync( | |
| ) | ||
| finally: | ||
| bg_db.close() | ||
| # Unload TTS model to free VRAM after speak | ||
| try: | ||
| tts_backend = get_tts_backend_for_engine(engine) | ||
| if tts_backend.is_loaded(): | ||
| tts_backend.unload_model() | ||
| except Exception: | ||
| logger.warning("Failed to unload TTS model after speak", exc_info=True) | ||
|
Comment on lines
331
to
+339
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Move TTS cleanup after synchronous inference. This Keep 🤖 Prompt for AI Agents |
||
|
|
||
| trim_fn = trim_tts_output if engine_needs_trim(engine) else None | ||
| runaway_detector = has_tts_runaway if engine_retries_runaway(engine) else None | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the official PyTorch and torch-directml documentation for the versions used by this repository, which of CUDA, MPS, DirectML, CPU, and XPU support loading and running Whisper in torch.bfloat16?💡 Result:
Support for loading and running the Whisper model in torch.bfloat16 varies by device, depending on hardware capabilities and PyTorch backend implementation. CUDA: Support depends on the underlying GPU architecture. NVIDIA GPUs with Compute Capability 8.0 or higher (e.g., Ampere architecture and newer) support bfloat16 natively. While PyTorch's
torch.bfloat16can be used on these devices, Whisper's specific implementation (as provided byopenai-whisper) is primarily optimized for float16 inference [1], and using bfloat16 may require manual conversion and handling of input features to match the model's dtype [2]. CPU: PyTorch supports bfloat16 on CPUs, particularly on Intel Xeon processors with AVX-512_BF16 or AMX support [3]. There is community-driven support to enable Whisper with bfloat16 on Intel hardware [4]. MPS (Metal Performance Shaders): PyTorch support for bfloat16 on MPS is limited and generally requires newer macOS versions (e.g., macOS 14.0+) [5]. While some operations might function, it is not broadly supported for full model execution for all architectures. DirectML: Thetorch-directmlbackend does not natively support bfloat16 for most operations [6][7]. It is primarily focused on supporting standard operators in float32 and float16 (where available), and users frequently encounter issues when attempting to use specialized dtypes or autocasting with this backend [8][6]. XPU: PyTorch provides explicit support for bfloat16 on XPU devices [9], leveraging Intel's hardware acceleration for these types [3]. Summary Table Device | bfloat16 Support | Notes --- | --- | --- CUDA | Yes | Requires Ampere (CC 8.0) or newer. CPU | Yes | Requires instruction set support (e.g., AVX-512_BF16/AMX). MPS | Limited | Requires macOS 14.0+; support is platform-dependent. DirectML | No | Generally not supported by the current torch-directml implementation. XPU | Yes | Supported via Intel extensions/PyTorch XPU. For all devices, note that Whisper's default inference code is designed for float32 or float16; running in bfloat16 often necessitates manual casting of the model and all input tensors to ensure compatibility [2].Citations:
🏁 Script executed:
Repository: jamiepine/voicebox
Length of output: 719
🏁 Script executed:
Repository: jamiepine/voicebox
Length of output: 9508
🏁 Script executed:
Repository: jamiepine/voicebox
Length of output: 17141
Query the device before requesting
torch.bfloat16._get_device()can return DirectML, XPU, CPU, or fallback CPU when MPS is unavailable, but_load_model_sync()hard-codestorch_dtype=torch.bfloat16. Add a device-specific dtype resolver with a tested fallback for Whisper, especially for DirectML wheretorch.bfloat16is not generally supported by the backend.🤖 Prompt for AI Agents