-
Notifications
You must be signed in to change notification settings - Fork 107
feat: Add default parameter values support for tool definitions #90 #150
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
feat: Add default parameter values support for tool definitions #90 #150
Conversation
- Add parameter_defaults field to ToolFunctionDef and ToolFunctionDefDict - Implement automatic default extraction from function signatures using inspect - Add manual default specification support - Update JSON schema generation to include default values - Add comprehensive test coverage for default parameter values - Maintain backward compatibility with existing tool definitions Closes lmstudio-ai#90
|
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
|
I have read the CLA Document and I hereby sign the CLA |
ncoghlan
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.
Thank you for this!
I think we should go for an inline default format using the same style as Zod in TypeScript, as well as JSON schema itself, allowing the type fields to contain {"type": some_type, "default": some_value} instead of simply the type annotation. At the same time, we can tighten up the type hinting to make it clear that parameter type declarations need to be valid type hints, not entirely arbitrary objects.
The rest of the comments are largely just a first pass at exploring the implications of that design clarification (I haven't tested them, or even type checked them, but they're at least a step in the desired direction)
One potential simplification is that once the defaults are being correctly populated, msgspec should take care of populating the generated JSON schema appropriately.
…e parameter_defaults with inline format - Use generic TypedDict with NotRequired for better type safety - Remove manual default value injection, rely on msgspec auto-handling - Update all tests to use inline format instead of separate mapping
32ed2f8 to
c5cee65
Compare
… with inline format
ncoghlan
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.
Thanks for the update!
There are a few minor cleanups still needed (details inline), but I'll go ahead and make those myself before merging.
i see some test fail , i will figure out |
|
Oh, that's annoying (Python 3.10): It's due to python/cpython#89026 (the limitation was resolved in Python 3.11). I'll just put a version guard on the |
75b1f4c to
1f1c225
Compare
|
Thank you for the PR! This will be released as part of lmstudio-python 1.5.0. |
Closes #90