[WIP] Add Chat endpoint

[megalon]

This adds the chat/completions endpoint, as specified by the OpenAI API docs.
I copied the style of the completions and embeds endpoints, so it should fit in with the project's style.

I've tested the async functions in my own project and it seems to work just fine.

Potential issues

I do not understand what the logit_bias is in the ChatRequest.
In the docs it is listed as a "map" and:

Accepts a json object that maps tokens (specified by their token ID in the tokenizer) to an associated bias value from -100 to 100.
Mathematically, the bias is added to the logits generated by the model prior to sampling.
The exact effect will vary per model, but values between -1 and 1 should decrease or increase likelihood of selection; values like -100 or 100 should result in a ban or exclusive selection of the relevant token.

It does not specify what data types to use for this "map", so I interpreted this as a Dictionary<string, float> where the key is the ID and the float value is the bias.
I am not sure if this is correct.

I have also not tested any of the Streaming functions.

TODO:

• Write tests. I am not familiar with the test system this project uses.
• Update the README

Closes #54

[gotmike]

good stuff. i was working on this in parallel. i'll take a look, i already wrote some simple tests. so i may be able to use them with yours if you followed the same format as the prior code.

[gotmike]

@megalon -- can you submit this as a PR to a new branch pls? it will make it easier for me to do code review and comments/edits.

[megalon]

@megalon -- can you submit this as a PR to a new branch pls? it will make it easier for me to do code review and comments/edits.

This is already on a separate branch named "feature/chat" on my repo. Is that what you mean?

[hughperkins]

Looks like ToString() is not compatible with Completions endpoint? Does not return the text itself:

[hughperkins]

(Nice work by the way :) )

[hughperkins]

streaming seems not to be working? For example, given the following code (and a file Auth.cs, containing open api key),

using System;
using System.Diagnostics;
using OpenAI_API;

