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
Add CRITIC agent integration #13108
Add CRITIC agent integration #13108
Conversation
llama-index-integrations/agent/llama-index-agent-critic/llama_index/agent/critic/step.py
Outdated
Show resolved
Hide resolved
llama-index-integrations/agent/llama-index-agent-critic/llama_index/agent/critic/step.py
Outdated
Show resolved
Hide resolved
llama-index-integrations/agent/llama-index-agent-critic/llama_index/agent/critic/step.py
Outdated
Show resolved
Hide resolved
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
||
# run reflective agent | ||
reflective_agent = self._reflective_agent_worker.as_agent() | ||
reflective_agent_response = reflective_agent.chat(main_agent_response.response) |
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 only thing the reflective agent has for context here is the response from the first agent. Shouldn't it also have some context on what the original query was too?
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.
Yeah, that's a 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.
This is the only one that I haven't resolved yet. I do pass the chat history to the ToolInteractiveReflectionAgent, but not to the critique agent which gets the task of reflection with tools.
To do this would require a bit of re-jigging on the prompts for the CritiqueAgent and then ofc the passing of the chat history, which can be done as prompt str.
I think it would be okay to do this in a future release of this package, in order to get this package out sooner. Thoughts?
...lama-index-agent-introspective/llama_index/agent/introspective/reflective/self_reflection.py
Outdated
Show resolved
Hide resolved
new_memory = ChatMemoryBuffer.from_defaults() | ||
|
||
# put current history in new memory | ||
messages = task.memory.get() |
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.
By calling get()
here, we only get the memory that fits in the current buffer window
Then later on for finalize
, we set the memory to all the memory from the task. This means that we've thrown away any memory that was outside of the original buffer window, which might not be desirable
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 don't think there's a perfect solution to this, but something to be aware of
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.
Thanks! I wonder if we should retain full memory and only use truncation when its about to be passed to an LLM.
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 how the react agent handles it is doing task_memory.get() + new_memory.get() -- but that also seems risky/not ideal.
Work for a future PR perhaps :)
state = step.step_state | ||
state["count"] += 1 | ||
|
||
messages = task.extra_state["new_memory"].get() |
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.
Does this include the initial task input? I might have missed where that gets added to memory
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.
yes it should, since the introspective agent
first adds the chat history from the main agent which includews the initial task input to the reflective agent's memory.
Line 162 in 0d05f90
reflective_agent_messages = task.extra_state["main"]["memory"].get() |
task.extra_state["new_memory"].put(critique_msg) | ||
|
||
# correct | ||
if is_done: |
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.
If self.stopping_callable
is none, is this just never done?
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.
oh in that case it will stop after max_iterations
has been reached. That gets implemented when building the TaskStepOutput
with is_last
field.
return TaskStepOutput(
output=agent_response,
task_step=step,
is_last=is_done | (self.max_iterations == state["count"]),
next_steps=new_steps,
)
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.
Hmm, interesting. I wonder if that's ideal or not. I would expect a combination of the main agent being is_done and the reflection agent being is_done to control when we return 🤔 Maybe work for a future PR
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.
oh interesting yea, at this point it always go to reflection and this is_done
is about when reflection/correction phases stop.
...x-integrations/agent/llama-index-agent-introspective/llama_index/agent/introspective/step.py
Show resolved
Hide resolved
llama-index-integrations/agent/llama-index-agent-introspective/tests/test_agent_critic.py
Show resolved
Hide resolved
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.
This looks good to me! Just a few comments/poossible edge cases.
Note for myself though -- need to do a better job of thinking about what goes in an integration vs. core 😅
Yeah, I had initially thought the |
alrighty @logan-markewich -- as discussed offline, I've cleaned up the memory for introspective agent removing the chat histories of the inner agents that it delegates tasks to. The final message in the history now lines up more nicely with the final response. |
Description
This PR adds a new package, namely
llama-index-agent-introspective
. This package introducesIntrospectiveAgent
's that perform tasks while utilizing the "reflection" agentic pattern. Two reflection agents that differ by their reflection mechanisms are also supplied in this package. Thus three main classes are introduced:IntrospectiveAgentWorker
ToolInteractiveReflectionAgentWorker
SelfReflectionAgentWorker
(adapted and ported over from add reflexion agent #13089 by @jerryjliu)Fixes # (issue)
New Package?
Did I fill in the
tool.llamahub
section in thepyproject.toml
and provide a detailed README.md for my new integration or package?Version Bump?
Did I bump the version in the
pyproject.toml
file of the package I am updating? (Except for thellama-index-core
package)Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration