Remove ImageContent and AudioContent
#5814
Conversation
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=928961&view=codecoverage-tab |
| parts.Add(new ChatMessageImageContentItem(BinaryData.FromBytes(data), imageContent.MediaType)); | ||
| break; | ||
| case DataContent dataContent when dataContent.HasImageMediaType(): | ||
| if (dataContent.Data is { IsEmpty: false } data) |
There was a problem hiding this comment.
This is fine, but fwiw I find the IsEmpty: false double-negative a little confusing. This could also be:
if (dataContent.ContainsData)
{
... dataContent.Data ...
}| /// <param name="content">The <see cref="DataContent"/>.</param> | ||
| /// <returns><see langword="true"/> if the content represents an image; otherwise, <see langword="false"/>.</returns> | ||
| public static bool HasImageMediaType(this DataContent content) | ||
| => content.MediaType is { } mediaType && mediaType.StartsWith("image/", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
| => content.MediaType is { } mediaType && mediaType.StartsWith("image/", StringComparison.OrdinalIgnoreCase); | |
| => content.MediaType?.StartsWith("image/", StringComparison.OrdinalIgnoreCase) is true; |
?
If this is something we expect every consumer to need to do, should we bake it in with a helper instance method on DataContent, e.g.
public bool HasMediaTypePrefix(string prefix)? ("Prefix" isn't ideal naming, but HasMediaTypeType doesn't sound right, either :))
| @@ -375,7 +375,7 @@ private IEnumerable<OllamaChatRequestMessage> ToOllamaChatRequestMessages(ChatMe | |||
| OllamaChatRequestMessage? currentTextMessage = null; | |||
| foreach (var item in content.Contents) | |||
| { | |||
| if (currentTextMessage is not null && item is not ImageContent) | |||
| if (currentTextMessage is not null && item is not DataContent) | |||
There was a problem hiding this comment.
What ends up happening to the logic if it's a DataContent but HasImageMediaType below is false? Might we end up coalescing incorrectly / yielding items out of order?
| new AudioContent("http://localhost/audio"), | ||
| new ImageContent("http://localhost/image"), | ||
| new DataContent("http://localhost/audio", mediaType: "audio/mpeg"), | ||
| new DataContent("http://localhost/image", mediaType: "image/png"), |
There was a problem hiding this comment.
Did these tests actually require providing a media type now when wasn't provided before?
Removes the
ImageContentandAudioContenttypes.The recommendation will be that chat client implementors and app developers use
DataContent.MediaTypeto distinguish between the various content types.Fixes #5719
Microsoft Reviewers: Open in CodeFlow