Repository navigation
.Net: Pack of changes from MEVD API review - #11882
Shay Rojansky (roji) merged 7 commits into
Conversation
Mark Wallace (markwallace-microsoft)
added
.NET
github-actions
Bot
changed the title
GitHub Actions
Inactive
f96d44c to
a085d34
Compare
GitHub Actions
Inactive
| /// Initializes a new instance of the <see cref="VectorStoreDataProperty"/> class by cloning the given source. | ||
| /// </summary> | ||
| /// <param name="source">The source to clone.</param> | ||
| public VectorStoreDataProperty(VectorStoreDataProperty source) |
There was a problem hiding this comment.
westey (@westey-m) please note this separate commit that also makes the record definition property types mutable.
We had this conversation previously in #10879, but since then we introduced the MEVD model, so the record definition is no longer referenced or used in any way after the model building step; that should make it safe to make it fully mutable, as it's only used during the construction of the collection (just like the option bags are only used during GetAsync, SearchAsync, etc).
Hopefully this all makes sense - if not let me know, I'm happy to pull this commit out of the PR and to have a separate discussion on it.
a085d34 to
2fde952
Compare
GitHub Actions
Inactive
2fde952 to
103a79f
Compare
GitHub Actions
Inactive
Adam Sitnik (adamsitnik)
left a comment
Adam Sitnik (adamsitnik)
left a comment
There was a problem hiding this comment.
I've reviewed each commit separately and overall it looks good to me. I've found few nits, feel free to address them before merging if you want to.
I know it's very subjective, but IMO 'VectorSearchOptions' was better name than RecordSearchOptions, but SimilaritySearchOptions would be even better.
Thank you Shay Rojansky (@roji) !
| /// Gets or sets a value indicating whether the key should be auto-generated by the vector store. | ||
| /// </summary> | ||
| public bool AutoGenerate { get; init; } | ||
| public bool AutoGenerate { get; set; } |
There was a problem hiding this comment.
I've just realized that the property was exposed, but we don't ever respect it (yet).
There was a problem hiding this comment.
Yeah, true - good catch... Maybe safer to remove it for now, until we have at least one implementation which actually makes use of it - simply because we're not 100% sure yet what the feature will look like... Made a note, will do that in a separate PR later. Will remove this.
| /// </summary> | ||
| public IReadOnlyList<VectorStoreProperty> Properties { get; init; } = s_emptyFields; | ||
| [AllowNull] | ||
| public IList<VectorStoreProperty> Properties |
There was a problem hiding this comment.
I still believe that we should expose just the list rather than an ability to provide your own collection. Anyway, it could be just a List here as we don't really benefit from exposing an abstraction (in case of IReadOnlyList it could be an array)
There was a problem hiding this comment.
I mainly did IList<T> to align with MEAI - see this comment. I don't really have strong feelings on this - if the user happens to have e.g. an array, IList allows that to be assigned directly (whereas List does not). Any specific thoughts/arguments for exposing List instead of IList?
(will go ahead and merge to move forward, but let's continue the conversation and I can always change it later)
IVectorSearch -> IVectorSearchable IKeywordHybridSearch -> IKeywordHybridSearchable
GetRecordOptions -> RecordRetrievalOptions GetFilteredRecordOptions -> FilteredRecordRetrievalOptions VectorSearchOptions -> RecordSearchOptions
Originally these were generic in order to support custom mappers, but these were removed.
This was proposed during API review in order to align the SearchAsync option bag with the GetAsync ones; on the GetAsync side we (now) have RecordRetrievalOptions and FilteredRecordRetrievalOptions, so on the SearchAsync side the proposal was to have RecordSearchOptions (the verb Search in MEVD implicitly means "similarity" or "vector" search). I can see where you're coming from though - VectorSearchOptions is possibly a little bit more clear. I'll go ahead and merge for now but I've taken a note to raise this again to see what people feel - we can always change it back. |
This especially makes sense now that we have a model that gets built based on the record definition (and attributes). Closes microsoft#10879
103a79f to
6d71500
Compare
GitHub Actions
Inactive
6d71500 to
3a260ea
Compare
GitHub Actions
Inactive
Note: this PR is based on top of #11879 - ignore first commit.This PR contains a pack of several unrelated changes from API review; each is quite simple on its own, so I submitted them as a single PR. I recommend reviewing this commit-by-commit.