Skip to content

Send half_float vectors to the remote index build service - #3608

Open
ManasviGoyal wants to merge 3 commits into
opensearch-project:mainfrom
ManasviGoyal:send-fp16-to-gpu
Open

ManasviGoyal wants to merge 3 commits into
opensearch-project:mainfrom
ManasviGoyal:send-fp16-to-gpu

Conversation

@ManasviGoyal

@ManasviGoyal ManasviGoyal commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Description

Upload half_float vectors to the remote index build service as fp16 (2 bytes per dimension) instead of fp32.

Today the data node expands half_float vectors to fp32 for upload and the service converts them back to fp16 before building on the GPU, which takes fp16 natively. Twice the bytes to S3 and a conversion pass for nothing.

The upload stream now encodes each half_float vector to fp16 with the existing SIMD encoder, and the size estimates that padded half_float to 4 bytes per dimension use the real 2 bytes. Only the half_float field type changes; float with the fp16 scalar quantizer still uploads fp32. Request format and search path are unchanged.

Requires opensearch-project/remote-vector-index-builder#155 deployed first as an older service misreads fp16 blobs as fp32.

Docker image tag changed from 'api-latest' to 'api-snapshot' for apply remote-vector-index-builder changes.

Related Issues

Resolves #3607

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit ea431df)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Service version coupling

