Add tool calling to the LLM base class, implement in OpenAI #322
Conversation
oskarhane
force-pushed
the
feature/llm-tool-invocation
branch
from
cd47dc2 to
882915f
Compare
oskarhane
force-pushed
the
feature/llm-tool-invocation
branch
from
fc2f734 to
7f9bb39
Compare
| **params, | ||
| ) | ||
|
|
||
| message = response.choices[0].message |
There was a problem hiding this comment.
Not for now because this is also the case in the already existing methods, but we should probably try to reduce code duplication between the sync and async calls.
oskarhane
force-pushed
the
feature/llm-tool-invocation
branch
from
913c8be to
b769242
Compare
| if not response.tool_calls: | ||
| raise ValueError("No tool calls found in response") | ||
|
|
||
| tool_call = response.tool_calls[0] |
There was a problem hiding this comment.
Is there a reason to print only the first tool?
There was a problem hiding this comment.
No, def not. Fixed in latest push.
tests/unit/llm/test_openai_llm.py
Outdated
| model="gpt", | ||
| ) | ||
| # Use assert_called_once() instead of assert_called_once_with() to avoid issues with overloaded functions | ||
| cast(Any, llm.client.chat.completions.create).assert_called_once() |
There was a problem hiding this comment.
I thought these cast(Any...) were fixed?
There was a problem hiding this comment.
Yeah, me too. Removed and added mypy ignores in test file
oskarhane
force-pushed
the
feature/llm-tool-invocation
branch
2 times, most recently
from
59bbb6a to
9ae73cb
Compare
| return {"type": ParameterType.BOOLEAN, "description": self.description} | ||
|
|
||
|
|
||
| class ObjectParameter(ToolParameter): |
There was a problem hiding this comment.
Nice! It's maybe a bit verbose, would be great if users could provide tool definition as dict, wdyt? I think converting the *Parameter to Pydantic model would make this easily possible (and we could also get rid of the to_dict methods).
There was a problem hiding this comment.
Pushed this change now
To not rely on json schema from openai
oskarhane
force-pushed
the
feature/llm-tool-invocation
branch
from
107d670 to
61a0e46
Compare
|
|
||
| def model_dump_tool(self) -> Dict[str, Any]: | ||
| """Convert the parameter to a dictionary format for tool usage.""" | ||
| result = {"type": self.type, "description": self.description} |
There was a problem hiding this comment.
We do not need required here?
| minimum: Optional[int] = None | ||
| maximum: Optional[int] = None | ||
|
|
||
| def model_dump_tool(self) -> Dict[str, Any]: |
There was a problem hiding this comment.
Is model_dump_tool only used to discard null values? If so, there is an option in model_dump for this (exclude_none=True by memory)
There was a problem hiding this comment.
I know I'm a bit nitpicking here, but I've started to work on the implementation for Vertex AI, and some parameters need to be excluded, so if we can rely on Pydantic for this instead of manually re-implementing it, that would be great :) but if it's not possible, it's not possible.
| raise ValueError("Parameter type is required") | ||
|
|
||
| # Find the appropriate class based on the type | ||
| param_classes = { |
There was a problem hiding this comment.
I feel like we could get rid of many of these by using either a field discriminator or some union, but I won't be strict on this point, experiments can be left for a cool down period.
Description
Type of Change
Complexity
Complexity: Medium
How Has This Been Tested?
Checklist
The following requirements should have been met (depending on the changes in the branch):