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.

Sign up for GitHub

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

Jump to bottom

community[patch]: Fix Playwright Tools bug with Pydantic schemas #27050

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+34 −3
Open

community[patch]: Fix Playwright Tools bug with Pydantic schemas #27050

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c7f5c02
Fix Playwright Tools bug with Pydantic schemas
colriot Oct 2, 2024
478f6b3
Merge branch 'master' into colriot/playwright_pydantic_fix
colriot Oct 8, 2024
b31e054
Merge branch 'master' into colriot/playwright_pydantic_fix
efriis Oct 8, 2024
be3b9e0
update
eyurtsev Oct 9, 2024
24cc3f1
lint
eyurtsev Oct 9, 2024
a7fa9fd
lint
eyurtsev Oct 9, 2024
808c53c
x
eyurtsev Oct 9, 2024
5a159af
Fix naming and docstrings
colriot Oct 14, 2024
c5210c8
Merge branch 'master' into colriot/playwright_pydantic_fix
colriot Oct 14, 2024
eb48040
Merge branch 'master' into colriot/playwright_pydantic_fix
colriot Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion libs/community/langchain_community/tools/playwright/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
from __future__ import annotations

from typing import TYPE_CHECKING, Any, Optional, Tuple, Type

from langchain_core.tools import BaseTool
from langchain_core.utils import guard_import
from pydantic import model_validator
from pydantic import model_validator, BaseModel

if TYPE_CHECKING:

Check failure on line 9 in libs/community/langchain_community/tools/playwright/base.py

View workflow job for this annotation

GitHub Actions / cd libs/community / make lint #3.12

Ruff (I001) 

langchain_community/tools/playwright/base.py:1:1: I001 Import block is un-sorted or un-formatted

Check failure on line 9 in libs/community/langchain_community/tools/playwright/base.py

View workflow job for this annotation

GitHub Actions / cd libs/community / make lint #3.9

Ruff (I001) 

langchain_community/tools/playwright/base.py:1:1: I001 Import block is un-sorted or un-formatted
from playwright.async_api import Browser as AsyncBrowser
from playwright.sync_api import Browser as SyncBrowser
else:
Expand All @@ -31,10 +31,14 @@
guard_import(module_name="playwright.sync_api").Browser,
)

class BaseBrowserToolInput(BaseModel):
"""Base class for browser tool input when tool has no arguments."""

class BaseBrowserTool(BaseTool):
"""Base class for browser tools."""

args_schema: Type[BaseModel] = BaseBrowserToolInput
Copy link
Collaborator

Choose a reason for hiding this comment

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

@colriot why do we need to define a default schema on the base class? Ideally sub-classes would be forced to define the schema? Could we remove it here and add an appropriate schema in each tool that uses it?

I think definition here would make sense if this was called "ParameterlessBaseBrowserTool", but I think it's meant to capture any sort of tool and the main role of the base abstraction is to make sure that it accepts the browser attributes properly

Copy link
Author

Choose a reason for hiding this comment

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

@eyurtsev because my suggestion was to have a default NoArgs schema applied. And only tools that actually require non-empty params will need to explicitly override it. I.e. I don't see the reason for subclasses to define their own schema if it's NoArgs (same as it was before the fix, but actually working).

I have a few suggested options:

  • Rename BaseBrowserToolInput to NoArgsToolInput (or smth along the lines) and have it used as default schema value in BaseBrowserTool
  • Rename input, but remove default value from BaseBrowserTool and explicitly use it in every tool
  • Keep your suggestion, but then we'll have a few empty classes of no real use, that have identical purpose

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 see the reason for subclasses to define their own schema if it's NoArgs

For me the main reason is that it's an abstract class with an incorrect default. I don't feel strongly in this case -- end users aren't expected to use these types or subclass from the base playwright tool, so all the solutions are equivalent for end-users (and this can vary by personal style).

Do you want me to revert to the previous iteration or do you want to make the update?


