-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Add require_tool param to function calling LLMs #18654
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
Conversation
...ama-index-llms-text-generation-inference/llama_index/llms/text_generation_inference/utils.py
Outdated
Show resolved
Hide resolved
llama-index-integrations/llms/llama-index-llms-ibm/llama_index/llms/ibm/base.py
Outdated
Show resolved
Hide resolved
llama-index-integrations/llms/llama-index-llms-gemini/llama_index/llms/gemini/base.py
Outdated
Show resolved
Hide resolved
b2383fb to
ed6e0b1
Compare
6ec8783 to
3cb762c
Compare
c287054 to
113c1f7
Compare
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.
hmm, I didn't read this quite right. Might want to retain the predefined tool choice thing to maintain behavior. I kind of just want to delete all of the existing tool_choice code. The only real use cases I can think of are "use the tool" or "use either the tool or not." Everything else seems much more intuitive to control via the provided tools list.
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.
That being said, this is a deprecated class with a warning
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.
Yea at this point, it might not be worth debugging lol
The point though is that with structured_predict, there is only one choice, so its trying to force/ensure a tool call
...-index-integrations/llms/llama-index-llms-google-genai/llama_index/llms/google_genai/base.py
Outdated
Show resolved
Hide resolved
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.
Should we raise an error if its set to True?
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.
@logan-markewich My thinking is that this will frequently get set more by internal library code rather than user code. Like, we'll want to migrate StructuredLLM to set tool_required=True (it's currently setting tool_choice="function_name" or something and hoping for the best from the LLM implementation). Seemed better to have it maybe give a tool response rather than blow up, like it currently is
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.
otherwise the alternative is to somehow advertise whether the LLM supports tool_required or not, and checking that before providing it, which seems like a lot of gymnastics for mostly just a few underused LLMs
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.
That's fair!
4dc1f95 to
ca3770d
Compare
|
This release breaks the llama-index-llm-anthropic package. I have had to revert back to release for this to work again 0.12.37 For incase anyone else runs into the same issue. |
|
@rosspurdonubertasconsulting thanks for the report. Rather than rolling back, |
Description
Adds a parameter,
tool_required, that adjust the LLM call to require a tool call if true. This will be useful for things like structured llms, which frequently rely on tool calling. There's a bit of an ad hoctool_choiceparameter that's passed around, but this is inconsistent between APIs. This change implements the llm provider specific param (usually calledtool_choice, but they expect varied values).This intentionally keeps the API simple and small: just require a tool or not. If you don't want a tool, don't pass tools.
New Package?
Not a new package
Version Bump?
Not yet. Should I do that now? The packages llama-index-core minimum version presumably needs to be updated to the latest
Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
I also added some integration style tests, and ran them with API keys. I am a bit concerned about accidentally breaking some of the harder to test integrations. I could back out of them for now
Suggested Checklist:
make format; make lintto appease the lint gods