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

Add metadata as a field in the MemoryRecord class in Python #1271

Closed
cschadewitz opened this issue May 30, 2023 · 6 comments · Fixed by #1323
Closed

Add metadata as a field in the MemoryRecord class in Python #1271

cschadewitz opened this issue May 30, 2023 · 6 comments · Fixed by #1323
Assignees
Labels
memory connector python Pull requests for the Python Semantic Kernel

Comments

@cschadewitz
Copy link
Contributor

Add metadata as a field in the python memory record class.
This would improve parity between the implementations.

The field would be a dict[str, str | int | bool | list[str]] this would allow for a flexible usage of the field and would mesh well with Pinecone's db record metadata implementation and could be stored in other dbs as Json or JSONb

Furthermore, the text, description and other such fields on the python memory record class could be moved to this dict as well similar to the .net Memory Record class

@cschadewitz cschadewitz changed the title Add metadata as a field in the MemoryRecord class in python Add metadata as a field in the MemoryRecord class in Python May 30, 2023
@evchaki evchaki added python Pull requests for the Python Semantic Kernel memory connector labels May 30, 2023
@alexchaomander
Copy link
Contributor

Good catch! We will add. This is already in C# so Python needs to catch up

@cschadewitz
Copy link
Contributor Author

cschadewitz commented May 31, 2023

I see that you assigned to @awharrison-28 but if youd like I would be happy to work on this along side the pinecone memory store implementation that I have taken over and the postgres memory store implementation that I have all but finished.

Alternatively, I would also be happy to just collaborate.

Either way, I'll prep branches for changes against those memory stores to go in after this feature.

@awharrison-28
Copy link
Contributor

@cschadewitz thanks for the offer. This is a really quick one to put out, so I'll take care of it asap.

@awharrison-28
Copy link
Contributor

@cschadewitz PR is live, please take a look to make sure it covers what you need for Pinecone and Postgres :)

@cschadewitz
Copy link
Contributor Author

@awharrison-28 I'll take a look this evening and let you know if I have any suggestions. Thanks for getting this in place so quickly!

@cschadewitz
Copy link
Contributor Author

@awharrison-28, sorry it took so long for me to get to this but it looks good to me! Thanks again!

@dluc dluc closed this as completed in #1323 Jun 7, 2023
dluc pushed a commit that referenced this issue Jun 7, 2023
…Os in ChromaMemoryStore (#1323)

### 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.
shawncal pushed a commit to shawncal/semantic-kernel that referenced this issue 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
memory connector python Pull requests for the Python Semantic Kernel
Projects
None yet
4 participants