Implement Chat Completion, see Issue #57 #58
Conversation
OpenAI API Changes for Chat Completion Changes to Existing Code - Runtime\Scenes\Api\Utility\EngineNames\EEngineName.cs: Added new model enumeration gpt_3_5_turbo - Runtime\Scenes\Api\Utility\EngineNames\UTEngineNames.cs: Added new model enumeration gpt_3_5_turbo - Runtime\Scripts\Api\V1\OpenAiApiV1.cs: Added ChatCompletions resource - Editor\Scripts\Unity\V1\EMPrefabs.cs: Added editor menu OpenAi/V1/CreateChatCompleter Additions - Editor\Scripts\Examples\EMExampleChatRuntimeScene.cs - Runtime\Prefabs\OpenAiChatCompleterV1.prefab - Runtime\Scenes\ExampleChatRuntimeScene.unity - Runtime\Scripts\Api\V1\Models\ChatCompletionV1.cs - Runtime\Scripts\Api\V1\Models\UsageV1.cs - Runtime\Scripts\Api\V1\Models\ChatChoiceV1.cs - Runtime\Scripts\Api\V1\Models\ChatCompletionV1.cs - Runtime\Scripts\Api\V1\Models\MessageV1.cs - Runtime\Scripts\Api\V1\Api\Engines\ChatCompletion\ChatCompletionRequestV1.cs - Runtime\Scripts\Api\V1\Api\Engines\ChatCompletion\ChatCompletionResourceV1.cs - Runtime\Scripts\Examples\ExampleChatRuntime.cs - Runtime\Scripts\Unity\V1\ChatCompleter\OpenAiChatCompleterV1.cs - Runtime\Scripts\Unity\V1\ChatCompleter\SOChatCompletionArgsV1.cs - Runtime\Config\DefaultChatCompletionArgs.asset
There was a problem hiding this comment.
Just wanted to say a big thanks for this PR. It's a great contribution to the project.
In order to get it passed the post:
- Address the comments on the code. There is a slight inconsitency with other resources. The main functionality looks sound though.
- Update the README.md to tell people about your new endpoint. I think people will be interested in trying this.
- I would like to see a Unit Test added to
/Tests/Runtime/Scripts/Core/V1/V1PlayTests.csthat verifys that the endpoint is working.
What I did not do in the PR:
- I did not test it. I am out of OpenAI tokens and need to talk to them to get more. So you will need to test it to verify that it works resonably well.
| public class ChatCompletionsResourceV1 : AApiResource<OpenAiApiV1> | ||
| { | ||
| /// <inheritdoc/> | ||
| public override string Endpoint => "/chat/completions"; |
There was a problem hiding this comment.
Currently in the library, when someone wants to use a REST Api call associated with <engine_id>/completions, they call EnginesResource.Engine(<engine_id>).Completions, which follows engines/<engine_id>/completions.
This ChatCompletionResource should be split into a ChatResource and a ChatCompletionsResources with the chat resource containing something along the lines of
public class ChatResourceV1 : AApiResource<OpenAiApiV1>
{
public ChatCompletionsV1 Completions => new ChatCompletionsV1(this);
}This will maintain the call pattern. engines/<engine_id>/chat/completions as EnginesResource.Engine(<engine_id>).Chat.Completions, instead of EnginesResource.Engine(<engine_id>).ChatCompletions
This is necessary becuase in the future chat might end up having more API endpoints following it.
| @@ -54,6 +59,7 @@ public OpenAiApiV1(SAuthArgsV1 authArgs) | |||
| Engines = new EnginesResourceV1(this); | |||
| Files = new FilesResourceV1(this); | |||
| Classifications = new ClassificationsResourceV1(this); | |||
| ChatCompletions = new ChatCompletionsResourceV1(this); | |||
There was a problem hiding this comment.
Since the ChatCompletions resource is accessed via the EnginesResource (Specifically in the EngineResource), it should not appear in this class. It should appear as a child of the EngineResource
I just took a look at the API quickly, and I see some major concepts have changed. Engines is depreceated.
However, I think that in this PR, the code should remain as consistent as possible with the existing Resources, so that when I or someone else does a refactor based on the new API concepts, everything can be moved in the same way.
So, to finalize this comment.
- Remove the
ChatCompletionsproperty from this class. Accessing this resource should be done throughOpenAiApiV1.Engines.Engine("engine_id").Chat.Completions
There was a problem hiding this comment.
I originally tried to treat chat completions as an engine consistent with prompt completions but chat completions cannot be accessed via the endpoint engines/<engine_id>/chat/completions. I think chat must be accessed via an endpoint that does not include the engine; the model is passed as part of the JSON payload, example below. So that's the rationale for adding the chat resource to the root of the API rather than as an engine. Let me know your thoughts when you have a chance. I'll look into your other PR change requests.
curl https://api.openai.com/v1/chat/completions
-H "Content-Type: application/json"
-H "Authorization: Bearer $OPENAI_API_KEY"
-d '{
"model": "gpt-3.5-turbo",
"messages": [{"role": "user", "content": "Hello!"}]
}'
There was a problem hiding this comment.
Ok, if that is the case (which I can see from reading the API specs), then we still need two objects instead of one. Since the endpoint is chat/completions to stay consistent we want the access to work as OpenAiApiV1.Chat.Completions rather than OpenAiApiV1.ChatCompletions. That way if there are more chat/ endpoints in the future they will be easy to add.
I know there is already a CompletionsResourceV1 object so naming the new one ChatCompletionsResourceV1 will work. But in the ChatResourceV1 the property name should still be Completions. I.E.:
public class ChatResourceV1 : AApiResource<OpenAiApiV1>
{
...
public ChatCompletionsResourceV1 Completions { get; private set;} // not ChatCompletionsResourceV1 ChatCompletions
...
}There was a problem hiding this comment.
Got it, will make this change. Thank you for taking the time to work with me on this James.
There was a problem hiding this comment.
Refactored OpenAiApiV1.ChatCompletions into OpenAiApiV1.Chat.Completions. Getting there...
Tests were based on tests covering existing prompt completion. Also fixed an issue with empty JSON objects and added unit tests. The last record of a chat stream has a finish reason and an empty delta object.
|
Added chat completion unit tests. Also needed to fix an issue with empty JSON objects (delta: {}). Have a look, might be a better way to fix this. Still need to address documentation, TODO. |
| // Handle empty object, i.e.: object: {} | ||
| if (syntax[index] == "}") | ||
| { | ||
| parent.NestedValues = new List<JsonObject>(); | ||
| return index + 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
Good catch, this is a decent solution.
Hey just took a look. Tests look good! |
Also created distinct enumeration type EChatModelNames and removed gtp-3.5-turbo out of EEngineNames. Did not include gpt-4 models as these are still in beta.
There was a problem hiding this comment.
As far as I can tell tell this looks good to me. Glad to have it added to the library.
To clarify, what I did do:
- Read code to look for consistency with rest of the library.
What I didn't do:
- Run the code on my side
- Run the tests on my side
- Check the objects against the actual API
If everything looks good your end and tests are passing I will merge the PR. Just give me the go ahead when you're ready.
Added some documentation for chat/completions and separated DeltaV1 from MessageV1 as role is not optional for messages but is for message deltas. Also need to see message role in Unity Inspector.
|
Committed another small change which included adding a few lines of documentation to the README.md and separating JSON objects DeltaV1 and MessageV1. I'm good for you to merge. Here's an unlisted video that demonstrates the example chat scene and unit tests. |
OpenAI API Changes for Chat Completion
Changes to Existing Code
Additions