-
Notifications
You must be signed in to change notification settings - Fork 105
Add Litellm API integration #273
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
- Implement LiteLLMModel and LiteLLMAPIMessageBuilder for LiteLLM API interaction. Enhance APIPayload to include reasoning_effort parameter. - Introduce get_pricing_litellm function for pricing information retrieval. - Create tests for multi-action, single tool call scenarios and force tool call. - Added an option to discard extra tool calls by setting use_only_first_toolcall.
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Unexplained type ignore ▹ view | ||
| Inefficient Message List Building ▹ view | ||
| Hardcoded Configuration Parameter ▹ view | ||
| Unused Reasoning Effort Parameter ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| src/agentlab/llm/tracking.py | ✅ |
| src/agentlab/llm/litellm_api.py | ✅ |
| src/agentlab/llm/response_api.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| if tool_calls is None: | ||
| return None | ||
| tool_call_list = [] | ||
| for tc in tool_calls: # type: ignore |
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.
Unexplained type ignore 
Tell me more
What is the issue?
Type ignore comment used without explanation of why type checking needs to be suppressed
Why this matters
Unexplained type ignores make it difficult to understand the code's type safety assumptions and maintainability
Suggested change ∙ Feature Preview
Add a comment explaining why the type ignore is necessary:
# type: ignore # tool_calls is dynamically typed from API response
for tc in tool_calls:Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| for msg in payload.messages: # type: ignore | ||
| input.extend(msg.prepare_message()) |
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.
Inefficient Message List Building 
Tell me more
What is the issue?
Messages are being processed one at a time with extend() calls, which can be inefficient for large message histories.
Why this matters
Multiple extend operations on a list cause repeated memory allocations and copies. This becomes more significant with larger message histories.
Suggested change ∙ Feature Preview
Use a list comprehension to build the input list in one operation:
input = [msg for messages in payload.messages for msg in messages.prepare_message()] # type: ignoreProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| temperature=temperature, | ||
| max_tokens=max_tokens, | ||
| ) | ||
| self.action_space_as_tools = True # this should be a config |
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.
Hardcoded Configuration Parameter 
Tell me more
What is the issue?
The action_space_as_tools parameter is hardcoded to True but commented as 'should be a config', without being included in the initialization parameters.
Why this matters
Users cannot configure this setting, forcing them to use tools-based action space even if they need text-based actions.
Suggested change ∙ Feature Preview
Add the parameter to the constructor:
def __init__(self, model_name: str, base_url: Optional[str] = None, api_key: Optional[str] = None, temperature: float | None = None, max_tokens: int | None = 100, use_only_first_toolcall: bool = False, action_space_as_tools: bool = True):
...
self.action_space_as_tools = action_space_as_toolsProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| cache_complete_prompt: bool = ( | ||
| False # If True, will cache the complete prompt in the last message. | ||
| ) | ||
| reasoning_effort: Literal["low", "medium", "high"] | None = None |
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.
Unused Reasoning Effort Parameter 
Tell me more
What is the issue?
The 'reasoning_effort' parameter is introduced but not utilized in any of the response models (OpenAI, Claude, or OpenAIChatCompletion).
Why this matters
Adding unused parameters creates confusion and technical debt. The code suggests this parameter is only for LiteLLM API, but the implementation is missing.
Suggested change ∙ Feature Preview
Either remove the parameter until LiteLLM integration is implemented, or implement it in the _call_api methods. For example:
def _call_api(self, payload: APIPayload) -> Any:
api_params = {...}
if payload.reasoning_effort is not None:
api_params['reasoning_effort'] = payload.reasoning_effort
# rest of the codeProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
use_only_first_toolcall.LiteLLM can be used with tool use agent as follows:
Description by Korbit AI
What change is being made?
Integrate LiteLLM API into the existing codebase, allowing the use of LiteLLM for chat completion and tool calls, and add tests for its functionalities.
Why are these changes being made?
The integration adds new API options for leveraging LiteLLM's capabilities in generating chat completion and executing tool actions, providing more flexibility and scalability when managing interactions. Tests are included to validate that the new functionality operates as expected, covering scenarios such as multiple tool calls, single tool calls, and forced tool calls. The change also includes tracking LiteLLM pricing, crucial for budgeting and analysis.