-
Notifications
You must be signed in to change notification settings - Fork 303
Fix const casting in non-writable numpy-based tensors #3086
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: master
Are you sure you want to change the base?
Conversation
| ov::Tensor image_copy(image.get_element_type(), image.get_shape()); | ||
| image.copy_to(image_copy); | ||
|
|
||
| m_request.set_tensor("image", image_copy); |
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.
Was it broken? Should be reported as OV bug if so
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.
No, we just needed tensor with non-const data for set_tensor, but we were receiving tensor with the const data. So i don't see it as broken in OV right now.
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.
It's a shame that a copy is needed just to please the API. I view it as a problem. @praasz, can you take a look
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.
I will check it
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.
The data copy for inputs should be not required. It should be possible use Inputs tensor which are read-only.
The exception on infer() call is error on OV side, investigation in progress.
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.
The data copy for inputs should be not required. It should be possible use Inputs tensor which are read-only. The exception on
infer()call is error on OV side, investigation in progress.
@praasz
FYI: unnecessary non-const access which I found while debugging this issue:
https://github.com/openvinotoolkit/openvino/blob/master/src/inference/src/dev/isync_infer_request.cpp#L289
OPENVINO_ASSERT(
std::dynamic_pointer_cast<ov::IRemoteTensor>(tensor._ptr) || tensor->data() != nullptr || is_dynamic,
"Tensor data equal nullptr!");
}Here: tensor->data()
Though there should be more places like this. Since fixing this one allow to move forward a bit, but didn't completely solve the issue
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.
@AlexanderKalistratov Do we have an issue for that, so i can reference? Or should i create one?
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.
@Wovchena do we still need this fix or it's better to wait for it to be resolved on OV side?
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.
Wait for OV
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.
The openvinotoolkit/openvino#33120 can be used for verification if that solve issue on GenAI side.
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.
Pull request overview
This PR fixes issues related to const casting in non-writable numpy-based tensors by creating writable copies of tensors before processing. The changes ensure that read-only numpy arrays (where writeable = False) can be safely used as inputs to various pipeline operations.
Key changes:
- Added defensive tensor copying in C++ code to handle read-only numpy tensors
- Added test coverage to verify read-only tensors work correctly across different pipeline types
- Modified tensor handling to use
copy_to()instead of direct assignment or pointer sharing
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/python_tests/test_vlm_pipeline.py | Added test case for VLM pipeline with read-only image tensors |
| tests/python_tests/test_llm_pipeline_static.py | Added test case for static LLM pipeline with read-only input tensors |
| tests/python_tests/test_llm_pipeline.py | Added test case for LLM pipeline with read-only input tensors |
| src/cpp/src/visual_language/phi4mm/classes.cpp | Creates writable copy of image tensor before processing |
| src/cpp/src/visual_language/inputs_embedder.cpp | Uses memcpy to create independent tensor copies instead of sharing pointers |
| src/cpp/src/speculative_decoding/speculative_decoding_stateful.cpp | Creates writable copies of input_ids and attention_mask tensors |
| src/cpp/src/llm/pipeline_static.cpp | Creates writable copies of input_ids and attention_mask tensors |
| src/cpp/src/image_generation/image_processor.cpp | Creates writable copy of image tensor before processing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix const casting in non-writable numpy-based tensors.