community[patch]: Fix Playwright Tools bug with Pydantic schemas #27050
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
dosubot
bot
added
size:M
|
|
||
| class BaseBrowserTool(BaseTool): | ||
| """Base class for browser tools.""" | ||
|
|
||
| args_schema: Type[BaseModel] = BaseBrowserToolInput |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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
BaseBrowserToolInputtoNoArgsToolInput(or smth along the lines) and have it used as default schema value inBaseBrowserTool - Rename input, but remove default value from
BaseBrowserTooland 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
There was a problem hiding this comment.
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?
|
Can merge if we can package the schema for the tool inputs together with the appropriate tools |
|
Taking a look at rolling out the fix right now |
eyurtsev
changed the title
dosubot
bot
added
the
lgtm
Head branch was pushed to by a user without write access
|
Left a comment in the above discussion for a final decision, @eyurtsev |
Test Plan:
poetry run pytest tests/unit_tests/tools/playwright/test_all.pyFixes #26758