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

.Net: Added support for BinaryData for ImageContent. #5008

Merged
merged 7 commits into from
Feb 20, 2024

Conversation

dersia
Copy link
Contributor

@dersia dersia commented Feb 14, 2024

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 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

@dersia dersia requested a review from a team as a code owner February 14, 2024 08:08
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Feb 14, 2024
@github-actions github-actions bot changed the title Feature/binary data image .Net: Feature/binary data image Feb 14, 2024
@dersia
Copy link
Contributor Author

dersia commented Feb 14, 2024

@RogerBarreto I fixed the comments. I also checked for the using pattern and I couldn't replicate the behavior I had in the past with the memory stream, so I reverted it back to using the using pattern in the test.

@dersia
Copy link
Contributor Author

dersia commented Feb 14, 2024

@Krzysztof318

@dersia btw. new class was introduced to kernel BinaryContent This class is currently used only by OpenAIFileService.

I know this news may not be relevant to this PR, but it is worth taking a view.

Interesting. Though I'm not sure I like the implementation, the Stream will advance if the GetStreamAsync or GetContentAsync is called and I also think that Null-Content/Stream would be fine and should not throw, it should just not be added to the request to the model.

@dersia dersia changed the title .Net: Feature/binary data image .Net: Added support for BinaryData for ImageContent. Feb 14, 2024
@Krzysztof318
Copy link
Contributor

Krzysztof318 commented Feb 14, 2024

@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().

@RogerBarreto
Copy link
Member

I still think you should add information to the doc class comment that BinaryData takes precedence over URI.

@Krzysztof318 Thats a good point, @dersia you can add this as a <remark></remark> in the ToString XmlDocs comments.

@RogerBarreto RogerBarreto added this pull request to the merge queue Feb 20, 2024
Merged via the queue into microsoft:main with commit 08569f5 Feb 20, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel.core kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

.Net: Add overload ctor to ImageContent that takes an string "url"
5 participants