Conversation
|
@filipi87 I started work on this here: Aleix raised this issue that prompted me to stop work on this: |
|
I am halfway through validating that all the TTS services are working after this refactor. I am only missing tests for 12 more TTS services. However, I believe this PR already addresses the main concern Aleix mentioned in the other PR: If you could already provide a review for this one, it would be greatly appreciated. I will also bring in any other commits that Mark made there which I haven’t addressed in this PR yet. |
|
@markbackman, @aconchillo, I think it’s now fully ready for review. |
| # Converts the text to audio. | ||
| @abstractmethod | ||
| async def run_tts(self, text: str) -> AsyncGenerator[Frame, None]: | ||
| async def run_tts(self, text: str, context_id: str) -> AsyncGenerator[Frame, None]: |
There was a problem hiding this comment.
Should context_id be Optional here? Without that, this is a breaking change for run_tts(). While you've covered all cases, there are developers that have built their own services that we should support.
There was a problem hiding this comment.
@mark, to be honest, I am having second thoughts about making this optional.
I know this would allow TTS services in third party projects to remain compatible with the current signature, but it would also introduce a downside: all the TTS services here would need to add extra logic to handle an optional context_id even though, in practice, it would never be optional.
So I think we should introduce this breaking change now, before 1.0.0. All the TTS services inside Pipecat will already be refactored to handle this anyway.
cc: @aconchillo for thoughts.
# Conflicts: # src/pipecat/services/tts_service.py
TTS services improvements.