Chat with message history (draft) #225
Conversation
leila-messallem
force-pushed
the
chat-history-support
branch
from
0e53580 to
3671e29
Compare
| @@ -18,17 +18,3 @@ repos: | |||
| language: system | |||
| types: [ python ] | |||
| stages: [ commit, push ] | |||
| - id: mypy | |||
There was a problem hiding this comment.
Sorry about this, but I couldn't bother with mypy just for opening a Draft.
There was a problem hiding this comment.
It's a good point actually, maybe we should disable this for draft PRs
| @@ -134,7 +136,10 @@ def search( | |||
| ) | |||
| logger.debug(f"RAG: retriever_result={retriever_result}") | |||
| logger.debug(f"RAG: prompt={prompt}") | |||
| answer = self.llm.invoke(prompt) | |||
| if chat_history is not None: | |||
There was a problem hiding this comment.
I know this part isn't great. The idea I want to demonstrate is to have two different flows for chat with message history and single invokation.
leila-messallem
force-pushed
the
chat-history-support
branch
from
3671e29 to
7c10077
Compare
| ) -> Iterable[ChatCompletionMessageParam]: | ||
| messages = [{"role": "system", "content": self.system_instruction}] | ||
| for i, message in enumerate(chat_history): | ||
| if i % 2 == 0: |
There was a problem hiding this comment.
This assumes that the chat history is a list of strings, alternating between user and assistant messages. It's obviously fragile and should probably be changed to some kind of UserMessage and AssistantMessage objects, or smtn like that.
There was a problem hiding this comment.
Maybe we can use dicts with "role" and "content" keys (it'd work for OpenAI, I don't know about the others)
There was a problem hiding this comment.
I can look into it.
| super().__init__(model_name, model_params) | ||
| self.model = GenerativeModel(model_name=model_name, **kwargs) | ||
| super().__init__(model_name, model_params, system_instruction) | ||
| self.model = GenerativeModel( |
There was a problem hiding this comment.
On vertexai, system_instruction is set on the GenerativeModel object. So it's separated from the question prompt.
There was a problem hiding this comment.
This is quite bad news actually, that means that we can use an LLM instance only in one specific use case? For instance, with RAG with a text2Cypher retriever, we can't use the same LLM for the retrieval and the generation? (I mean, if we want to use the system prompt, which is not what we do at the moment)
There was a problem hiding this comment.
I guess we don't have to always use the system_instruction. Although, it's the recommended way.
|
Something that is currently missing is to include the chat history (or at least some of it) in the embeddings calculation. |
| @@ -52,6 +54,21 @@ def invoke(self, input: str) -> LLMResponse: | |||
| LLMGenerationError: If anything goes wrong. | |||
| """ | |||
|
|
|||
| @abstractmethod | |||
| def chat(self, input: str, chat_history: list[str]) -> LLMResponse: | |||
There was a problem hiding this comment.
I feel like it could be done in one method, with an optional chat_history. The difference would be dealt with in a get_messages method, that would do something like this:
def get_messages(self, input, chat_history) -> list:
messages = []
if self.system_instructions:
messages.append({"role": "system", "content": self.system_instructions}})
if chat_history:
# handle chat history messages format
messages.extend(chat_history)
messages.append({"role": "user", "content": input})
return messages
This is an advantage if most of the LLM providers share the same format for messages, in which case we can implement this method only once in the interface.
There was a problem hiding this comment.
Ok, that works.
Description
Suggestion for adding chat functionality with message history.
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):