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

Export pack_strings() and unpack_strings() #2

Conversation

Wovchena
Copy link

No description provided.

@@ -66,6 +66,8 @@ target_include_directories(${TARGET_NAME} PRIVATE
"${CMAKE_BINARY_DIR}/third_party/dart/src/extern_dart/include/"
"${CMAKE_BINARY_DIR}/third_party/install/re2/include/")

target_include_directories(${TARGET_NAME} PUBLIC .)

Choose a reason for hiding this comment

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

typically, OV extensions does not imply to provide any extra C++ API..

Copy link
Author

Choose a reason for hiding this comment

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

It's @slyalin's request to call pack_strings() from OV extensions.

Copy link

Choose a reason for hiding this comment

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

Please consider the fact that we are going to remove pack_strings/unpack_strigs after string support is added to OV. But we are not really certain when it happens. I think it makes sense to add it now to have functional distribution and single point of reference (people started to copy this function from one place to another because we are not providing distribution for these methods). I hope it won't cross OV releases boundary.

@apaniukov
Copy link
Owner

@Wovchena you should move unpack_strings to utils.hpp too.

@Wovchena
Copy link
Author

Moved. I also created a separate include dir as requested in openvinotoolkit/openvino.genai#8 (comment).

@apaniukov apaniukov merged commit fb37580 into apaniukov:tokenizer-fix-decode Nov 15, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants