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

[Tokenizers][TF FE] Fix MUSE conversion #854

Merged

Conversation

apaniukov
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the category: custom operations OpenVINO Runtime Extension with custom operations label Jan 26, 2024
@ilya-lavrenov
Copy link
Contributor

build_jenkins

Copy link
Contributor

@rkazants rkazants left a comment

Choose a reason for hiding this comment

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

please productize changes and let us merge it.
Just remove string unpacking in SentencePieceOp op extension due to native support of string tensors in OV.

@apaniukov apaniukov requested a review from rkazants January 31, 2024 13:43
@apaniukov apaniukov marked this pull request as ready for review January 31, 2024 13:43
@apaniukov apaniukov requested a review from a team as a code owner January 31, 2024 13:43
Copy link
Contributor

@rkazants rkazants left a comment

Choose a reason for hiding this comment

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

one minor comment remained

Comment on lines 183 to 184
// }
// set_node_name(node.get_name(), reshape); // TODO: requires dependencies from TF FE internals
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this? Please clean the code a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted

Comment on lines 200 to 184
if(auto pack = dynamic_cast<StringTensorPack*>(tensor.get_node())) {
// TODO: If it is a beginning of the graph, how to detect strings? It falls in 'else' branch in this case.
// FIXME: Needs extension for a Parameter to prepare it first
auto begins = std::make_shared<Reshape>(pack->input_value(0), shape, false);
auto ends = std::make_shared<Reshape>(pack->input_value(1), shape, false);
auto chars = pack->input_value(2);
auto reshape = post_translate_string_tensor_output({begins, ends, chars});
return {reshape};
} else {
auto reshape = std::make_shared<Reshape>(tensor, shape, false);
return {reshape};
}
auto reshape = std::make_shared<Reshape>(tensor, shape, false);
return {reshape};
// }
// set_node_name(node.get_name(), reshape); // TODO: requires dependencies from TF FE internals
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should revert these changes, they does not relate MUSE model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

@apaniukov
Copy link
Contributor Author

@dtrawins @slyalin sentencepiece operation translated from TF with string input by default. It also allows input of a decomposed string representation, but this must be changed manually after the model is converted. This PR allows you to experiment with data format optimizations without having to recompile the extension.

@apaniukov
Copy link
Contributor Author

@ilya-lavrenov ilya-lavrenov merged commit 60c3035 into openvinotoolkit:master Feb 8, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: custom operations OpenVINO Runtime Extension with custom operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants