-
Notifications
You must be signed in to change notification settings - Fork 835
Address M.E.VectorData feedback for IEmbeddingGenerator #6058
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
Address M.E.VectorData feedback for IEmbeddingGenerator #6058
Conversation
Specifically for language, IIRC MIME types don't officially support that - maybe |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/TextContent.cs
Outdated
Show resolved
Hide resolved
I was referring to programming languages, for eg code generated by the llm as part of a code interpreter tool, like text/x-python, text/x-csharp, text/html, or application/typescript. |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/DataContent.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Embeddings/EmbeddingGeneratorMetadata.cs
Outdated
Show resolved
Hide resolved
6fd8d15
to
e964967
Compare
@roji, @westey-m, @SteveSandersonMS, I've pushed a revision based on our discussion. This now only includes:
@rogerbarreto, FYI @shyamnamboodiripad, @peterwald, I don't think this should affect you much if at all, but FYI. |
cc: @luisquintanilla, since you were previously asking about DataContent representing both in-memory data and remote content |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/DataContent.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/UriContent.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Embeddings/IEmbeddingGenerator.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/DataContent.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (see comments)!
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/DataContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Embeddings/IEmbeddingGenerator.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/DataContent.cs
Show resolved
Hide resolved
Addressed feedback:
|
Debug.Assert(Data is not null, "Expected Data to be initialized."); | ||
_uri = string.Concat("data:", MediaType, ";base64,", Convert.ToBase64String(Data.GetValueOrDefault() | ||
Debug.Assert(_data is not null, "Expected _data to be initialized."); | ||
_uri = string.Concat("data:", MediaType, ";base64,", Convert.ToBase64String(_data.GetValueOrDefault() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the _dataUri
is generated from the _data
, since it's a readonly whouldn't it be nice to cache it and vice-versa?
_uri = string.Concat("data:", MediaType, ";base64,", Convert.ToBase64String(_data.GetValueOrDefault() | |
_uri = string.Concat("data:", MediaType, ";base64,", Convert.ToBase64String(_data.GetValueOrDefault() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would _dataUri end up being used again?
* The .NET side code for the `ScenarioRunResult` was recently changed (#5998) to include `ChatResponse` (which can contain multiple `ChatMessage`s) in place of a single `ChatMessage`. Unfortunately, we missed updating the TypeScript reporting code to account for this. (#6061) This change fixes the problem by updating the deserialization code in TypeScript to match what .NET code serializes. * Automatically add 'untriaged' label to new issues without milestones (#6060) * Address M.E.VectorData feedback for IEmbeddingGenerator (#6058) * Move GetService down to a non-generic IEmbeddingGenerator interface * Separate UriContent from DataContent * Address feedback --------- Co-authored-by: Shyam N <shyamnamboodiripad@users.noreply.github.com> Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com> Co-authored-by: Stephen Toub <stoub@microsoft.com>
* Move GetService down to a non-generic IEmbeddingGenerator interface * Separate UriContent from DataContent * Address feedback
…tor (dotnet#6058) Address M.E.VectorData feedback for IEmbeddingGenerator (dotnet#6058) * Move GetService down to a non-generic IEmbeddingGenerator interface * Separate UriContent from DataContent
Necessary, according to @roji:
Optional (we should discuss which of these we actually want to take):
IAsyncEnumerable<TEmbedding>
instead ofGeneratedEmbeddings<TEmbedding>
. The positive aspect of this is it allows for embeddings to be streamed as they're generated (though the main embedding generation services today don't stream), and also enables TEmbedding to be covariant (though it's not clear how much benefit that actually yields). The negative is we lose a place to hang metadata that's about the whole batch, in particular usage data. This moves that usage data to instead be on the Embedding, which is helpful for cases where such information is available per embedding, but isn't ideal for cases where it's about the whole batch. This PR deals with that by just putting such information onto one of the generated embeddings. We'll need to discuss the tradeoff. The alternative is we keep GeneratedEmbeddings but make it an IAsyncEnumerable instead of an IList. That gives a place to hang such batch information, but only if it's all available at the start of the streaming; however, it's also a bit harder to consume, as you need to await the initial call and then await foreach the embeddings.Not addressed:
GetService<EmbeddingGeneratorMetadata>()
is sufficient, but we can discuss it further. We might want to consider an analyzer that would make diagnostic suggestions about supporting it in GetService and filling in as much data as possible.cc: @roji, @SteveSandersonMS
Microsoft Reviewers: Open in CodeFlow