Skip to content
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

Enable human interaction in AutoGenStudio #3445

Closed

Conversation

SailorJoe6
Copy link
Collaborator

Why are these changes needed?

The AutoGenStudio UI doesn't currently support user input modes 'ALWAYS', or 'TERMINATE' which are vital when allowing agents to generate and run code, so that the user can verify the code before it gets execute in their environment. This is likely due to difficulties encountered with Python's asyncio event loop getting blocked by synchronous calls to the AutoGen chat functions. This PR fixes those issues and adds support for all human input modes

  • Updated the code to call the synchronous runWorkflow code in a separate thread, thus eliminating the blockage on the asyncio event loop
  • Completely removed the message_handler_thread as it's no longer needed, since the event loop is no longer blocked
  • Split out AutoGenChatManager into it's own file, and enhanced it for user feedback
  • Enhanced WebSocketConnectionManager as well for user feedback
  • Updates to chatbox.tsx, agentconfig.tsx and atoms.tsx (though atoms changes just for troubleshooting)
  • updates to app.py to handle getting human input

Related issue number

Resolves #1358
Resolves #1664
Part of roadmap #737

Checks

New-World-2019 and others added 3 commits August 28, 2024 16:34
* Add support for tool calling cohere

* update tool calling code

* make client name configurable with default

* formatting nits

* update docs

---------

Co-authored-by: Mark Sze <[email protected]>
Co-authored-by: Li Jiang <[email protected]>
- Updated the code to call the synchronous `runWorkflow` code in a separate thread, thus eliminating the blockage on the asyncio event loop
- Completely removed the message_handler_thread as it's no longer needed, since the event loop is no longer blocked
- Split out AutoGenChatManager into it's own file, and enhanced it for user feedback
- Enhanced WebSocketConnectionManager as well for user feedback
- Updates to chatbox.tsx, agentconfig.tsx and atoms.tsx (though atoms changes just for troubleshooting)
- updates to app.py to handle getting human input
@Hk669 Hk669 requested a review from victordibia August 29, 2024 04:39
@gagb gagb added the proj-studio Related to AutoGen Studio. label Aug 29, 2024
print(f"Error in sending message: {str(e)}", user_prompt)
await self.disconnect(websocket)

message = data.get("data").get("content")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SailorJoe6

Thanks for the rewrite and 3.10 support.

I am seeing some errors while testing ... .

With that fixed on my local version, I am still seeing a few other socket connection errors.
Tried to address this using a asynccontext manager but still seeing some of the timeout/delay issues where receive_json() does not return.

@asynccontextmanager
async def timeout_manager(timeout):
    try:
        yield await asyncio.wait_for(asyncio.sleep(timeout), timeout)
    except asyncio.TimeoutError:
        raise asyncio.TimeoutError

... 
async def get_user_input(self, user_prompt: Dict, timeout: int, websocket: WebSocket) -> str:
        await self.send_message(user_prompt, websocket)
        message = f"exit" 
        try:
            async with timeout_manager(20):
                try:
                    data = await websocket.receive_json()
                except WebSocketDisconnect:
                    print("Error: Tried to send a message to a closed WebSocket")
                    await self.disconnect(websocket)
                except websockets.exceptions.ConnectionClosedOK:
                    print("Error: WebSocket connection closed normally")
                    await self.disconnect(websocket)
                except Exception as e:
                    print(
                        f"Error in sending message: {str(e)}, {user_prompt}")
                    await self.disconnect(websocket)

                message = data.get("data").get("content")
                # break
        except asyncio.TimeoutError:
            print(">> Timeout")

        return message

Overall, once the core errors are addressed (even without the delay), I am happy to mark this feature as experimental (always/terminate human input), merge in and then improve iteratively.

Also, if you can attach a quick recording of how you are testing and results, that would be useful. Thanks.

Copy link
Collaborator Author

@SailorJoe6 SailorJoe6 Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll address these issues to the best of my abilities tomorrow. Thanks again for the review!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @victordibia, I'm working on these issues now. I see there's a new commit on autogenstudio branch. I can merge or rebase. Personally, I prefer rebase, but it's your call. Do you have a preference?

