Fix KFS multi-dimensional string tensor deserialization#4358
Open
atobiszei wants to merge 6 commits into
Open
Conversation
Both convertStringRequestToOVTensor (contents path) and
convertBinaryExtensionStringFromBufferToNativeOVTensor (raw_input_contents path)
hardcoded a 1D ov::Shape{N}, ignoring the shape field in the request.
For models with string inputs of rank > 1 (e.g. [batch, seq_len]) this caused
inference to fail with:
'model input (shape=[?,?]) and the tensor (shape=(N)) are incompatible'
Fix: read shape from request, use it when its product matches element count;
fall back to 1D otherwise to preserve backward compatibility.
Add two new unit tests (StringInputsConversionKFSTest):
- native_ov_string_2d_shape (contents path)
- rawInputContents_native_ov_string_2d_shape (raw_input_contents path)
Resolves CVS-187884
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes KFS string tensor deserialization so multi-dimensional request shapes are preserved (when consistent with element count), preventing inference failures for models with >1D string inputs.
Changes:
- Read and apply
shapefromKFSRequest::InferInputTensorfor string inputs (contents +raw_input_contents) when shape product matches the parsed element count. - Fall back to 1D shape on mismatch to preserve backward compatibility.
- Add unit tests verifying 2D string tensor shape handling for both KFS paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/test/tensor_conversion_test.cpp | Adds KFS-focused unit tests validating 2D string shape handling for both contents and raw buffer paths. |
| src/kfs_frontend/tensor_conversion.cpp | Updates KFS string conversion to honor request-provided shapes (with mismatch fallback). |
| src/kfs_frontend/kfs_utils.cpp | Updates raw buffer string conversion to honor request-provided shapes (with mismatch fallback). |
Comment on lines
+70
to
+85
| int numElements = getBinaryInputsSize(src); | ||
| // Build ov::Shape from request shape, preserving multi-dimensional shapes (e.g. [1,5]). | ||
| ov::Shape shape; | ||
| if (src.shape_size() > 0) { | ||
| size_t shapeElements = 1; | ||
| for (int i = 0; i < src.shape_size(); i++) { | ||
| shape.push_back(static_cast<size_t>(src.shape().at(i))); | ||
| shapeElements *= static_cast<size_t>(src.shape().at(i)); | ||
| } | ||
| if (static_cast<int>(shapeElements) != numElements) { | ||
| SPDLOG_DEBUG("String input shape product {} != contents size {}; falling back to 1D", shapeElements, numElements); | ||
| shape = ov::Shape{static_cast<size_t>(numElements)}; | ||
| } | ||
| } else { | ||
| shape = ov::Shape{static_cast<size_t>(numElements)}; | ||
| } |
Comment on lines
+311
to
+323
| if (src.shape_size() > 0) { | ||
| size_t shapeElements = 1; | ||
| for (int i = 0; i < src.shape_size(); i++) { | ||
| shape.push_back(static_cast<size_t>(src.shape().at(i))); | ||
| shapeElements *= static_cast<size_t>(src.shape().at(i)); | ||
| } | ||
| if (shapeElements != batchSize) { | ||
| SPDLOG_DEBUG("Input string shape mismatch: shape product {} != parsed string count {}", shapeElements, batchSize); | ||
| shape = ov::Shape{batchSize}; | ||
| } | ||
| } else { | ||
| shape = ov::Shape{batchSize}; | ||
| } |
Add forward declaration of inference::ModelInferRequest_InferInputTensor and explicit specialization declaration for convertStringRequestToOVTensor in tensor_conversion.hpp so TUs that include the header suppress primary template instantiation for KFSRequest::InferInputTensor instead of using the old 1D-only body.
atobiszei
force-pushed
the
atobisze_fix_2d_string
branch
from
07b395e to
0628365
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both convertStringRequestToOVTensor (contents path) and convertBinaryExtensionStringFromBufferToNativeOVTensor (raw_input_contents path) hardcoded a 1D ov::Shape{N}, ignoring the shape field in the request.
For models with >1 string inputs this caused inference to fail.
Fix: read shape from request, use it when its product matches element count.
Add two new unit tests (StringInputsConversionKFSTest):
Problem originally detected on TFS API but since it is being removed it appears fix is needed for KFS as well.
Ticket:CVS-187884,CVS-190650