Change visibility modifiers on MemoryRecord and associated collection classes#250
Merged
adrianwyatt merged 3 commits intomicrosoft:mainfrom Mar 31, 2023
Merged
Conversation
…memory collections
dmytrostruk
previously approved these changes
Mar 31, 2023
Member
dmytrostruk
left a comment
There was a problem hiding this comment.
There are a couple of inline GitHub Actions warnings which need to be fixed, but in general LGTM!
SergeyMenshykh
approved these changes
Mar 31, 2023
adrianwyatt
approved these changes
Mar 31, 2023
dehoward
pushed a commit
to lemillermicrosoft/semantic-kernel
that referenced
this pull request
Jun 1, 2023
… classes (microsoft#250) ### Motivation and Context `MemoryRecord`, along with several useful classes in the `SemanticKernel.Memory.Collections` namespace currently have an internal visibility modifier. This prevents 3rd party libraries from reusing code when creating implementations of `IEmbeddingWithMetadata<float>`, which is needed in implementations of `IMemoryStore<TEmbedding>` Two implementations are currently blocked on this: * Skills.Memory.Sqlite * Skills.Memory.CosmosDB As both of these backing stores are not vector DB stores, they implement cosine similarity comparisons locally, which is performed using `TopNCollection` and its dependencies, all of which are currently internal. Related community feedback item here: microsoft#202 ### Description Change the following classes from internal to public: * MemoryRecord * MinHeap * Score * ScoredValue * TopNCollection
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
MemoryRecord, along with several useful classes in theSemanticKernel.Memory.Collectionsnamespace currently have an internal visibility modifier.This prevents 3rd party libraries from reusing code when creating implementations of
IEmbeddingWithMetadata<float>, which is needed in implementations ofIMemoryStore<TEmbedding>Two implementations are currently blocked on this:
As both of these backing stores are not vector DB stores, they implement cosine similarity comparisons locally, which is performed using
TopNCollectionand its dependencies, all of which are currently internal.Related community feedback item here: #202
Description
Change the following classes from internal to public:
Contribution Checklist
dotnet format