-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Agent, Eval] Fixes LLM config issue for delegation & Add eval to measure the delegation accuracy #2948
Conversation
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.
Basically LGTM, but just had one question to make sure that you intentionally removed the thought from the delegation task.
Co-authored-by: Graham Neubig <[email protected]>
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.
LGTM then!
extra={'msg_type': 'STEP'}, | ||
) | ||
|
||
if self.state.iteration >= self.state.max_iterations: |
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.
Moving this code around won't solve the real problem. The real problem is we don't accumulate steps across parents and children, and needs to be fixed by #2296.
This code move would likely introduce a bug: you will see the parent agent die while its child still running.
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.
Considering the slow progress of #2296, I can take it over unless someone else volunteers.
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.
you will see the parent agent die while its child still running.
On second thought, we do hope the parent agent dies when it exceeds the (global)_max_iteration
. Maybe i can just move this code block back for this PR, and wait for the gloabl_max_iteration
fix.
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.
Fix is coming! #2990
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 is still not fixed yet :( -
We probably need the headless_mode
change from https://github.com/OpenDevin/OpenDevin/pull/2994/files#diff-37dff7c7cda815bf00598e78acaaebb91df0bf68b820bdca78a86b89f67dfc9b to fix it:
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.
Right, a new problem arises after fixing the old problem lol
…me llm for cost accum purpose" This reverts commit 81034c4.
What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?
AgentDelegationAction
does not re-use theLLM
instance from the callerAgent
, which makes it hard to accumulate cost metrics. It won't work correctly use the right LLM config during evaluation.if self.delegate
is checked before the max iteration check, which makes the whole agent loop never stop.Give a summary of what the PR does, explaining any non-trivial design decisions
LLM
as an additional argument forAgentDelegationAction
so that the child agent can re-use the same instance from the parent (that particular instance won't be saved to the event stream, though).if self.delegate
check and the max iteration checkOther references