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

feat: add web search #274

Merged
merged 35 commits into from
Oct 5, 2023
Merged

feat: add web search #274

merged 35 commits into from
Oct 5, 2023

Conversation

zhiyu-01
Copy link
Contributor

@zhiyu-01 zhiyu-01 commented Sep 3, 2023

Description

Search web for information.
When agent ask a question, google search can provide a list of urls which relative with the question, then using requests to get
each url and extract text. Split the text to small chunks and feed to chatgpt to get answer.

Notice: these functions require google api key you can find here https://developers.google.com/custom-search/v1/overview,
and search engine id you can find here https://cse.google.com/cse/all.

Motivation and Context

Why is this change required? What problem does it solve?

Give the agent ability to search internet.

  • I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds core functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)
  • Example (update in the folder of example)

Implemented Tasks

  • Subtask 1
  • Subtask 2
  • Subtask 3

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!

  • I have read the CONTRIBUTION guide. (required)
  • My change requires a change to the documentation.
  • I have updated the tests accordingly. (required for a bug fix or a new feature)
  • I have updated the documentation accordingly.

@zhiyu-01 zhiyu-01 mentioned this pull request Sep 3, 2023
13 tasks
@HalberdOfPineapple
Copy link
Member

The functionality is overall good. But there exists a vital problem that this should not be a version that can be publically merged because you use your own Google Key in the example... (you definitely do not hope all the public users use your api key, do you?) A good idea to make these be input via environment variables, but I think you definitely should talk with team members such as Guohao @lightaime first to have our team's API key beforehand and embed it into the Github workflow as secret variables just as the OpenAI key. (I guess these depend on whether our team approves to support this and buy a Google API key).
And I guess you need to get a new key now because even the commits here are publically available, which can be potentially abused by others.

@zhiyu-01
Copy link
Contributor Author

zhiyu-01 commented Sep 3, 2023

The functionality is overall good. But there exists a vital problem that this should not be a version that can be publically merged because you use your own Google Key in the example... (you definitely do not hope all the public users use your api key, do you?) A good idea to make these be input via environment variables, but I think you definitely should talk with team members such as Guohao @lightaime first to have our team's API key beforehand and embed it into the Github workflow as secret variables just as the OpenAI key. (I guess these depend on whether our team approves to support this and buy a Google API key). And I guess you need to get a new key now because even the commits here are publically available, which can be potentially abused by others.

Thank you for your concern, but don't worry, this google api key is free, and only 100 times search every day.

@HalberdOfPineapple
Copy link
Member

HalberdOfPineapple commented Sep 5, 2023

Guohao has approved adding the API key into the secret later. So we can focus on others.
I thought the functionality should be overall good but the documentation needs to be further modified. Especially the annotations of the functions, which are essentially the prompts to be input to the LM.
Do you mind my directly modifying the code and submitting commits in this branch for collaboration? @zhiyu-01 I thought it might be more straightforward and efficent.

@zhiyu-01
Copy link
Contributor Author

zhiyu-01 commented Sep 5, 2023

@HalberdOfPineapple
Ok, i also think it need to be improved.

Copy link
Member

@HalberdOfPineapple HalberdOfPineapple left a comment

Choose a reason for hiding this comment

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

Well I thought the function implementation needs to be changed.
In your implementation of search_web, you defined a function single_step_agent which will temporarily create an agent to summarize and decide on which result to be returned. In my opinion, this is too costly... I think there is no need to involve the LM itself in the functions. Instead, the raw results obtained from the web can be directly parsed into a single string as the function execution result and this will be recorded in the message history. The model will generate content taking consideration of all the information in the message history (strictly speaking, within its token limit), and hence there is no need to do a separate summarization and answer decision stage, which is costly and may block the agent from observing all information.
On the other hand, it may be not practical for the function search_google to be added to the function list because it seems it can only return links and the model cannot directly make usage of them.
The contents above are just my personal suggestions and we could discuss more.

As for the annotations/documentation, before I add comments line by line, I guess it would be better for you to modify annotations by yourself with the following rules in mind:

  1. Capitalize the first letter of each comment/statement.
  2. Make the annotations for each argument or each returned result clearer, at least making the user quickly knowing what the variable simply from the single annotation. For example, the documentation of the returned result in function create_chunks, List[str]: a list of chunks, is not clear enough.
  3. Make types of variables clear. Sometimes you did not specify the element type of a List and omit the type in the documentation explaining the argument.

@zhiyu-01
Copy link
Contributor Author

zhiyu-01 commented Sep 5, 2023

