feat: support controlling agent task state. #1094

iFurySt · 2024-04-14T12:43:14Z

feat: add agent task state to agent status bar.
feat: add agent task control bar to FE.

#1049

iFurySt · 2024-04-14T12:48:47Z

Screen.Recording.mov

ignore the error BE threw, i just started a 2b LLM to fastly demonstrate.

rbren

This is awesome! Thanks for taking it on

rbren · 2024-04-14T13:38:14Z

opendevin/controller/agent_controller.py

+
+            if self._task_state == TaskState.FINISHED:
+                await self._stop_task(is_finished=True)  # type: ignore[unreachable]
+                return


should this be break so we hit the log statement below?

actually, i don't what you mean 😅

break means "exit the loop". return means "exit the function"

since you're returning here, we never reach the log statement that's below the loop

rbren · 2024-04-14T13:39:35Z

opendevin/controller/agent_controller.py

+        self.state = None
+        self._cur_step = 0


should we call reset_task here?

good idea, reuse code.

rbren · 2024-04-14T13:40:20Z

opendevin/server/agent/agent.py


    def close(self):
+        # self.stop_task()


rbren · 2024-04-14T13:40:54Z

opendevin/server/agent/agent.py

+        if self.controller is not None:
+            event_dict['task_state'] = self.controller.get_task_state().value
+        self.send(event_dict)


instead of sending this constantly, should we send an event when state changes?

you are right. let me adjust it. btw, the asyncio.create_task to start the agent task will very likely block other threads. i think we should consider using multiprocessing to run the agent task in Python to realize the true async.

* feat: add agent task state to agent status bar. * feat: add agent task control bar to FE.

iFurySt · 2024-04-14T14:57:15Z

have been updated. sorry for the force push, take care next time😅

iFurySt · 2024-04-14T15:01:14Z

the changes mainly includes:

delete asyncio.Event(), i think it's unnecessary for now.
add TaskStateChangedAction to carry the changed task state which is sent to the client.

rbren · 2024-04-15T05:08:56Z

sorry for the force push, take care next time

I force push to my branches all the time 😄 as long as you don't force push to main it's all good!

I use git push origin +branch-name to only force push a single branch.

rbren · 2024-04-15T05:29:16Z

Tried this locally. Seems to work well!

Two things I think we can improve on:

"pause" waits for the current step to complete before giving feedback. If the step is taking a long time (e.g. step 0) then it feels kind of broken, because you don't get any feedback that it worked. In the future it'd be cool if we can just cancel the current step
"reset" and "stop" feel duplicative. I kind of like that "reset" totally clears everything--I wonder if that should be the "stop" behavior.

I'm good merging this as-is as it's already a big improvement. But the second bullet might be easy to address at least

li-boxuan · 2024-04-15T05:36:26Z

opendevin/server/agent/agent.py

@@ -133,15 +169,24 @@ async def create_controller(self, start_event: dict):
                callbacks=[self.on_agent_event],
            )
        except Exception as e:
-            logger.exception(f'Error creating controller: {e}')
+            logger.exception(f"Error creating controller: {e}")


Could you please try running the following command locally?

poetry run pre-commit run --files opendevin/**/* agenthub/**/* --show-diff-on-failure --config ./dev_config/python/.pre-commit-config.yaml

I am surprised to see 1) your pre-commit hook doesn't fix this, and 2) CI doesn't fail.

FWIW I ran the above command on my end, and it would change this double quote to single quote.

nothing happened.

Poetry (version 1.8.2) pre-commit 3.7.0

