.Net: Added support for BinaryData for ImageContent.#5008
.Net: Added support for BinaryData for ImageContent.#5008rogerbarreto merged 7 commits intomicrosoft:mainfrom
Conversation
|
@rogerbarreto I fixed the comments. I also checked for the |
Interesting. Though I'm not sure I like the implementation, the Stream will advance if the |
|
@dersia I'm not thrilled with it either but I sent it to you as an overview. I think the way it is now is ok but it's best for @rogerbarreto to comment. btw. I still think you should add information to the doc class comment that BinaryData takes precedence over URI. Without this information, connector implementors may not consider the order in ToString(). |
@Krzysztof318 Thats a good point, @dersia you can add this as a |
235c3ae to
8771a90
Compare
dotnet/src/SemanticKernel.Abstractions/Contents/ImageContent.cs
Outdated
Show resolved
Hide resolved
dotnet/src/SemanticKernel.UnitTests/Contents/ImageContentTests.cs
Outdated
Show resolved
Hide resolved
dotnet/src/SemanticKernel.UnitTests/Contents/ImageContentTests.cs
Outdated
Show resolved
Hide resolved
Reopend PR. See microsoft#4919 for previous PR. Reopening this PR from another feature branch makes rebasing easier. ### Motivation and Context As Described in microsoft#4781 right now there is no possibility in SK to add Images as DataUris to ChatCompletion APIs, although the Azure OpenAI API and the Open AI API both support this. Fixes microsoft#4781 ### Description As per Discussion added overload to the ImageContent ctor that takes BinaryData. For backward Compat we kept the ctor that takes an URI. Also the new ctor throws, if the BinaryData is null, empty or if there is not MediaType provided. I thought about allowing plain, non base64 encoded DataUris with BinaryData. The Idea was to not encode to base64, if the MediaType is set to "text/plain", but then I decided, that this is not needed, since `Uri` in general allows for DataUris like `new Uri("data:text/plain;http://exmpaledomain.com")` just not for DataUris that are longer than 65520 bytes. I feel like that is ok, for plain DataUris. We can still add this if needed. Also as per discussion in the issue, I did not add additional overloads for direct Streams support. ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 --------- Co-authored-by: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com>
Reopend PR. See microsoft#4919 for previous PR. Reopening this PR from another feature branch makes rebasing easier. ### Motivation and Context As Described in microsoft#4781 right now there is no possibility in SK to add Images as DataUris to ChatCompletion APIs, although the Azure OpenAI API and the Open AI API both support this. Fixes microsoft#4781 ### Description As per Discussion added overload to the ImageContent ctor that takes BinaryData. For backward Compat we kept the ctor that takes an URI. Also the new ctor throws, if the BinaryData is null, empty or if there is not MediaType provided. I thought about allowing plain, non base64 encoded DataUris with BinaryData. The Idea was to not encode to base64, if the MediaType is set to "text/plain", but then I decided, that this is not needed, since `Uri` in general allows for DataUris like `new Uri("data:text/plain;http://exmpaledomain.com")` just not for DataUris that are longer than 65520 bytes. I feel like that is ok, for plain DataUris. We can still add this if needed. Also as per discussion in the issue, I did not add additional overloads for direct Streams support. ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 --------- Co-authored-by: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com>
Reopend PR. See #4919 for previous PR.
Reopening this PR from another feature branch makes rebasing easier.
Motivation and Context
As Described in #4781 right now there is no possibility in SK to add Images as DataUris to ChatCompletion APIs, although the Azure OpenAI API and the Open AI API both support this.
Fixes #4781
Description
As per Discussion added overload to the ImageContent ctor that takes BinaryData.
For backward Compat we kept the ctor that takes an URI.
Also the new ctor throws, if the BinaryData is null, empty or if there is not MediaType provided.
I thought about allowing plain, non base64 encoded DataUris with BinaryData.
The Idea was to not encode to base64, if the MediaType is set to "text/plain", but then I decided, that this is not needed, since
Uriin general allows for DataUris likenew Uri("data:text/plain;http://exmpaledomain.com")just not for DataUris that are longer than 65520 bytes. I feel like that is ok, for plain DataUris.We can still add this if needed.
Also as per discussion in the issue, I did not add additional overloads for direct Streams support.
Contribution Checklist