Protocol models for ChatClient #778
Conversation
api/OpenAI.net8.0.cs
Outdated
| public ClientPipeline Pipeline { get; } | ||
| public virtual ClientResult<ChatCompletion> CompleteChat(params ChatMessage[] messages); | ||
| [Experimental("OPENAI001")] | ||
| public virtual ClientResult<ChatCompletionResult> CompleteChat(CreateChatCompletionOptions options, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Can I pass in a RequestOptions?
There was a problem hiding this comment.
You could use the protocol method overload to do that like this:
ChatCompletionResult response = (ChatCompletionResult)await client.CompleteChatAsync(options, new RequestOptions());The CreateChatCompletionOptions would be implicitly cast to BinaryContent.
christothes
requested review from
ShivangiReja,
joseharriaga and
trrwilson
as code owners
| [Experimental("OPENAI001")] | ||
| public virtual Task<ClientResult<ChatCompletionDeletionResult>> DeleteChatCompletionAsync(string completionId, CancellationToken cancellationToken = default); | ||
| [Experimental("OPENAI001")] | ||
| public virtual ClientResult<ChatCompletionResult> GetChatCompletion(GetCompletionOptions options, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Should the name of the options class match the method name (i.e. GetChatCompletionOptions instead of GetCompletionOptions)?
| public Uri Endpoint { get; } | ||
| [Experimental("OPENAI001")] | ||
| public string Model { get; } | ||
| public virtual string Model { get; } |
There was a problem hiding this comment.
Why is the Model property virtual? 🤔
| public virtual Task<ClientResult> CompleteChatAsync(BinaryContent content, RequestOptions options = null); | ||
| public virtual Task<ClientResult<ChatCompletion>> CompleteChatAsync(IEnumerable<ChatMessage> messages, ChatCompletionOptions options = null, CancellationToken cancellationToken = default); | ||
| public virtual CollectionResult<StreamingChatCompletionUpdate> CompleteChatStreaming(params ChatMessage[] messages); | ||
| public virtual CollectionResult<StreamingChatCompletionUpdate> CompleteChatStreaming(CompleteChatOptions options, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
This new method is missing the Experimental attribute.
| public virtual CollectionResult<StreamingChatCompletionUpdate> CompleteChatStreaming(CompleteChatOptions options, CancellationToken cancellationToken = default); | ||
| public virtual CollectionResult<StreamingChatCompletionUpdate> CompleteChatStreaming(IEnumerable<ChatMessage> messages, ChatCompletionOptions options = null, CancellationToken cancellationToken = default); | ||
| public virtual AsyncCollectionResult<StreamingChatCompletionUpdate> CompleteChatStreamingAsync(params ChatMessage[] messages); | ||
| public virtual AsyncCollectionResult<StreamingChatCompletionUpdate> CompleteChatStreamingAsync(CompleteChatOptions options, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
This new method is missing the Experimental attribute.
| public class ChatCompletionMessageListDatum : IJsonModel<ChatCompletionMessageListDatum>, IPersistableModel<ChatCompletionMessageListDatum> { | ||
| public IReadOnlyList<ChatMessageAnnotation> Annotations { get; } | ||
| public string Content { get; } | ||
| public IList<ChatMessageContentPart> ContentParts { get; } |
There was a problem hiding this comment.
Was this removal intentional?
| public enum ChatRequestModality { | ||
| Text = 0, | ||
| Audio = 1 | ||
| } |
There was a problem hiding this comment.
This can probably be replaced by the existing ChatResponseModality. Currently, ChatResponseModality is implemented as flags, but I think that changing it to a normal enum like you have here might actually better. With this in mind, I'm in favor of the following:
- Rename this
ChatRequestModalitytoChatResponseModality. - Turn this
ChatRequestModalityinto an extensible enum. - Delete the existing flags called
ChatResponseModalities(this is possible because it's experimental). - Change the type of the
ResponeModalitiesproperty inChatCompletionOptionsfromChatResponseModalitiestoIList<ChatResponseModality>(just like in the protocol model).
| protected override void WriteCore(Utf8JsonWriter writer, ModelReaderWriterOptions options); | ||
| } | ||
| [Experimental("OPENAI001")] | ||
| public class CompletionCollection : IJsonModel<CompletionCollection>, IPersistableModel<CompletionCollection> { |
There was a problem hiding this comment.
Should this be called ChatCompletionCollection ?
| [Experimental("OPENAI001")] | ||
| public virtual Task<ClientResult<CompletionCollection>> GetChatCompletionsAsync(ChatCompletionsOptions options, CancellationToken cancellationToken = default); | ||
| [Experimental("OPENAI001")] | ||
| public virtual ClientResult<ChatCompletionResult> UpdateChatCompletion(UpdateChatCompletionOptions options, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
ChatCompletionsOptions looks identical to ChatCompletionCollectionOptions. Could we remove the former?
| public virtual AsyncCollectionResult GetChatCompletionsAsync(ChatCompletionsOptions options, RequestOptions requestOptions); | ||
| [Experimental("OPENAI001")] | ||
| public virtual Task<ClientResult<CompletionCollection>> GetChatCompletionsAsync(ChatCompletionsOptions options, CancellationToken cancellationToken = default); | ||
| [Experimental("OPENAI001")] |
There was a problem hiding this comment.
I think it's a little unexpected that the output of this new method appears to be lower level than the output of the protocol method (which I have always understood as the lowest level). How do we feel about the protocol method supporting pagination while this new method doesn't?
| [Experimental("OPENAI001")] | ||
| public virtual Task<ClientResult<ChatCompletion>> GetChatCompletionAsync(string completionId, CancellationToken cancellationToken = default); | ||
| [Experimental("OPENAI001")] | ||
| public virtual CollectionResult GetChatCompletionMessages(GetCompletionMessageOptions options, RequestOptions requestOptions = null); |
There was a problem hiding this comment.
I think that the only difference between GetCompletionMessageOptions and ChatCompletionMessageCollectionOptions is that the former includes the CompletionId. I wonder if we should normalize it somehow so that we don't need two classes and two sets of methods.
4 participants
No description provided.