feat:remote_voices #33
Conversation
dont depend on piper from pypi improve voice caching/download dynamic voice list from huggingface
WalkthroughThis pull request introduces significant modifications to the Piper TTS plugin for OpenVoiceOS, focusing on voice model management and configuration flexibility. The Changes
Possibly Related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
setup.py (2)
Line range hint
19-26: Improve package_files() implementation to filter unwanted filesThe current implementation includes all files without filtering, which could package unnecessary files like
.pyc,__pycache__, etc.Consider applying this improvement:
def package_files(directory): paths = [] for (path, directories, filenames) in os.walk(directory): + # Skip __pycache__ directories + if '__pycache__' in directories: + directories.remove('__pycache__') for filename in filenames: + # Skip compiled python files + if filename.endswith(('.pyc', '.pyo')): + continue paths.append(os.path.join('..', path, filename)) return paths
Remove EOL Python version classifiers and add python_requires
The review comment is correct. The package claims support for EOL Python versions in classifiers but lacks explicit version requirements. Additionally:
- No
python_requiresis specified in setup.py- No Python version requirement in requirements.txt
- Package uses Python 3 shebang but claims Python 2 support
- Lists support for unsupported versions (2.x, 3.0-3.6)
Recommendations:
- Remove EOL Python version classifiers as suggested in the review
- Add
python_requires='>=3.7'to setup.py to match the earliest supported version- Keep only Python 3.7+ classifiers as they align with currently supported Python versions
🔗 Analysis chain
Line range hint
69-85: Update Python version classifiersThe package claims support for EOL Python versions which could mislead users and cause security concerns.
Remove support for EOL Python versions:
classifiers=[ 'Development Status :: 3 - Alpha', 'Intended Audience :: Developers', 'Topic :: Text Processing :: Linguistic', 'License :: OSI Approved :: Apache Software License', - 'Programming Language :: Python :: 2', - 'Programming Language :: Python :: 2.7', 'Programming Language :: Python :: 3', - 'Programming Language :: Python :: 3.0', - 'Programming Language :: Python :: 3.1', - 'Programming Language :: Python :: 3.2', - 'Programming Language :: Python :: 3.3', - 'Programming Language :: Python :: 3.4', - 'Programming Language :: Python :: 3.5', - 'Programming Language :: Python :: 3.6', 'Programming Language :: Python :: 3.7', 'Programming Language :: Python :: 3.8', 'Programming Language :: Python :: 3.9', 'Programming Language :: Python :: 3.10', 'Programming Language :: Python :: 3.11', ],Additionally, verify the minimum Python version requirement in requirements.txt:
Let me check the setup.py file to verify the Python version requirements there.
Let me check the actual content of setup.py to see if there's a python_requires specification that might be missed by grep.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Python version requirements in requirements.txt echo "Checking Python version requirements:" if [ -f "requirements.txt" ]; then grep -i "python" requirements.txt || echo "No explicit Python version requirement found" fiLength of output: 247
Script:
#!/bin/bash # Check Python version requirements in setup.py echo "Checking Python version requirements in setup.py:" if [ -f "setup.py" ]; then # Look for python_requires and install_requires grep -A 5 "python_requires\|install_requires" setup.py || echo "No explicit Python version requirement found" # Also check for any tox.ini or pyproject.toml echo -e "\nChecking for tox.ini or pyproject.toml:" if [ -f "tox.ini" ]; then echo "Found tox.ini:" grep -A 5 "envlist\|requires" tox.ini fi if [ -f "pyproject.toml" ]; then echo "Found pyproject.toml:" grep -A 5 "requires\|python =" pyproject.toml fi fiLength of output: 555
Script:
#!/bin/bash # Get the full content of setup.py echo "Content of setup.py:" cat setup.pyLength of output: 3570
🧹 Nitpick comments (2)
ovos_tts_plugin_piper/__init__.py (1)
21-21: Remove unused importget_lang_voicesThe function
get_lang_voicesis imported but not used in this module. To keep the code clean and avoid unnecessary imports, consider removing it.🧰 Tools
🪛 Ruff (0.8.2)
21-21:
ovos_tts_plugin_piper.download.get_lang_voicesimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
ovos_tts_plugin_piper/download.py (1)
25-35: Consider using a more secure hash function for file integrity checksThe
get_file_hashfunction uses MD5 for hashing. MD5 is considered cryptographically insecure due to collision vulnerabilities. While acceptable for basic file integrity checks, it's recommended to use a more secure hash function like SHA256 to enhance security.Update the function to use SHA256:
-def get_file_hash(path: Union[str, Path], bytes_per_chunk: int = 8192) -> str: - """Hash a file in chunks using md5.""" - path_hash = hashlib.md5() +def get_file_hash(path: Union[str, Path], bytes_per_chunk: int = 8192) -> str: + """Hash a file in chunks using SHA256.""" + path_hash = hashlib.sha256() with open(path, "rb") as path_file: chunk = path_file.read(bytes_per_chunk) while chunk: path_hash.update(chunk) chunk = path_file.read(bytes_per_chunk) return path_hash.hexdigest()Ensure that the
voices.jsonfile and other related metadata use SHA256 hashes as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ovos_tts_plugin_piper/__init__.py(6 hunks)ovos_tts_plugin_piper/download.py(1 hunks)ovos_tts_plugin_piper/piper.py(1 hunks)requirements.txt(1 hunks)setup.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_tts_plugin_piper/__init__.py
21-21: ovos_tts_plugin_piper.download.get_lang_voices imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (6)
ovos_tts_plugin_piper/__init__.py (3)
29-29: Update constructor to explicitly handle language configuration
The constructor now relies on self.lang from the base TTS class or the provided config. Ensure that self.lang is properly initialized before it's used, especially if config might not contain a lang key.
Consider verifying that self.lang is set correctly in all usage scenarios.
125-129: Ensure backward compatibility with legacy Neon TTS signatures
The code handles legacy Neon TTS signatures by checking if speaker is a dictionary and setting it to None. This maintains compatibility with older versions. Ensure that this approach does not introduce side effects with current implementations.
146-148:
Fix syntax error in dictionary comprehension for PiperTTSPluginConfig
There's a syntax error in the dictionary comprehension used to define PiperTTSPluginConfig. The colon after lang should not be there in this context. It should be a key-value pair within the comprehension.
Apply this diff to fix the syntax error:
-PiperTTSPluginConfig = {
- lang: [{v: {"voice": v, "offline": True}} for v in voices]
+PiperTTSPluginConfig = {
+ lang: [{v: {"voice": v, "offline": True}} for v in voices]
for lang, voices in LANG2VOICES.items()
}Likely invalid or redundant comment.
ovos_tts_plugin_piper/piper.py (1)
238-240: Confirm the bug fix for speaker ID handling
The adjustment in lines 238-240 ensures that the sid argument is correctly passed to the ONNX model, preventing crashes in single-speaker scenarios. Verify that this change aligns with the expected behavior of the model and does not introduce issues with multi-speaker models.
requirements.txt (1)
2-6: Verify compatibility and security of new dependencies
New dependencies have been added: ovos-utils, langcodes, onnxruntime, numpy<2.0.0, and piper_phonemize. Ensure that these packages are compatible with your codebase and verify that there are no known security vulnerabilities associated with them.
Run the following script to check for security advisories:
✅ Verification successful
Let me generate additional scripts to check the version constraints and compatibility information.
Let me check the actual usage of these packages to understand their implementation.
Dependencies are safe to use with specified constraints
The security scan and code analysis reveals:
- No vulnerabilities found in
ovos-utils,langcodes,onnxruntime, andpiper_phonemize - The
numpy<2.0.0constraint is appropriate as it avoids known vulnerabilities in older versions while maintaining compatibility - All packages are used appropriately in the codebase:
numpyfor audio processing and array operationsonnxruntimefor model inferenceovos-utilsfor logging and language utilitiespiper_phonemizefor text-to-phoneme conversion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories for the new dependencies.
# Dependencies to check
packages=("ovos-utils" "langcodes" "onnxruntime" "numpy" "piper_phonemize")
for package in "${packages[@]}"; do
echo "Checking $package for vulnerabilities..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: PIP, package: "'"$package"'") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
done
Length of output: 3580
Script:
#!/bin/bash
# Check for version constraints and dependencies in setup.py or pyproject.toml
echo "Searching for version constraints in setup files..."
fd -e py -e toml "setup|pyproject" --exec cat {}
# Check import usage of these packages
echo -e "\nChecking usage of new packages..."
rg -l "import (ovos_utils|langcodes|onnxruntime|numpy|piper_phonemize)" --type py
# Get specific numpy version from pip freeze to verify it's within safe range
echo -e "\nChecking installed numpy version..."
pip freeze | grep -i "numpy"
Length of output: 4113
Script:
#!/bin/bash
# Check the implementation in the files where packages are used
echo "Checking piper.py implementation..."
cat ovos_tts_plugin_piper/piper.py
echo -e "\nChecking __init__.py implementation..."
cat ovos_tts_plugin_piper/__init__.py
# Check if there are any version constraints in the code
echo -e "\nChecking for version constraints in implementation..."
rg "(__version__|version|VERSION)" ovos_tts_plugin_piper/
Length of output: 14288
setup.py (1)
64-64: Verify the package data inclusion
The addition of package_data is crucial for the new voice caching and download mechanisms mentioned in the PR objectives. However, ensure that the included files are necessary for the package functionality.
Let's verify the included files:
✅ Verification successful
Package data inclusion is appropriate
The voices.json file is essential for the plugin's functionality as it:
- Serves as a fallback when downloaded voices are not available
- Contains critical voice metadata including language codes, quality settings, and file checksums
- Is actively used by the download mechanism to verify and obtain voice models
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all files that would be included in the package
# to ensure only necessary files are packaged
# List all files in the package directory
echo "Files to be packaged:"
fd . ovos_tts_plugin_piper -t f
# Check for potential unwanted files
echo -e "\nPotential unwanted files:"
fd . ovos_tts_plugin_piper -e pyc -e pyo -e pyd -g '*__pycache__*'
Length of output: 656
Script:
#!/bin/bash
# Let's examine the package_files function and check the content of voices.json
# First, let's see how package_files is defined
rg -A 5 "def package_files" setup.py
# Then, let's examine the content of voices.json to verify it's necessary
cat ovos_tts_plugin_piper/voices.json
# Let's also check if download.py references voices.json
rg "voices.json" ovos_tts_plugin_piper/download.py
Length of output: 65856
Script:
#!/bin/bash
# Let's check if download.py uses voices.json and how it's used
rg -A 5 "voices.json" ovos_tts_plugin_piper/download.py
# Also check if there are any other files in the package that might reference voices.json
rg "voices.json" ovos_tts_plugin_piper/
Length of output: 1113
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ovos_tts_plugin_piper/download.py (4)
46-53: Suggestion: Catch specific exceptions instead of generalExceptionTo improve error handling and avoid masking unexpected exceptions, consider catching specific exceptions such as
URLErrorandHTTPErrorfromurllib.errorinstead of catching all exceptions withException.Apply this diff to catch specific exceptions:
+from urllib.error import URLError, HTTPError ... try: with urlopen(voices_url) as response, open( voices_download, "wb" ) as download_file: shutil.copyfileobj(response, download_file) - except Exception as e: + except (URLError, HTTPError) as e: LOG.error(f"Failed to download {voices_url}: {e}") raise e
131-139: Suggestion: Catch specific exceptions instead of generalExceptionTo enhance error handling and prevent unexpected exceptions from being masked, consider catching specific network-related exceptions such as
URLErrorandHTTPErrorrather than using a broadException.Apply this diff:
try: with urlopen(quote(file_url, safe=":/")) as response, open( download_file_path, "wb" ) as download_file: shutil.copyfileobj(response, download_file) LOG.info("Downloaded %s (%s)", download_file_path, file_url) - except Exception as e: + except (URLError, HTTPError) as e: LOG.error(f"Failed to download {file_url}: {e}") raise VoiceNotFoundError(f"Could not download file {file_name}") from e
166-169: Nitpick: Define the threshold value as a constantThe value
10used as a threshold for language tag distance is hardcoded. Consider defining it as a constant or configurable parameter to enhance maintainability and allow easy adjustments in the future.
175-178: Nitpick: Define the threshold value as a constantThe hardcoded value
10is used again for filtering language tag distances. Consider using the same constant defined earlier to maintain consistency.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
ovos_tts_plugin_piper/voice_models.py (1)
199-217: Potential thread-safety issues due to modification of global variablesThe function
add_local_modelmodifies the global variablesLOCALMODELSandLANG2VOICESwithout any synchronization mechanism. In a multithreaded environment, this could lead to race conditions.Consider encapsulating these variables within a class or using threading locks to ensure thread safety.
README.md (2)
11-11: Convert bare URL to a clickable Markdown linkA bare URL is used in line 11. For better readability and proper formatting, convert it into a Markdown hyperlink.
Apply this change:
-voice models are automatically downloaded from https://huggingface.co/rhasspy/piper-voices into `~/.local/share/piper_tts` +Voice models are automatically downloaded from [https://huggingface.co/rhasspy/piper-voices](https://huggingface.co/rhasspy/piper-voices) into `~/.local/share/piper_tts`🧰 Tools
🪛 Markdownlint (0.37.0)
11-11: null
Bare URL used(MD034, no-bare-urls)
25-25: Add missing comma and hyphen for clarityThere is a missing comma and hyphen in the sentence. Adding them improves readability.
Apply this change:
-if no voice is set it will be auto selected based on language +If no voice is set, it will be auto-selected based on language🧰 Tools
🪛 LanguageTool
[typographical] ~25-~25: It seems that a comma is missing.
Context: ..."alan-low" } } ``` if no voice is set it will be auto selected based on langu...(IF_COMMA)
[uncategorized] ~25-~25: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...} } ``` if no voice is set it will be auto selected based on language you can also define ...(AUTO_HYPHEN)
ovos_tts_plugin_piper/__init__.py (2)
24-24: Remove unused importget_lang_voicesThe function
get_lang_voicesis imported but not used in this module. Removing unused imports helps keep the code clean.Apply this diff to remove the unused import:
from ovos_tts_plugin_piper.voice_models import add_local_model, LOCALMODELS, LANG2VOICES, SHORTNAMES, \ VoiceNotFoundError, \ - get_voice_files, get_lang_voices, get_default_voice + get_voice_files, get_default_voice🧰 Tools
🪛 Ruff (0.8.2)
24-24:
ovos_tts_plugin_piper.voice_models.get_lang_voicesimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
145-146: Address theHACKcomment to improve code maintainabilityThe code contains a
HACKcomment indicating a workaround for a bug in some neon-core versions. Consider implementing a permanent solution or adding compatibility checks.Would you like assistance in creating a more robust solution or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)ovos_tts_plugin_piper/__init__.py(5 hunks)ovos_tts_plugin_piper/voice_models.py(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
11-11: null
Bare URL used
(MD034, no-bare-urls)
🪛 LanguageTool
README.md
[typographical] ~25-~25: It seems that a comma is missing.
Context: ..."alan-low" } } ``` if no voice is set it will be auto selected based on langu...
(IF_COMMA)
[uncategorized] ~25-~25: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...} } ``` if no voice is set it will be auto selected based on language you can also define ...
(AUTO_HYPHEN)
🪛 Ruff (0.8.2)
ovos_tts_plugin_piper/__init__.py
24-24: ovos_tts_plugin_piper.voice_models.get_lang_voices imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
There was a problem hiding this comment.
Consider including the alan-low model with the package, so that it can at least run the default without an internet connection.
If pulling from a newer major version of ovos-utils is required, wouldn't that also make this a breaking change? It's a bit of a grey area, especially since most people run this in a separate virtual environment, but it does force the user to be on a certain version of utils.
Will try to build this as a Docker image and test it out. It's a big PR
| providers=["CPUExecutionProvider"] | ||
| if not self.use_cuda | ||
| else ["CUDAExecutionProvider"], |
There was a problem hiding this comment.
nit(non-blocking): consider breaking this into a variable above for readability
| engine = PiperVoice( | ||
| config=PiperConfig.from_dict(config_dict), | ||
| session=onnxruntime.InferenceSession( | ||
| str(model), |
There was a problem hiding this comment.
You've already defined model as a string in the method signature. Is there a use case where it might come in as a different type?
There was a problem hiding this comment.
it can be a Path i think, not sure why the cast to str was there so i didnt change that
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
ovos_tts_plugin_piper/__init__.py (5)
22-24: Remove unused importget_lang_voicesThe
get_lang_voicesfunction is imported but never used in this file. Consider removing it to keep the imports clean and maintainable.-from ovos_tts_plugin_piper.voice_models import add_local_model, LOCALMODELS, LANG2VOICES, SHORTNAMES, \ - VoiceNotFoundError, \ - get_voice_files, get_lang_voices, get_default_voice +from ovos_tts_plugin_piper.voice_models import add_local_model, LOCALMODELS, LANG2VOICES, SHORTNAMES, \ + VoiceNotFoundError, \ + get_voice_files, get_default_voice🧰 Tools
🪛 Ruff (0.8.2)
24-24:
ovos_tts_plugin_piper.voice_models.get_lang_voicesimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
103-110: Enhance error message with available voicesWhen a voice is not found, it would be helpful to include information about available voices in the error message.
try: model, model_config = get_voice_files(voice) except VoiceNotFoundError as e: - LOG.error(f"Voice files for '{voice}' not found: {e}") + available = list(SHORTNAMES.keys()) + [v for v in LANG2VOICES.get(self.lang, []) if v not in SHORTNAMES] + LOG.error(f"Voice '{voice}' not found. Available voices for {self.lang}: {available}") raise
111-129: Add docstring and improve provider selection readabilityThe method would benefit from documentation and clearer provider selection logic.
def get_model(self, model: str, model_config: str, voice: str = None, speaker=0): + """Initialize and return a Piper voice model. + + Args: + model (str): Path to the ONNX model file + model_config (str): Path to the model configuration file + voice (str, optional): Voice identifier. Defaults to None. + speaker (int, optional): Speaker ID. Defaults to 0. + + Returns: + tuple: (PiperVoice, speaker_id) + """ voice = voice or self.voice + provider = "CUDAExecutionProvider" if self.use_cuda else "CPUExecutionProvider" with open(model_config, "r", encoding="utf-8") as config_file: config_dict = json.load(config_file) engine = PiperVoice( config=PiperConfig.from_dict(config_dict), session=onnxruntime.InferenceSession( str(model), sess_options=onnxruntime.SessionOptions(), - providers=["CPUExecutionProvider"] - if not self.use_cuda - else ["CUDAExecutionProvider"], + providers=[provider], ), )
145-150: Create issue to track legacy TTS signature handlingThe HACK comment indicates technical debt that should be tracked and addressed properly.
Would you like me to create a GitHub issue to track the removal of legacy TTS signature support?
172-189: Move test code to dedicated test filesThe test code in the
__main__block should be moved to proper test files for better organization and maintainability.Consider:
- Creating a dedicated test file (e.g.,
test_piper_tts.py)- Using proper test frameworks (e.g., pytest)
- Moving the remote model URLs to configuration files
- Adding proper assertions to validate the output
Would you like me to help create a proper test file structure?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_tts_plugin_piper/__init__.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_tts_plugin_piper/__init__.py
24-24: ovos_tts_plugin_piper.voice_models.get_lang_voices imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (1)
ovos_tts_plugin_piper/__init__.py (1)
174-176: Verify remote voice loading mechanism
Let's verify that the remote voice loading from Hugging Face works correctly.
✅ Verification successful
Remote voice loading mechanism is properly implemented
The verification shows a robust implementation for remote voice loading:
- Proper error handling with try/catch blocks in
_download_filefunction - File integrity verification through size and MD5 hash checks
- Automatic download of missing or corrupted files
- Support for both local and remote (Hugging Face) model sources
- Caching mechanism to store downloaded models in the user's data directory
The implementation includes:
- URL validation and safe handling using
urllib.parse.quote - Proper file system operations with parent directory creation
- Detailed logging for debugging and error tracking
- Fallback mechanisms when voice files are missing or corrupt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify remote voice loading functionality
# Test: Check if the code can handle various Hugging Face model URLs and configurations
# Check for other remote voice references in the codebase
rg -l "huggingface.co.*piper.*onnx" --type py
# Check for potential error handling of remote voice loading
rg "get_voice_files.*try|catch" --type py
Length of output: 125
Script:
#!/bin/bash
# Let's check the implementation details of remote voice loading
# Check the implementation around the URL handling
rg -B 5 -A 10 "huggingface.co.*piper.*onnx" --type py
# Look for download/fetch related functions
ast-grep --pattern 'def $_(self, $$$) { $$$ download $$$ }'
# Check error handling patterns around URL/remote operations
rg "requests\.(get|post)|urllib|download" -B 2 -A 5 --type py
Length of output: 10583
|
This plugin cannot be build on Debian Trixie which runs Python 3.12. |
why? |
There is no |
|
maybe we could build a wheel until upstream solves it properly rhasspy/piper-phonemize#33 |
|
|
Tested with Python 3.10, seems fine so far. |
dont depend on piper from pypi
improve voice caching/download
dynamic voice list from huggingface https://huggingface.co/rhasspy/piper-voices
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores