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

Add CRITIC agent integration #13108

Merged
merged 40 commits into from May 3, 2024
Merged

Add CRITIC agent integration #13108

merged 40 commits into from May 3, 2024

Conversation

nerdai
Copy link
Contributor

@nerdai nerdai commented Apr 25, 2024

Description

This PR adds a new package, namely llama-index-agent-introspective. This package introduces IntrospectiveAgent'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:

  1. IntrospectiveAgentWorker
  2. ToolInteractiveReflectionAgentWorker
  3. 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 the pyproject.toml and provide a detailed README.md for my new integration or package?

  • Yes
  • No

Version Bump?

Did I bump the version in the pyproject.toml file of the package I am updating? (Except for the llama-index-core package)

  • Yes
  • No

Type of Change

Please delete options that are not relevant.

  • New package
  • This change requires a documentation update — notebooks have been added and module guides updated.

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

  • Added new unit/integration tests
  • Added new notebook (that tests end-to-end)
  • I stared at the code and made sure it makes sense

Copy link

Check out this pull request on  ReviewNB

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)
Copy link
Collaborator

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?

Copy link
Contributor Author

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!

Copy link
Contributor Author

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?

@nerdai nerdai requested a review from jerryjliu May 2, 2024 04:12
@nerdai nerdai marked this pull request as ready for review May 2, 2024 04:12
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label May 2, 2024
@nerdai nerdai mentioned this pull request May 2, 2024
new_memory = ChatMemoryBuffer.from_defaults()

# put current history in new memory
messages = task.memory.get()
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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()
Copy link
Collaborator

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

Copy link
Contributor Author

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.

reflective_agent_messages = task.extra_state["main"]["memory"].get()

task.extra_state["new_memory"].put(critique_msg)

# correct
if is_done:
Copy link
Collaborator

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?

Copy link
Contributor Author

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,
        )

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

@logan-markewich logan-markewich left a 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 😅

@nerdai
Copy link
Contributor Author

nerdai commented May 2, 2024

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 IntrospectiveAgent should be in core as reflection is a key pattern. But I opted to keep it contained in this package with reflection agents cause it seemed like a better package together than not.

@nerdai
Copy link
Contributor Author

nerdai commented May 3, 2024

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.

image

@nerdai nerdai merged commit b233b56 into main May 3, 2024
8 checks passed
@nerdai nerdai deleted the nerdai/critic branch May 3, 2024 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants