-
Notifications
You must be signed in to change notification settings - Fork 511
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
Handle custom_ops like any other ET lib in Android and examples/models/lla{m,v}a/CMakeLists.txt #8946
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8946
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit b478275 with merge base 484231c ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
linker errors say reduce_util.cpp is in both portable_lib and custom_ops. indeed, executorch_srcs.cmake says it's there too! can't get rid of that file (executorch_srcs.cmake) soon enough, but until then I'll have to fix it. |
looks like we have to handle the Android demo as well |
@@ -15,7 +15,7 @@ | |||
# ~~~ | |||
# It should also be cmake-lint clean. | |||
# | |||
cmake_minimum_required(VERSION 3.19) | |||
cmake_minimum_required(VERSION 3.24) # 3.24 is required for WHOLE_ARCHIVE |
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.
wondering how high we can bump this and if we can raise it globally
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.
as I said on Discord:
the concern would be if there are projects that would want to take an ExecuTorch dep, but actually need an older CMake version. seems like we can raise it, but should avoid doing so unnecessarily.
so, i'm going to make this change in this PR but not roll it out to the top-level ExecuTorch CMakeLists.txt.
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.
is that the only way to achieve whole-archive? I dont know much on the implication of requiring this but I suppose non-trivial number of folks using et may come to use for llama
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.
we pip install latest cmake anyway. people who follow our directions will get 3.31.
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.
It would be good if we can do a codemod to change all CMakeLists.txt to have the same requirement.
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.
change all CMakeLists.txt to have the same requirement.
@larryliu0820 I wouldn't mind doing this, I just thought it might be beneficial to only increase the requirement for the llama example use cases that need this stuff and leave the core/embedded cases lower. What do you think?
noting that CI is 100% green in case I rebase |
ping reviewers @lucylq @jackzhxng @GregoryComer @kirklandsign @manuelcandales @kimishpatel |
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 think @larryliu0820 should review this but left some comments
The old way was a holdover from when custom_ops lived in examples/models/llama (and was copy/pasted to llava and Android).
Needed for #8947.