-
Notifications
You must be signed in to change notification settings - Fork 31
Developer microsoft memory kernel #160
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
Conversation
Optimize the VectorDB class in the Senparc.AI.Interfaces class Debug Embedding Function Content
Optimize Problem
…Senparc/Senaprc.AI into Developer-MicrosoftMemoryKernel
[2025-11-01] v0.24.4 Combine VectorDBType from different projects
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.
Pull Request Overview
This PR consolidates VectorDB type enumerations from different projects into a single unified definition. The main purpose is to simplify the codebase by combining VectorDBType enumerations and updating the vector store configuration logic to use the consolidated enum.
Key changes:
- Replaced the extensive VectorDBType enum (with 19 values) with a simplified enum containing 9 basic types (Memory, HardDisk, Redis, Mulivs, Chroma, PostgreSQL, Sqlite, SqlServer, Qdrant)
- Updated switch-case statements in KernelConfigExtensions to map to the new enum values
- Removed redundant code and unused SenparcSetting parameter from sample application
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Senparc.AI.csproj | Updated version to 0.24.4 and added release note for VectorDBType consolidation |
| ISenparcAiSetting.cs | Replaced the complex VectorDBType enum (19 values) with a simplified version (9 values), commented out the old enum |
| Enums.cs | Commented out the duplicate VectorDBType enum definition |
| Senparc.AI.Kernel.csproj | Updated version to 0.25.3 |
| KernelConfigExtensions.cs | Refactored switch statements to use new VectorDBType values, added using statement for VectorDB |
| Senparc.AI.Agents.csproj | Updated version to 0.5.7 |
| Program.cs | Removed unused SenparcSetting initialization and parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Senparc.AI.Kernel/KernelConfigExtensions/KernelConfigExtensions.cs
Outdated
Show resolved
Hide resolved
src/Senparc.AI.Kernel/KernelConfigExtensions/KernelConfigExtensions.cs
Outdated
Show resolved
Hide resolved
src/Senparc.AI.Kernel/KernelConfigExtensions/KernelConfigExtensions.cs
Outdated
Show resolved
Hide resolved
src/Senparc.AI.Kernel/KernelConfigExtensions/KernelConfigExtensions.cs
Outdated
Show resolved
Hide resolved
| case VectorDB.VectorDBType.CosmosDBMongoDB: | ||
| case VectorDBType.Memory: | ||
| { | ||
| vectorStore = new InMemoryVectorStore(); |
Copilot
AI
Nov 1, 2025
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.
Disposable 'InMemoryVectorStore' is created but not disposed.
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.
@copilot open a new pull request to apply changes based on this feedback
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.
@copilot open a new pull request to apply changes based on this feedback
src/Senparc.AI.Kernel/KernelConfigExtensions/KernelConfigExtensions.cs
Outdated
Show resolved
Hide resolved
…sions.cs Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…sions.cs Co-authored-by: Copilot <[email protected]>
…sions.cs Co-authored-by: Copilot <[email protected]>
…sions.cs Co-authored-by: Copilot <[email protected]>
…sions.cs Co-authored-by: Copilot <[email protected]>
|
@JeffreySu I've opened a new pull request, #161, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@JeffreySu I've opened a new pull request, #162, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@JeffreySu I've opened a new pull request, #163, to work on those changes. Once the pull request is ready, I'll request review from you. |
…sposal Co-authored-by: JeffreySu <[email protected]>
…ment Co-authored-by: JeffreySu <[email protected]>
Wrap disposable vector stores in using statements to prevent resource leaks
Fix VectorStore lifecycle management and remove premature disposal
|
@JeffreySu I've opened a new pull request, #164, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: JeffreySu <[email protected]>
[WIP] Update based on feedback from PR #160
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.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // JDBC=8, | ||
| // Milvus=9, //Planed (not included in https://learn.microsoft.com/en-us/semantic-kernel/concepts/vector-store-connectors/out-of-the-box-connectors/inmemory-connector?pivots=programming-language-csharp) | ||
| // MongoDB=10, | ||
| // Pinecon=11, |
Copilot
AI
Nov 1, 2025
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.
Possible typo: 'Pinecon' should likely be 'Pinecone' (the vector database name) in commented code.
| // Pinecon=11, | |
| // Pinecone=11, |
| case VectorDB.VectorDBType.CosmosDBMongoDB: | ||
| case VectorDBType.Memory: | ||
| { | ||
| vectorStore = new InMemoryVectorStore(); |
Copilot
AI
Nov 1, 2025
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.
Disposable 'InMemoryVectorStore' is created but not disposed.
| vectorStore = new RedisVectorStore(database, | ||
| new() { StorageType = RedisStorageType.Json }); | ||
| collection = vectorStore.GetCollection<TKey, TRecord>(name, vectorStoreRecordDefinition); |
Copilot
AI
Nov 1, 2025
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.
Disposable 'RedisVectorStore' is created but not disposed.
| vectorStore = new RedisVectorStore(database, | |
| new() { StorageType = RedisStorageType.Json }); | |
| collection = vectorStore.GetCollection<TKey, TRecord>(name, vectorStoreRecordDefinition); | |
| using (var redisVectorStore = new RedisVectorStore(database, | |
| new() { StorageType = RedisStorageType.Json })) | |
| { | |
| collection = redisVectorStore.GetCollection<TKey, TRecord>(name, vectorStoreRecordDefinition); | |
| } |
No description provided.