-
Couldn't load subscription status.
- Fork 6.5k
Fix FunctionTool param doc parsing and signature mutation; update tests (#19506) #19532
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
Fix FunctionTool param doc parsing and signature mutation; update tests (#19506) #19532
Conversation
|
PR Ready for Review and Merge
Summary of Changes
Requesting review and merge. Let me know if anything further is needed! |
|
Hi @logan-markewich, |
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! I would just augment the tests as per comment :))
|
|
||
| def test_fn_schema_docstring_descriptions(): | ||
| def sample_tool(a: int, b: Optional[str] = None) -> 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.
Can we test all the docstring styles?
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.
Also can we test with self and context?
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 @AstraBert — makes sense!
I’ll augment the test to cover:
- different docstring styles (e.g. Google-style, )
- cases with self and context parameters
The current test covers a basic :param: style docstring — I’ll now add test variants to handle self, context, and alternate docstring formats. Thanks for the suggestion!
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.
Hi @AstraBert,
Just a quick note regarding the CI status:
Some of the integration test failures appear to be unrelated to the changes in this PR:
llama-index-indices-managed-lancedb fails due to flaky picsum.photos requests (requests.get(...) returns a 525 SSL error).
llama-index-llms-nvidia-tensorrt, colbert-rerank, and colpali-rerank fail due to the unavailability of nvidia-cublas-cu12==12.6.4.1 on NVIDIA’s PyPI index.
All relevant packages and tests directly impacted by this PR are passing.
Let me know if anything else is needed on my side!
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.
it is not a problem if it fails, we can see where the failure is and we can easily ignore when we merge the change, no need to try and fix unrelated failing tests!
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 tests!
Description
This PR improves the behavior of the
FunctionTool.from_defaultsmethod by enhancing how function signatures are parsed and modified. Specifically:FieldInfodefaults during signature extraction to avoid retaining internal metadata.partial_paramsare correctly excluded from the final parameter list using a cleanerkey not in dictapproach.Fixes #19506
New Package?
Did I fill in the
tool.llamahubsection in thepyproject.tomland provide a detailed README.md for my new integration or package?Version Bump?
Did I bump the version in the
pyproject.tomlfile of the package I am updating? (Except for thellama-index-corepackage)Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.
Suggested Checklist:
uv run make format; uv run make lintto appease the lint gods