sync_browser: Optional["SyncBrowser"] = None
async_browser: Optional["AsyncBrowser"] = None

Expand Down
4 changes: 1 addition & 3 deletions libs/community/langchain_community/tools/playwright/current_page.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
from __future__ import annotations

from typing import Optional, Type
from typing import Optional

from langchain_core.callbacks import (
AsyncCallbackManagerForToolRun,
CallbackManagerForToolRun,
)
from pydantic import BaseModel

from langchain_community.tools.playwright.base import BaseBrowserTool
from langchain_community.tools.playwright.utils import (
Expand All @@ -20,7 +19,6 @@ class CurrentWebPageTool(BaseBrowserTool):

name: str = "current_webpage"
description: str = "Returns the URL of the current page"
args_schema: Type[BaseModel] = BaseModel

def _run(
self,
Expand Down
6 changes: 2 additions & 4 deletions libs/community/langchain_community/tools/playwright/extract_text.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,24 @@
from __future__ import annotations

from typing import Any, Optional, Type
from typing import Any, Optional

from langchain_core.callbacks import (
AsyncCallbackManagerForToolRun,
CallbackManagerForToolRun,
)
from pydantic import BaseModel, model_validator
from pydantic import model_validator

from langchain_community.tools.playwright.base import BaseBrowserTool
from langchain_community.tools.playwright.utils import (
aget_current_page,
get_current_page,
)


class ExtractTextTool(BaseBrowserTool):

Check failure on line 17 in libs/community/langchain_community/tools/playwright/extract_text.py

View workflow job for this annotation

GitHub Actions / cd libs/community / make lint #3.12

Ruff (I001) 

langchain_community/tools/playwright/extract_text.py:1:1: I001 Import block is un-sorted or un-formatted

Check failure on line 17 in libs/community/langchain_community/tools/playwright/extract_text.py

View workflow job for this annotation

GitHub Actions / cd libs/community / make lint #3.9

Ruff (I001) 

langchain_community/tools/playwright/extract_text.py:1:1: I001 Import block is un-sorted or un-formatted
"""Tool for extracting all the text on the current webpage."""

name: str = "extract_text"
description: str = "Extract all the text on the current webpage"
args_schema: Type[BaseModel] = BaseModel

@model_validator(mode="before")
@classmethod
Expand Down
4 changes: 1 addition & 3 deletions libs/community/langchain_community/tools/playwright/navigate_back.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
from __future__ import annotations

from typing import Optional, Type
from typing import Optional

from langchain_core.callbacks import (
AsyncCallbackManagerForToolRun,
CallbackManagerForToolRun,
)
from pydantic import BaseModel

from langchain_community.tools.playwright.base import BaseBrowserTool
from langchain_community.tools.playwright.utils import (
Expand All @@ -20,7 +19,6 @@ class NavigateBackTool(BaseBrowserTool):

name: str = "previous_webpage"
description: str = "Navigate back to the previous page in the browser history"
args_schema: Type[BaseModel] = BaseModel

def _run(self, run_manager: Optional[CallbackManagerForToolRun] = None) -> str:
"""Use the tool."""
Expand Down
Empty file added libs/community/tests/unit_tests/tools/playwright/__init__.py
View file
Open in desktop
Empty file.
18 changes: 18 additions & 0 deletions libs/community/tests/unit_tests/tools/playwright/test_all.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
"""Test Playwright's Tools."""

from langchain_community.agent_toolkits import PlayWrightBrowserToolkit

from langchain_community.tools.playwright.utils import create_sync_playwright_browser


def test_playwright_tools_schemas() -> None:
"""Test calling 'tool_call_schema' for every tool to check to serialization issues."""

sync_browser = create_sync_playwright_browser()
tools = PlayWrightBrowserToolkit.from_browser(sync_browser=sync_browser).get_tools()

for tool in tools:
try:
tool.tool_call_schema
except Exception as e:
raise AssertionError(f"Error for '{tool.name}' tool: {type(e).__name__}: {e}") from e
Loading