-
Notifications
You must be signed in to change notification settings - Fork 1.5k
introduce uninterruptible frames #3189
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| - Data and control frames can now be marked as non-interruptible by using the | ||
| `UninterruptibleFrame` mixin. Frames marked as `UninterruptibleFrame` will not | ||
| be interrupted during processing, and any queued frames of this type will be | ||
| retained in the internal queues. This is useful when you need ordered frames | ||
| (data or control) that should not be discarded or cancelled due to | ||
| interruptions. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| - `FunctionCallInProgressFrame` and `FunctionCallResultFrame` have changed from | ||
| system frames to a control frame and a data frame, respectively, and are now | ||
| both marked as `UninterruptibleFrame`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| InterruptionTaskFrame, | ||
| StartFrame, | ||
| SystemFrame, | ||
| UninterruptibleFrame, | ||
| ) | ||
| from pipecat.metrics.metrics import LLMTokenUsage, MetricsData | ||
| from pipecat.observers.base_observer import BaseObserver, FrameProcessed, FramePushed | ||
|
|
@@ -211,6 +212,7 @@ def __init__( | |
| # The input task that handles all types of frames. It processes system | ||
| # frames right away and queues non-system frames for later processing. | ||
| self.__should_block_system_frames = False | ||
| self.__input_queue = FrameProcessorQueue() | ||
| self.__input_event: Optional[asyncio.Event] = None | ||
| self.__input_frame_task: Optional[asyncio.Task] = None | ||
|
|
||
|
|
@@ -220,8 +222,10 @@ def __init__( | |
| # called. To resume processing frames we need to call | ||
| # `resume_processing_frames()` which will wake up the event. | ||
| self.__should_block_frames = False | ||
| self.__process_queue = asyncio.Queue() | ||
| self.__process_event: Optional[asyncio.Event] = None | ||
| self.__process_frame_task: Optional[asyncio.Task] = None | ||
| self.__process_current_frame: Optional[Frame] = None | ||
|
|
||
| # To interrupt a pipeline, we push an `InterruptionTaskFrame` upstream. | ||
| # Then we wait for the corresponding `InterruptionFrame` to travel from | ||
|
|
@@ -805,8 +809,12 @@ async def _start_interruption(self): | |
| # interruption). Instead we just drain the queue because this is | ||
| # an interruption. | ||
| self.__reset_process_task() | ||
| elif isinstance(self.__process_current_frame, UninterruptibleFrame): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. 👍 |
||
| # We don't want to cancel UninterruptibleFrame, so we simply | ||
| # cleanup the queue. | ||
| self.__reset_process_queue() | ||
| else: | ||
| # Cancel and re-create the process task including the queue. | ||
| # Cancel and re-create the process task. | ||
| await self.__cancel_process_task() | ||
| self.__create_process_task() | ||
| except Exception as e: | ||
|
|
@@ -872,7 +880,6 @@ def __create_input_task(self): | |
|
|
||
| if not self.__input_frame_task: | ||
| self.__input_event = asyncio.Event() | ||
| self.__input_queue = FrameProcessorQueue() | ||
| self.__input_frame_task = self.create_task(self.__input_frame_task_handler()) | ||
|
|
||
| async def __cancel_input_task(self): | ||
|
|
@@ -890,9 +897,7 @@ def __create_process_task(self): | |
| return | ||
|
|
||
| if not self.__process_frame_task: | ||
| self.__should_block_frames = False | ||
| self.__process_event = asyncio.Event() | ||
| self.__process_queue = asyncio.Queue() | ||
| self.__reset_process_task() | ||
| self.__process_frame_task = self.create_task(self.__process_frame_task_handler()) | ||
|
|
||
| def __reset_process_task(self): | ||
|
|
@@ -902,10 +907,26 @@ def __reset_process_task(self): | |
|
|
||
| self.__should_block_frames = False | ||
| self.__process_event = asyncio.Event() | ||
| self.__reset_process_queue() | ||
|
|
||
| def __reset_process_queue(self): | ||
| """Reset non-system frame processing queue.""" | ||
| # Create a new queue to insert UninterruptibleFrame frames. | ||
| new_queue = asyncio.Queue() | ||
|
|
||
| # Process current queue and keep UninterruptibleFrame frames. | ||
| while not self.__process_queue.empty(): | ||
| self.__process_queue.get_nowait() | ||
| item = self.__process_queue.get_nowait() | ||
| if isinstance(item, UninterruptibleFrame): | ||
| new_queue.put_nowait(item) | ||
| self.__process_queue.task_done() | ||
|
|
||
| # Put back UninterruptibleFrame frames into our process queue. | ||
| while not new_queue.empty(): | ||
| item = new_queue.get_nowait() | ||
| self.__process_queue.put_nowait(item) | ||
| new_queue.task_done() | ||
|
Comment on lines
+924
to
+928
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn’t it be enough to simply do
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because the process task is using the old queue still and we are not cancelling the task. |
||
|
|
||
| async def __cancel_process_task(self): | ||
| """Cancel the non-system frame processing task.""" | ||
| if self.__process_frame_task: | ||
|
|
@@ -959,8 +980,12 @@ async def __input_frame_task_handler(self): | |
| async def __process_frame_task_handler(self): | ||
| """Handle non-system frames from the process queue.""" | ||
| while True: | ||
| self.__process_current_frame = None | ||
|
|
||
| (frame, direction, callback) = await self.__process_queue.get() | ||
|
|
||
| self.__process_current_frame = frame | ||
|
|
||
| if self.__should_block_frames and self.__process_event: | ||
| logger.trace(f"{self}: frame processing paused") | ||
| await self.__process_event.wait() | ||
|
|
||
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.
Shouldn't we also change
FunctionCallsStartedFrameandFunctionCallCancelFrame? Otherwise, it feels like a race condition could happen.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 the case of
FunctionCallsStartedFramewe might want to notify right away that function calls are about to start . ForFunctionCallCancelFrameit doesn't really matter. These two don't modify the context and don't trigger any inference. So, I didn't see a reason to change them.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, I was wondering if we might have a race condition, but I think we’re okay. 👍