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

Rebuilt ChatSession class #344

Merged
merged 14 commits into from
Dec 11, 2023
Merged

Conversation

philippjbauer
Copy link
Contributor

  • Saves with serialized ChatHistory of session
  • Only allows use of ChatHistory.Message (instead of raw text) for easy post-processing with IHistoryTransform implementation
  • Provides History Management methods
  • Allows user to regenerate last assistant message

philippjbauer and others added 8 commits November 16, 2023 14:09
- Saves with serialized ChatHistory of session
- Only allows use of ChatHistory.Message (instead of raw text)
   for easy post-processing with IHistoryTransform implementation
- Provides History Management methods
- Allows user to regenerate last assistant message
@martindevans
Copy link
Member

Thanks for re-submitting this based on master! Overall looks good, just that conflict to resolve and one comment/question.

@philippjbauer
Copy link
Contributor Author

Thanks for re-submitting this based on master! Overall looks good, just that conflict to resolve and one comment/question.

I don't have write access to the file to resolve the conflict.

The lines in red need to be removed, the ones in green kept:

Screenshot 2023-12-07 at 8 55 59 AM

@martindevans
Copy link
Member

I don't have write access to the file to resolve the conflict.

I've resolved that for you, please check that I did it right!

@philippjbauer
Copy link
Contributor Author

I don't have write access to the file to resolve the conflict.

I've resolved that for you, please check that I did it right!

LGTM 👍

@martindevans
Copy link
Member

The tests seems to be failing due to a type checking error, looks like something isn't right with that merge?

@AsakusaRinne
Copy link
Collaborator

This format of prompt looks good. Is it a commonly used format in LLM now?

@philippjbauer
Copy link
Contributor Author

The tests seems to be failing due to a type checking error, looks like something isn't right with that merge?

Hmm, I didn't run into these issues before. I thought I had updated everything that depended on the ChatSession class before pushing this PR. I'll have a look tomorrow and fix them up.

@philippjbauer
Copy link
Contributor Author

This format of prompt looks good. Is it a commonly used format in LLM now?

What exactly are you referring to? I haven't touched the default IHistoryTransform implementation so I'm not sure where you're referring to the prompt format.

@AsakusaRinne
Copy link
Collaborator

What exactly are you referring to? I haven't touched the default IHistoryTransform implementation so I'm not sure where you're referring to the prompt format.

I mean the following format in the example prompt:

{
    "messages": [
        {
            "author_role": "System",
            "content": "Transcript of a dialog, where the User interacts with an Assistant named Bob. Bob is helpful, kind, honest, good at writing, and never fails to answer the User's requests immediately and with precision."
        },
        {
            "author_role": "User",
            "content": "Hello, Bob."
        },
        {
            "author_role": "Assistant",
            "content": "Hello. How may I help you today?"
        },
        {
            "author_role": "User",
            "content": "Please tell me the largest city in Europe."
        },
        {
            "author_role": "Assistant",
            "content": "Sure. The largest city in Europe is Istanbul, Turkey."
        }
    ]
}

@philippjbauer
Copy link
Contributor Author

What exactly are you referring to? I haven't touched the default IHistoryTransform implementation so I'm not sure where you're referring to the prompt format.

I mean the following format in the example prompt:

{
    "messages": [
        {
            "author_role": "System",
            "content": "Transcript of a dialog, where the User interacts with an Assistant named Bob. Bob is helpful, kind, honest, good at writing, and never fails to answer the User's requests immediately and with precision."
        },
       { ... }
    ]
}

This is simply a json representation of the chat history. This will be ultimately transformed by the IHistoryTransform.HistoryToText() method into the prompt format that the user specifies for the model they're using. Saving it as json makes the system "prompt format agnostic".

As such the user (developer) is able to introduce multiple implementations of the IHistoryTransform interface for multiple models and easily switch between them. I ran into the problem where I had to use regular expressions to extract the content for each turn from an already formatted chat history (like chat-with-bob.txt).

This also allows us to package specific IHistoryTransform implementations for popular prompt formats, like ChatML, Alpaca, etc. The DefaultHistoryTransform is kinda-sorta working for most, but not everyone is aware that the correct template does make difference for most models.

@philippjbauer
Copy link
Contributor Author

philippjbauer commented Dec 10, 2023

The tests seems to be failing due to a type checking error, looks like something isn't right with that merge?

I synced my master with the latest to get the files that caused issues and updated them to use the new ChatSession integration. The sample for the Chinese character encoding works, but I have a hard time evaluating whether its output is actually correct (I sadly don't know Mandarin). This was a best guess effort on my part.

@AsakusaRinne I saw that you committed the code, are you able to verify this?

@AsakusaRinne
Copy link
Collaborator

@philippjbauer I've added some changes and the Chinese example is generating correct answer now. It's absolutely a good work!

@philippjbauer
Copy link
Contributor Author

@philippjbauer I've added some changes and the Chinese example is generating correct answer now. It's absolutely a good work!

Thank you!

@martindevans
Copy link
Member

Thanks for fixing the merge conflict and the Chinese example. This all looks good to me, ready for merging?

@philippjbauer
Copy link
Contributor Author

Good on my part

@martindevans martindevans merged commit 50c1b2d into SciSharp:master Dec 11, 2023
3 checks passed
@martindevans
Copy link
Member

Thanks for all your hard work on this PR!

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.

3 participants