-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Resolve "$def" for nested tool arg type #8534
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
Resolve "$def" for nested tool arg type #8534
Conversation
8d3de63 to
8813049
Compare
tests/adapters/test_tool.py
Outdated
| async def async_complex_dummy_function( | ||
| profile: UserProfile, priority: int, notes: str | None = None | ||
| ) -> dict[str, Any]: | ||
| async def async_complex_dummy_function(profile: UserProfile, priority: int, notes: str | None = None) -> dict[str, Any]: |
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 it intentional to keep notes here an optional str?
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.
good catch!
| assert tool.args["profile"]["properties"]["contact"]["type"] == "object" | ||
| assert tool.args["profile"]["properties"]["contact"]["properties"]["email"]["type"] == "string" | ||
|
|
||
| # Reference should be resolved for nested pydantic models |
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.
nit: it's totally fine as it is, but since the expected json schema is somehow complex, it might be more readable if we define the entire dict and assert assert tool.args == expected_schema
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.
This is actually intentional, I tried to use the expected_schema, but realized it was too long to be readable. So I moved back to the dedicated fields' check.
TomeHirata
left a comment
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.
Overall LGTM!
Previously when an argument is of type
list[MyPydanticModel], then$defwill be kept inside the json schema. To talk to the LM, we should fully resolve these references.