Add scalar quantization support for Fp16 vector encoding - #16473
Conversation
Pulkitg64
changed the title
msokolov
left a comment
msokolov
left a comment
There was a problem hiding this comment.
overall looks good: i just have some small comments and questions
| // This is asymmetric quantization, we will pack the vector | ||
| targetQuantized = new byte[scalarEncoding.getQueryPackedLength(scratch.length)]; | ||
| } | ||
| // Inflate the fp16 query to fp32 and normalize there; quantization operates on fp32. |
There was a problem hiding this comment.
Do we want to add a TODO: linking to an issue for implementing quantization directly over fp16?
There was a problem hiding this comment.
BTW I just saw https://opensearch.org/blog/accelerating-fp16-vector-search-performance-using-bulk-simd-in-opensearch-3-5/ maybe there is some goodness there we can incorporate?
There was a problem hiding this comment.
Do we want to add a TODO:
Created this issue: #16533
maybe there is some goodness there we can incorporate?
Thanks Mike, I will check how are they doing the quantization with Fp16
| targetQuantized = new byte[scalarEncoding.getQueryPackedLength(scratch.length)]; | ||
| } | ||
| // Inflate the fp16 query to fp32 and normalize there; quantization operates on fp32. | ||
| float[] copy = new float[target.length]; |
There was a problem hiding this comment.
this should all be a copy of the logic above in float[] case right? can we factor out into a utility method?
| @Override | ||
| public RandomVectorScorer getRandomVectorScorer(String field, short[] target) throws IOException { | ||
| return rawVectorsReader.getRandomVectorScorer(field, target); | ||
| FieldEntry fi = fields.get(field); |
There was a problem hiding this comment.
again, can we DRY this up?
| * | ||
| * <p>Used for read-only indexes whose raw float16 vectors have been dropped to save storage: only | ||
| * the scalar-quantized bytes remain, so {@link #vectorValue(int)} reconstructs float16 values by | ||
| * dequantizing them, with some precision loss. |
There was a problem hiding this comment.
the precision loss is relative to the original fp16 vectors I guess, not relative to the quantized vectors. Maybe just add "relative to the original fp16 vectors" to be explicit
| this.correctiveValues = new float[3]; | ||
| this.encoding = encoding; | ||
| int docPackedLength = encoding.getDocPackedLength(dimension); | ||
| this.byteSize = docPackedLength + (Float.BYTES * 3) + Integer.BYTES; |
There was a problem hiding this comment.
what is this calculation about?
There was a problem hiding this comment.
hmm I guess we must read corrective values, and ... an int
| } | ||
| } | ||
| @Override | ||
| public T copyValue(T vectorValue) { |
There was a problem hiding this comment.
No it existed before as well. We just made it generic too and the position got changed.
| @Override | ||
| public void addValue(int docID, float[] vectorValue) throws IOException { | ||
| flatFieldVectorsWriter.addValue(docID, vectorValue); | ||
| /** The ordinal's stored vector as fp32, ready for quantization (unit-length for COSINE). */ |
There was a problem hiding this comment.
I think it will also be expected to be unit-length for DOT_PRODUCT? It's just not guaranteed
There was a problem hiding this comment.
adding in comments
|
|
||
| @Override | ||
| float[] floatVectorValue(int ord) { | ||
| float[] vector = flatFieldVectorsWriter.getVectors().get(ord); |
There was a problem hiding this comment.
I wonder if we should be normalizing "on the way in" -- in addValue?
There was a problem hiding this comment.
I guess I don't really care about stupid COSINE
| String fieldName = "field"; | ||
| int numVectors = random().nextInt(99, 500); | ||
| int dims = random().nextInt(4, 65); | ||
| if (dims % 2 == 1) { |
There was a problem hiding this comment.
or 2 * random().nextInt(2, 33)?
| /** | ||
| * fp16 counterpart of {@link #testQuantizedVectorsWriteAndRead()}: indexes float16 vectors and | ||
| * verifies the persisted quantized bytes + corrective terms match a reference re-quantization. | ||
| * The reference mirrors the writer's fp16 path exactly — inflate fp16->fp32 (normalizing |
There was a problem hiding this comment.
wait! I see the dreaded mdash! Did AI write this??!
There was a problem hiding this comment.
Yes, I asked AI to replicate existing fp32 methods and test cases. Sorry for the ignoring the comments here.
There was a problem hiding this comment.
oh funny, it actually seemed fine and I thought you wrote it. I was just poking fun at the emdash
| } | ||
| }; | ||
| return getRandomQuantizedVectorScorer( | ||
| similarityFunction, qv, ArrayUtil.copyOfSubArray(target, 0, target.length)); |
There was a problem hiding this comment.
We have copyArray as sugar for this
There was a problem hiding this comment.
Sure Mike, fixed in next revision.
|
Thank you Mike for all the feedbacks. |
2 participants
Description
As part of this PR (#16383) we added support for FP16 vector encoding. This PR target support for adding scalar quantization to it.