Delete aiconfig_complete stream response, replace with aiconfig
#911
Conversation
Before we used to not support streaming, so when we would return `aiconfig` it would be from a blocking hanging operation. This meant that we needed to set `isRunning` prompt state to be true while we were waiting, but now we don't need to do that anymore after we migrated all run events to return in streaming response format, even for non-streaming models: #806 Also we are now no longer using the `streamApi` helper since we added and are now using `streamingApiChain`, which was added in #789 Finally, if you want more resources on how streaming is connected, you can check out #910 which is a teaching guide I built for adding explaining how the code is connected ## Test Plan Both streaming and non-streaming models work as before https://github.com/lastmile-ai/aiconfig/assets/151060367/b62e7887-20af-4c0c-ab85-eeaacaab64e0
rossdanlm
requested review from
saqadri,
rholinshead,
suyoglastmileai,
Ankush-lastmile and
jonathanlastmileai
as code owners
| @@ -641,17 +637,9 @@ export default function EditorContainer({ | |||
| type: "CONSOLIDATE_AICONFIG", | |||
| action: { | |||
| ...action, | |||
| // Ensure we keep the prompt in a running state since this is an in-progress update | |||
| isRunning: true, | |||
There was a problem hiding this comment.
Maybe also remove this property from the CONSOLIDATE_AICONFIG action in aiConfigReducer
There was a problem hiding this comment.
Wasn't this added specifically to support streaming? It wasn't needed for the single awaited request run calls. Also, just noting that we still probably want to support non-streaming execution since external deployments might not have streaming implemented
There was a problem hiding this comment.
Fixed some bugs on the frontend along the way -- specifically, because we return the full aiconfig on every chunk update, we were resetting isRunning to false after the first chunk was received (by dispatching CONSOLIDATE_AICONFIG). Ideally, we update just the output chunk, but to fix this I've added a separate aiconfig_complete event to distinguish the finalized aiconfig from the in-progress one
I guess my main concern is will this be needed for gradio streaming?
There was a problem hiding this comment.
Ah that's a good point. Gradio would need the full updated value
There was a problem hiding this comment.
Also, just noting that we still probably want to support non-streaming execution since external deployments might not have streaming implemented
We still have another CONSOLIDATE_AICONFIG call at the end after the stream outputs that listens for the non-streaming aiconfig responses (server must return a response instead of yielding)
Wasn't this added specifically to support streaming? It wasn't needed for the single awaited request run calls.
I think you're right about this, if we return the entire AIConfig as a streamed response this will incorrectly make the isRunning param to false. However if we keep it here, we'll never terminate the isRunning state to false, because everything we return from the server is in stream format.
b120c319-fa11-40e7-a31f-2bdb5bbe3ec7.mp4
What I will do in a followup PR is add another streaming event called aiconfig_stream and have the isRunning there set to true (kind of the equivalent of aiconfig stream event today, while aiconfig_complete is now the same functionality as aiconfig). I just feel the names for that are a bit simpler and it's apparent that one is for streaming, while the other is for "regular" output as a "non-stream" (even if it's being implemented by our particular backend as a streaming event, other people can implement it as a non-stream event and know what it is supposed to do)
I think this is cleaner and better for us to deal with. It also breaks down the aiconfig streaming logic to be more explicitly handled for next PR. See comments about this in - #907 (comment) - #911 (comment) ## Test Plan Both the streaming and non-streaming models work the same as before
I think this is cleaner and better for us to deal with. It also breaks down the aiconfig streaming logic to be more explicitly handled for next PR. See comments about this in - #907 (comment) - #911 (comment) ## Test Plan Both the streaming and non-streaming models work the same as before https://github.com/lastmile-ai/aiconfig/assets/151060367/2c3fd802-e95e-417c-ac80-7b6e18983520
I think this is cleaner and better for us to deal with. It also breaks down the aiconfig streaming logic to be more explicitly handled for next PR. See comments about this in - #907 (comment) - #911 (comment) ## Test Plan Both the streaming and non-streaming models work the same as before https://github.com/lastmile-ai/aiconfig/assets/151060367/2c3fd802-e95e-417c-ac80-7b6e18983520
…914) Add explicit "stop_streaming" event to signal streaming is finished I think this is cleaner and better for us to deal with. It also breaks down the aiconfig streaming logic to be more explicitly handled for next PR. See comments about this in - #907 (comment) - #911 (comment) ## Test Plan Both the streaming and non-streaming models work the same as before https://github.com/lastmile-ai/aiconfig/assets/151060367/2c3fd802-e95e-417c-ac80-7b6e18983520
Delete
aiconfig_completestream response, replace withaiconfigBefore we used to not support streaming, so when we would return
aiconfigit would be from a blocking hanging operation. This meant that we needed to setisRunningprompt state to be true while we were waiting, but now we don't need to do that anymore after we migrated all run events to return in streaming response format, even for non-streaming models: #806Also we are now no longer using the
streamApihelper since we added and are now usingstreamingApiChain, which was added in #789Finally, if you want more resources on how streaming is connected, you can check out #910 which is a teaching guide I built for adding explaining how the code is connected
Test Plan
Both streaming and non-streaming models work as before
b120c319-fa11-40e7-a31f-2bdb5bbe3ec7.mp4
Stack created with Sapling. Best reviewed with ReviewStack.
aiconfig_completestream response, replace withaiconfig#911