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

Convert tokenizers with openvino_tokenizers #500

Closed
wants to merge 2 commits into from

Conversation

slyalin
Copy link
Contributor

@slyalin slyalin commented Jan 5, 2024

What does this PR do?

Export tokenizer and detokenizer as OpenVINO models using openvino_tokenizers from https://github.com/openvinotoolkit/openvino_contrib/tree/master/modules/custom_operations/user_ie_extensions/tokenizer/python. Activated by default as a part of optimum-cli export openvino command line tool. Compatible with https://github.com/openvinotoolkit/openvino.genai/tree/master/text_generation/causal_lm/cpp.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

@slyalin
Copy link
Contributor Author

slyalin commented Jan 5, 2024

@apaniukov will cover this functionality. This PR is just to initiate discussion.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

save_model(ov_tokenizer, tokenizer_path)
save_model(ov_detokenizer, detokenizer_path)
except Exception as exception:
print("[ WARNING ] OpenVINO tokenizer/detokenizer models couldn't be exported because of exception:", exception)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
print("[ WARNING ] OpenVINO tokenizer/detokenizer models couldn't be exported because of exception:", exception)
logger.warning(OpenVINO tokenizer/detokenizer models couldn't be exported because of exception:", exception)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

Comment on lines +350 to +355
try:
# TODO: Avoid loading the tokenizer again if loaded before
tokenizer = AutoTokenizer.from_pretrained(model_name_or_path)
tokenizer_export(tokenizer, output)
except:
print("[ WARNING ] Could not load tokenizer using specified model ID or path. OpenVINO tokenizer/detokenizer models won't be generated.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

tokenizer already loaded in maybe_load_preprocessors function, I recommend to check this function result instead. Also possibly you should take into account trust_remote_code parameter if you want load tokenizer explicitly

Copy link
Contributor

@apaniukov apaniukov Jan 10, 2024

Choose a reason for hiding this comment

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

Reuse tokenizer from maybe_load_preprocessors result. But a new problem is that there are two identical tokenizers - one from AutoTokenizer and one from AutoProcessor. I haven't figured out how to deduplicate them yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the PR: slyalin#2

@@ -46,6 +47,24 @@
logger = logging.getLogger(__name__)


def tokenizer_export(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be in convert.py file together with other conversion functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved.

@apaniukov apaniukov mentioned this pull request Jan 12, 2024
3 tasks
@slyalin
Copy link
Contributor Author

slyalin commented Jan 18, 2024

Closing in favor of #513

@slyalin slyalin closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants