-
Notifications
You must be signed in to change notification settings - Fork 68
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
Pulling the chat history into LLMChatHistory #244
base: release/v2.3.0
Are you sure you want to change the base?
Pulling the chat history into LLMChatHistory #244
Conversation
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.
Hi, thanks a lot for the PR, you are awesome ⭐ !!
It makes sense to separate the 2 entities, but I feel it complicates a bit the code since the chatHistory before was a simple list.
Could you give me an example of how it could be used?
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.
Thanks for the changes! I have added some further comments
{ | ||
// If no filename has been provided, create one | ||
if (ChatHistoryFilename == string.Empty) { | ||
ChatHistoryFilename = $"chat_{Guid.NewGuid()}"; |
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.
The problem with creating a new filename is that the file won't be usable since it will not be persisted anywhere.
If someones wants to use the chat history, they would need to get and store the ChatHistoryFilename somewhere at runtime.
Is there some way to automatically create a new LLMChatHistory GameObject for a LLMCharacter when added to the scene?
This way we could have similar behavior as before, the use would need to specify the chat filename if they want to autosave
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 in the LLMCharacter InitHistory() we already create and add a chat history component if ther isn't one. I'll just have it set the filename there to match the cache filename.
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.
thanks! is there some case where it is helpful to create a new filename?
I'm thinking that you can't find the file later and it adds space to the disk
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.
Hmm, I guess my thought was that we need to save it somewhere. If the developer doesn't provide us with any name (either from the character or from the ChatHistoryFilename) then we've got to do something. We could just require them to provide a name instead?
{ | ||
// If no filename has been provided, create one | ||
if (ChatHistoryFilename == string.Empty) { | ||
ChatHistoryFilename = $"chat_{Guid.NewGuid()}"; |
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.
thanks! is there some case where it is helpful to create a new filename?
I'm thinking that you can't find the file later and it adds space to the disk
/// <summary> | ||
/// The current chat history | ||
/// </summary> | ||
protected List<ChatMessage> _chatHistory; |
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 have the chat history as public because some people want to manipulate the chat history on their own e.g. remove messages
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.
If we make the _chatHistory public we're kind of breaking the encapsulation that the class gives us. We can add additional methods to make sure devs can update the history as they need but I'd recommend against just making it public.
Added a new component LLMChatHistory, which takes on the responsiblity of managing a list of chat messages. The LLMCharacter now simply calls it's attached LLMChatHistory component. First part of a larger effort to modularize the logic for future extensibility.