mirror of
https://github.com/jamiepine/voicebox.git
synced 2026-10-03 09:05:17 -07:00
address CodeRabbit review: fix 4 critical + 12 major issues
Critical: - Remove dead backend.utils.validation PyInstaller hidden import - Fix story_items table rebuild to preserve track/trim/version columns - Guard cache migration against same source/destination path - Fix regeneration audio overwrite (use random uuid suffix per take) Major: - Engine selector: validate language on Qwen switch, clear stale modelSize - Sync language validation regex between profile create and generate (22 langs) - Guard CUDA download against duplicate concurrent requests - Only set model_size for engines that support multiple sizes - Fix 404 swallowed by generic except in history export - Validate audio_path before FileResponse in export-audio - Transcription: stream uploads in 1MB chunks, use robust cache check, call complete_download() on Whisper download success - Set clean version as default when effects chain validation fails - Return explicit error when Windows port occupied by non-voicebox process
This commit is contained in:
@@ -42,8 +42,15 @@ function handleEngineChange(form: UseFormReturn<GenerationFormValues>, value: st
|
|||||||
const [, modelSize] = value.split(':');
|
const [, modelSize] = value.split(':');
|
||||||
form.setValue('engine', 'qwen');
|
form.setValue('engine', 'qwen');
|
||||||
form.setValue('modelSize', modelSize as '1.7B' | '0.6B');
|
form.setValue('modelSize', modelSize as '1.7B' | '0.6B');
|
||||||
|
// Validate language is supported by Qwen
|
||||||
|
const currentLang = form.getValues('language');
|
||||||
|
const available = getLanguageOptionsForEngine('qwen');
|
||||||
|
if (!available.some((l) => l.value === currentLang)) {
|
||||||
|
form.setValue('language', available[0]?.value ?? 'en');
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
form.setValue('engine', value as GenerationFormValues['engine']);
|
form.setValue('engine', value as GenerationFormValues['engine']);
|
||||||
|
form.setValue('modelSize', undefined as unknown as '1.7B' | '0.6B');
|
||||||
if (ENGLISH_ONLY_ENGINES.has(value)) {
|
if (ENGLISH_ONLY_ENGINES.has(value)) {
|
||||||
form.setValue('language', 'en');
|
form.setValue('language', 'en');
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -66,7 +66,7 @@ def build_server(cuda=False):
|
|||||||
'--hidden-import', 'backend.utils.cache',
|
'--hidden-import', 'backend.utils.cache',
|
||||||
'--hidden-import', 'backend.utils.progress',
|
'--hidden-import', 'backend.utils.progress',
|
||||||
'--hidden-import', 'backend.utils.hf_progress',
|
'--hidden-import', 'backend.utils.hf_progress',
|
||||||
'--hidden-import', 'backend.utils.validation',
|
|
||||||
'--hidden-import', 'backend.services.cuda',
|
'--hidden-import', 'backend.services.cuda',
|
||||||
'--hidden-import', 'backend.services.effects',
|
'--hidden-import', 'backend.services.effects',
|
||||||
'--hidden-import', 'backend.utils.effects',
|
'--hidden-import', 'backend.utils.effects',
|
||||||
|
|||||||
@@ -92,14 +92,20 @@ def _migrate_story_items(engine, inspector, tables: set[str]) -> None:
|
|||||||
story_id VARCHAR NOT NULL,
|
story_id VARCHAR NOT NULL,
|
||||||
generation_id VARCHAR NOT NULL,
|
generation_id VARCHAR NOT NULL,
|
||||||
start_time_ms INTEGER NOT NULL DEFAULT 0,
|
start_time_ms INTEGER NOT NULL DEFAULT 0,
|
||||||
|
track INTEGER NOT NULL DEFAULT 0,
|
||||||
|
trim_start_ms INTEGER NOT NULL DEFAULT 0,
|
||||||
|
trim_end_ms INTEGER NOT NULL DEFAULT 0,
|
||||||
|
version_id VARCHAR,
|
||||||
created_at DATETIME,
|
created_at DATETIME,
|
||||||
FOREIGN KEY (story_id) REFERENCES stories(id),
|
FOREIGN KEY (story_id) REFERENCES stories(id),
|
||||||
FOREIGN KEY (generation_id) REFERENCES generations(id)
|
FOREIGN KEY (generation_id) REFERENCES generations(id)
|
||||||
)
|
)
|
||||||
"""))
|
"""))
|
||||||
conn.execute(text("""
|
conn.execute(text("""
|
||||||
INSERT INTO story_items_new (id, story_id, generation_id, start_time_ms, created_at)
|
INSERT INTO story_items_new (id, story_id, generation_id, start_time_ms, track, trim_start_ms, trim_end_ms, version_id, created_at)
|
||||||
SELECT id, story_id, generation_id, start_time_ms, created_at FROM story_items
|
SELECT id, story_id, generation_id, start_time_ms,
|
||||||
|
COALESCE(track, 0), COALESCE(trim_start_ms, 0), COALESCE(trim_end_ms, 0), version_id, created_at
|
||||||
|
FROM story_items
|
||||||
"""))
|
"""))
|
||||||
conn.execute(text("DROP TABLE story_items"))
|
conn.execute(text("DROP TABLE story_items"))
|
||||||
conn.execute(text("ALTER TABLE story_items_new RENAME TO story_items"))
|
conn.execute(text("ALTER TABLE story_items_new RENAME TO story_items"))
|
||||||
|
|||||||
+1
-1
@@ -64,7 +64,7 @@ class GenerationRequest(BaseModel):
|
|||||||
|
|
||||||
profile_id: str
|
profile_id: str
|
||||||
text: str = Field(..., min_length=1, max_length=50000)
|
text: str = Field(..., min_length=1, max_length=50000)
|
||||||
language: str = Field(default="en", pattern="^(zh|en|ja|ko|de|fr|ru|pt|es|it|he)$")
|
language: str = Field(default="en", pattern="^(zh|en|ja|ko|de|fr|ru|pt|es|it|he|ar|da|el|fi|hi|ms|nl|no|pl|sv|sw|tr)$")
|
||||||
seed: Optional[int] = Field(None, ge=0)
|
seed: Optional[int] = Field(None, ge=0)
|
||||||
model_size: Optional[str] = Field(default="1.7B", pattern="^(1\\.7B|0\\.6B)$")
|
model_size: Optional[str] = Field(default="1.7B", pattern="^(1\\.7B|0\\.6B)$")
|
||||||
instruct: Optional[str] = Field(None, max_length=500)
|
instruct: Optional[str] = Field(None, max_length=500)
|
||||||
|
|||||||
@@ -29,6 +29,11 @@ async def download_cuda_backend():
|
|||||||
if cuda.get_cuda_binary_path() is not None:
|
if cuda.get_cuda_binary_path() is not None:
|
||||||
raise HTTPException(status_code=409, detail="CUDA backend already downloaded")
|
raise HTTPException(status_code=409, detail="CUDA backend already downloaded")
|
||||||
|
|
||||||
|
progress_manager = get_progress_manager()
|
||||||
|
existing = progress_manager.get_progress(cuda.PROGRESS_KEY)
|
||||||
|
if existing and existing.get("status") == "downloading":
|
||||||
|
raise HTTPException(status_code=409, detail="CUDA backend download already in progress")
|
||||||
|
|
||||||
async def _download():
|
async def _download():
|
||||||
try:
|
try:
|
||||||
await cuda.download_cuda_binary()
|
await cuda.download_cuda_binary()
|
||||||
|
|||||||
@@ -33,7 +33,7 @@ async def generate_speech(
|
|||||||
from ..backends import engine_has_model_sizes
|
from ..backends import engine_has_model_sizes
|
||||||
|
|
||||||
engine = data.engine or "qwen"
|
engine = data.engine or "qwen"
|
||||||
model_size = data.model_size or "1.7B"
|
model_size = (data.model_size or "1.7B") if engine_has_model_sizes(engine) else None
|
||||||
|
|
||||||
generation = await history.create_generation(
|
generation = await history.create_generation(
|
||||||
profile_id=data.profile_id,
|
profile_id=data.profile_id,
|
||||||
|
|||||||
+19
-16
@@ -126,28 +126,28 @@ async def export_generation(
|
|||||||
db: Session = Depends(get_db),
|
db: Session = Depends(get_db),
|
||||||
):
|
):
|
||||||
"""Export a generation as a ZIP archive."""
|
"""Export a generation as a ZIP archive."""
|
||||||
|
generation = db.query(DBGeneration).filter_by(id=generation_id).first()
|
||||||
|
if not generation:
|
||||||
|
raise HTTPException(status_code=404, detail="Generation not found")
|
||||||
|
|
||||||
try:
|
try:
|
||||||
generation = db.query(DBGeneration).filter_by(id=generation_id).first()
|
|
||||||
if not generation:
|
|
||||||
raise HTTPException(status_code=404, detail="Generation not found")
|
|
||||||
|
|
||||||
zip_bytes = export_import.export_generation_to_zip(generation_id, db)
|
zip_bytes = export_import.export_generation_to_zip(generation_id, db)
|
||||||
|
|
||||||
safe_text = "".join(c for c in generation.text[:30] if c.isalnum() or c in (" ", "-", "_")).strip()
|
|
||||||
if not safe_text:
|
|
||||||
safe_text = "generation"
|
|
||||||
filename = f"generation-{safe_text}.voicebox.zip"
|
|
||||||
|
|
||||||
return StreamingResponse(
|
|
||||||
io.BytesIO(zip_bytes),
|
|
||||||
media_type="application/zip",
|
|
||||||
headers={"Content-Disposition": safe_content_disposition("attachment", filename)},
|
|
||||||
)
|
|
||||||
except ValueError as e:
|
except ValueError as e:
|
||||||
raise HTTPException(status_code=400, detail=str(e))
|
raise HTTPException(status_code=400, detail=str(e))
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
raise HTTPException(status_code=500, detail=str(e))
|
raise HTTPException(status_code=500, detail=str(e))
|
||||||
|
|
||||||
|
safe_text = "".join(c for c in generation.text[:30] if c.isalnum() or c in (" ", "-", "_")).strip()
|
||||||
|
if not safe_text:
|
||||||
|
safe_text = "generation"
|
||||||
|
filename = f"generation-{safe_text}.voicebox.zip"
|
||||||
|
|
||||||
|
return StreamingResponse(
|
||||||
|
io.BytesIO(zip_bytes),
|
||||||
|
media_type="application/zip",
|
||||||
|
headers={"Content-Disposition": safe_content_disposition("attachment", filename)},
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
@router.get("/history/{generation_id}/export-audio")
|
@router.get("/history/{generation_id}/export-audio")
|
||||||
async def export_generation_audio(
|
async def export_generation_audio(
|
||||||
@@ -159,8 +159,11 @@ async def export_generation_audio(
|
|||||||
if not generation:
|
if not generation:
|
||||||
raise HTTPException(status_code=404, detail="Generation not found")
|
raise HTTPException(status_code=404, detail="Generation not found")
|
||||||
|
|
||||||
|
if not generation.audio_path:
|
||||||
|
raise HTTPException(status_code=404, detail="Generation has no audio file")
|
||||||
|
|
||||||
audio_path = Path(generation.audio_path)
|
audio_path = Path(generation.audio_path)
|
||||||
if not audio_path.exists():
|
if not audio_path.is_file():
|
||||||
raise HTTPException(status_code=404, detail="Audio file not found")
|
raise HTTPException(status_code=404, detail="Audio file not found")
|
||||||
|
|
||||||
safe_text = "".join(c for c in generation.text[:30] if c.isalnum() or c in (" ", "-", "_")).strip()
|
safe_text = "".join(c for c in generation.text[:30] if c.isalnum() or c in (" ", "-", "_")).strip()
|
||||||
|
|||||||
@@ -129,6 +129,9 @@ async def migrate_models(request: models.ModelMigrateRequest):
|
|||||||
if not source.exists():
|
if not source.exists():
|
||||||
raise HTTPException(status_code=404, detail="Current model cache directory not found")
|
raise HTTPException(status_code=404, detail="Current model cache directory not found")
|
||||||
|
|
||||||
|
if source.resolve() == destination.resolve():
|
||||||
|
raise HTTPException(status_code=400, detail="Source and destination are the same directory")
|
||||||
|
|
||||||
model_dirs = [d for d in source.iterdir() if d.name.startswith("models--") and d.is_dir()]
|
model_dirs = [d for d in source.iterdir() if d.name.startswith("models--") and d.is_dir()]
|
||||||
if not model_dirs:
|
if not model_dirs:
|
||||||
return {"moved": 0, "errors": [], "source": str(source), "destination": str(destination)}
|
return {"moved": 0, "errors": [], "source": str(source), "destination": str(destination)}
|
||||||
|
|||||||
@@ -13,6 +13,8 @@ from ..utils.tasks import get_task_manager
|
|||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
|
|
||||||
|
UPLOAD_CHUNK_SIZE = 1024 * 1024 # 1MB
|
||||||
|
|
||||||
|
|
||||||
@router.post("/transcribe", response_model=models.TranscriptionResponse)
|
@router.post("/transcribe", response_model=models.TranscriptionResponse)
|
||||||
async def transcribe_audio(
|
async def transcribe_audio(
|
||||||
@@ -21,8 +23,8 @@ async def transcribe_audio(
|
|||||||
):
|
):
|
||||||
"""Transcribe audio file to text."""
|
"""Transcribe audio file to text."""
|
||||||
with tempfile.NamedTemporaryFile(suffix=".wav", delete=False) as tmp:
|
with tempfile.NamedTemporaryFile(suffix=".wav", delete=False) as tmp:
|
||||||
content = await file.read()
|
while chunk := await file.read(UPLOAD_CHUNK_SIZE):
|
||||||
tmp.write(content)
|
tmp.write(chunk)
|
||||||
tmp_path = tmp.name
|
tmp_path = tmp.name
|
||||||
|
|
||||||
try:
|
try:
|
||||||
@@ -32,27 +34,20 @@ async def transcribe_audio(
|
|||||||
duration = len(audio) / sr
|
duration = len(audio) / sr
|
||||||
|
|
||||||
whisper_model = transcribe.get_whisper_model()
|
whisper_model = transcribe.get_whisper_model()
|
||||||
|
|
||||||
model_size = whisper_model.model_size
|
model_size = whisper_model.model_size
|
||||||
whisper_hf_repos = {
|
|
||||||
"large": "openai/whisper-large-v3",
|
|
||||||
"turbo": "openai/whisper-large-v3-turbo",
|
|
||||||
}
|
|
||||||
model_name = whisper_hf_repos.get(model_size, f"openai/whisper-{model_size}")
|
|
||||||
|
|
||||||
from huggingface_hub import constants as hf_constants
|
if not whisper_model.is_loaded() and not whisper_model._is_model_cached(model_size):
|
||||||
|
|
||||||
repo_cache = Path(hf_constants.HF_HUB_CACHE) / ("models--" + model_name.replace("/", "--"))
|
|
||||||
if not repo_cache.exists():
|
|
||||||
progress_model_name = f"whisper-{model_size}"
|
progress_model_name = f"whisper-{model_size}"
|
||||||
|
task_manager = get_task_manager()
|
||||||
|
|
||||||
async def download_whisper_background():
|
async def download_whisper_background():
|
||||||
try:
|
try:
|
||||||
await whisper_model.load_model_async(model_size)
|
await whisper_model.load_model_async(model_size)
|
||||||
|
task_manager.complete_download(progress_model_name)
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
get_task_manager().error_download(progress_model_name, str(e))
|
task_manager.error_download(progress_model_name, str(e))
|
||||||
|
|
||||||
get_task_manager().start_download(progress_model_name)
|
task_manager.start_download(progress_model_name)
|
||||||
create_background_task(download_whisper_background())
|
create_background_task(download_whisper_background())
|
||||||
|
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
|
|||||||
@@ -176,7 +176,11 @@ def _save_generate(
|
|||||||
|
|
||||||
error_msg = validate_effects_chain(effects_chain)
|
error_msg = validate_effects_chain(effects_chain)
|
||||||
if error_msg:
|
if error_msg:
|
||||||
print(f"Warning: invalid effects chain, skipping: {error_msg}")
|
import logging
|
||||||
|
logging.getLogger(__name__).warning("invalid effects chain, skipping: %s", error_msg)
|
||||||
|
versions_mod.set_default_version(
|
||||||
|
versions_mod.list_versions(generation_id, db)[0].id, db
|
||||||
|
)
|
||||||
else:
|
else:
|
||||||
processed_audio = apply_effects(audio, sample_rate, effects_chain)
|
processed_audio = apply_effects(audio, sample_rate, effects_chain)
|
||||||
processed_path = config.get_generations_dir() / f"{generation_id}_processed.wav"
|
processed_path = config.get_generations_dir() / f"{generation_id}_processed.wav"
|
||||||
@@ -225,7 +229,9 @@ def _save_regenerate(
|
|||||||
"""
|
"""
|
||||||
from . import versions as versions_mod
|
from . import versions as versions_mod
|
||||||
|
|
||||||
suffix = version_id[:8] if version_id else generation_id[:8]
|
import uuid as _uuid
|
||||||
|
|
||||||
|
suffix = _uuid.uuid4().hex[:8]
|
||||||
audio_path = config.get_generations_dir() / f"{generation_id}_{suffix}.wav"
|
audio_path = config.get_generations_dir() / f"{generation_id}_{suffix}.wav"
|
||||||
save_audio(audio, str(audio_path), sample_rate)
|
save_audio(audio, str(audio_path), sample_rate)
|
||||||
|
|
||||||
|
|||||||
@@ -113,11 +113,17 @@ async fn start_server(
|
|||||||
&format!("127.0.0.1:{}", SERVER_PORT).parse().unwrap(),
|
&format!("127.0.0.1:{}", SERVER_PORT).parse().unwrap(),
|
||||||
std::time::Duration::from_secs(1),
|
std::time::Duration::from_secs(1),
|
||||||
).is_ok() {
|
).is_ok() {
|
||||||
// Port is in use — check if it's a voicebox process via tasklist
|
// Port is in use — check if it's a voicebox process
|
||||||
if let Some(pid) = find_voicebox_pid_on_port(SERVER_PORT) {
|
if let Some(pid) = find_voicebox_pid_on_port(SERVER_PORT) {
|
||||||
println!("Found existing voicebox-server on port {} (PID: {}), reusing it", SERVER_PORT, pid);
|
println!("Found existing voicebox-server on port {} (PID: {}), reusing it", SERVER_PORT, pid);
|
||||||
*state.server_pid.lock().unwrap() = Some(pid);
|
*state.server_pid.lock().unwrap() = Some(pid);
|
||||||
return Ok(format!("http://127.0.0.1:{}", SERVER_PORT));
|
return Ok(format!("http://127.0.0.1:{}", SERVER_PORT));
|
||||||
|
} else {
|
||||||
|
return Err(format!(
|
||||||
|
"Port {} is already in use by another application. \
|
||||||
|
Close the other application or change the Voicebox port.",
|
||||||
|
SERVER_PORT
|
||||||
|
));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user