Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
support chat history #438
support chat history #438
Changes from 2 commits
d81dc97
1be9013
1e2b626
213bccd
6e3caef
212b94e
772ee4f
1ed2349
6310533
57f5949
08e49ab
2f192be
f26616c
4cfc198
97f9eba
0fb10d3
d7fbccb
36beb72
dee268b
65d5ef3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Example of what this could look like for OpenAI-like assistants. There could also be an attribute on the Assistant that tells this function how to truncate the message list based on the size of the target LLM's context window.
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.
Depending on what order we try to merge PRs - this piece will eventually be overwritten by the notion of the preprocess stage. It's probably fine for this version imo.
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.
It feels easiest to include the basic system prompt here, but maybe it should be regenerated and prepended to the list of messages for every call to the LLM and not stored here. This would make the most sense if the Assistant might truncate the message list and chop off the system prompt.
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.
I don't understand why the chat would need the system prompt of the assistant. Instead of creating it here and pass it to the Assistant as part of the messages, why not let the Assistant create it in the first place?
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.
I think I agree. I also mostly talked myself out of putting it here as well. I think I wanted
_messages
to be a complete historical list of messages that could be sent to the LLM, but system prompts can stay hard coded on the Assistant (or better yet, in a config somewhere?)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.
Just be sure there's no attempt at a 'universal' system prompt anywhere - on the assistant we'll have a general one for .answer(), but e.g. we'll want different system prompts for things like all the pre-process steps or anything that's outside the standard chain. Simply to say, it's reasonable to have a standard system prompt on .answer() but we don't want it fixed on the general llm calls in .generate() (wip).
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.
Until this, we only ever attached the sources to the answer and not the question. I'm open to a "philosophical debate" on where they should go, but I don't want them on the question and the answer. Why not just keep passing the sources to
Assistant.answer
?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.
This was something I meant to ask about in my original list of questions last night but I ran out of steam. I'm really curious about this from a design perspective. Let's imagine a design where
chat._messages
is a list ofMessage
objects, as currently implemented. Now also say the assistant is responsible for formatting each raw message for the LLM API call. I can imagine at least a couple scenarios for how we would format historical messages:.answer
call. So if we want to split up a prompt/sources pair into two separate json objects (i.e.{'content': '<user question>', 'role': 'user'}
+{'content': 'please use only the following sources: ...', 'role': 'system'}
we either need the sources to be attached to the question or we need to reach forward in the messages list to grab the sources from the answer. Something feels a little strange about the latter, but I agree that sources shouldn't be on both the question and the answer, just one or the other.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.
Recall, we eventually want ['chat history' messages]+prompt->preprocess->retrieval so the messages coming in the history before the current prompt are(were) all strings to the LLM, but we may want to be able to act upon them with the distinction between content and source - e.g. if messages from the user are tracked at a 'content' + sources level, then it might be easier to process the sources down in later stages - e.g. you may not want to carry content+N-chunks forward from every user prompt.
Keeping the sources attached to prompts would be particularly useful in preprocess tracking where a given user-role message may be (content+sources) and you want to streamline the context there (parse out/summarize/select sources).
Just keep in mind that 'previous' messages make sense to have the sources attached, while the 'current' prompt we'll still be sending through processing and retrieval.
If I'm thinking about a pre-process stage class, receiving
messages = [{system,...}, {user,.. content, sources}, {assistant,.. answer}...]
makes more sense than the source being with the answer - the 'content' in all previous user messages would contain the sources in that string as well (minus current prompt, which would not yet have 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.
So you're thinking of the stage of the pipeline where we have the most recent user prompt but have not yet retrieved any sources from source storage, is that right? Would you want
.content
to contain string-formatted sources from previous questions, or would it be more flexible to still have them asSource
objects?