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

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

Merged
merged 14 commits into from
Oct 30, 2024

Conversation

colriot
Copy link
Contributor

@colriot colriot commented Oct 2, 2024

  • Add tests for Playwright tools schema serialization
  • Introduce base empty args Input class for BaseBrowserTool

Test Plan: poetry run pytest tests/unit_tests/tools/playwright/test_all.py

Fixes #26758

Copy link

vercel bot commented Oct 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Oct 30, 2024 11:43pm

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. community Related to langchain-community 🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature labels Oct 2, 2024
@colriot
Copy link
Contributor Author

colriot commented Oct 7, 2024

Hello @efriis @eyurtsev !
This fix deals with a nasty blocker bug in Playwright toolkit. Can you please help unblock it and I'll iterate on it if needed?

@efriis efriis assigned efriis and eyurtsev and unassigned efriis Oct 8, 2024

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
Contributor 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?

@eyurtsev
Copy link
Collaborator

eyurtsev commented Oct 8, 2024

Can merge if we can package the schema for the tool inputs together with the appropriate tools

@eyurtsev
Copy link
Collaborator

eyurtsev commented Oct 9, 2024

Taking a look at rolling out the fix right now

@eyurtsev eyurtsev changed the title Fix Playwright Tools bug with Pydantic schemas community[patch]: Fix Playwright Tools bug with Pydantic schemas Oct 9, 2024
@eyurtsev eyurtsev enabled auto-merge (squash) October 9, 2024 14:10
Copy link
Collaborator

@eyurtsev eyurtsev left a comment

Choose a reason for hiding this comment

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

Incorporated changes

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Oct 9, 2024
auto-merge was automatically disabled October 14, 2024 14:31

Head branch was pushed to by a user without write access

@colriot
Copy link
Contributor Author

colriot commented Oct 14, 2024

Left a comment in the above discussion for a final decision, @eyurtsev

@rapcal
Copy link

rapcal commented Oct 26, 2024

Any plans to merge this? It's a super simple PR and I am sure a lot of people are being blocked from upgrading to langchain@0.3 by not having it merged.

@efriis
Copy link
Member

efriis commented Oct 30, 2024

It looks like the test is unable to run in CI and also messes up the event loop for other tests - aware of any other testing strategies that could work (e.g. mocking the relevant playwright calls, not relying on the sdk), or would recommendation just be to remove the test?

@efriis
Copy link
Member

efriis commented Oct 30, 2024

also appreciate your patience on this one! sorry for the delays in review cycle

@efriis
Copy link
Member

efriis commented Oct 30, 2024

merging with a mock - can do something different in a future PR if desired

@efriis efriis enabled auto-merge (squash) October 30, 2024 23:45
@efriis efriis merged commit 8180637 into langchain-ai:master Oct 30, 2024
18 checks passed
@rapcal
Copy link

rapcal commented Oct 31, 2024

And it's already on 0.3.6! Thanks, @efriis!

@colriot colriot deleted the colriot/playwright_pydantic_fix branch November 1, 2024 17:15
@colriot
Copy link
Contributor Author

colriot commented Nov 1, 2024

Thank you @efriis ! Sorry about the CI, I wonder why it originally didn't blow up, might be related to some updates in between PR creation and merging it, maybe playwright version... Thanks for fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature community Related to langchain-community lgtm PR looks good. Use to confirm that a PR is ready for merging. size:M This PR changes 30-99 lines, ignoring generated files.
Projects
Archived in project
4 participants