-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix up conversation initialization #6430
base: main
Are you sure you want to change the base?
Conversation
- Modified OpenHands.createConversation() to accept initialUserMsg and files - Changed API to use FormData for sending both text fields and files - Updated backend to handle file uploads during conversation creation - Removed InitSessionRequest model in favor of Form and File parameters
ChangeAgentStateAction(AgentState.AWAITING_USER_INPUT), | ||
EventSource.ENVIRONMENT, | ||
) | ||
|
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.
The initial state is now AWAITING_USER_INPUT rather than INIT. I like this because I think there has been some general confusion on what the INIT state means - "INITIALIZED" vs "INITIALIZING".
Do you foresee any consequences of this in the frontend?
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.
The main consequence here is that, if you start with a repo/zip, instead of seeing a blue "Initialized" indicator, you now see a yellow "waiting for user input" indicator
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 agree, I like it too. INIT was the only thing different in the stream between a UI session and a CLI session, I think - CLI streams never had anything with it.
Happy to see it go 👍
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.
changed this to a blue indicator now
@@ -28,10 +28,13 @@ export const useCreateConversation = () => { | |||
throw new Error("No query provided"); | |||
} | |||
|
|||
if (variables.q) dispatch(setInitialQuery(variables.q)); | |||
if (variables.q) dispatch(setInitialPrompt(variables.q)); | |||
|
|||
return OpenHands.createConversation( |
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.
Given that we marked this function async, shouldn't this now be:
return await OpenHands.createConversation(
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.
(Otherwise the result will be a promise wrapped in a promise?)
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 think this works just fine--promises are pretty intelligent about nesting IIRC.
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.
With all the changes to init, I would like to test some of the workflows when you kill / restart the server and try to reconnect before merging.
@@ -6,10 +6,6 @@ class AgentState(str, Enum): | |||
"""The agent is loading. | |||
""" | |||
|
|||
INIT = 'init' |
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 think INIT exists in the event stream, at least as value of an agent change observation. It might be worth to test restoring a session, to see if we can recreate the events.
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.
ooh good point
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.
It works!
Confirmed that we can:
|
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.
🥳
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.
🍰
End-user friendly description of the problem this fixes or functionality that this introduces
no changelog
Give a summary of what the PR does, explaining any non-trivial design decisions
The biggest issue this fixes is that currently, if you start a conversation, then navigate away, then navigate back, your first message is destroyed and never gets to the agent.
The fix here is that we now post the initial prompt, plus any files, to the new_conversation API endpoint, and the backend handles all the initialization.
Link of any specific issues this addresses
To run this PR locally, use the following command: