-
Notifications
You must be signed in to change notification settings - Fork 594
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.
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
base: main
Are you sure you want to change the base?
Conversation
This way it avoids the CMake target name collision between XNNPACK's `memory` and Abseil's `memory`
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11550
Note: Links to docs will display an error until the docs builds have been completed. ❌ 14 New FailuresAs of commit e92ac85 with merge base 09a4b9d ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
add_subdirectory( | ||
${EXECUTORCH_ROOT}/extension/llm/tokenizers | ||
${CMAKE_CURRENT_BINARY_DIR}/../../../extension/llm/tokenizers | ||
ExternalProject_Add( |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way it avoids the CMake target name collision between XNNPACK's
memory
and Abseil'smemory
This pull request updates the build configuration for the
extension_llm_runner
target inCMakeLists.txt
and modifies thetokenizers
submodule reference. The changes aim to improve dependency management by transitioning from a subdirectory approach to usingExternalProject_Add
, and updating thetokenizers
submodule to a new commit.Build Configuration Updates:
extension/llm/runner/CMakeLists.txt
: Replacedadd_subdirectory
for thetokenizers
dependency withExternalProject_Add
, enabling more flexible configuration options such as setting regex lookahead support and specifying an installation directory. Updated dependency handling by addingtokenizers_external_project
as a dependency and usingfind_package
to locatetokenizers
. Adjustedtarget_include_directories
to useTOKENIZERS_INCLUDE_DIRS
and the new installation path.Submodule Update:
extension/llm/tokenizers
: Updated thetokenizers
submodule reference to commita2bac3b7a54a3816d66b8fd3e705f8724d2d5f2f
, to incorporate changes in this PR: Enable install find package pytorch-labs/tokenizers#82