:(

Something's wrong with Fix double quoted strings hook

IMO, it doesn't always detect the f"" pattern, even in the GitHub workflow CI env it seems to only detect some cases but not all about this pattern 🤔

I am able to reproduce it using 3.12.2. With 3.11.x, this line will be fixed by the plugin. (That's sad, isn't it? We don't want to force everyone to use the same python version just because of a linting tool). That being said, I don't know why CI (which uses 3.11) would ignore this line.

pre-commit/pre-commit-hooks#973

seems already to be fixed? i can take a look in my env later.

seems already to be fixed?

Nope I tried, it didn't work (even after upgrading pre-commit-hooks to latest version that includes this fix)

i've read the source code of pre-commit-hooks, they just easy skip the fstring if python version is greater than 3.12~

That's the most confusing part because that code supposably only exists in their latest version, while we are using an old version.

iFurySt · 2024-04-15T05:36:45Z

Tried this locally. Seems to work well!

Two things I think we can improve on:

"pause" waits for the current step to complete before giving feedback. If the step is taking a long time (e.g. step 0) then it feels kind of broken, because you don't get any feedback that it worked. In the future it'd be cool if we can just cancel the current step

"reset" and "stop" feel duplicative. I kind of like that "reset" totally clears everything--I wonder if that should be the "stop" behavior.

I'm good merging this as-is as it's already a big improvement. But the second bullet might be easy to address at least

do you mean removing the stop and just keeping the reset for now?

yep, pause may get the feedback for a long time. i originally added an icon loading overlapping with the pause button, but then i removed it. i'll try to handle it to get better ux.

li-boxuan · 2024-04-15T05:44:34Z

Tried locally and it worked well, this is awesome!

I think it would be great if we could disable some buttons in certain states, e.g. the pause button could be disabled when already in pause state. Could be a follow-up, though.

…l-agent-task-state

iFurySt · 2024-04-15T05:47:04Z

i've deleted the stop action for now.

iFurySt · 2024-04-15T05:47:55Z

Tried locally and it worked well, this is awesome!

I think it would be great if we could disable some buttons in certain states, e.g. the pause button could be disabled when already in pause state.

yep, i think we can add disabled and loading status to improve UX.

rbren · 2024-04-17T17:43:50Z

@iFurySt we should probably combine play and pause into the same button (showing "play" if it's paused, and showing "pause" if it's running)

iFurySt · 2024-04-18T00:49:30Z

@iFurySt we should probably combine play and pause into the same button (showing "play" if it's paused, and showing "pause" if it's running)

Okay, let me adjust it.

…led status for action buttons.

iFurySt · 2024-04-18T06:29:38Z

Screen.Recording.mov

@rbren have a look. i've just combined pause and resume into one button. and made some improvements to the loading and disabled status for buttons.

frontend/src/services/socket.ts

rbren reviewed Apr 14, 2024

View reviewed changes

feat: support controlling agent task state.

906ad0d

* feat: add agent task state to agent status bar. * feat: add agent task control bar to FE.

iFurySt force-pushed the feat-support-control-agent-task-state branch from 2290615 to 906ad0d Compare April 14, 2024 14:53

rbren approved these changes Apr 15, 2024

View reviewed changes

li-boxuan reviewed Apr 15, 2024

View reviewed changes

iFurySt added 2 commits April 15, 2024 13:45

Remove stop agent task action.

f2d5da5

Merge remote-tracking branch 'upstream/main' into feat-support-contro…

0c9d407

…l-agent-task-state

li-boxuan mentioned this pull request Apr 15, 2024

Fix python linter inconsistent behaviour with quotes #1112

Merged

rbren mentioned this pull request Apr 15, 2024

Reset the Docker container from the UI #565

Closed

This was linked to issues Apr 15, 2024

Add pause/resume functionality (and button) #1049

Closed

Add "stop/cancel" button #470

Closed

iFurySt added 2 commits April 18, 2024 11:55

merge from upstream/main

26709e2

Merge pause and resume buttons into one button; Add loading and disab…

7f7c249

…led status for action buttons.

rbren approved these changes Apr 18, 2024

View reviewed changes

frontend/src/services/socket.ts Outdated Show resolved Hide resolved

frontend/src/services/socket.ts Outdated Show resolved Hide resolved

frontend/src/services/socket.ts Outdated Show resolved Hide resolved

frontend/src/services/socket.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

8dc3a28

rbren enabled auto-merge (squash) April 18, 2024 10:58

Merge branch 'main' into feat-support-control-agent-task-state

4b0c040

rbren merged commit 1356da8 into OpenDevin:main Apr 18, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support controlling agent task state. #1094

feat: support controlling agent task state. #1094

iFurySt commented Apr 14, 2024

iFurySt commented Apr 14, 2024

rbren left a comment

rbren Apr 14, 2024

iFurySt Apr 14, 2024

rbren Apr 17, 2024

rbren Apr 14, 2024

iFurySt Apr 14, 2024

rbren Apr 14, 2024

rbren Apr 14, 2024

iFurySt Apr 14, 2024

rbren Apr 14, 2024

iFurySt commented Apr 14, 2024

iFurySt commented Apr 14, 2024

rbren commented Apr 15, 2024

rbren commented Apr 15, 2024

li-boxuan Apr 15, 2024 •

edited

iFurySt Apr 15, 2024

iFurySt Apr 15, 2024

li-boxuan Apr 15, 2024

iFurySt Apr 15, 2024

li-boxuan Apr 15, 2024 •

edited

iFurySt Apr 15, 2024

li-boxuan Apr 15, 2024

iFurySt Apr 15, 2024

li-boxuan Apr 15, 2024

iFurySt commented Apr 15, 2024

li-boxuan commented Apr 15, 2024 •

edited

iFurySt commented Apr 15, 2024

iFurySt commented Apr 15, 2024

rbren commented Apr 17, 2024

iFurySt commented Apr 18, 2024

iFurySt commented Apr 18, 2024

feat: support controlling agent task state. #1094

feat: support controlling agent task state. #1094

Conversation

iFurySt commented Apr 14, 2024

iFurySt commented Apr 14, 2024

rbren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iFurySt commented Apr 14, 2024

iFurySt commented Apr 14, 2024

rbren commented Apr 15, 2024

rbren commented Apr 15, 2024

li-boxuan Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

li-boxuan Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iFurySt commented Apr 15, 2024

li-boxuan commented Apr 15, 2024 • edited

iFurySt commented Apr 15, 2024

iFurySt commented Apr 15, 2024

rbren commented Apr 17, 2024

iFurySt commented Apr 18, 2024

iFurySt commented Apr 18, 2024

li-boxuan Apr 15, 2024 •

edited

li-boxuan Apr 15, 2024 •

edited

li-boxuan commented Apr 15, 2024 •

edited