Skip to content
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

Open
wants to merge 7 commits into
base: release/v2.3.0
Choose a base branch
from

Conversation

nabrown737
Copy link

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.

@amakropoulos amakropoulos changed the base branch from main to v2.3.0 September 17, 2024 13:29
Copy link
Collaborator

@amakropoulos amakropoulos left a 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?

Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMChatHistory.cs Show resolved Hide resolved
Samples~/SimpleInteraction/SimpleInteraction.cs Outdated Show resolved Hide resolved
Tests/Runtime/TestLLMChatHistory.cs Show resolved Hide resolved
Runtime/LLMChatHistory.cs Show resolved Hide resolved
Runtime/LLMCharacter.cs Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Samples~/SimpleInteraction/SimpleInteraction.cs Outdated Show resolved Hide resolved
Tests/Runtime/TestLLM.cs Show resolved Hide resolved
Runtime/LLMCharacter.cs Show resolved Hide resolved
Runtime/LLMChatHistory.cs Show resolved Hide resolved
Copy link
Collaborator

@amakropoulos amakropoulos left a 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

Runtime/LLMChatHistory.cs Show resolved Hide resolved
{
// If no filename has been provided, create one
if (ChatHistoryFilename == string.Empty) {
ChatHistoryFilename = $"chat_{Guid.NewGuid()}";
Copy link
Collaborator

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

Copy link
Author

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.

Copy link
Collaborator

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

Copy link
Author

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?

Runtime/LLMCharacter.cs Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Show resolved Hide resolved
Runtime/LLMCharacter.cs Show resolved Hide resolved
Runtime/LLMChatHistory.cs Show resolved Hide resolved
{
// If no filename has been provided, create one
if (ChatHistoryFilename == string.Empty) {
ChatHistoryFilename = $"chat_{Guid.NewGuid()}";
Copy link
Collaborator

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;
Copy link
Collaborator

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

Copy link
Author

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.

Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Runtime/LLMCharacter.cs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants