-
Notifications
You must be signed in to change notification settings - Fork 69
fixed snake_case migration #13
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
fixed snake_case migration #13
Conversation
BB-955 Fix snake_case migration on Python
Requests are now snake case, but this is causing an issue in the body for the API request where it expects camelCase. |
..., description="Instruction specifying what data to extract using AI." | ||
) | ||
model_name: Optional[AvailableModel] = Field(None, alias="modelName") | ||
model_name: Optional[AvailableModel] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the model name really optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes there's a default in config
"""Serialize schema_definition to a JSON schema if it's a Pydantic model""" | ||
if isinstance(schema_definition, type) and issubclass(schema_definition, BaseModel): | ||
return schema_definition.model_json_schema() | ||
return schema_definition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have a test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smoke tested locally w/ example.py:
class News(BaseModel):
description: str
url: str
...
data = await page.extract(ExtractOptions(
instruction="extract the first result from the search",
schema_definition=News,
))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, confirming it works for me as well!
LGTM - just need to test the schema serializer for extract and confirm it works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, tested locally with the example as well!
feel free to merge this :) |
No description provided.