public class TestOpenAPIChat
{
    public static async Task Main()
    {
        Console.WriteLine("TestOpenAPIChat");

        OpenAIAPI api = new OpenAIAPI(Auth.openai_api_key);
        Console.WriteLine("1");
        List<OpenAI_API.Chat.ChatMessage> messages = new List<OpenAI_API.Chat.ChatMessage>();
        Console.WriteLine("1");
        //messages.Add(new OpenAI_API.Chat.ChatMessage("system", "Please write a story of 100 words"));
        messages.Add(new OpenAI_API.Chat.ChatMessage("user", "Please write a story of 100 words"));
        Console.WriteLine("1");
        OpenAI_API.Chat.ChatRequest request = new OpenAI_API.Chat.ChatRequest
        {
            Messages = messages,
            Model = "gpt-3.5-turbo",
            Temperature = 0.7f,
            MaxTokens = 256
        };
        Console.WriteLine("1");
        await foreach (var res in api.Chat.StreamChatEnumerableAsync(request))
        {
            if (res.Choices.Length > 0)
            {
                Console.WriteLine(res.Choices[0].Message.Content);
            }
        }
    }
}
``` ... then the output is: 

(10-gptnpc) Hughs-MacBook-Air:cs_prot hugh$ bin/Debug/net7.0/cs_prot
TestOpenAPIChat
1
1
1
1
Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
at TestOpenAPIChat.Main() in /Users/hugh/git/unity-priv/cs_prot/cs_prot/cs_prot/TestOpenAPIChat.cs:line 37
at TestOpenAPIChat.Main() in /Users/hugh/git/unity-priv/cs_prot/cs_prot/cs_prot/TestOpenAPIChat.cs:line 26
at TestOpenAPIChat.

()
Abort trap: 6

[hughperkins]

I feel like the streaming responses from openai, e.g.

{"id":"chatcmpl-6qKFn","object":"chat.completion.chunk","created":1677928931,"model":"gpt-3.5-turbo-0301","choices":[{"delta":{"role":"assistant"},"index":0,"finish_reason":null}]}

don't match the structure of ChatResult ChatChoice class?

    /// <summary>
    /// A message received from the API, including the message text, index, and reason why the message finished.
    /// </summary>
    public class ChatChoice
    {
        /// <summary>
        /// The index of the choice in the list of choices
        /// </summary>
        [JsonProperty("index")]
        public int Index { get; set; }

        /// <summary>
        /// The message that was presented to the user as the choice
        /// </summary>
        [JsonProperty("message")]
        public ChatMessage Message { get; set; }

        /// <summary>
        /// The reason why the chat interaction ended after this choice was presented to the user
        /// </summary>
        [JsonProperty("finish_reason")]
        public string FinishReason { get; set; }
    }

[hughperkins]

"choices":[{"delta":{"content":" transported"},"index":0,"finish_reason":null}]

[hughperkins]

change ChatChoice, and adding ChatChoiceDelta as follows fixes this issue:

    /// <summary>
    /// A message received from the API, including the message text, index, and reason why the message finished.
    /// </summary>
    public class ChatChoice
    {
        /// <summary>
        /// The index of the choice in the list of choices
        /// </summary>
        [JsonProperty("index")]
        public int Index { get; set; }

        /// <summary>
        /// The message that was presented to the user as the choice
        /// </summary>
        [JsonProperty("message")]
        public ChatMessage Message { get; set; }

        /// <summary>
        /// The reason why the chat interaction ended after this choice was presented to the user
        /// </summary>
        [JsonProperty("finish_reason")]
        public string FinishReason { get; set; }

        public ChatChoiceDelta delta;
    }

    public class ChatChoiceDelta
    {
        public string role;
        public string content;
    }

[hughperkins]

megalon#1

[haacked]

Hi there! I hope you don't mind some drive-by feedback from a user of the library. 😄

[haacked]

Same point as above: I'd recommend making this an IReadOnlyDictionary<string, float> which supports more ways of calling this.

[megalon]

Should this be an IReadOnlyDictionary or a IDictionary?

Since this is in the outgoing request, logitBias is something the user has created themselves.
Shouldn't it be a regular dictionary since the user is the one making it?

I noted it in the PR, but I do not actually understand what logit_bias is used for, so I am not sure about it's implementation here.

[haacked]

It's a good question. The TL;DR is by taking the least derived type, an IReadOnlyDictionary<> in this case, you make the intention of your method clearer and it's more broadly callable. The caller doesn't need to create a specific Dictionary<>, they can pass any type that implements the interface (which includes Dictionary<>, so you lose nothing)

As the Framework Design Guidelines ponit out, method arguments should be the least derived type needed by the method. This makes it more broadly callable. For example, the caller of the method might not have a Dictionary<> handy. They might have something that implements the interface though.

IReadOnlyDictionary<string, float> biases = GetBiases(...);

Now then they want to pass the biases into your method, they should be able to without having to call ToDictionary().

Since this is in the outgoing request

I'd argue that ChatRequest.LogitBias (the outgoing request) could also be an IReadOnlyDictionary<string, float> (or IReadOnlyDictionary<string, int>, but it's not clear from the documentation). That will get serialized properly.

The only time your method needs to take in a Dictionary<> is if it's supposed to modify the passed in dictionary, which should be rare. And if your method. modifies the dictionary, it should be very clear to the caller that's what it does.

As a caller of an API, I'd be worried about passing in a concrete Dictionary<> to a method because maybe the method modifies it and I don't realize it, which could lead to bugs in my code. If I saw a method argument of Dictionary<>, I'd probably create a copy of my dictionary first via ToDictionary() to be safe so that anything your method does won't affect my dictionary.

By making the argument IReadOnlyDictionary<>, you let the caller know that you don't intend to modify the dictionary, but only read its values. Advertising your intentions is good API design. 😄

[haacked]

I'd argue that ChatRequest.LogitBias (the outgoing request) could also be an IReadOnlyDictionary<string, float> (or IReadOnlyDictionary<string, int>, but it's not clear from the documentation). That will get serialized properly.

I should be clear, if the ChatRequest object is using a builder pattern, then it's fine if the property is Dictionary<>. But if it's meant to be created all at once, it could be a read only dictionary. This one is not that important to me. My focus is more on the method arguments.

[megalon]

As a caller of an API, I'd be worried about passing in a concrete Dictionary<> to a method because maybe the method modifies it and I don't realize it, which could lead to bugs in my code. If I saw a method argument of Dictionary<>, I'd probably create a copy of my dictionary first via ToDictionary() to be safe so that anything your method does won't affect my dictionary.

By making the argument IReadOnlyDictionary<>, you let the caller know that you don't intend to modify the dictionary, but only read its values. Advertising your intentions is good API design. 😄

That makes sense to me!

I'd argue that ChatRequest.LogitBias (the outgoing request) could also be an IReadOnlyDictionary<string, float> (or IReadOnlyDictionary<string, int>, but it's not clear from the documentation). That will get serialized properly.

Yeah, it's not clear to me from the documentation either. I might have to look at some other implementations and see what they do. I was hoping someone else would chime in here with the correct answer eventually.

[megalon]

Hi there! I hope you don't mind some drive-by feedback from a user of the library. 😄

Hey, thanks for the review! All of those changes sound good to me.
I will test them out when I get the chance and update the PR

[megalon]

change ChatChoice, and adding ChatChoiceDelta as follows fixes this issue:

    /// <summary>
    /// A message received from the API, including the message text, index, and reason why the message finished.
    /// </summary>
    public class ChatChoice
    {
        /// <summary>
        /// The index of the choice in the list of choices
        /// </summary>
        [JsonProperty("index")]
        public int Index { get; set; }

        /// <summary>
        /// The message that was presented to the user as the choice
        /// </summary>
        [JsonProperty("message")]
        public ChatMessage Message { get; set; }

        /// <summary>
        /// The reason why the chat interaction ended after this choice was presented to the user
        /// </summary>
        [JsonProperty("finish_reason")]
        public string FinishReason { get; set; }

        public ChatChoiceDelta delta;
    }

    public class ChatChoiceDelta
    {
        public string role;
        public string content;
    }

It looks like you're right about this. In the Chat docs "deltas" are just briefly noted in the "stream" description.

I missed it because it's not in their example response!

It looks like instead of using a new class, I could just use the ChatMessage, since it has the same properties.

[hughperkins]

It looks like instead of using a new class, I could just use the ChatMessage, since it has the same properties.

Oh great! Oh right, good point :D

[megalon]

For anyone updating this from my original PR, ChatResult.Choices is an IReadOnlyList now, instead of an Array
You may need to change the way you are interacting with it!

I have also added the Delta property in the ChatResult.
Currently this will be null if the request was not a stream.
I am open to suggestions for handling this better.

[hughperkins]

Could we also make ToString() consistent with how it works on Completions please? On Completions, it will return the full text content. e.g. for Streaming, this is the most recent word or similar, and for non-streaming, it is the entire returned text.

I admit that for Chat, it's a little bit ambiguous whether such text should also add the role into such text, i.e. assistant. My own vote would be for it not to include the role, since we'd then get into the weeds about how to format that role etc. And also because in many use-cases, we don't actually care about the name of the role, or want to display it, we simply want to display the text returned.

Edit: ok this is what Completions ToString does:

OpenAI-API-dotnet/OpenAI_API/Completions/CompletionResult.cs

Lines 83 to 89 in e949bd4

	public override string ToString()
	{
	if (Completions != null && Completions.Count > 0)
	return Completions[0].ToString();
	else
	return $"CompletionResult {Id} has no valid output";
	}

(We'd probably want an if added to this, to detect if Delta is null or not , and act accordingly)

[OkGoDoIt]

I'm going to go ahead an merge this, and then commit some additional fixes afterwards, including tests, readme updates, and an alternate Conversation class.

[JasonWei512]

Wish we have a AsyncEnumerable<string> Conversation.StreamResponseFromChatbot() shorthand.