Skip to content

Conversation

@chenmoneygithub
Copy link
Collaborator

Previously when an argument is of type list[MyPydanticModel], then $def will be kept inside the json schema. To talk to the LM, we should fully resolve these references.

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]:
Copy link
Collaborator

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?

Copy link
Collaborator Author

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
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

@TomeHirata TomeHirata left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

@chenmoneygithub chenmoneygithub merged commit 0d0af3c into stanfordnlp:main Jul 16, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants