Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of text-only user messages in AzureAIInferenceChatClient #5714

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Dec 2, 2024

ChatRequestUserMessage has three constructors:

public ChatRequestUserMessage(string content)
public ChatRequestUserMessage(IEnumerable<ChatMessageContentItem> content)
public ChatRequestUserMessage(params ChatMessageContentItem[] content)

but even though all of the parameters are named content and represent the message's content, they behave differently. The first assigns the string content to the instance's Content property and leaves its MultimodalContentItems property null, and the others leave Content null and set MultimodalContentItems to the property. For models that don't support multi-modal, using the latter two constructors breaks, even when the content is a single text item.

I think this should be improved in Azure.AI.Inference, but regardless, this fixes the ToAzureAIInferenceChatMessages helper to special-case text-only inputs and use the first string-based constructor rather than always using the IEnumerable<ChatMessageContentItem>-based one.

Fixes #5708

Microsoft Reviewers: Open in CodeFlow

`ChatRequestUserMessage` has three constructors:
```csharp
public ChatRequestUserMessage(string content)
public ChatRequestUserMessage(IEnumerable<ChatMessageContentItem> content)
public ChatRequestUserMessage(params ChatMessageContentItem[] content)
```
but even though all of the parameters are named `content` and represent the message's content, they behave differently. The first assigns the string content to the instance's `Content` property and leaves its `MultimodalContentItems` property null, and the others leave `Content` null and set `MultimodalContentItems` to the property. For models that don't support multi-modal, using the latter two constructors breaks, even when the content is a single text item.

I think this should be improved in Azure.AI.Inference, but regardless, this fixes the ToAzureAIInferenceChatMessages helper to special-case text-only inputs and use the first `string`-based constructor rather than always using the `IEnumerable<ChatMessageContentItem>`-based one.
@stephentoub
Copy link
Member Author

cc: @trangevi

@dotnet-comment-bot
Copy link
Collaborator

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.Diagnostics.Probes 70 76
Microsoft.Extensions.Caching.Hybrid 75 86
Microsoft.Extensions.AI.AzureAIInference 83 91

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=884548&view=codecoverage-tab

@stephentoub stephentoub merged commit 1120b29 into dotnet:main Dec 3, 2024
6 checks passed
@stephentoub stephentoub deleted the azureaitext branch December 3, 2024 03:14
@dotnet dotnet deleted a comment from dotnet-comment-bot Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Microsoft.Extensions.AI] The Cohere Command R+ always returns "Bad Request"
3 participants