feat: add advanced generation parameters to TTSConfig and update related tests #85
Conversation
📝 WalkthroughWalkthroughThis pull request adds five advanced generation parameters (max_new_tokens, repetition_penalty, min_chunk_length, condition_on_previous_chunks, early_stop_threshold) to TTSConfig and TTSRequest, propagates them into TTS request construction, adjusts a Visibility literal, and adds unit tests covering the new fields and defaults. Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are field additions and payload propagation without new multi-component control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds five new advanced generation parameters to the TTS configuration system and corrects type definitions for Voice model states and visibility options.
- Introduces advanced generation control parameters:
max_new_tokens,repetition_penalty,min_chunk_length,condition_on_previous_chunks, andearly_stop_threshold - Fixes type definition typo: "unlist" → "unlisted" in Visibility literal
- Adds missing "ready" state to ModelState literal
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/fishaudio/types/tts.py |
Added 5 new advanced generation parameters to TTSConfig and TTSRequest with defaults and documentation |
src/fishaudio/resources/tts.py |
Updated config-to-request conversion to propagate new parameters |
src/fishaudio/types/shared.py |
Fixed Visibility literal typo ("unlist" → "unlisted") and added "ready" to ModelState |
tests/unit/test_types.py |
Added comprehensive test coverage for new parameters in both TTSConfig and TTSRequest, plus tests for corrected type literals |
tests/unit/test_tts.py |
Added integration tests verifying new parameters are correctly sent in API requests with both custom and default values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/fishaudio/types/shared.py:
- Line 30: The Visibility Literal type was updated to use "unlisted" but the
docstring in the voices resource still says "unlist"; update the docstring in
the voices resource (the docstring around the voices-related function/method at
or near the docstring referenced as voices.py:146) to "visibility: Visibility
setting (public, unlisted, private)" and search for any other occurrences of the
old "unlist" string to replace with "unlisted" so docs and types remain
consistent (Visibility is the type to reference).
🧹 Nitpick comments (3)
src/fishaudio/types/tts.py (3)
78-82: Enhance documentation for advanced generation parameters.The docstrings for the new parameters are minimal. Consider adding more detail:
- What unit are "tokens" measured in for this TTS system?
- How does
repetition_penaltyaffect generation quality?- How does
min_chunk_lengthrelate to the existingchunk_lengthparameter?- What are practical use cases for disabling
condition_on_previous_chunks?- What scale is
early_stop_thresholdon, and what happens when the threshold is reached?
105-110: Add validation constraints to advanced generation parameters.Consider adding Pydantic Field validators to ensure valid parameter ranges:
max_new_tokens: Should be positive (e.g.,ge=1)repetition_penalty: Typically should be ≥1.0 in LLMs (e.g.,ge=1.0)min_chunk_length: Should be positive (e.g.,ge=1)early_stop_threshold: Should have a defined range (e.g.,ge=0.0, le=1.0if it's a probability)🔧 Proposed validation constraints
# Advanced generation parameters -max_new_tokens: int = 1024 -repetition_penalty: float = 1.2 -min_chunk_length: int = 50 +max_new_tokens: Annotated[int, Field(ge=1)] = 1024 +repetition_penalty: Annotated[float, Field(ge=1.0)] = 1.2 +min_chunk_length: Annotated[int, Field(ge=1)] = 50 condition_on_previous_chunks: bool = True -early_stop_threshold: float = 1.0 +early_stop_threshold: Annotated[float, Field(ge=0.0)] = 1.0
154-158: Apply consistent validation to TTSRequest parameters.If validation constraints are added to TTSConfig (lines 105-110), apply the same constraints here for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/fishaudio/resources/tts.pysrc/fishaudio/types/shared.pysrc/fishaudio/types/tts.pytests/unit/test_tts.pytests/unit/test_types.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_types.py (2)
src/fishaudio/types/tts.py (2)
TTSConfig(55-110)TTSRequest(113-158)src/fishaudio/types/voices.py (1)
Voice(41-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Integration Tests
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (python)
🔇 Additional comments (9)
tests/unit/test_tts.py (2)
421-445: LGTM! Good test coverage for custom parameter values.The test correctly verifies that custom values for the new advanced parameters are properly included in the payload.
447-464: LGTM! Good test coverage for default parameter values.The test correctly verifies that default values for the new advanced parameters are properly included in the payload when not explicitly provided.
tests/unit/test_types.py (5)
102-119: LGTM! Comprehensive test for TTSConfig defaults.The test thoroughly verifies all default values including the new advanced generation parameters.
121-134: LGTM! Good coverage for custom TTSConfig parameters.The test correctly verifies that custom values for the new parameters are properly assigned.
136-149: LGTM! Comprehensive test for TTSRequest defaults.The test thoroughly verifies all default values including the new advanced generation parameters.
151-165: LGTM! Good coverage for custom TTSRequest parameters.The test correctly verifies that custom values for the new parameters are properly assigned.
168-181: LGTM! Tests align with type literal updates.These tests verify the new "ready" state and "unlisted" visibility values added to the Literal types in shared.py.
src/fishaudio/resources/tts.py (1)
44-48: Verify API support for undocumented parameters.Fish Audio's official API documentation confirms
max_new_tokensandrepetition_penaltysupport, but the other three parameters (min_chunk_length,condition_on_previous_chunks,early_stop_threshold) lack public documentation. The code correctly propagates all parameters, and unit tests confirm the payload includes all five fields with proper defaults and custom values. However, verify that the three undocumented parameters are actually supported by the Fish Audio backend before merging.src/fishaudio/types/shared.py (1)
36-36: Verify "ready" is a valid model state and document its usage.The ModelState literal was updated to include "ready" as part of the v1.0.0 release, with a dedicated test (
test_voice_with_ready_state). However, the backend API support for this state and the distinction between "ready" and "trained" states are not documented. Confirm with the API team that "ready" is supported by the backend and add documentation explaining when a model transitions to each state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/test_types.py (1)
121-134: Good test for custom parameter values.The test correctly validates that custom values for the new parameters are properly set. Consider adding validation tests for edge cases (e.g., negative values, extreme values) to ensure robustness, though this is optional for now.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/fishaudio/types/shared.pytests/unit/test_types.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fishaudio/types/shared.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_types.py (3)
src/fishaudio/types/tts.py (2)
TTSConfig(55-110)TTSRequest(113-158)tests/unit/conftest.py (1)
sample_voice_response(21-49)src/fishaudio/types/voices.py (1)
Voice(41-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Integration Tests
- GitHub Check: Integration Tests
🔇 Additional comments (5)
tests/unit/test_types.py (5)
14-15: LGTM! Imports are correct.The new imports for
TTSConfigandTTSRequestare properly added and align with the type definitions insrc/fishaudio/types/tts.py.
102-119: Excellent test coverage for default values.The test properly validates all default values for
TTSConfig, including both existing and new parameters. All assertions correctly match the type definitions insrc/fishaudio/types/tts.py.
136-149: LGTM! Comprehensive test for TTSRequest defaults.The test properly validates all default values for
TTSRequest, including the requiredtextfield and all new generation parameters.
151-165: Solid test for custom TTSRequest parameters.The test correctly validates that
TTSRequestaccepts and properly stores custom values for the new generation parameters.
168-175: Good addition for testing the updated visibility literal.The test properly validates that
Voiceaccepts the new "unlisted" visibility value. The test is well-isolated in its own class and correctly modifies the fixture data to test the specific scenario.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.