Skip to content
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

relevanceThreshold is not utilized preventing additional memory items #851

Closed
habanoz opened this issue Mar 9, 2024 · 4 comments
Closed
Labels
bug Something isn't working triage

Comments

@habanoz
Copy link
Contributor

habanoz commented Mar 9, 2024

Describe the bug
ISemanticMemoryClientExtensions.SearchMemoryAsync does not utilize relevanceThreshold parameter provided. As a result, memory check for similar items always returns the first inserted item and subsequent items are not added to the memory.

To Reproduce
Steps to reproduce the behavior:
Chat using the UI and check for LongTermMemory and WorkingMemory. It does not accumulate.

Expected behavior
Working and Long term memory should accommodate as chat history expands.

Screenshots
image

@habanoz
Copy link
Contributor Author

habanoz commented Mar 9, 2024

Would you like me to generate a PR?

@evchaki evchaki added bug Something isn't working triage labels Mar 20, 2024
@glahaye
Copy link
Collaborator

glahaye commented Mar 25, 2024

@habanoz Hi! Thanks for reporting this problem,

Yes, if you have code that fixes the issue, please send in a PR.

@habanoz
Copy link
Contributor Author

habanoz commented Apr 6, 2024

@habanoz Hi! Thanks for reporting this problem,

Yes, if you have code that fixes the issue, please send in a PR.

PR #920 is sent to fix the situation.

github-merge-queue bot pushed a commit that referenced this issue Apr 8, 2024
… memory items (#920)

fix bug related to issue #851.
Unused parameter relevanceThreshold is passed to SearchAsync method to
ensure only relevant documents are fetched.

### Motivation and Context

Please help reviewers and future users, providing the following
information:
1. The code change is related because current code retrieves all memory
items regardless of similarity which prevents adding new memory items.
Check SemanticChatMemoryExtractor.ExtractCognitiveMemoryAsync.
2. The issue is only the first memory item is preserved. Additional
memory items are lost because they always seem to be similar to the
first memory item.
3. With this fix, a memory item is skipped only if there is similar
memory item in the memory. As a result distinct memory items will be
preserved.
  4. It fixes an open issue #851 .

### Description

The chance is simple, it utilizes unutilized relevanceThreshold
parameter in ISemanticMemoryClientExtensions.SearchAsync method.
SemanticChatMemoryExtractor.ExtractCognitiveMemoryAsync sends an upper
threshold to find similar items, but since it is not used all non
relevant memory items are returned.

### Contribution Checklist

- [X] The code builds clean without any errors or warnings
- [X] The PR follows the [Contribution
Guidelines](https://github.com/microsoft/chat-copilot/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/chat-copilot/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [?] All unit tests pass, and I have added new tests where possible. I
could not find any unit tests to run or extend. Please point out if
there are any tests relevant to memory retrieval.
- [X] I didn't break anyone 😄
@habanoz
Copy link
Contributor Author

habanoz commented Apr 10, 2024

PR is merged. Closing...

@habanoz habanoz closed this as completed Apr 10, 2024
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage
Projects
None yet
Development

No branches or pull requests

3 participants