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.

Sign up for GitHub

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

Sync history to stream #2640

Merged
merged 6 commits into from
Jun 29, 2024
Merged

Sync history to stream #2640

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
64d3465
add event to stream before budget check
enyst Jun 25, 2024
2ae5d1b
Merge branch 'main' into enyst/history_sync
enyst Jun 26, 2024
3ad34a9
Merge branch 'main' of github.com:OpenDevin/OpenDevin into enyst/hist…
enyst Jun 29, 2024
05e4406
make the budget check before the step
enyst Jun 29, 2024
12ab433
Merge branch 'main' into enyst/history_sync
enyst Jun 29, 2024
6da5c7d
Update opendevin/controller/agent_controller.py
enyst Jun 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions opendevin/controller/agent_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,13 +319,13 @@ async def _step(self):
else:
await self.add_history(action, NullObservation(''))

if not isinstance(action, NullAction):
await self.event_stream.add_event(action, EventSource.AGENT)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would cause some trouble for delegation at least. When you add an event to the stream, its callback is called. In delegation scenario, the control is then handed over to child agent (controller). Even if we just exceeded the budget during this step, the child agent would still continue because we haven't computed and checked budget yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this scenario you're thinking of at the beginning of delegation, so when the event is a delegate action?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, when event is AgentDelegateAction

Copy link
Collaborator Author

@enyst enyst Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, we could stop it at some point ( perhaps on_event or follow up), though I'm not sure it will be pretty. Or when the controller is about to start and initializes the budget (oops error). I guess we could even, if it seems better, set the agent to ERROR, then add the event; to check for that state and stop the process. I'm not anymore at the computer, will give it some thought. If we can't, IMHO an alternative is to cancel, that is, this action "doesn't exist". I still don't see it in history unless it's in the stream...

(On a side note, the funny thing here is that I literally couldn't merge the current logic. Because histories are copies of the event stream in main, and the copies will vanish when we refactor to have the event stream as the source of truth. There's no 'space' between add_history and add_event anymore 😅 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the worst case, we could merge this PR as it is, and don't care about the budget thing I mentioned. It won't be ideal but not too bad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would agree, but maybe the simplest is to just move the budget check before the call to the agent step. It seems like it would work, to stop the next step... Will check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but maybe the simplest is to just move the budget check before the call to the agent step

Oh yeah sounds like a plan.


await self.update_state_after_step()
if self.state.agent_state == AgentState.ERROR:
return

if not isinstance(action, NullAction):
await self.event_stream.add_event(action, EventSource.AGENT)

if self._is_stuck():
await self.report_error('Agent got stuck in a loop')
await self.set_agent_state_to(AgentState.ERROR)
Expand Down