Repository navigation
Python: Fix VolatileMemoryStore ignoring with_embeddings=False in get_batch and get_nearest_matches - #14524
Conversation
…_batch and get_nearest_matches
GitHub Actions
Inactive
GitHub Actions
Inactive
semantic-kernel-automation
Bot
added
the
python
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused fix correctly addresses the discarded-copy bug and includes appropriate regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes VolatileMemoryStore to honor with_embeddings=False without mutating stored records.
Changes:
- Deep-copies batch and nearest-match results before removing embeddings.
- Adds unit tests for both embedding modes and storage preservation.
| File | Description |
|---|---|
python/semantic_kernel/memory/volatile_memory_store.py |
Corrects embedding removal on returned copies. |
python/tests/unit/memory/test_volatile_memory_store_embeddings.py |
Tests embedding inclusion, exclusion, and persistence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Approved as-is and resolved my earlier loop-refactor request. Direct queue entry is blocked because two required checks are missing; automatic queue entry is enabled for when requirements are met. The old Merge Gatekeeper, .NET build, and Spell Check runs failed before producing checks, and GitHub refuses to rerun all three because they were created over a month ago. Please update/rebase this branch onto current main so fresh validation runs are created. |
…ction (microsoft#14536) ### Motivation and Context `VolatileMemoryStore.get_nearest_matches` raises `ValueError: cannot reshape array of size 0 into shape (0,newaxis)` when the collection exists but has no records. A search on a new or emptied collection should return no results, as it already does for a missing collection. Fixes microsoft#14534 ### Description Return `[]` early when the collection has no records, before the embeddings are stacked and reshaped. Added two tests: a freshly created collection, and a collection whose only record was removed. Both fail on `main` with the ValueError and pass with the fix. The tests are in a new file to avoid touching the files used by open PRs microsoft#14523 and microsoft#14524. Checks: ruff format and check clean, mypy clean on the changed file. ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄
GitHub Actions
Inactive
|
I updated this branch with current main (df1cb33), as you asked. I merged main in instead of force pushing, so the new head 3cbe389 is a normal merge commit. There were no conflicts and the PR diff is unchanged: 2 files, 53 lines added, 4 removed. On the pushed commit, tests/unit/memory gives 11 passed. With volatile_memory_store.py taken back to the main version, 2 of the 4 tests in test_volatile_memory_store_embeddings.py fail (get_batch and get_nearest_matches without embeddings). The new workflow runs for 3cbe389 show action_required, so a maintainer needs to approve them before they start. |
Motivation and Context
VolatileMemoryStore.get_batchandget_nearest_matchesdocumentedwith_embeddings=Falseas "do not include embeddings", but the code rebound a loop variable to adeepcopyand dropped it, so the original records (with embeddings) were returned. Fixes #14522.Description
Build the list of deep copies first and clear the embedding on the copies, so the stored records keep their embeddings. Added
tests/unit/memory/test_volatile_memory_store_embeddings.py(4 tests). Two of them fail on main and all pass with the change.Contribution Checklist