The data node now encodes half_float vectors to fp16 bytes before upload, but older remote-vector-index-builder service versions interpret these bytes as fp32 (as documented in the PR description). There is no client-side version negotiation or guard preventing this new client from sending fp16 blobs to an older service. If the required service version (opensearch-project/remote-vector-index-builder#155) is not deployed before this change rolls out, builds for half_float fields will silently produce incorrect indices. Consider documenting a deployment ordering requirement or adding a service-capability check.

private void reloadBuffer() throws IOException {
    currentBuffer.clear();
    if (vectorDataType == FLOAT) {
        float[] floatVector = ((KNNFloatVectorValues) knnVectorValues).getVector();
        currentBuffer.asFloatBuffer().put(floatVector);
    } else if (vectorDataType == HALF_FLOAT) {
        // Encoded to fp16 (2 bytes per dimension) on the data node; the remote build service consumes fp16
        // directly. This is the HALF_FLOAT field type only - a FLOAT field with the fp16 SQ encoder is
        // still uploaded as raw fp32 and converted by the service.
        float[] floatVector = ((KNNHalfFloatVectorValues) knnVectorValues).getVector();
        KNNVectorAsCollectionOfHalfFloatsSerializer.INSTANCE.floatToByteArray(floatVector, halfFloatVectorBytes, floatVector.length);
        currentBuffer.put(halfFloatVectorBytes);
    } else if (vectorDataType == BYTE) {
        byte[] byteVector = ((KNNByteVectorValues) knnVectorValues).getVector();
        currentBuffer.put(byteVector);
    } else if (vectorDataType == BINARY) {

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

PR Code Suggestions ✨

Latest suggestions up to ea431df

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Size the fp16 buffer explicitly by dimension

For HALF_FLOAT, knnVectorValues.bytesPerVector() already returns dimension *
Short.BYTES (the fp16 on-disk size), so bytesPerVector is correct. However,
halfFloatVectorBytes is sized to bytesPerVector (fp16 size) while
KNNVectorAsCollectionOfHalfFloatsSerializer.floatToByteArray is called with
floatVector.length dimensions - verify this buffer size matches the serializer's
expected output length (2 bytes per dimension), otherwise an
ArrayIndexOutOfBoundsException will occur in reloadBuffer.

src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/VectorValuesInputStream.java [73-74]

 this.bytesPerVector = this.knnVectorValues.bytesPerVector();
-this.halfFloatVectorBytes = vectorDataType == HALF_FLOAT ? new byte[bytesPerVector] : null;
+// fp16 encoding produces 2 bytes per dimension; ensure buffer is sized accordingly
+this.halfFloatVectorBytes = vectorDataType == HALF_FLOAT ? new byte[this.knnVectorValues.dimension() * Short.BYTES] : null;
 // We use currentBuffer == null to indicate that there are no more vectors to be read
 this.currentBuffer = ByteBuffer.allocate(bytesPerVector).order(ByteOrder.LITTLE_ENDIAN);
Suggestion importance[1-10]: 2

__

Why: For HALF_FLOAT, bytesPerVector() already returns dimension * Short.BYTES, so the existing allocation is equivalent to the suggested change. The suggestion offers only a verification/clarity benefit without fixing an actual bug.

Low

Previous suggestions

Suggestions up to commit f42efd3
CategorySuggestion                                                                                                                                    Impact
General
Verify fp16 byte order matches service expectation

The currentBuffer is allocated with LITTLE_ENDIAN byte order, but floatToByteArray
likely writes fp16 shorts in a fixed order (typically via ByteBuffer or
Short.reverseBytes). Verify that the serializer's byte order matches what the remote
build service expects; a mismatch here would silently corrupt every half_float
vector sent remotely. Add an explicit test asserting the on-wire byte order of a
known fp16 value.

src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/VectorValuesInputStream.java [215-217]

+float[] floatVector = ((KNNHalfFloatVectorValues) knnVectorValues).getVector();
+KNNVectorAsCollectionOfHalfFloatsSerializer.INSTANCE.floatToByteArray(floatVector, halfFloatVectorBytes, floatVector.length);
+currentBuffer.put(halfFloatVectorBytes);
 
-
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about byte order correctness but only asks for verification and provides identical existing_code and improved_code. Its impact is limited to a verification request rather than a concrete fix.

Low
Suggestions up to commit 7fe9fb9
CategorySuggestion                                                                                                                                    Impact
Possible issue
Verify endianness of fp16 encoded bytes

The currentBuffer was allocated with LITTLE_ENDIAN byte order, but
KNNVectorAsCollectionOfHalfFloatsSerializer.floatToByteArray may encode fp16 shorts
using a different (likely big-endian, via ByteBuffer.allocate default) byte order.
This mismatch means the remote build service will receive incorrectly-ordered fp16
bytes. Verify the serializer's endianness matches what the remote service expects
(little-endian) and align it explicitly.

src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/VectorValuesInputStream.java [215-217]

 float[] floatVector = ((KNNHalfFloatVectorValues) knnVectorValues).getVector();
 KNNVectorAsCollectionOfHalfFloatsSerializer.INSTANCE.floatToByteArray(floatVector, halfFloatVectorBytes, floatVector.length);
+// Ensure halfFloatVectorBytes is little-endian encoded to match currentBuffer's byte order and remote service expectation
 currentBuffer.put(halfFloatVectorBytes);
Suggestion importance[1-10]: 5

__

Why: The concern about endianness mismatch between the fp16 serializer output and the remote service's expected format is valid to consider, but the suggestion only asks to verify without providing a concrete fix, and the improved_code is essentially identical to existing_code with just an added comment.

Low
Suggestions up to commit 5df8439
CategorySuggestion                                                                                                                                    Impact
General
Guard fixed-size fp16 encode buffer

The reused halfFloatVectorBytes buffer is sized once from bytesPerVector at
construction. If KNNHalfFloatVectorValues#getVector() ever returns a vector whose
length differs from the initial dimension() (or if the returned array is longer than
dimension), floatToByteArray could either write past halfFloatVectorBytes or
under-fill currentBuffer. Passing floatVector.length while the byte buffer is
fixed-size makes this fragile; either use a dimension constant or guard with an
assertion.

src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/VectorValuesInputStream.java [211-217]

 } else if (vectorDataType == HALF_FLOAT) {
     // Encoded to fp16 (2 bytes per dimension) on the data node; the remote build service consumes fp16
     // directly. This is the HALF_FLOAT field type only - a FLOAT field with the fp16 SQ encoder is
     // still uploaded as raw fp32 and converted by the service.
     float[] floatVector = ((KNNHalfFloatVectorValues) knnVectorValues).getVector();
+    assert floatVector.length * Short.BYTES == halfFloatVectorBytes.length;
     KNNVectorAsCollectionOfHalfFloatsSerializer.INSTANCE.floatToByteArray(floatVector, halfFloatVectorBytes, floatVector.length);
     currentBuffer.put(halfFloatVectorBytes);
Suggestion importance[1-10]: 3

__

Why: The suggestion adds a defensive assertion but doesn't address a real bug, as vector dimensions are consistent across a field. It's a minor robustness improvement with limited impact.

Low

@codecov

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.10%. Comparing base (7318951) to head (ea431df).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3608      +/-   ##
============================================
+ Coverage     83.08%   83.10%   +0.01%     
  Complexity     5013     5013              
============================================
  Files           489      489              
  Lines         17793    17783      -10     
  Branches       2399     2396       -3     
============================================
- Hits          14783    14778       -5     
+ Misses         2103     2101       -2     
+ Partials        907      904       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit f41bd45

@navneet1v

Copy link
Copy Markdown
Collaborator

@ManasviGoyal please check the CI failures.

@ManasviGoyal

Copy link
Copy Markdown
Contributor Author

@ManasviGoyal please check the CI failures.

@navneet1v the remote vector changes need to be merged first and then we need to republish the api-latest for the CIs to pass

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 7fe9fb9

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit f42efd3.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

PathLineSeverityDescription
.github/workflows/remote_index_build.yml88highDocker image tag changed from 'api-latest' to 'api-snapshot' for opensearchstaging/remote-vector-index-builder. Per mandatory rule, all container image changes must be flagged. The 'api-snapshot' tag typically denotes an unstable/development build and may not have the same provenance guarantees as 'api-latest'. Maintainers should verify this image tag is intentional, points to a trusted artifact, and has been reviewed by the OpenSearch team.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@navneet1v navneet1v added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label Oct 1, 2026
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit f42efd3

Signed-off-by: Manasvi Goyal <mg.manasvi@gmail.com>
Signed-off-by: Manasvi Goyal <mg.manasvi@gmail.com>
Signed-off-by: Manasvi Goyal <mg.manasvi@gmail.com>
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit ea431df

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Upload half_float vectors to the remote index build service as fp16 instead of fp32

2 participants