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

Python: Add additional_metadata field to MemoryRecord and address TODOs in ChromaMemoryStore #1323

Merged

Conversation

awharrison-28
Copy link
Contributor

Motivation and Context

Closes #1271 and addresses several TODOs in ChromaMemoryStore

Description

  • added field additional_metadata to MemoryRecord and all downstream methods/classes (see SemanticTextMemoryBase, SemanticTextMemory, MemoryQueryResult, and ChromaMemoryStore)
  • addressed ChromaMemoryStore TODO 'what is key used for?' For chromadb, key and id are effectively the same value, but the two fields are differentiated for memory stores like Qdrant which require that key be a uuid. I updated the logic so that this is more apparent. Note that this results in no change in end-user functionality.
  • Updated ChromaMemoryStore TODO 'get_async say embedding is optional but Record constructor requires it.` MemoryRecord constructor no longer requires an embedding. Logic is updated and a test added to show that embedding retrieval is functionally optional.

Contribution Checklist

@github-actions github-actions bot added the python Pull requests for the Python Semantic Kernel label Jun 2, 2023
alexchaomander
alexchaomander previously approved these changes Jun 5, 2023
Copy link
Contributor

@alexchaomander alexchaomander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alexchaomander alexchaomander added the PR: ready for review All feedback addressed, ready for reviews label Jun 5, 2023
@dluc dluc merged commit 6cbea85 into microsoft:main Jun 7, 2023
26 checks passed
@dluc dluc mentioned this pull request Jun 12, 2023
dluc added a commit that referenced this pull request Jun 12, 2023
* [e4781e5] Add sample notebook to demo weaviate memory store (#1359)
* [dae1c16] Python: Added examples of using ChatCompletion models for
skill building in Jupyter Notebooks (#1242)
* [f4e92eb] fix: Add Azure OpenAI support for
python/08-native-function-inline (#1365)
* [de74668] Fixing typos (#1377)
* [67aa732] Python: Fix weaviate integration tests (#1422)
* [f60d7ba] Fix functions_view.py (#1213)
* [b2e1548] Python: Multiple results per prompt (incl. streaming)
(#1316)
* [4c4670a] Using dotenv instead of parsing keys ourselves (#1295)
* [05d9e72] Python: Sync pyproject.toml with requirements.txt (#1150)
* [6cbea85] Python: Add additional_metadata field to MemoryRecord and
address TODOs in ChromaMemoryStore (#1323)
* [8947e68] Weaviate: Fix to be compatible with python 3.8 (#1349)
shawncal pushed a commit to shawncal/semantic-kernel that referenced this pull request Jul 6, 2023
…Os in ChromaMemoryStore (microsoft#1323)

### Motivation and Context
Closes microsoft#1271 and addresses several TODOs in ChromaMemoryStore


### Description
- added field `additional_metadata` to `MemoryRecord` and all downstream
methods/classes (see `SemanticTextMemoryBase`, `SemanticTextMemory`,
`MemoryQueryResult`, and `ChromaMemoryStore`)
- addressed ChromaMemoryStore TODO 'what is key used for?' For chromadb,
key and id are effectively the same value, but the two fields are
differentiated for memory stores like Qdrant which require that key be a
uuid. I updated the logic so that this is more apparent. Note that this
results in no change in end-user functionality.
- Updated ChromaMemoryStore TODO 'get_async say embedding is optional
but Record constructor requires it.` MemoryRecord constructor no longer
requires an embedding. Logic is updated and a test added to show that
embedding retrieval is functionally optional.
shawncal pushed a commit to shawncal/semantic-kernel that referenced this pull request Jul 6, 2023
* [e4781e5] Add sample notebook to demo weaviate memory store (microsoft#1359)
* [dae1c16] Python: Added examples of using ChatCompletion models for
skill building in Jupyter Notebooks (microsoft#1242)
* [f4e92eb] fix: Add Azure OpenAI support for
python/08-native-function-inline (microsoft#1365)
* [de74668] Fixing typos (microsoft#1377)
* [67aa732] Python: Fix weaviate integration tests (microsoft#1422)
* [f60d7ba] Fix functions_view.py (microsoft#1213)
* [b2e1548] Python: Multiple results per prompt (incl. streaming)
(microsoft#1316)
* [4c4670a] Using dotenv instead of parsing keys ourselves (microsoft#1295)
* [05d9e72] Python: Sync pyproject.toml with requirements.txt (microsoft#1150)
* [6cbea85] Python: Add additional_metadata field to MemoryRecord and
address TODOs in ChromaMemoryStore (microsoft#1323)
* [8947e68] Weaviate: Fix to be compatible with python 3.8 (microsoft#1349)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready for review All feedback addressed, ready for reviews python Pull requests for the Python Semantic Kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add metadata as a field in the MemoryRecord class in Python
4 participants