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

RedisMemory #208

Merged
merged 2 commits into from
Jan 4, 2024
Merged

RedisMemory #208

merged 2 commits into from
Jan 4, 2024

Conversation

slorello89
Copy link
Contributor

@slorello89 slorello89 commented Dec 15, 2023

Motivation and Context (Why the change? What's the scenario?)

Implements #199

High level description (Approach, Design)

Adds Redis Vector Search as an IMemoryDb.

Couple of high-level notes:

  1. Tags in Redis need to be pre-defined if you want to filter on them. Also the way the Redis stores tags in a hash means you use a separator character to mark individual tags e.g. 'topics': ['tech', 'microsoft', 'redis'] would be stored in Redis as HSET key topics tech,microsoft,redis then when you search on the tag e.g. @topics:{redis} it will match. What this means from the user's perspective if that if they want to use tags they will need to predefined it before they create the index. I've added a bit to the dependency injection to note this. I also added as a blurb in the README (don't know if there's anywhere else to add those notes to socialize it better)
  2. Redis Vector Search uses outputs a cosinal distance rather than cosinal relevance (hence the weird 1 - distance in the post-filter)
  3. WRT compatibility - I wrote this against the latest Redis Search and Query library, so the items that are listed in the README (Azure Cache for Redis Enterprise tier, Redis Enterprise, Redis Cloud, Redis Stack).

I also added a new test project in line with the other console test projects I saw in here. There's really not too much to test at a unit level here so idk if there's anything else you want on that end.

Comment on lines 40 to 43
if (!this.Tags.TryAdd(tag.Key, tag.Value))
{
this.Tags[tag.Key] = tag.Value;
}

Choose a reason for hiding this comment

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

Maybe we could just use Tags[tag.Key] = tag.Value

Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it a dictionary rather than a list of tag names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WeihanLi - good point not sure why I didn't do that in the first place

@dluc - due to the Semantics of a tag field in Redis. Tag fields are separated by a separator character (default is a comma)

Hence you could have blog.tag tech,ai,redis,azure and match on tech, ai, redis, or azure. The dictionary is to give the user the flexibility to use fields with those tag separators in them. Might be worth adding a constructor with a list that just defaults everything to a comma.

@dluc
Copy link
Collaborator

dluc commented Dec 29, 2023

A couple of thoughts:

  • I would consider having a prefix for DBs used to store an index, so that DeleteIndex can not delete Redis DBs used for other data (see the same logic in Postgres connector)
  • can we normalize DB names similarly to Postgres or Azure AI Search, e.g. restrict the chars to [a-z0-9] + "dash", automatically converting some other common chars? (see Postgres and Azure AI Search connectors)

@dluc
Copy link
Collaborator

dluc commented Dec 29, 2023

I just added this folder: https://github.com/microsoft/kernel-memory/tree/main/extensions/Redis
Could you move all the code there? (see also comment in the initial GH issue). Thanks! :-)

@slorello89
Copy link
Contributor Author

Hi @dluc - FYI:

I just added this folder: https://github.com/microsoft/kernel-memory/tree/main/extensions/Redis
Could you move all the code there? (see also comment in the initial GH issue). Thanks! :-)

I've moved everything over to the new directory structure/project

I would consider having a prefix for DBs used to store an index, so that DeleteIndex can not delete Redis DBs used for other data (see the same logic in Postgres connector)

Done, I changed RedisConfig -> RedisIndexConfig and added AppPrefix to it (defaults to km). That AppPrefix is used as the prefix for whatever indexes you add to Redis.

can we normalize DB names similarly to Postgres or Azure AI Search, e.g. restrict the chars to [a-z0-9] + "dash", automatically converting some other common chars? (see Postgres and Azure AI Search connectors)

I did this, but I would at least consider moving this logic into a utility class in Abstractions and having all the extensions leverage the same normalizer. FWIW all strings in Redis are parameterized in RESP(Redis Serialization Protocol) and are binary safe, so you could call your indexes/key-names whatever.

inverting min-relevance for Redis

making Tags optional in configuration, updating README

changing to KNN from vector-range

adding tag escapes

cleanup

adding back NRedisStack

RedisMemoryConfiguration->RedisConfig

config updates

moving to seperate project/directory

adding packing

packageId

conn-string out of index config, RedisConfig -> RedisIndexConfig, Prefixes, normalized index names

fix conflicts

sealing RedisMemory

handling pre-existing index.

Making CreateIndex async

Code style, and changes to DI

Fix test project and test build settings
@microsoft microsoft deleted a comment from slorello89 Jan 4, 2024
@dluc
Copy link
Collaborator

dluc commented Jan 4, 2024

Merging - will continue on a separate PR

@dluc dluc merged commit 94250e6 into microsoft:main Jan 4, 2024
2 checks passed
@slorello89 slorello89 mentioned this pull request Jan 4, 2024
dluc pushed a commit that referenced this pull request Jan 4, 2024
## Motivation and Context (Why the change? What's the scenario?)
Couple follow on items from #208 

1. making vector algorithm configurable (defaulting to HNSW)
2. Updating Readme to explain the presence of Redis Insight
3. Changing tooling to launch redis-stack with Redis insight as well
4. throwing an exception when you attempt to add a tag with the defined
separator in it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants