Address M.E.AI API feedback #5860
Conversation
- Remove DataContent.ContainsData. While ContainsData could be used to avoid lazily-instantiating a `ReadOnlyMemory<byte>` from a data URI, in all cases examined ContainsData was being used to guard accessing Data, in which case Data.HasValue is sufficient.
- Make ToolMode optional. We'd previously said that an implementation could use null to imply None if it wanted, but with this being optional we'd want null to be the same as auto, so I added in an explicit null options. That matches as well with what various other client libs do.
- Remove IChatClient/IEmbeddingGenerator.Metadata. GetService can be used instead, reducing what an implementer must implement and what's exposed to a consumer.
- Add Complete{Streaming}Async for a single message. We have such accelerators in other places but not here, and it provides a natural grow-up from string to ChatMessage to `IList<ChatMessage>`.
- Change UsageDetails.XxTokenCount properties from int? to long?. The AdditionalCounts is already based on longs, and in theory the token counts could exceed int, especially in a situation where UsageData is being used to sum many other call data.
- Rename GenerateEmbeddingVectorAsync's TEmbedding to TEmbeddingElement. Everywhere else TEmbedding is used where it represents an Embedding, but here it represents a numerical element in an embedding vector.
- Remove setters on FunctionCall/ResultContent for required ctor parameters. Having those setters goes against .NET design guidelines.
- Remove FunctionResultContent.Name. It's unnecessary and is actually causing us to do more work in places.
stephentoub
requested review from
jeffhandley,
SteveSandersonMS,
eiriktsarpalis and
MackinnonBuck
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=944835&view=codecoverage-tab |
| /// <returns>The response messages generated by the client.</returns> | ||
| public static Task<ChatCompletion> CompleteAsync( | ||
| this IChatClient client, | ||
| ChatMessage chatMessage, |
There was a problem hiding this comment.
This new set of extensions can only be used in cases where the chat history consists of only that one message. How common is such a scenario? Could it lead callers into incorrectly assuming that they're appending a chat message to a log that is being encapsulated by the client?
There was a problem hiding this comment.
This new set of extensions can only be used in cases where the chat history consists of only that one message. How common is such a scenario?
It's pretty common, for at least:
- Getting started scenarios, where you want to send a single input
- Non-chat scenarios, where you're sending input to have the LLM give you back a response as part of the logic of you're program.
- IChatClient implementations where the state is stored server-side, ala assistants
It's no different from the existing string-based overloads that are helpful in similar circumstances but that are limited to TextContent; these overloads allow you to customize beyond that.
| @@ -61,16 +64,14 @@ public JsonSerializerOptions ToolCallJsonSerializerOptions | |||
| } | |||
|
|
|||
| /// <inheritdoc /> | |||
| public ChatClientMetadata Metadata { get; } | |||
There was a problem hiding this comment.
Even if no longer part of IChatClient, it seems useful enough to surface as a property on the implementing class.
There was a problem hiding this comment.
I'd rather we expose it as an extension property once those are possible.
ReadOnlyMemory<byte>from a data URI, in all cases examined ContainsData was being used to guard accessing Data, in which case Data.HasValue is sufficient.IList<ChatMessage>.Microsoft Reviewers: Open in CodeFlow