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: Revise BinaryContent class design #5295

Closed
SergeyMenshykh opened this issue Mar 4, 2024 · 0 comments · Fixed by #6319
Closed

.Net: Revise BinaryContent class design #5295

SergeyMenshykh opened this issue Mar 4, 2024 · 0 comments · Fixed by #6319
Assignees
Labels
.NET Issue or Pull requests regarding .NET code sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)

Comments

@SergeyMenshykh
Copy link
Member

This task is a result of the discussion: #5229 (comment)

ToDo:

  • Revise the design of the BinaryContent class to address the concerns pointed out in the discussion.
  • Refactor the class.

CC: @crickman, the author of the class.

@SergeyMenshykh SergeyMenshykh added .NET Issue or Pull requests regarding .NET code triage labels Mar 4, 2024
@SergeyMenshykh SergeyMenshykh moved this to Backlog in Semantic Kernel Mar 4, 2024
@markwallace-microsoft markwallace-microsoft added sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community) and removed triage labels Mar 6, 2024
@RogerBarreto RogerBarreto self-assigned this May 3, 2024
@RogerBarreto RogerBarreto moved this from Backlog to Sprint: In Progress in Semantic Kernel May 3, 2024
@RogerBarreto RogerBarreto moved this from Sprint: In Progress to Sprint: In Review in Semantic Kernel May 27, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 10, 2024
### Motivation and Context

⚠️ Breaking changes on non-experimental types **ImageContent**

Resolves #5625
Resolves #5295

For a brief time this changes will keep the content below as
experimental.

- BinaryContent
- AudioContent
- ImageContent 
- FunctionCallContent
- FunctionResultContent

Changes:

### **BinaryContent** 
- Removed providers for lazy loading content, simplifying its usage and
APIs.
- Removed `Stream` constructor to avoid IDisposable resource consumption
or bad practices.
- Added `Uri` dedicated for Referenced Uri information
- Added `DataUri` property which can be set or get dynamically (auto
generated if you created the
content using byte array with a mime type) 
Setting a `DataUri` will automatically update the `MimeType` property
and add any extra metadata that may be available in the data scheme
definition.
- Added a required `mimeType` property to the ByteArray constructor, to
encourage passing the mimeType when creating BinaryContent directly or
from specializations.
- Added `Data` property which can be set or get dynamically (auto
generated if you created the content using a data uri format)
Setting a Data on an existing BinaryContent will also reflect on the
getter of `DataUri` for the given content.
- Added DataUri and Base64 validation when setting DataUri on the
contents.
- When using DataUri parameters those merge with the current content
metadata.
i.e:
`data:image/jpeg;parameter1=value1;parameter2=value2;base64,binary==`

### ⚠️ **ImageContent** Fixes bugs and inconsistency behavior:
- Setting the Data of an image doesn't change the current data uri and
vice versa, allowing the sema image content instance to have different
binary data to representations.
- When an Image could not have DataUri and Uri in the same instance,
this limits scenarios where you have the image data but want to have a
reference to where that content is from.
- Wasn't possible to create an Image from a data uri greater than the
size limit of .Net System.Uri type here:

[https://github.com/dotnet/runtime/issues/96544](https://github.com/dotnet/runtime/issues/96544).

### **FunctionResultContent**
- Update `Id` property to `CallId`.
@github-project-automation github-project-automation bot moved this from Sprint: In Review to Sprint: Done in Semantic Kernel Jun 10, 2024
LudoCorporateShark pushed a commit to LudoCorporateShark/semantic-kernel that referenced this issue Aug 25, 2024
### Motivation and Context

⚠️ Breaking changes on non-experimental types **ImageContent**

Resolves microsoft#5625
Resolves microsoft#5295

For a brief time this changes will keep the content below as
experimental.

- BinaryContent
- AudioContent
- ImageContent 
- FunctionCallContent
- FunctionResultContent

Changes:

### **BinaryContent** 
- Removed providers for lazy loading content, simplifying its usage and
APIs.
- Removed `Stream` constructor to avoid IDisposable resource consumption
or bad practices.
- Added `Uri` dedicated for Referenced Uri information
- Added `DataUri` property which can be set or get dynamically (auto
generated if you created the
content using byte array with a mime type) 
Setting a `DataUri` will automatically update the `MimeType` property
and add any extra metadata that may be available in the data scheme
definition.
- Added a required `mimeType` property to the ByteArray constructor, to
encourage passing the mimeType when creating BinaryContent directly or
from specializations.
- Added `Data` property which can be set or get dynamically (auto
generated if you created the content using a data uri format)
Setting a Data on an existing BinaryContent will also reflect on the
getter of `DataUri` for the given content.
- Added DataUri and Base64 validation when setting DataUri on the
contents.
- When using DataUri parameters those merge with the current content
metadata.
i.e:
`data:image/jpeg;parameter1=value1;parameter2=value2;base64,binary==`

### ⚠️ **ImageContent** Fixes bugs and inconsistency behavior:
- Setting the Data of an image doesn't change the current data uri and
vice versa, allowing the sema image content instance to have different
binary data to representations.
- When an Image could not have DataUri and Uri in the same instance,
this limits scenarios where you have the image data but want to have a
reference to where that content is from.
- Wasn't possible to create an Image from a data uri greater than the
size limit of .Net System.Uri type here:

[https://github.com/dotnet/runtime/issues/96544](https://github.com/dotnet/runtime/issues/96544).

### **FunctionResultContent**
- Update `Id` property to `CallId`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants