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

Introduce ChatHistory interface #669

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

StevenCyb
Copy link

@StevenCyb StevenCyb commented Apr 15, 2024

This PR changed the way ChatHistory is used by abstracting it with an interface IChatHistory.
This PR has breaking changes.

Examples teste with the changes.
Unit tests:

Passed!  - Failed:     0, Passed:    85, Skipped:     3, Total:    88, Duration: 8 s - LLama.Unittest.dll (net8.0)

CHANGES:

  • Chat Message is not an inner class of ChatHistory anymore
  • IChatHistory is now introduces to allow own chat history implementations
  • Chat history serialization is now handled by ChatHistorySerializer

@AsakusaRinne
Copy link
Collaborator

Hi, the overall of this PR looks good. Could you please tell about why you want to make this change?

@StevenCyb
Copy link
Author

Hey @AsakusaRinne ,
In my opinion, such components should be equipped with interfaces as far as possible to enable customisations. By using an interface in the ChatHistory, it is easier to use your own classes (which implement the interface) without recreating other common components.
For example, in addition to the "simple" ChatHistory, a Buffer Window or Summary approach could be implemented for a more suited chat history.

@AsakusaRinne
Copy link
Collaborator

Hi Steven, thank you very much for this PR! However I prefer to blocking its merging for now because it introduces break changes, while I think we'll soon add some changes to the high-level APIs. Thus your modifications are very likely to be changed again in the near future, making users a bit confused. You don't need to close this PR -- that will remind me about your requested APIs and take your proposal into account when I work on these APIs in the future. Your proposal this PR is reasonable to me. :)

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