-
Notifications
You must be signed in to change notification settings - Fork 22
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
Load message history to core chat from schema chat #429
Conversation
Co-authored-by: Philip Meier <[email protected]>
Co-authored-by: Philip Meier <[email protected]>
ragna/deploy/_api/schemas.py
Outdated
def to_core(self) -> ragna.core.Message: | ||
return ragna.core.Message( | ||
content=self.content, | ||
role=self.role, | ||
sources=[], | ||
) |
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.
@pmeier Tell me if this makes sense:
For the purpose of chat history, we don't need Sources for each individual message since they already exist on the chat object. So we don't need to rebuild them here (?)
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.
since they already exist on the chat object
They don't. Source
's are returned by the SourceStorage
and are never saved on the Chat
but only ever on the Message
Lines 221 to 227 in 8ea62a3
sources = await self._run(self.source_storage.retrieve, self.documents, prompt) | |
answer = Message( | |
content=self._run_gen(self.assistant.answer, prompt, sources), | |
role=MessageRole.ASSISTANT, | |
sources=sources, | |
) |
If you have a look at the ORM model for the chat, we also have no connection to the sources table, but only the messages
ragna/ragna/deploy/_api/orm.py
Lines 75 to 92 in 8ea62a3
class Chat(Base): | |
__tablename__ = "chats" | |
id = Column(types.Uuid, primary_key=True) # type: ignore[attr-defined] | |
user_id = Column(ForeignKey("users.id")) | |
name = Column(types.String, nullable=False) | |
documents = relationship( | |
"Document", | |
secondary=document_chat_association_table, | |
back_populates="chats", | |
) | |
source_storage = Column(types.String, nullable=False) | |
assistant = Column(types.String, nullable=False) | |
params = Column(Json, nullable=False) | |
messages = relationship( | |
"Message", cascade="all, delete", order_by="Message.timestamp" | |
) | |
prepared = Column(types.Boolean, nullable=False) |
The messages table however is connected to the sources table
ragna/ragna/deploy/_api/orm.py
Lines 95 to 100 in 8ea62a3
source_message_association_table = Table( | |
"source_message_association_table", | |
Base.metadata, | |
Column("source_id", ForeignKey("sources.id"), primary_key=True), | |
Column("message_id", ForeignKey("messages.id"), primary_key=True), | |
) |
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.
LGTM after 8a984a6 is reverted as explained in #429 (comment).
This reverts commit 8a984a6.
@pmeier Mypy points out that you can't construct a
I'm leaning toward option 1 unless there's a simpler way. |
Ah yeah, I'm hitting the same on a another branch as well. I agree we should go with option 1 especially in the light of #256. No need to duplicate work here. Just set |
Co-authored-by: Philip Meier <[email protected]>
Basic prereq for passing chat history down the pipeline. History is already present in the
schema.Chat
object loaded from the database, so we just need to make sure it also gets loaded into theragna.core.Chat
object inside schema_to_core_chat.ragna/ragna/deploy/_api/core.py
Lines 290 to 297 in 647a61e