-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Propose vector-set API #2939
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
base: main
Are you sure you want to change the base?
Propose vector-set API #2939
Conversation
Additional thoughts on FastHash:
|
- misc FastHash core improvements
nits and "avoid the massive number of parameters" type decl: d53f1ab |
Brilliant, glad to see this landing as we look into vector sets in Garnet as well. |
@NickCraver @philon-msft bump |
In this vein, as I'm working on Garnet's Vector Set api support, I want to have a discussion about likely extensions. It seems quite possible that Redis will add new quantization options (to join It also seems reasonable for there to be new vector data formats for the commands (to join Obviously all these extensions could be handled with the Just spit-balling, but a struct (which wraps some /cc @NickCraver Since we discuss this a little bit this morning. |
Sounds interesting. We've moved to an args parameter for that API, so one option here might be to, at some future date, unseal it with a view to using some polymorphic "the subclass can fill this buffer with what it wants" API. I think this gives us any wriggle room for future changes. Thoughts? In particular, I wonder if I move the existing "by vector" bits into a subclass, presumably with a suitable "fill this" API. |
Lemme see if I follow. Current API for VADD is: bool VectorSetAdd(
RedisKey key,
RedisValue element,
ReadOnlyMemory<float> values,
int? reducedDimensions = null,
VectorSetQuantization quantization = VectorSetQuantization.Int8,
int? buildExplorationFactor = null,
int? maxConnections = null,
bool useCheckAndSet = false,
string? attributesJson = null,
CommandFlags flags = CommandFlags.None);
public enum VectorSetQuantization
{
Unknown,
None,
Int8,
Binary,
} So we'd change (or add as a new overload) that to something like: bool VectorSetAdd<TVectorData>(
RedisKey key,
RedisValue element,
TVectorData values,
int? reducedDimensions = null,
VectorSetQuantization quantization = VectorSetQuantization.Int8,
int? buildExplorationFactor = null,
int? maxConnections = null,
bool useCheckAndSet = false,
string? attributesJson = null,
CommandFlags flags = CommandFlags.None)
where TVectorData: IVectorData;
public readonly struct VectorSetQuantization(ReadOnlyMemory<byte> Name)
{
public static readonly VectorSetQuantization Unknown = new(default);
public static readonly VectorSetQuantization None = new("NOQUANT"u8);
public static readonly VectorSetQuantization Int8 = new("Q8"u8);
public static readonly VectorSetQuantization Binary = new("BIN"u8);
}
public interface IVectorData
{
// Some sort of write callback here?
} That makes sense to me. |
Sorry, I was only thinking about VSIM - my bad. It would seem desirable to unify the approach between VSIM and VADD, and maybe marry the data and quant encoding (understanding that there is some overlap). I wonder if we should take a leaf from System.Text.Encoding here. Suggestion:
Where both VSIM and VADD could take a TVector and a VectorEncoding-TVector Thoughts? |
Or are the quantization and encoding entirely independent? They don't feel entirely independent.... |
They are, though the feel is weird I agree. But quant ( Like, Some of this confusion is because |
OK. Right. I'll see if I can keep the two concepts separate, then. But there's definitely facility in abstraction - no point sending FP32 queries or data-loads to data that is quantized to FP8, so I can easily imagine that you'd allow an efficient transport syntax |
@kevin-montrose I looked at this this morning. I think there's a very strong likelihood of over-designing if we don't have concrete requirements, and in any event: we'd probably still want the default and most obvious API / overload to be the existing one. I think, for now, we can do a minimal change here that keeps the door open:
so: - [SER001]StackExchange.Redis.VectorSetSimilaritySearchRequest.Member.get -> StackExchange.Redis.RedisValue
- [SER001]StackExchange.Redis.VectorSetSimilaritySearchRequest.Member.set -> void
- [SER001]StackExchange.Redis.VectorSetSimilaritySearchRequest.Vector.get -> System.ReadOnlyMemory<float>
- [SER001]StackExchange.Redis.VectorSetSimilaritySearchRequest.Vector.set -> void
- [SER001]StackExchange.Redis.VectorSetSimilaritySearchRequest.VectorSetSimilaritySearchRequest() -> void
+ [SER001]static StackExchange.Redis.VectorSetSimilaritySearchRequest.ByMember(StackExchange.Redis.RedisValue member) -> StackExchange.Redis.VectorSetSimilaritySearchRequest!
+ [SER001]static StackExchange.Redis.VectorSetSimilaritySearchRequest.ByVector(System.ReadOnlyMemory<float> vector) -> StackExchange.Redis.VectorSetSimilaritySearchRequest! This means that in the future, we can add whatever other factory methods we need to support additional encodings, and we haven't forced On the "add" side: we can just overload trivially. I'm adding this as a commit - feedback encouraged before I merge ;p (the actual API to allow extensibility is |
- make VectorSetSimilaritySearchRequest abstract - remove Member and Vector - add ByMember and ByVector factory methods - (internal changes to support the above, in the message etc)
API change formalized ^^^ |
I'm fine being conservative until we have the final Garnet (or others) implementation to compare against (and obviously, consult about), and that seems reasonable. Though this still leaves the question of |
/// <summary> | ||
/// Binary quantization. This maps to "BIN" or "bin". | ||
/// </summary> | ||
Binary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, let's add int values to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed doing an object for VectorSetAdd
for future additions, otherwise looking good 👍
@kevin-montrose see Nick's comment above: working exactly that |
as per https://redis.io/docs/latest/develop/data-types/vector-sets/
VectorSet...
VLINKS
; the server returns this as nested data, but the nesting is not meaningful to the caller (instead being related to the server core), so I've flattened the resultVectorSetIntegrationTests
, in particularVectorSetSimilaritySearch_WithFilter
is useful for overview - since the main two primary APIs areVADD
andVSIM
Lease<T>
rather thanT[]
. Inputs areReadOnlyMemory<T>
; the exception to this isstring
data for JSON and filters; these are explicitly text, never blobs, soRedisValue
seems inappropriate. I think forcingstring
is OK hereVectorSet*
bits out viapartial
files; I will follow this up with "everything else" (.Strings.cs, .Hashes.cs) in a separate PRThis PR also introduces the start of a new literal-matching API, ala
FastHash
. It is intended that this will be extended at a later date.CI: may be dependent on the
vectorset
module; if it fails, I'll add suitable validation. AllVectorSetTests
pass locally: