-
Notifications
You must be signed in to change notification settings - Fork 629
Build EXTENSION_DATA_LOADER with size_test #12678
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
Conversation
[ghstack-poisoned]
extension/llm/tokenizers
Outdated
@@ -1 +1 @@ | |||
Subproject commit ea8691c6200c702cf28dd2aa2da902e9299b6bb3 | |||
Subproject commit f09feca15849a790c05b3b7855e7c62ce26ba94b |
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.
Intentional?
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.
nope, good catch, should be in the EXPORT PR or split out
@@ -249,7 +249,7 @@ check_required_options_on( | |||
|
|||
check_conflicting_options_on( | |||
IF_ON EXECUTORCH_BUILD_ARM_BAREMETAL CONFLICTS_WITH | |||
EXECUTORCH_BUILD_EXTENSION_DATA_LOADER EXECUTORCH_BUILD_PTHREADPOOL |
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.
I suspect this or install_executorch
used by the ci scripts was the reason it was working before?
If we want to not enable this by default then perhaps we should also update install_executorch
(if not using the default preset already).
And that should also make the baremetal build fail. I also realized that we don't run the baremetal size-test binary on the CI, so we may need to fix that too.
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.
the reason it was working before?
you may be interested in later PRs in this stack, particularly #12744
build-demo-ios looks like a flake, noting CI looks good before removing the accidental tokenizers bump and starting merge of the stack |
@@ -23,11 +23,17 @@ cmake_install_executorch_lib() { | |||
update_tokenizers_git_submodule | |||
local EXTRA_BUILD_ARGS="${@}" | |||
|
|||
if [[ "$EXTRA_BUILD_ARGS" == *"-DEXECUTORCH_BUILD_ARM_BAREMETAL=ON"* ]]; then | |||
local BUILD_DATA_LOADER="OFF" |
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.
Nit: is double quote needed?
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.
I don't think so, but I also don't think it hurts
merging based on previous good CI run |
size_test.cpp uses FileDataLoader. Looks like this happens to work currently because of the missing cmake_deps.toml entry that gets fixed in #12744