@Knucklessg1
Copy link
Contributor

One small issue I was seeing when building the front end was when I ran yarn build, the icon.png inside the frontend/src/images/icon.png appeared to be corrupt. Applying the fix here resolved the issue: #2806

This may require it's own separate PR.

@Knucklessg1
Copy link
Contributor

Not sure if it's just on my end, but the UI seems squished on the playground screen.
Screenshot from 2024-08-29 22-05-55

I am able to see the drop down for the Human input mode.

@SailorJoe6
Copy link
Collaborator Author

Squished UI was present In my environment before I started work on this feature. Have to admit though, it wasn't there in the previous version? Goes away as soon as there's text from the agent conversation.

I had the issue with the corrupt icon too, but that seemed to just "go away" at some point. Totally baffled by it.

@victordibia
Copy link
Collaborator

@Knucklessg1 , thanks for helping test this.
Thanks for noting the icon issue. I havent really faced it. It also might be due to a git-lfs issue (this repo uses lfs for images ).
I also have not seen the squishy UI issue but will be looking out for it.

@SailorJoe6
Copy link
Collaborator Author

Here's a link to a video of how I've been testing the UI. https://www.loom.com/share/7698d7b4264d49c4a2b9d9c84dbbb4b7?sid=383dc455-f69c-4b07-9977-68513c172885

@victordibia when you asked about how I was testing, did you mean this or are you asking more about the weird delay in the json response?

@SailorJoe6
Copy link
Collaborator Author

ps. let me know if you prefer me to merge the recent updates on the autogenstudio branch, or rebase this off of it.

@victordibia
Copy link
Collaborator

I seem to still be getting the following error (I am using a mac)

/autogenstudio/web/websocketmanager.py", line 84, in get_user_input
    message = data.get("data").get("content")
UnboundLocalError: local variable 'data' referenced before assignment

@SailorJoe6
Copy link
Collaborator Author

Good catch. Would have only hit the error when an exception occurs, such as WebSocketDisconnect. That's probably why I didn't hit it when testing. Fix is trivial. Will push it now.

@SailorJoe6
Copy link
Collaborator Author

So I also noticed that any attempt to create a new session, or click on a different session while awaiting user input does not work as expected, so I added a commit to disable the buttons during user input and re-enable them when user feedback is given. Let me know your thoughts.

@victordibia
Copy link
Collaborator

victordibia commented Aug 31, 2024

Looking good, thanks for the recording. The behavior in your recording is ideal.
I am seeing differences on my end. For the most part, it appears the default exit message always gets set and terminates the loop.

screen_ags_ui.mov

There is a chance that this has something to do with my mac env.
@Hk669 , @Knucklessg1 would be glad to verify you are seeing the behaviours similar to the recording here

Also seeing the error below

Error in sending message: The future belongs to a different loop than the one specified as the loop argument {'type': 'user_input_request', 'data': {'recipient': 'user_proxy', 'sender': 'system', 'message': {'content': "Replying as user_proxy. Provide feedback to default_assistant. Press enter to skip and use auto-reply, or type 'exit' to end the conversation: ", 'role': 'system'}, 'timestamp': '2024-08-31T14:15:28.070697', 'sender_type': 'system', 'connection_id': '2351103a-9309-406b-b785-9493561dd85d', 'message_type': 'agent_message'}, 'connection_id': '2351103a-9309-406b-b785-9493561dd85d'}

@SailorJoe6
Copy link
Collaborator Author

