PipelineTask: add IdleFrameObserver to detect idle pipelines #2949
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
aconchillo
force-pushed
the
aleix/idle-timeout-observer
branch
from
fe3220b to
9fbab86
Compare
|
|
||
| self._idle_queue.task_done() | ||
|
|
||
| await asyncio.wait_for(self._idle_event.wait(), timeout=self._idle_timeout_secs) |
| # If we are cancelling, just exit the task. | ||
| if self._cancelled: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
What effect does flipping this value have?
There was a problem hiding this comment.
It seems like the return value of _idle_timeout_detected determines whether to stop the pipeline (based on the assumption that the _idle_monitor_task is the only thing keeping the pipeline alive still...right? what if that's not true?).
If we're already mid-cancelling the pipeline when we detect that the pipeline is idle...then now we're saying we keep the _idle_monitor_task alive and keep monitoring for future idle detections?
I'm probably misunderstanding this mechanism.
There was a problem hiding this comment.
Oh, this mechanism is just for detecting a pipeline that's not being used, not one that's stuck/blocked for some reason. Got it.
But wait, shouldn't we actually return True here, to tell the _idle_monitor_task to stop? We don't need it anymore if we're already canceling the pipeline, right?
Side note: the comments in this method state that the return value indicates whether the pipeline task should continue running, which doesn't seem accurate—it indicates whether the idle timeout monitor should continue, right?
There was a problem hiding this comment.
We don't need it anymore if we're already canceling the pipeline, right?
Hmm...is the thinking that if cancelling takes a long time and _cancel_on_idle_timeout is False the user might still want to get an "on_idle_timeout" event mid-cancellation? 🤔
There was a problem hiding this comment.
I suppose I can see this value going either way...unclear to me what the right behavior is in this edge case.
One piece of feedback, though, is to fix the comments in this method to better describe the return value.
There was a problem hiding this comment.
If it's True we keep running the task. If it's False then running becomes False so we exit.
There was a problem hiding this comment.
We don't need it anymore if we're already canceling the pipeline, right?
Hmm...is the thinking that if cancelling takes a long time and
_cancel_on_idle_timeoutisFalsethe user might still want to get an "on_idle_timeout" event mid-cancellation? 🤔
If we cancel we will likely not get the frames we are expecting, but I don't think we want to trigger an idle timeout in that case, we just don't want to keep checking for the idle timeout frames.
There was a problem hiding this comment.
Ah right False means stop. OK I got turned around. This makes sense. No need to keep running the timeout checker. 👍
kompfner
left a comment
kompfner
left a comment
There was a problem hiding this comment.
One nit, buried in a stream-of-consciousness series of comments (pardon me)
aconchillo
force-pushed
the
aleix/idle-timeout-observer
branch
from
9fbab86 to
0f5030b
Compare
…-observer PipelineTask: add IdleFrameObserver to detect idle pipelines
Please describe the changes in your PR. If it is addressing an issue, please reference that as well.
We the following report:
idle_timeout_framesset toLLMFullResponseEndFrameLLMFullResponseEndFramewas being generatedThe reason is because the LLM assistant aggregator swallows
LLMFullResponseEndFrame.Since we don't know when frames can be swallowed we now use an observer and an asyncio event that gets set when the expected frames are generated. This also simplifies the timeout logic since we just rely on
asyncio.wait_for().