Overloaded MultipleCompletionLLMModel.call type #13
Conversation
…her a list or a single element of LLMResult depending on how many completions are requested
llmclient/llms.py
Outdated
| async def call( | ||
| self, | ||
| messages: list[Message], | ||
| callbacks: list[Callable] | None = None, | ||
| output_type: type[BaseModel] | None = None, | ||
| tools: list[Tool] | None = None, | ||
| tool_choice: Tool | str | None = TOOL_CHOICE_REQUIRED, | ||
| n: Literal[1] = 1, | ||
| **chat_kwargs, | ||
| ) -> LLMResult: ... | ||
|
|
||
| @overload | ||
| async def call( | ||
| self, | ||
| messages: list[Message], | ||
| callbacks: list[Callable] | None = None, | ||
| output_type: type[BaseModel] | None = None, | ||
| tools: list[Tool] | None = None, | ||
| tool_choice: Tool | str | None = TOOL_CHOICE_REQUIRED, | ||
| n: int | None = None, | ||
| **chat_kwargs, | ||
| ) -> list[LLMResult]: ... |
There was a problem hiding this comment.
When would we expect someone to use these overloads instead of the dedicated methods call_single and call_multiple?
IMO, it would be easier to maintain just two methods:
async def call(self, ..., n: int) -> list[LLMResult]:
assert n > 0
...
async def call_single(self, ...) -> LLMResult:
return self.call(..., n=1)[0]
There was a problem hiding this comment.
I like Sid's suggestion too. Also, let's make a docstring somewhere mentioning what n does on the "back end". Readers won't intuitively know what n means, it can refer to so many things.
On a related note, MultipleCompletionLLMModel.achat calls litellm.acompletion. Can we rename MultipleCompletionLLMModel.achat to be MultipleCompletionLLMModel.acompletion to standardize with the actual API endpoint ultimately being invoked?
There was a problem hiding this comment.
It looks like we still have the overloads, call_single, and call_multiple here - can we reduce this to call and call_single?
llmclient/llms.py
Outdated
| async def call( | ||
| self, | ||
| messages: list[Message], | ||
| callbacks: list[Callable] | None = None, | ||
| output_type: type[BaseModel] | None = None, | ||
| tools: list[Tool] | None = None, | ||
| tool_choice: Tool | str | None = TOOL_CHOICE_REQUIRED, | ||
| n: Literal[1] = 1, | ||
| **chat_kwargs, | ||
| ) -> LLMResult: ... | ||
|
|
||
| @overload | ||
| async def call( | ||
| self, | ||
| messages: list[Message], | ||
| callbacks: list[Callable] | None = None, | ||
| output_type: type[BaseModel] | None = None, | ||
| tools: list[Tool] | None = None, | ||
| tool_choice: Tool | str | None = TOOL_CHOICE_REQUIRED, | ||
| n: int | None = None, | ||
| **chat_kwargs, | ||
| ) -> list[LLMResult]: ... |
There was a problem hiding this comment.
I like Sid's suggestion too. Also, let's make a docstring somewhere mentioning what n does on the "back end". Readers won't intuitively know what n means, it can refer to so many things.
On a related note, MultipleCompletionLLMModel.achat calls litellm.acompletion. Can we rename MultipleCompletionLLMModel.achat to be MultipleCompletionLLMModel.acompletion to standardize with the actual API endpoint ultimately being invoked?
llmclient/llms.py
Outdated
| if not n or n <= 0: | ||
| logger.info( | ||
| "Invalid number of completions `n` requested to the call function. " | ||
| "Will get it from the model's configuration." | ||
| ) |
There was a problem hiding this comment.
We should raise an error if n<=0. And I don't think we need to emit a logging message if n is unspecified, since that will be a common case
llmclient/llms.py
Outdated
| async def call( | ||
| self, | ||
| messages: list[Message], | ||
| callbacks: list[Callable] | None = None, | ||
| output_type: type[BaseModel] | None = None, | ||
| tools: list[Tool] | None = None, | ||
| tool_choice: Tool | str | None = TOOL_CHOICE_REQUIRED, | ||
| n: Literal[1] = 1, | ||
| **chat_kwargs, | ||
| ) -> LLMResult: ... | ||
|
|
||
| @overload | ||
| async def call( | ||
| self, | ||
| messages: list[Message], | ||
| callbacks: list[Callable] | None = None, | ||
| output_type: type[BaseModel] | None = None, | ||
| tools: list[Tool] | None = None, | ||
| tool_choice: Tool | str | None = TOOL_CHOICE_REQUIRED, | ||
| n: int | None = None, | ||
| **chat_kwargs, | ||
| ) -> list[LLMResult]: ... |
There was a problem hiding this comment.
It looks like we still have the overloads, call_single, and call_multiple here - can we reduce this to call and call_single?
Messages are not implemented in llmclient anymore
Overloaded typing in
MultipleCompletionLLMModel.call. It returns a list or a single element ofLLMResult, depending on how many completions are requested.This PR is motivated by the fact that
LDPhad a dummy class,LLMModel, which only filtered the return ofMultipleCompletionLLMModel. To make it more general,MultipleCompletionLLMModelnow adapts its call return to be eitherLLMResult(if only one completionnis requested) orlist[LLMResult](ifn>1).The
callmethod was overloaded to satisfy pydantic.