That is very odd. I'm also in a mac environment, Apple M1 Pro - Sonoma 14.5 OS. However, I don't think it's your mac. Do you have any other local edits to your code you might need to stash? The reason I ask is because, from the looks of your UI, it definitely looks like you're not getting the messages sent to the browser in the correct order. Open up your Chrome developer tools, go to the "sources" view and put breakpoints in the client.onmessage = (message) => {...}; function. Put breakpoints inside of each of the following:

  • if (data && data.type === "agent_message") {...
  • else if (data && data.type === "user_input_request") {...
  • else if (data && data.type === "agent_status") {...
  • else if (data && data.type === "agent_response") {...

Put breakpoints inside the "if" block, not on the "if" statement. Then you'll see what I mean. Your UI looks as if it's getting the "agent_response" message before it ever gets the "user_input_request" message, and that's not right. You should get a dialog of "agent_message" and "user_input_request" until after you type exit. Only then, after you end the conversation, you should get "agent_status" and finally "agent_response".

Getting these messages in the wrong order was being caused by the "message thread" blocking the asyncio thread loop, which is why I removed that thread. If you have code in there other than my code, that could be causing this. Look at my first commit, 9429355. i have a comment there that says "this is where the problem is" and it's the problem you're seeing that I'm talking about.

Happy to set up a pairing session some time if you like.

@Hk669
Copy link
Contributor

Hk669 commented Sep 1, 2024

Looking good, thanks for the recording. The behavior in your recording is ideal. I am seeing differences on my end. For the most part, it appears the default exit message always gets set and terminates the loop.

screen_ags_ui.mov
There is a chance that this has something to do with my mac env. @Hk669 , @Knucklessg1 would be glad to verify you are seeing the behaviours similar to the recording here

Also seeing the error below

Error in sending message: The future belongs to a different loop than the one specified as the loop argument {'type': 'user_input_request', 'data': {'recipient': 'user_proxy', 'sender': 'system', 'message': {'content': "Replying as user_proxy. Provide feedback to default_assistant. Press enter to skip and use auto-reply, or type 'exit' to end the conversation: ", 'role': 'system'}, 'timestamp': '2024-08-31T14:15:28.070697', 'sender_type': 'system', 'connection_id': '2351103a-9309-406b-b785-9493561dd85d', 'message_type': 'agent_message'}, 'connection_id': '2351103a-9309-406b-b785-9493561dd85d'}

sure, let me confirm from my side.

@SailorJoe6
Copy link
Collaborator Author

SailorJoe6 commented Sep 1, 2024

@victordibia, I'm curious. Do you still have the code you listed in this comment in your local, wrt asynccontext manager?

@@ -6,6 +6,7 @@
"api_type": "cohere",
"model": "command-r-plus",
"api_key": os.environ.get("COHERE_API_KEY")
"client_name": "autogen-cohere", # Optional parameter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SailorJoe6 ,

Can we restrict/isolate all changes to samples/apps/autogen-studio/* ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @victordibia, terribly sorry about this one. It was unintentional. I synced my Fork, and when I did, github merged main into my branch. Not sure why, but one guess is that it's because autogenstudio branch and main were pointed at the same commit on the day I created my branch, so github assumed my branch was based on main when I synced my fork. Other guess is I was tired at the time and clicked a wrong button. Anyway, based on my other comment below, we may prefer to start clean anyway.

@victordibia
Copy link
Collaborator

I have your PR checked out clean (deleted prev version, checked out clean), still having same issues.
I'll do more testing in a clean env later today, also waiting to learn what others are seeing.
Thanks again for moving this forward, eager to merge in soon as its a useful and repeatedly requested feature!

@SailorJoe6
Copy link
Collaborator Author

I think I've figured out why this is acting so differently on your machine than it is on mine, and I think I've got a solution. However, in order to test it, I've had to re-write this feature yet again on a different branch. I should be done testing tomorrow, so I'll submit another PR to share that version. If that one works fine on everyone's machine, we should just close this PR and take that one instead.

I'll @ mention you on the other PR as soon as it's ready.

@victordibia
Copy link
Collaborator

Thank you so much!!

@SailorJoe6
Copy link
Collaborator Author

@victordibia - A day late, but (hopefully) not a penny short!

#3476 Enable human interaction in AutoGenStudio - Solution 2

@victordibia
Copy link
Collaborator

Closing this in favour of #3476 which has been merged into the ags branch

@victordibia victordibia closed this Sep 5, 2024
@SailorJoe6 SailorJoe6 deleted the issue_#1358_human_input branch September 7, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proj-studio Related to AutoGen Studio.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants