Skip to content

Use ExternalProject_Add to build tokenizers #11550

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Use ExternalProject_Add to build tokenizers #11550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions extension/llm/runner/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,23 @@ target_include_directories(
add_library(extension_llm_runner STATIC ${_extension_llm_runner__srcs})

# add tokenizers
add_subdirectory(
${EXECUTORCH_ROOT}/extension/llm/tokenizers
${CMAKE_CURRENT_BINARY_DIR}/../../../extension/llm/tokenizers
ExternalProject_Add(
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if i understand correctly, ExternalProject_Add creates a separate build process and as a result cmake variables aren't shared.

If you're cross-compiling, don't you wanna propagate toolchain settings and compiler flags?

I think FetchContent propagates compiler flags and toolchain setting. Looks like FetchContent_Declare and FetchContent_MakeAvailable will do the job, and it happens during config time, not build time.

Also, fwiw, looks like XNNPACK is fixing this issue (google/XNNPACK#8488) with this PR (google/XNNPACK#8552)

Copy link
Contributor

Choose a reason for hiding this comment

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

alt text

tokenizers_external_project
PREFIX ${CMAKE_CURRENT_BINARY_DIR}/../tokenizers
SOURCE_DIR ${EXECUTORCH_ROOT}/extension/llm/tokenizers
CMAKE_ARGS -DSUPPORT_REGEX_LOOKAHEAD=ON -DCMAKE_INSTALL_PREFIX=${CMAKE_CURRENT_BINARY_DIR}/../tokenizers
INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/../tokenizers
)

set(runner_deps executorch_core extension_module extension_tensor tokenizers)
add_dependencies(extension_llm_runner tokenizers_external_project)
find_package(tokenizers CONFIG HINTS ${CMAKE_INSTALL_PREFIX} ${CMAKE_CURRENT_BINARY_DIR}/../tokenizers)
set(runner_deps executorch extension_module extension_tensor ${TOKENIZERS_LIBRARIES})

target_link_libraries(extension_llm_runner PUBLIC ${runner_deps})

target_include_directories(
extension_llm_runner INTERFACE ${_common_include_directories}
${EXECUTORCH_ROOT}/extension/llm/tokenizers/include
extension_llm_runner PUBLIC ${_common_include_directories}
${TOKENIZERS_INCLUDE_DIRS}
${CMAKE_CURRENT_BINARY_DIR}/../tokenizers/include
)

if(BUILD_TESTING)
Expand Down
2 changes: 1 addition & 1 deletion extension/llm/tokenizers
Open in desktop
Submodule tokenizers updated 3 files
+90 −50 CMakeLists.txt
+53 −0 cmake/tokenizers-config.cmake.in
+48 −0 test/CMakeLists.txt
Loading