.Net MEVD: Make Top mandatory argument #11376
Conversation
markwallace-microsoft
added
.NET
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
LGTM thanks Adam! My only comment is about adding an overload without the options (see below), but that can be done later (with our current rhythm, better to merge quickly).
|
|
||
| // Search for similar prompts in cache. | ||
| var searchResults = await collection.VectorizedSearchAsync(promptEmbedding, new() { Top = 1 }, context.CancellationToken); | ||
| var searchResults = await collection.VectorizedSearchAsync(promptEmbedding, top: 1, cancellationToken: context.CancellationToken); |
There was a problem hiding this comment.
It's (slightly) annoying to have to specify the cancellationToken named argument just because we don't need to specify the options :/
Should we add an overload that doesn't accept the options, and have a default interface implementation - for .NET 8.0 only - that calls into the main one? We should try to not let the .NET Framework support cause an inferior dev experience to people using modern .NET...
@westey-m @adamsitnik what do you think?
There was a problem hiding this comment.
I would ask a question during API review about what is the recommended approach to tackle this particular problem (I agree it's annoying).
| var vectorizedQuery = await this._textEmbeddingGeneration!.GenerateEmbeddingAsync(query, cancellationToken: cancellationToken).ConfigureAwait(false); | ||
|
|
||
| return await this._vectorizedSearch.VectorizedSearchAsync(vectorizedQuery, vectorSearchOptions, cancellationToken).ConfigureAwait(false); | ||
| return await this._vectorizedSearch.VectorizedSearchAsync(vectorizedQuery, searchOptions.Top, vectorSearchOptions, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
We should consider applying the same decision (make Top a mandatory parameter) for ITextSearch - the same logic applies (/cc @markwallace-microsoft)
There was a problem hiding this comment.
Great catch, but IMO we need a separate PR for that (it's used in plenty of spaces, I am not sure how to run all related tests to ensure I dont break anything).
adamsitnik
merged commit f703f69
into
microsoft:feature-vector-data-preb2
4 participants
fixes #10193