-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Fix: Exclude tool params for models without function calling support (#21125) #21244
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,8 +96,31 @@ def get_complete_url( | |
| return api_base | ||
|
|
||
| def get_supported_openai_params(self, model: str) -> list: | ||
| """Get supported OpenAI params from base class""" | ||
| return super().get_supported_openai_params(model=model) | ||
| """Get supported OpenAI params, excluding tool-related params for models | ||
| that don't support function calling.""" | ||
| from litellm._logging import verbose_logger | ||
| from litellm.utils import supports_function_calling | ||
|
|
||
| supported_params = super().get_supported_openai_params(model=model) | ||
|
|
||
| try: | ||
| _supports_fc = supports_function_calling( | ||
| model=model, custom_llm_provider=provider.slug | ||
| ) | ||
| except Exception: | ||
| _supports_fc = False | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant try/except — The underlying If you want to keep defensive coding here for safety, that's fine — but be aware that the Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right |
||
|
|
||
| if not _supports_fc: | ||
| tool_params = ["tools", "tool_choice", "function_call", "functions", "parallel_tool_calls"] | ||
| for param in tool_params: | ||
| if param in supported_params: | ||
| supported_params.remove(param) | ||
| verbose_logger.debug( | ||
| f"Model {model} on provider {provider.slug} does not support " | ||
| f"function calling — removed tool-related params from supported params." | ||
| ) | ||
|
|
||
| return supported_params | ||
|
|
||
| def map_openai_params( | ||
| self, | ||
|
|
||
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.
Inline imports should be at module level
Per
CLAUDE.md, imports should be placed at the top of the file rather than inside methods.verbose_loggeris already imported at the top of the siblingjson_loader.pyfile, confirming it's safe at module level. Thesupports_function_callingimport fromlitellm.utilsis used inline in a few other provider files (bedrock, sambanova) — likely to avoid circular imports — so that one may be justified. Consider moving at leastverbose_loggerto the top-level imports.Context Used: Context from
dashboard- CLAUDE.md (source)Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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.
Good catch! Moved
verbose_loggerto the top-level imports. Keptsupports_function_callingas an inline import to avoid circular imports (same pattern used in other provider files like bedrock and sambanova). Fixed in 83f4d30.