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
feat: react agent integration #179
base: master
Are you sure you want to change the base?
Conversation
The file structure in master branch was modified to be different as this branch. The merge is for convenience of developing later.
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.
Overall looks good. Tests must be added.
camel/agents/ReAct_agent.py
Outdated
@@ -0,0 +1,222 @@ | |||
# =========== Copyright 2023 @ CAMEL-AI.org. All Rights Reserved. =========== |
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.
Rename to lower case react_agent.py
camel/agents/ReAct_agent.py
Outdated
self, | ||
input_message: ChatMessage, | ||
max_turns: int = 10, | ||
) -> Tuple[ChatMessage, bool, Dict[str, Any]]: |
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 signature of step does not follow the signature of the base class. If you want to introduce a new signature you must name the method differently
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.
Sorry but this pattern was borrowed fromEmbodiedAgent
and ChatAgent
where they both used a completely different signature as the one in base class (def step(self) -> None:
). I think the version in the base (BaseAgent
) is too abstract and does not return anything and should not be used?
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.
Please do not use EmbodiedAgent as a reference. There are some deficiencies in the code that we will fix soon. Please check your code with mypy and make sure you code does not introduce any warnings. Soon mypy will be made obligatory.
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 do not totally agree with naming all the methods differently which will make it very difficult for the user to use. Just like PyTorch, every deviated Module
has a different forward
method with a different signature. It is still ok. IMO, the base class method's signature could be def step(self, *args, **kwargs) -> Any:
.
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 key feature of Module is polymorphism, a Module can be composed, the implementation can be abstracted away. In camel I do not see a single instance of polymorphic use of step() and reset(), implying it is not needed so far.
camel/agents/ReAct_agent.py
Outdated
return action, action_input | ||
|
||
|
||
class ReActAgent(ChatAgent): |
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.
In the nearest future we are going to move away from inheritance to a "part of" principle for agents. A chat agent must be a field of ReactAgent.
*args: Any, | ||
) -> None: | ||
try: | ||
from langchain.utilities import SerpAPIWrapper |
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 are bringing in langchain as a dependency which is not reflected in requirements.txt
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.
In pyproject.toml, it is
examples/ReAct/tool_usage.py
Outdated
from camel.typing import RoleType, ModelType | ||
|
||
|
||
def main(): |
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.
Every example must be tested with pytest, parametrized with at least ModelType.STUB
camel/agents/ReAct_agent.py
Outdated
|
||
|
||
class ReActAgent(ChatAgent): | ||
r"""Class for managing conversions of a CAMEL agent following ReAct pattern. |
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.
Every introduced code must be covered by a test.
from camel.agents.tool_agents import BaseToolAgent | ||
|
||
|
||
class GoogleToolAgent(BaseToolAgent): |
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.
Every introduced code must be covered by a test.
return p.encode().decode("unicode-escape").encode("latin1").decode("utf-8") | ||
|
||
|
||
class WikiToolAgent(BaseToolAgent): |
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.
Every introduced code must be covered by a test.
Thanks very much for these comments. I know there are many problems with this commit and because I spent past several days working on another functionality (OpenAI function calling) I have not updated them on time. I will continue on this following your guidance later :) |
Merge to see the changes brought by introducing mypy
Hi, currently I ran into some problems:
|
For extra packages, put them in pyproject.toml poetry section. |
Description
Basically implemented the agent which outputs in ReAct pattern but only tested on several QA tasks. This PR is mainly for review purposes.
Motivation and Context
Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.
You can use the syntax
close #15213
if this solves the issue #15213Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Implemented Tasks
Checklist
Go over all the following points, and put an
x
in all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!