-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Add ErrorFrame emission to TTS/STT services for pipeline error detection #2881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Add ErrorFrame emission to TTS/STT services for pipeline error detection #2881
Conversation
|
@markbackman could you look into this PR? |
| except Exception as e: | ||
| logger.error(f"Failed to connect to AssemblyAI: {e}") | ||
| self._connected = False | ||
| await self.push_error(ErrorFrame(error=e, fatal=False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use this as a pattern?
await self.push_error(ErrorFrame(error=f"{self} error: {e}, fatal=True"))
In this specific instance, is a connection error fatal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, re checking it I believe it is fatal = True, I am updating on the patterns to be followed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markbackman should we also standardize the logger.error messages as logger.error(f"{self} exception: {e}")
or leave them as they are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in some cases its not the format and in some places its there
src/pipecat/services/azure/tts.py
Outdated
|
|
||
| except Exception as e: | ||
| logger.error(f"{self} exception: {e}") | ||
| await self.push_error(ErrorFrame(error=e, fatal=False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add an ErrorFrame to the Exception block starting in line 357?
tests/test_tts_stt_error_handling.py
Outdated
| @@ -0,0 +1,252 @@ | |||
| """ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for offering this, but we need to create a pattern for this type of testing. For now, I'd say to just remove this and we can consider the best way to proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted, I would be removing this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good. I would recommend revisiting all STT and TTS classes to make sure that all Exceptions have ErrorFrames. Also, let's stick to the pattern of:
await self.push_error(ErrorFrame(error=f"{self} error: {e}", fatal=True/False))
Thanks so much for contributing this 🙏. I was actually just looking to do this on Friday, now that we have an on_pipeline_error event which benefits from these ErrorFrames being push. 🙌
Can you also do the following?
- Rebase on main
- Make sure code is linted (scripts/fix-ruff.sh or install pre-commit hook:
uv run pre-commit install) - Add a changelog entry
|
Noted. I'll be making these changes |
fdf129c to
bd654fa
Compare
|
@markbackman Can you check the updates made? Changelog Entry |
|
@markbackman I updated to add all the TTS and STT services |
| logger.error(f"Failed to connect to AssemblyAI: {e}") | ||
| logger.error(f"{self} exception: {e}") | ||
| self._connected = False | ||
| await self.push_error(ErrorFrame(error=f"{self} error: {e}", fatal=True)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably not be fatal. A switcher processor could just switch to another STT service. In general, I believe we shouldn't really be using fatal=True unless for uncaught exceptions, which we handle somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for the comment so should we remove the fatal flag, through all the stt tts services again? and where to use True
| except Exception as e: | ||
| logger.error(f"Error processing WebSocket message: {e}") | ||
| logger.error(f"{self} exception: {e}") | ||
| await self.push_error(ErrorFrame(error=f"{self} error: {e}", fatal=True)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.Don't make it fatal. I believe we should just remove fatal=X form this PR and use the default (which is False). Fatal means the whole bot should end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So bascially you suggest we remove all exception frames from all stt and tts files
src/pipecat/services/cartesia/stt.py
Outdated
| elif isinstance(frame, UserStoppedSpeakingFrame): | ||
| # Send finalize command to flush the transcription session | ||
| if self._connection and self._connection.state is State.OPEN: | ||
| await self._connection.send("finalize") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding these 3 methods? These seem to conflict with other methods already defined (e.g. _disconnect).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that disconnect part, I would be removing this
|
@Angad-2002 This is great, thank you for taking the time and do this. I've added some comments. |
|
Thx for the comments so what changes should be made overall? |
Remove |
Also, there were some functions added. Not sure what the purpose of those were. |
|
I apologize for the duplication, actually I was rebasing on main, and merge conflicts led to duplicates |
…detection - Add ErrorFrame emission to all major TTS/STT services during initialization and runtime failures - Services updated: Cartesia, ElevenLabs, Deepgram, AssemblyAI, Rime, Azure - ErrorFrame objects emitted with fatal=False for graceful degradation - Enables on_pipeline_error event handler to detect service failures programmatically - Add comprehensive pytest test suite to verify ErrorFrame emission - Fixes issue where services failed gracefully but didn't emit ErrorFrame objects This allows developers to implement real-time error monitoring and alerting using the on_pipeline_error event handler introduced in v0.0.90.
- Improves error handling consistency across all services
0a7bba7 to
68ea479
Compare
|
@aconchillo I have rebased on main again and pushed new commit for flag removal and duplicate removal |
|
@aconchillo @markbackman pls check the changes |
PR for #2876
Changes Made -
Services Updated:
push_error())Key Features:
fatal=Falsefor graceful degradationTesting
Added pytest test suite (
tests/test_tts_stt_error_handling.py) that verifies:on_pipeline_errorevent handler