In my test, the texts extract from website sometimes may have a huge amount, 10k+ or even more. I have set a limit length 3000 to cut texts, if that's still smaller than your thought, we can change it. And we can't guarantee the first website will give us enough information, so i think we do need an angent to decide whether to continue search or not.
As for search_google, the returned dict also contain the description of the website, I just thought it might can provide suggestion of websites but doesn't actually get in it.
@HalberdOfPineapple

Copy link
Contributor Author

@zhiyu-01 zhiyu-01 left a comment

Choose a reason for hiding this comment

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

I have modified some annotations.

Copy link
Collaborator

@Obs01ete Obs01ete left a comment

Choose a reason for hiding this comment

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

See the comments

test/agents/test_role_playing.py Outdated Show resolved Hide resolved
@@ -294,7 +294,8 @@ def step(
a boolean indicating whether the chat session has terminated,
and information about the chat session.
"""
messages = self.update_messages('user', input_message)
messages = self.update_messages(input_message.role_type.value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait a second. I recall figuring out this complex behavior of changing the roles. I even put an explicit comment:

Its `role` field that specifies the role at backend may be either
            `user` or `assistant` but it will be set to `user` anyway since
            for the self agent any incoming message is external.

also see the doctoring above:

def submit_message(self, message: BaseMessage) -> None:
        r"""Submits the externally provided message as if it were an answer of
        the chat LLM from the backend. Currently, the choice of the critic is
        submitted with this method.

Please let me know if you think this is in the scope of this change or not. If not, make a separate bug ticket and a separate PR with the proper fix and the test, and revert the irrelevant changes in thin PR.

@zhiyu-01
Copy link
Contributor Author

This pr has been reverted, and I opened an issue #298, you can see more detail in it.
@Obs01ete

@zhiyu-01
Copy link
Contributor Author

zhiyu-01 commented Sep 24, 2023

I have changed the role at backend to match the message, this function is works well now.
The test of role sequence is also added.
@Obs01ete

Copy link
Member

@lightaime lightaime left a comment

Choose a reason for hiding this comment

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

Thanks @zhiyu-01 for the updates. I still have some doubts.

@@ -294,7 +295,8 @@ def step(
a boolean indicating whether the chat session has terminated,
and information about the chat session.
"""
messages = self.update_messages('user', input_message)
messages = self.update_messages(input_message.role_type.value,
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why this change is needed. @Obs01ete @zhiyu-01 can you explain?

Comment on lines 368 to 371
content=(f"{self.user_sys_msg.content}. "
content=(f"{self.assistant_sys_msg.content}. "
"Now start to give me instructions one by one. "
"Only reply with Instruction and Input."))

user_msg = BaseMessage.make_user_message(
role_name=self.user_sys_msg.role_name,
content=f"{self.assistant_sys_msg.content}")
assistant_response = self.assistant_agent.step(user_msg)
assistant_response = self.assistant_agent.step(assistant_msg)
Copy link
Member

Choose a reason for hiding this comment

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

The assistant_msg is the message to be sent to the user by the assistant agent. So I guess the self.user_sys_msg is needed to be sent. Do you confirm this change is correct?

Comment on lines 151 to 178
def test_role_playing_role_sequence(model_type=None):
task_prompt = "Develop a trading bot for the stock market"
role_playing = RolePlaying(
assistant_role_name="Python Programmer",
assistant_agent_kwargs=dict(model=model_type),
user_role_name="Stock Trader",
user_agent_kwargs=dict(model=model_type),
task_prompt=task_prompt,
with_task_specify=True,
task_specify_agent_kwargs=dict(model=model_type),
)
assistant_role_sequence = []
user_role_sequence = []

input_assistant_msg, _ = role_playing.init_chat()
assistant_response, user_response = role_playing.step(input_assistant_msg)
input_assistant_msg = assistant_response.msg
assistant_response, user_response = role_playing.step(input_assistant_msg)

for record in role_playing.user_agent.stored_messages:
user_role_sequence.append(record.role_at_backend)
for record in role_playing.assistant_agent.stored_messages:
assistant_role_sequence.append(record.role_at_backend)

assert user_role_sequence == \
['system', 'assistant', 'user', 'assistant', 'user']
assert assistant_role_sequence == \
['system', 'assistant', 'user', 'assistant', 'user', 'assistant']
Copy link
Member

Choose a reason for hiding this comment

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

I have some doubts about this test. Should it be ['system', 'user', 'assistant', 'user', 'assistant', ...] normally?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very important to have tested! I request this change in #298.

@zhiyu-01
Copy link
Contributor Author

Here is an example of the original code, it's the trading bot task, it seems all good if just look at the agents' response, but the stored message is not that good:
origin-suer
origin-assistant

The role type is not match the content, or the role_at_backend. The assistant_msg didn't sent to the user either, it just have a assitant role type, the content is still the user's.

Even have these irrationality, the agents still can give the correctly response.But when calling function, agents can not behave normally, it gets confused:
function-user

The conversation is getting stuck:
image

I think it's the role type mismatch the content that caused the confusion, though common tasks can work well, I stiil think the change is needed. Besides, after the change, I have runned several tests, they all work fine.
modified-user
modified-assistant
image

About the role sequence, in user, the first system message containts user info:
image
then recives the assistant message:
image
return the response:
image

In assitant, the system message contains assitant message:
image
But before the role playing start, there is a init which will update a message to assitant:
image
image
So, after the system it will be an assistant.
Then recives the user message:
image
return the response:
image

These is the reason why I want the change, I hope it have some correctness.
@lightaime

Copy link
Collaborator

@Obs01ete Obs01ete left a comment

Choose a reason for hiding this comment

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

Revert all the changes marked Revert. Also create a PR for #298 and move the test there.

Comment on lines 151 to 178
def test_role_playing_role_sequence(model_type=None):
task_prompt = "Develop a trading bot for the stock market"
role_playing = RolePlaying(
assistant_role_name="Python Programmer",
assistant_agent_kwargs=dict(model=model_type),
user_role_name="Stock Trader",
user_agent_kwargs=dict(model=model_type),
task_prompt=task_prompt,
with_task_specify=True,
task_specify_agent_kwargs=dict(model=model_type),
)
assistant_role_sequence = []
user_role_sequence = []

input_assistant_msg, _ = role_playing.init_chat()
assistant_response, user_response = role_playing.step(input_assistant_msg)
input_assistant_msg = assistant_response.msg
assistant_response, user_response = role_playing.step(input_assistant_msg)

for record in role_playing.user_agent.stored_messages:
user_role_sequence.append(record.role_at_backend)
for record in role_playing.assistant_agent.stored_messages:
assistant_role_sequence.append(record.role_at_backend)

assert user_role_sequence == \
['system', 'assistant', 'user', 'assistant', 'user']
assert assistant_role_sequence == \
['system', 'assistant', 'user', 'assistant', 'user', 'assistant']
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very important to have tested! I request this change in #298.

test/agents/test_role_playing.py Outdated Show resolved Hide resolved
camel/agents/chat_agent.py Outdated Show resolved Hide resolved
camel/agents/chat_agent.py Outdated Show resolved Hide resolved
camel/societies/role_playing.py Outdated Show resolved Hide resolved
camel/societies/role_playing.py Outdated Show resolved Hide resolved
@Obs01ete
Copy link
Collaborator

@zhiyu-01 thank you for following the instructions! Please remind, did you send code coverage report for this PR? If not, please check CONTRIBUTING.md and attached zipped HTML folder.

@zhiyu-01
Copy link
Contributor Author

zhiyu-01 commented Oct 1, 2023

htmlcov.zip
@Obs01ete Here is the coverage.

@zhiyu-01
Copy link
Contributor Author

zhiyu-01 commented Oct 1, 2023

@lightaime I have some misunderstand about how camel works, thanks for @HalberdOfPineapple , I have understanded now. Sorry for the bother.

@zhiyu-01
Copy link
Contributor Author

zhiyu-01 commented Oct 1, 2023

@HalberdOfPineapple Hi, you need to submmit an approve review, so I can merge this pr.

@HalberdOfPineapple
Copy link
Member

@zhiyu-01 Well after @Obs01ete or @lightaime approve this change you would be able to merge it. The write access is not controlled by me. And we are still discussing the implementation of web search.

Copy link
Collaborator

@Obs01ete Obs01ete left a comment

Choose a reason for hiding this comment

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

Good to see the role playing code reverted. All good now.

@Obs01ete
Copy link
Collaborator

Obs01ete commented Oct 2, 2023

@HalberdOfPineapple @lightaime approve pls

Copy link
Member

@lightaime lightaime left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @zhiyu-01!! Please feel free to merge.

Copy link
Member

@HalberdOfPineapple HalberdOfPineapple left a comment

Choose a reason for hiding this comment

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

LGTM

@zhiyu-01 zhiyu-01 merged commit 3ba1754 into master Oct 5, 2023
8 checks passed
@zhiyu-01 zhiyu-01 deleted the function branch October 5, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants