feat(models): use tool for litellm structured_output when supports_response_schema=false #957
Conversation
dbschmigelski
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
| if len(response.choices) > 1: | ||
| raise ValueError("Multiple choices found in the response.") | ||
| if not response.choices or response.choices[0].finish_reason != "tool_calls": |
There was a problem hiding this comment.
wouldn't this break the logic here? are structured output responses from all models returned as tool_calls?
There was a problem hiding this comment.
According to #483, there are some models that don't return the responses with tool_calls. So the claim the contributor makes is we are currently blocking the models that do not.
But that is something that is currently broken not directly related to this PR.
This PR addresses the case when supports_response_schema = false. #483, is when supports_response_schema=True, but there is no tool_calls
dbschmigelski
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
|
|
||
|
|
||
| def test_structured_output_unsupported_model(model, nested_weather): | ||
| # Mock supports_response_schema to return False to test fallback mechanism |
There was a problem hiding this comment.
Is there a real model we can use so we can avoid mocking?
There was a problem hiding this comment.
We could create a proxy, but I wanted to avoid the case where litellm updates and then our test fails.
There was a problem hiding this comment.
Why do we need a proxy? Can we just provide an api key to litellm, and choose a model that would work with this? Otherwise, this is more of a unit test.
There was a problem hiding this comment.
It's still verifying that the tool extraction works
There was a problem hiding this comment.
The models that we have access to I believe are all statically defined to support structured output
|
|
||
| yield {"output": result} | ||
|
|
||
| async def _structured_output_using_response_schema( |
There was a problem hiding this comment.
Can we add debug logging for the different paths, so we know which is used
|
|
||
|
|
||
| def test_structured_output_unsupported_model(model, nested_weather): | ||
| # Mock supports_response_schema to return False to test fallback mechanism |
There was a problem hiding this comment.
Why do we need a proxy? Can we just provide an api key to litellm, and choose a model that would work with this? Otherwise, this is more of a unit test.
dbschmigelski
changed the title
Description
In LiteLLMModel we currently attempt to perform structured_output using the model's native capabilities. This is checked using the
supports_response_schemamethod.This has two edge cases which are causing customer failures.
The first obvious one is that if a model does not support structured output we are failing hard. This means that if a customer changes the model id, code that previously worked will now fail with
ValueError("Model does not support response_format").The second is what appears to be a bug in LiteLLM.
supports_response_schemadoes not appear to work with proxies. Looking at their code, they are not passing along proxy information, meaning there is no ability to perform a runtime check for proxies.To fix this, when
supports_response_schemareturns false, we now fallback to the similar tool approach used in other model providers. A fix in LiteLLM will be explored but in the immediate term we need to unblock customers.Related Issues
#862
#909
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.