Skip to content
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

[ONNX] Added translation for string constants/inputs #24189

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

gkrivor
Copy link
Contributor

@gkrivor gkrivor commented Apr 23, 2024

Details:

  • Added support for accepting string as inputs and constants

Tickets:

  • 139685

@gkrivor gkrivor requested a review from a team as a code owner April 23, 2024 11:44
@github-actions github-actions bot added the category: ONNX FE OpenVINO ONNX FrontEnd label Apr 23, 2024
@gkrivor gkrivor changed the title [ONNX] Added support for strings as inputs and constants [ONNX] Added support for accepting string as inputs and constants Apr 24, 2024
FRONT_END_THROW("Loading strings from raw data isn't supported");
}
if (m_tensor_proto->data_type() == TensorProto_DataType::TensorProto_DataType_STRING) {
return std::vector<std::string>(m_tensor_proto->string_data().begin(), m_tensor_proto->string_data().end());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use detail::__get_data? Is it already string objects in memory? can we create view tensor on it without recreation string objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an easy way to perform it without copying, and it isn't a purpose of this PR.
As I can see, when string is stored in a memory (disk or ram) - it isn't represented as a C-string. It looks like a bytes without an ending zero.
Sounds like you are talking about usage of std::string_view. If nothing changed - we haven't moved to C++17 standard yet. Moreover, I have some concerns string_view actually will solve the pointed issue, I don't remember I saw a code which allows to use string_view in constants in the core.
So, I think I'm obligated to use this approach to get strings from a protobuf format...

@gkrivor gkrivor changed the title [ONNX] Added support for accepting string as inputs and constants [ONNX] Added translation for string constant/inputs Apr 26, 2024
@gkrivor gkrivor changed the title [ONNX] Added translation for string constant/inputs [ONNX] Added translation for string constants/inputs Apr 26, 2024
@gkrivor gkrivor requested review from a team as code owners June 14, 2024 17:06
@github-actions github-actions bot added the category: IE Tests OpenVINO Test: plugins and common label Jun 14, 2024
Comment on lines +6742 to +6744
test_case.add_input<std::string>({"strinpt1", "strinpt2"});
test_case.add_expected_output<int64_t>({2});
test_case.add_expected_output<std::string>({"strinpt1", "strinpt2"});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test_case.add_input<std::string>({"strinpt1", "strinpt2"});
test_case.add_expected_output<int64_t>({2});
test_case.add_expected_output<std::string>({"strinpt1", "strinpt2"});
test_case.add_input<std::string>({"string1", "string2"});
test_case.add_expected_output<int64_t>({2});
test_case.add_expected_output<std::string>({"string1", "string2"});

Copy link
Contributor Author

@gkrivor gkrivor Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was an original option and it was equal to string_constant test, I want to be sure we use correct model for testing.

@gkrivor gkrivor added this pull request to the merge queue Jun 21, 2024
Merged via the queue into openvinotoolkit:master with commit 9575a96 Jun 21, 2024
119 checks passed
@gkrivor gkrivor deleted the onnx_string branch June 21, 2024 09:38
allnes pushed a commit to allnes/openvino that referenced this pull request Jun 27, 2024
…#24189)

### Details:
 - Added support for accepting string as inputs and constants

### Tickets:
 - 139685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: IE Tests OpenVINO Test: plugins and common category: ONNX FE OpenVINO ONNX FrontEnd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants