This commit introduces a series of improvements across the codebase:
1. **Logging:**
* Integrated Python's `logging` module throughout the application.
* Replaced custom `print_step/print_substep` functions with standard logging calls (INFO, WARNING, ERROR, DEBUG).
* Configured file logging (rotating logs) and console logging (using RichHandler if available).
* Enhanced FFmpeg error logs to include commands and stderr.
2. **Error Handling:**
* Made error handling more robust in TTS modules, Playwright interactions (screenshotting), API calls, and file operations.
* Replaced `sys.exit()` calls in modules with exceptions to allow better control by the caller.
* Improved reporting of unhandled exceptions in `main.py` with tracebacks to the log file.
3. **Code Quality & Modularity:**
* Refactored `main.py` and `video_creation/final_video.py` into smaller, more manageable functions, reducing global state and improving readability.
* Replaced `eval()` in configuration parsing (`utils/settings.py`, `utils/gui_utils.py`) with safer type conversion methods.
* Decoupled background choice management from the static TOML template, now dynamically loaded from `backgrounds.json`.
* Standardized path handling using `pathlib.Path` across numerous modules, replacing `os.path` and manual string concatenations.
* Corrected OS dispatcher logic in `utils/ffmpeg_install.py`.
* Clarified voice selection logic in `TTS/pyttsx.py`.
4. **FFmpeg Interaction:**
* Reviewed and refined the `ProgressFfmpeg` class for more robust progress parsing and thread management.
5. **Testing Groundwork:**
* Created `tests/` directory and added initial unit tests for utility functions `name_normalize` and `_safe_str_to_bool` using `unittest`.
6. **Documentation:**
* Added/updated module-level and key function docstrings in `main.py` and `video_creation/final_video.py`.
pull/2364/head
google-labs-jules[bot]
2 months ago
parent
aa15172430
commit
47612a09ac
@ -1,22 +1,41 @@
|
||||
import random
|
||||
|
||||
from gtts import gTTS
|
||||
import logging # Added for logging
|
||||
from gtts import gTTS, gTTSError
|
||||
|
||||
from utils import settings
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
class GTTS:
|
||||
def __init__(self):
|
||||
self.max_chars = 5000
|
||||
self.voices = []
|
||||
logger.debug("Initializing GTTS engine.")
|
||||
self.max_chars = 5000 # gTTS has its own limits, but this is for consistency if we pre-validate.
|
||||
# self.voices = [] # gTTS doesn't have selectable voices in the same way as pyttsx or TikTok; lang is the main variant.
|
||||
|
||||
def run(self, text: str, filepath: str):
|
||||
language = settings.config["reddit"]["thread"]["post_lang"] or "en"
|
||||
logger.info(f"Requesting GTTS for text: '{text[:30]}...' using lang: '{language}'. Output: {filepath}")
|
||||
|
||||
def run(self, text, filepath):
|
||||
try:
|
||||
tts = gTTS(
|
||||
text=text,
|
||||
lang=settings.config["reddit"]["thread"]["post_lang"] or "en",
|
||||
slow=False,
|
||||
lang=language,
|
||||
slow=False, # Speed is not highly configurable; 'slow' is the only option.
|
||||
)
|
||||
logger.debug(f"Saving GTTS audio to {filepath}")
|
||||
tts.save(filepath)
|
||||
logger.info(f"Successfully saved GTTS audio to {filepath}")
|
||||
except gTTSError as e: # Catch specific gTTS errors
|
||||
logger.error(f"gTTS API error: {e}", exc_info=True)
|
||||
# Decide if to raise a custom exception or re-raise
|
||||
raise RuntimeError(f"gTTS failed: {e}")
|
||||
except Exception as e: # Catch any other unexpected errors during gTTS processing
|
||||
logger.error(f"An unexpected error occurred with GTTS: {e}", exc_info=True)
|
||||
raise RuntimeError(f"Unexpected GTTS failure: {e}")
|
||||
|
||||
def randomvoice(self):
|
||||
return random.choice(self.voices)
|
||||
# gTTS language is the primary "voice" variant. No list of voices to pick from.
|
||||
# This method might be redundant for GTTS or could return a random language if desired.
|
||||
# For now, it's not actively used by the engine_wrapper for GTTS in a meaningful way.
|
||||
logger.debug("randomvoice called for GTTS, but GTTS primarily uses language codes, not distinct voices.")
|
||||
return settings.config["reddit"]["thread"]["post_lang"] or "en" # Return current lang as a placeholder
|
||||
|
||||
@ -1,38 +1,96 @@
|
||||
import random
|
||||
|
||||
from elevenlabs import save
|
||||
import logging # Added for logging
|
||||
from elevenlabs import save, APIError # Import APIError for specific exception handling
|
||||
from elevenlabs.client import ElevenLabs
|
||||
|
||||
|
||||
from utils import settings
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
class elevenlabs:
|
||||
def __init__(self):
|
||||
self.max_chars = 2500
|
||||
logger.debug("Initializing ElevenLabs TTS engine (client will be created on first run or randomvoice call).")
|
||||
self.max_chars = 2500 # Character limit for ElevenLabs (check their current limits)
|
||||
self.client: ElevenLabs = None
|
||||
self.available_voices = [] # To store fetched voice names
|
||||
|
||||
def run(self, text, filepath, random_voice: bool = False):
|
||||
def _ensure_client_initialized(self):
|
||||
"""Initializes the ElevenLabs client if not already done."""
|
||||
if self.client is None:
|
||||
self.initialize()
|
||||
logger.info("ElevenLabs client not initialized. Initializing now...")
|
||||
api_key = settings.config["settings"]["tts"].get("elevenlabs_api_key")
|
||||
if not api_key:
|
||||
logger.error("ElevenLabs API key is not set in config (ELEVENLABS_API_KEY).")
|
||||
raise ValueError("ElevenLabs API key is missing. Please set ELEVENLABS_API_KEY in config.")
|
||||
|
||||
try:
|
||||
self.client = ElevenLabs(api_key=api_key)
|
||||
# Fetch and store available voices upon successful initialization
|
||||
all_voices_response = self.client.voices.get_all()
|
||||
self.available_voices = [v.name for v in all_voices_response.voices if v.name]
|
||||
if not self.available_voices:
|
||||
logger.warning("No voices returned from ElevenLabs API after initialization.")
|
||||
else:
|
||||
logger.debug(f"Fetched {len(self.available_voices)} voices from ElevenLabs: {self.available_voices}")
|
||||
logger.info("ElevenLabs client initialized successfully.")
|
||||
except APIError as e:
|
||||
logger.error(f"Failed to initialize ElevenLabs client due to API error: {e}", exc_info=True)
|
||||
raise RuntimeError(f"ElevenLabs API initialization failed: {e}")
|
||||
except Exception as e: # Catch other potential errors during client init
|
||||
logger.error(f"An unexpected error occurred during ElevenLabs client initialization: {e}", exc_info=True)
|
||||
raise RuntimeError(f"Unexpected error initializing ElevenLabs client: {e}")
|
||||
|
||||
|
||||
def run(self, text: str, filepath: str, random_voice: bool = False):
|
||||
self._ensure_client_initialized()
|
||||
|
||||
selected_voice_name = ""
|
||||
if random_voice:
|
||||
voice = self.randomvoice()
|
||||
selected_voice_name = self.randomvoice() # randomvoice now also ensures client init
|
||||
logger.debug(f"Using random ElevenLabs voice: {selected_voice_name}")
|
||||
else:
|
||||
selected_voice_name = settings.config["settings"]["tts"].get("elevenlabs_voice_name")
|
||||
if not selected_voice_name:
|
||||
logger.error("ElevenLabs voice name (elevenlabs_voice_name) not set in config.")
|
||||
# Fallback to a random voice if no specific voice is set, or raise error
|
||||
# For now, let's try a random voice as a fallback.
|
||||
logger.warning("elevenlabs_voice_name not set. Attempting to use a random voice.")
|
||||
selected_voice_name = self.randomvoice()
|
||||
if not selected_voice_name: # If randomvoice also fails to find one
|
||||
logger.error("No ElevenLabs voice configured and no random voice available.")
|
||||
raise ValueError("ElevenLabs voice not configured and no random voice found.")
|
||||
else:
|
||||
voice = str(settings.config["settings"]["tts"]["elevenlabs_voice_name"]).capitalize()
|
||||
# Check if configured voice is in available list (case-sensitive for ElevenLabs names usually)
|
||||
if self.available_voices and selected_voice_name not in self.available_voices:
|
||||
logger.warning(f"Configured ElevenLabs voice '{selected_voice_name}' not found in fetched available voices. "
|
||||
f"Available: {self.available_voices}. Attempting to use it anyway.")
|
||||
logger.debug(f"Using configured ElevenLabs voice: {selected_voice_name}")
|
||||
|
||||
audio = self.client.generate(text=text, voice=voice, model="eleven_multilingual_v1")
|
||||
logger.info(f"Requesting ElevenLabs TTS for text: '{text[:30]}...' Voice: {selected_voice_name}. Output: {filepath}")
|
||||
|
||||
try:
|
||||
# Consider making model configurable e.g. "eleven_multilingual_v2"
|
||||
audio = self.client.generate(text=text, voice=selected_voice_name, model="eleven_multilingual_v1")
|
||||
logger.debug(f"Saving ElevenLabs audio to {filepath}")
|
||||
save(audio=audio, filename=filepath)
|
||||
logger.info(f"Successfully saved ElevenLabs TTS audio to {filepath}")
|
||||
except APIError as e:
|
||||
logger.error(f"ElevenLabs API error during audio generation or save: {e}", exc_info=True)
|
||||
raise RuntimeError(f"ElevenLabs API operation failed: {e}")
|
||||
except Exception as e:
|
||||
logger.error(f"An unexpected error occurred with ElevenLabs processing: {e}", exc_info=True)
|
||||
raise RuntimeError(f"Unexpected ElevenLabs failure: {e}")
|
||||
|
||||
def initialize(self):
|
||||
if settings.config["settings"]["tts"]["elevenlabs_api_key"]:
|
||||
api_key = settings.config["settings"]["tts"]["elevenlabs_api_key"]
|
||||
else:
|
||||
raise ValueError(
|
||||
"You didn't set an Elevenlabs API key! Please set the config variable ELEVENLABS_API_KEY to a valid API key."
|
||||
)
|
||||
|
||||
self.client = ElevenLabs(api_key=api_key)
|
||||
def randomvoice(self) -> str:
|
||||
self._ensure_client_initialized() # Ensure client and self.available_voices are populated
|
||||
|
||||
def randomvoice(self):
|
||||
if self.client is None:
|
||||
self.initialize()
|
||||
return random.choice(self.client.voices.get_all().voices).voice_name
|
||||
if not self.available_voices:
|
||||
logger.error("No voices available from ElevenLabs to choose randomly.")
|
||||
# This could raise an error or return a default/empty string depending on desired strictness
|
||||
raise RuntimeError("ElevenLabs: No voices available for random selection.")
|
||||
|
||||
choice = random.choice(self.available_voices)
|
||||
logger.debug(f"Randomly selected ElevenLabs voice: {choice}")
|
||||
return choice
|
||||
|
||||
@ -0,0 +1,75 @@
|
||||
import unittest
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
# Add project root to sys.path to allow importing project modules
|
||||
# Assuming 'tests' directory is at the root of the project, or one level down.
|
||||
# If tests/ is in root, then Path(__file__).parent.parent should be project root.
|
||||
# If tests/ is utils/tests/, then Path(__file__).parent.parent.parent
|
||||
# For now, let's assume tests/ is at the project root.
|
||||
# If this script is run from the project root (e.g., python -m unittest discover),
|
||||
# then imports might work without sys.path modification if modules are packaged or top-level.
|
||||
# However, to be safe for direct execution of this test file or discovery from tests/ dir:
|
||||
PROJECT_ROOT = Path(__file__).resolve().parent.parent
|
||||
sys.path.insert(0, str(PROJECT_ROOT))
|
||||
|
||||
from video_creation.final_video import name_normalize
|
||||
|
||||
class TestNameNormalize(unittest.TestCase):
|
||||
|
||||
def test_empty_string(self):
|
||||
self.assertEqual(name_normalize(""), "")
|
||||
|
||||
def test_no_special_chars(self):
|
||||
self.assertEqual(name_normalize("Simple Name 123"), "Simple Name 123")
|
||||
|
||||
def test_forbidden_chars(self):
|
||||
self.assertEqual(name_normalize('Test? \\ " * : | < > Name'), "Test Name")
|
||||
|
||||
def test_slash_variants_word_or(self):
|
||||
self.assertEqual(name_normalize("cat/dog"), "cat or dog")
|
||||
self.assertEqual(name_normalize("cat / dog"), "cat or dog")
|
||||
self.assertEqual(name_normalize("cat /dog"), "cat or dog")
|
||||
self.assertEqual(name_normalize("cat/ dog"), "cat or dog")
|
||||
|
||||
def test_slash_variants_numbers_of(self):
|
||||
self.assertEqual(name_normalize("1/2"), "1 of 2")
|
||||
self.assertEqual(name_normalize("1 / 2"), "1 of 2")
|
||||
self.assertEqual(name_normalize("10 / 20"), "10 of 20")
|
||||
|
||||
def test_slash_variants_with_without(self):
|
||||
self.assertEqual(name_normalize("test w/ feature"), "test with feature")
|
||||
self.assertEqual(name_normalize("test W / feature"), "test with feature")
|
||||
self.assertEqual(name_normalize("test w/o feature"), "test without feature")
|
||||
self.assertEqual(name_normalize("test W / O feature"), "test without feature")
|
||||
self.assertEqual(name_normalize("test W / 0 feature"), "test without feature") # '0' for 'o'
|
||||
|
||||
def test_remove_remaining_slashes(self):
|
||||
self.assertEqual(name_normalize("a/b/c"), "a or b or c") # First pass
|
||||
self.assertEqual(name_normalize("leading/trailing/"), "leading or trailing") # after or, / is removed
|
||||
# The function applies rules sequentially. "a/b/c" -> "a or b/c" -> "a or b or c"
|
||||
# "test / only / remove" -> "test or only or remove"
|
||||
# A single remaining slash after other rules: "path/to/file" -> "path or to or file"
|
||||
# If a literal single slash needs removing without 'or', 'of', 'with', 'without' interpretation:
|
||||
# e.g. "text/ single" -> "text single" (this is what it currently does due to the final re.sub(r"\/", r"", name))
|
||||
self.assertEqual(name_normalize("text/ single"), "text single")
|
||||
|
||||
|
||||
def test_combined_rules(self):
|
||||
self.assertEqual(name_normalize('File <1>/<2> w/ option?'), "File 1 of 2 with option")
|
||||
|
||||
def test_translation_skip(self):
|
||||
# This test assumes 'post_lang' is not set in settings for name_normalize to skip translation.
|
||||
# To properly test translation, we'd need to mock settings.config or have a way to set it.
|
||||
# For now, testing the non-translation path.
|
||||
# If settings.config["reddit"]["thread"]["post_lang"] is None or empty, it should not translate.
|
||||
# This requires settings to be loaded; for a unit test, it's better if name_normalize
|
||||
# can take lang as a parameter or if settings is easily mockable.
|
||||
# For now, we assume the default state or test its behavior when lang is None.
|
||||
# To test this properly, name_normalize might need a slight refactor
|
||||
# to accept 'lang' as an argument, or we mock 'settings.config'.
|
||||
# Given current structure, we just test a name that would be unchanged if no translation.
|
||||
self.assertEqual(name_normalize("A simple name"), "A simple name")
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
@ -0,0 +1,77 @@
|
||||
import unittest
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
# Add project root to sys.path
|
||||
PROJECT_ROOT = Path(__file__).resolve().parent.parent
|
||||
sys.path.insert(0, str(PROJECT_ROOT))
|
||||
|
||||
# Assuming _safe_str_to_bool is accessible for testing.
|
||||
# If it's meant to be private, this import might be an issue,
|
||||
# but for unit testing helpers, it's often practical.
|
||||
from utils.settings import _safe_str_to_bool
|
||||
|
||||
class TestSafeStrToBool(unittest.TestCase):
|
||||
|
||||
def test_true_strings(self):
|
||||
self.assertTrue(_safe_str_to_bool("true"))
|
||||
self.assertTrue(_safe_str_to_bool("True"))
|
||||
self.assertTrue(_safe_str_to_bool("TRUE"))
|
||||
self.assertTrue(_safe_str_to_bool("yes"))
|
||||
self.assertTrue(_safe_str_to_bool("Yes"))
|
||||
self.assertTrue(_safe_str_to_bool("1"))
|
||||
self.assertTrue(_safe_str_to_bool("on"))
|
||||
self.assertTrue(_safe_str_to_bool("On"))
|
||||
|
||||
def test_false_strings(self):
|
||||
self.assertFalse(_safe_str_to_bool("false"))
|
||||
self.assertFalse(_safe_str_to_bool("False"))
|
||||
self.assertFalse(_safe_str_to_bool("FALSE"))
|
||||
self.assertFalse(_safe_str_to_bool("no"))
|
||||
self.assertFalse(_safe_str_to_bool("No"))
|
||||
self.assertFalse(_safe_str_to_bool("0"))
|
||||
self.assertFalse(_safe_str_to_bool("off"))
|
||||
self.assertFalse(_safe_str_to_bool("Off"))
|
||||
|
||||
def test_boolean_input(self):
|
||||
self.assertTrue(_safe_str_to_bool(True))
|
||||
self.assertFalse(_safe_str_to_bool(False))
|
||||
|
||||
def test_integer_input(self):
|
||||
# Note: The function converts input to str, so int 1 becomes "1" -> True
|
||||
self.assertTrue(_safe_str_to_bool(1))
|
||||
self.assertFalse(_safe_str_to_bool(0))
|
||||
# Other integers will raise ValueError as they don't match "true"/"false" strings
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool(2)
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool(-1)
|
||||
|
||||
def test_invalid_strings(self):
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool("T")
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool("F")
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool("Y")
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool("N")
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool("maybe")
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool("") # Empty string
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool(" true ") # Contains spaces, current impl fails
|
||||
|
||||
def test_string_with_spaces_strict(self):
|
||||
# Current implementation is strict about surrounding spaces.
|
||||
# If " true ".strip() was used, this would pass.
|
||||
# Testing current behavior.
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool(" true ")
|
||||
with self.assertRaises(ValueError):
|
||||
_safe_str_to_bool("false ")
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
@ -1,20 +1,62 @@
|
||||
import os
|
||||
import shutil
|
||||
from os.path import exists
|
||||
from pathlib import Path
|
||||
import logging
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
def _listdir(d): # listdir with full path
|
||||
return [os.path.join(d, f) for f in os.listdir(d)]
|
||||
|
||||
|
||||
def cleanup(reddit_id) -> int:
|
||||
"""Deletes all temporary assets in assets/temp
|
||||
# The _listdir function is no longer needed with pathlib's rglob or iterdir.
|
||||
|
||||
def cleanup(reddit_id_or_path: str) -> int:
|
||||
"""
|
||||
Deletes the specified temporary assets directory.
|
||||
The input can be just the reddit_id, or a full path to the directory.
|
||||
Returns:
|
||||
int: How many files were deleted
|
||||
int: 1 if directory was found and removed, 0 otherwise.
|
||||
"""
|
||||
directory = f"../assets/temp/{reddit_id}/"
|
||||
if exists(directory):
|
||||
shutil.rmtree(directory)
|
||||
# Determine if the input is a full path or just an ID
|
||||
# This makes the function more flexible if a direct path is ever passed.
|
||||
if Path(reddit_id_or_path).is_absolute() or Path(reddit_id_or_path).parent != Path("."):
|
||||
# Looks like a full or relative path with parent components
|
||||
temp_dir_to_delete = Path(reddit_id_or_path)
|
||||
else:
|
||||
# Assume it's just the reddit_id, construct path relative to expected structure
|
||||
# The original path "../assets/temp/" implies this script might be run from a different CWD.
|
||||
# For robustness, let's define base path relative to this script file's location or a well-known project root.
|
||||
# Assuming this script is in `utils/` and assets is `../assets/` from there.
|
||||
# A more robust way would be to have a global constant for project root or assets root.
|
||||
# For now, mimicking original relative path logic but with pathlib:
|
||||
# current_script_dir = Path(__file__).parent
|
||||
# temp_base_dir = current_script_dir.parent / "assets" / "temp"
|
||||
# For simplicity and consistency with other path constructions, let's assume a base assets path.
|
||||
# Let's use a path relative to a potential project root if run from there.
|
||||
# Or, more simply, the original relative path.
|
||||
# The original `../assets/temp/` suggests it's being called from a script one level down from project root.
|
||||
# e.g. if project_root/main.py calls it.
|
||||
# Let's make it relative to CWD for now as `Path()` defaults to that.
|
||||
# The original path was "../assets/temp/{reddit_id}/"
|
||||
# If main.py is in root, and it calls something in utils which calls this,
|
||||
# then Path("assets/temp") would be more appropriate from root.
|
||||
# The `../` is concerning. Let's assume this is called from a script within `utils` or similar.
|
||||
# For now, to match original intent:
|
||||
# If reddit_id_or_path is just an ID, it implies `assets/temp/{ID}` from some root.
|
||||
# The original path `../assets/temp/{reddit_id}/` means from where `cleanup.py` is, go up one, then to assets.
|
||||
# This means project_root/assets/temp/{reddit_id} if cleanup.py is in project_root/utils/
|
||||
|
||||
# Safest assumption: the caller (main.py) provides the `safe_thread_id`.
|
||||
# `main.py` is in the root. `assets` is also in the root.
|
||||
# So, the path should be `assets/temp/{reddit_id_or_path}`.
|
||||
temp_dir_to_delete = Path("assets") / "temp" / reddit_id_or_path
|
||||
|
||||
logger.info(f"Attempting to cleanup temporary directory: {temp_dir_to_delete}")
|
||||
|
||||
return 1
|
||||
if temp_dir_to_delete.exists() and temp_dir_to_delete.is_dir():
|
||||
try:
|
||||
shutil.rmtree(temp_dir_to_delete)
|
||||
logger.info(f"Successfully removed directory: {temp_dir_to_delete}")
|
||||
return 1 # Indicate one directory tree was removed
|
||||
except OSError as e:
|
||||
logger.error(f"Error removing directory {temp_dir_to_delete}: {e}", exc_info=True)
|
||||
return 0 # Indicate failure or partial success
|
||||
else:
|
||||
logger.warning(f"Temporary directory {temp_dir_to_delete} not found or is not a directory. Skipping cleanup for it.")
|
||||
return 0
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
Loading…
Reference in new issue