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.

Sign up for GitHub

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

Jump to bottom

.Net Kernel Contents Graduation #6319

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

.Net Kernel Contents Graduation #6319

wants to merge 31 commits into from
+1,978 −278

Conversation

RogerBarreto
Copy link
Member

@RogerBarreto RogerBarreto commented May 17, 2024 •
edited

Motivation and Context

⚠️ Breaking changes on non-experimental types ImageContent

Resolves #5625

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

  • BinaryContent
  • AudioContent
  • ImageContent
  • FunctionCallContent
  • FunctionRequestContent

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:
    Remove or relax the length limitation of Uri dotnet/runtime#96544.

FunctionResultContent

  • Update Id property to CallId.

@RogerBarreto
Content reset
dc5e81e
@RogerBarreto
Changes to ADRs + POC Binary
0245a5f
@RogerBarreto
Binary COntent changes
53354b5
@RogerBarreto
Binary Content more changes
2b921b3
@RogerBarreto
Adding constructors and mimetype req
f4a7cf6
@RogerBarreto
Using ImageContentV2
4a93b09
@RogerBarreto
Binary Content Provider with MimeTypes update
1e0697c
@RogerBarreto
Serialization and Examples
858aabd
@RogerBarreto
Making ImageContent a bit less of a breaking change, more WIP and tes…
a4fb379 
…ts needed
@RogerBarreto
Renaming
1f2e39b
@RogerBarreto
Kernel Binary Simplified, No Deferred Factories
cf17153
@RogerBarreto
UT passing for simple binary
36cb77e
@RogerBarreto
UT for ImageContent passes
32712d2
@RogerBarreto
Ensuring Old Image Behavior
6c09334
@RogerBarreto
Adding Tests, BinaryContent with Data + Uri, consolidation
1a60b45
@RogerBarreto
Meeting presentation status
06fe63c
@RogerBarreto
ImageContent consistency breaking change
ea7d29e
@RogerBarreto
Adding DataUri Formatter + Base64 validation + UT
eadda16
@RogerBarreto
Fix usings
d17a5a1
@RogerBarreto
Adjusting Unit Tests
9251e34
@RogerBarreto
Update CallId, bring back Experimental, remove SmartBinaryContent POC…
639dd7e 
…, Added AudioContent UT's
@RogerBarreto
Removing draft examples
6f5ed3b
@RogerBarreto
Adding Suppressions + Fix Warnings
4a11118
@RogerBarreto RogerBarreto requested a review from a team as a code owner May 17, 2024 18:45
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core documentation labels May 17, 2024
@RogerBarreto RogerBarreto self-assigned this May 20, 2024
@RogerBarreto RogerBarreto mentioned this pull request May 20, 2024
Open
@dmytrostruk dmytrostruk mentioned this pull request May 30, 2024
Merged
4 tasks
github-merge-queue bot pushed a commit that referenced this pull request May 31, 2024
@dmytrostruk
.Net: Fixed ImageContent usage in OpenAI connector (#6450)
8d9c3cb 
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

Fixes: #6443

This is temporary fix before new updates to `ImageContent` class will be
in place: #6319

cc: @RogerBarreto

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### 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 😄
markwallace-microsoft
markwallace-microsoft reviewed May 31, 2024
View reviewed changes
dotnet/src/Connectors/Connectors.OpenAI/CompatibilitySuppressions.xml
<Left>lib/netstandard2.0/Microsoft.SemanticKernel.Connectors.OpenAI.dll</Left>
<Right>lib/netstandard2.0/Microsoft.SemanticKernel.Connectors.OpenAI.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to review this PR and understand the breaking change.

Are there any options that will allow us introduce the new pattern and also avoid the breaking change?

markwallace-microsoft
markwallace-microsoft reviewed May 31, 2024
View reviewed changes
dotnet/src/SemanticKernel.Abstractions/Contents/FunctionResultContent.cs
@@ -16,7 +16,7 @@ public sealed class FunctionResultContent : KernelContent
/// The function call ID.
/// </summary>
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public string? Id { get; }
public string? CallId { get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change?

We should be avoiding breaking changes. So if this name change is critical then we should obsolete the old property.

markwallace-microsoft
markwallace-microsoft reviewed May 31, 2024
View reviewed changes
dotnet/src/SemanticKernel.Abstractions/Contents/ImageContent.cs
public sealed class ImageContent : KernelContent
#pragma warning disable SKEXP0010 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
public class ImageContent : BinaryContent
#pragma warning restore SKEXP0010 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be SKEXP0001 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markwallace-microsoft markwallace-microsoft markwallace-microsoft left review comments

@SergeyMenshykh SergeyMenshykh SergeyMenshykh approved these changes

@westey-m westey-m westey-m approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees

@RogerBarreto RogerBarreto

Labels
documentation kernel.core kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Graduate Content Types
4 participants
@RogerBarreto @SergeyMenshykh @markwallace-microsoft @westey-m