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

Google Search API #3049

Open
wants to merge 11 commits into
base: 0.2
Choose a base branch
from

Conversation

MohammedNagdy
Copy link

Why are these changes needed?

Bing is great in retrieving results. However, variety of search engines can be advantageous to other AI developers .

Related issue number

There is no issue related it is a new feature.

Example of Using Websurfer Agent with Google or Bing

Google

  • You can get the Google API key and the Google CX from the Google Search API. They are essential for the google search to work.
# Google
web_surfer = WebSurferAgent(
    name="Websurfer",
    llm_config=llm_configurations,
    browser_name="google",
    browser_config={"viewport_size": 4096, "api_key": os.environ["GOOGLE_API_KEY"], "cx": os.environ["GOOGLE_CX"]}
)

Bing

# Bing
web_surfer = WebSurferAgent(
    name="Websurfer",
    llm_config=llm_configurations,
    browser_name="bing",
    browser_config={"viewport_size": 4096, "api_key": "1111"]}
)

Checks

@MohammedNagdy
Copy link
Author

MohammedNagdy commented Jul 1, 2024 via email

name: Yifan Zeng
title: PhD student at Oregon State University
url: https://xhmy.github.io/
sonichi:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems there is a problem with CRLF and LR format for file ending with this file.

@sonichi sonichi requested a review from joshkyh July 4, 2024 15:10
@sonichi
Copy link
Contributor

sonichi commented Jul 4, 2024

@MohammedNagdy Could you please fix the conflict with main? Thanks.

@MohammedNagdy
Copy link
Author

@sonichi I fixed the conflict with the main branch. Could you take a look again?

Copy link

gitguardian bot commented Jul 20, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@@ -40,6 +43,7 @@ def __init__(
llm_config: Optional[Union[Dict, Literal[False]]] = None,
summarizer_llm_config: Optional[Union[Dict, Literal[False]]] = None,
default_auto_reply: Optional[Union[str, Dict, None]] = "",
browser_name: str = "bing",
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we use enum for this lookup table?

from ...code_utils import content_str
from ...oai.openai_utils import filter_config
from ...token_count_utils import count_token, get_max_token_limit

logger = logging.getLogger(__name__)

BROWSERS = {"google": GoogleTextBrowser, "bing": BingTextBrowser}
Copy link
Collaborator

Choose a reason for hiding this comment

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

here enums too, or at least some configuration so that users can register new text browsers if they implement new ones (like wikimedia servers for example) maybe a better idea is to pass the textbroser instance to the agent factory itself

pass


class SimpleTextBrowser:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could have a better name to highlight that this is the class to be derived for creating other TextBrowsers, TextBroswerBase ? Also do we want to have a class with virtual methods to override or a pure abstract and then SimpleTextBrowser is just an implementation of it?

Copy link
Author

Choose a reason for hiding this comment

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

@colombod I believe a base class would suffice and we can override the methods we need from there. More than this is honestly over engineering and will add unnecessary complexity. Let me know your opinion.

def _fetch_page(self, url: str) -> None:
try:
# Prepare the request parameters
request_kwargs = self.request_kwargs.copy() if self.request_kwargs is not None else {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you considered the benefits of using Playwright as opposed to a get request? A lot of pages will need dom composition and js execution to fully compose a usable page. Llamaindex web laoder is doing the same. That will make a much more reliable user experience and will meet expectations for we scraping tasks

Copy link
Author

Choose a reason for hiding this comment

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

I have not thought about using a third party library. As daunting as it might sound, keeping it simple and as it was was my first approach. However, we can go with either Playwright or Llamaindex web loader both are fine. But then we will need to make sure that the whole library is implementing the requests the same way. Let me know your thoughts. @colombod

Copy link
Collaborator

@colombod colombod Aug 13, 2024

Choose a reason for hiding this comment

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

The proposal I am doing is to just use that and move from a web get into actual web navigation. Playwright is what also LlamaIndex is using for navigation, that will also be useful as we see more multimodel approach

Copy link
Author

Choose a reason for hiding this comment

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

@colombod I started working on adding Playwright into the PR. However, it seems its async functionality is a bit unstable giving me timeout errors, then working properly on another request. I am not sure if you have any experience of overcoming this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is not the experience I have with it, can you show one of the issues you are facing? Maybe I can help or we could get Playwright engineers to investigate.

Copy link
Author

@MohammedNagdy MohammedNagdy Sep 24, 2024

Choose a reason for hiding this comment

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

@colombod Well, I have been trying in the past couple of weeks. But I always get these failed tests

FAILED test/test_browser_utils_google.py::test_simple_text_browser - assert 'Redmond' in 'Page.goto: net::ERR_ABORTED at https://en.wikipedia.org/wiki/Microsoft\nCall log:\nnavigating to "https://en.wikipedia.org/wiki/Microsoft", waiting until "networkidle"\n'
FAILED test/test_browser_utils_google.py::test_google_search - assert "A Google search for 'Microsoft' found" in 'Page.goto: net::ERR_ABORTED at google: Microsoft\nCall log:\nnavigating to "google: Microsoft", waiting until "networkidle"\n'

Although I have the timeout increased to 600 seconds

response = await self._page.goto(url, wait_until='networkidle', timeout=600000)

Copy link
Collaborator

@colombod colombod 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 added some comments and would like to discuss those further

@MohammedNagdy
Copy link
Author

@colombod I added some changes let me know what do you think?

@MohammedNagdy
Copy link
Author

@colombod I added the change for the enum part you can check and let me know if you have any comments

@MohammedNagdy MohammedNagdy mentioned this pull request Aug 1, 2024
7 tasks
@gagb gagb requested review from afourney and removed request for sonichi and qingyun-wu September 9, 2024 05:52
@ekzhu ekzhu changed the base branch from main to 0.2 October 2, 2024 18:27
@jackgerrits jackgerrits added the 0.2 Issues which were filed before re-arch to 0.4 label Oct 4, 2024
@rysweet rysweet added the awaiting-op-response Issue or pr has been triaged or responded to and is now awaiting a reply from the original poster label Oct 10, 2024
@rysweet
Copy link
Collaborator

rysweet commented Oct 12, 2024

Hi @MohammedNagdy - this is nice work and good iteration - I think it is close - we rebased it for you and updated. Can you please have a look at the conflicts and green the CI? Then we will get someone to review.

Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
@MohammedNagdy
Copy link
Author

@rysweet I will take a look last time I worked on it. I was stuck on network errors for Playwright it was annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.2 Issues which were filed before re-arch to 0.4 awaiting-op-response Issue or pr has been triaged or responded to and is now awaiting a reply from the original poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants