Refactor CMake build for examples/models/llama#18825
Refactor CMake build for examples/models/llama#18825GregoryComer wants to merge 1 commit intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18825
Note: Links to docs will display an error until the docs builds have been completed. ❌ 15 New Failures, 2 Cancelled Jobs, 3 Unrelated FailuresAs of commit 5bb2f3e with merge base 38fdfae ( NEW FAILURES - The following jobs have failed:
CANCELLED JOBS - The following jobs were cancelled. Please retry:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
3f9c81d to
965a4a9
Compare
965a4a9 to
5bb2f3e
Compare
|
CI should be passing now. The MLX failures are failing due to being on a forked PR. The android emulator and unittest failures are flakes. |
|
|
||
| if(EXECUTORCH_BUILD_KERNELS_LLM) | ||
| list(APPEND link_libraries $<LINK_LIBRARY:WHOLE_ARCHIVE,custom_ops>) | ||
| list(APPEND link_libraries custom_ops) |
There was a problem hiding this comment.
are these not needed anymore? Do we rely on whole archive flag added in executorch-config.cmake?
larryliu0820
left a comment
There was a problem hiding this comment.
Change itself looks ok. I don't think this approach is scalable though, like we should not make all the example cmake target visible to the top level cmake. I tend to make executorch easy to use such that, when a real application needs to depend on it, they just do add_subdirectory(executorch) and can link to all the executorch libraries and when they do that we should not expose example cmake targets to them. I guess since you exclude llama_main from being built by default it's an ok change.
Summary
Update the CMake build for examples/models/llama to be in-tree in the top-level build but excluded from all. This means that it can be built in a single step, but will only be built when explicitly requested with --target llama_main.
This simplifies the build quite a bit (as evidenced by how many lines we remove in this PR) and prevents issues with iterative rebuild or mismatched build options between llama_main and the core runtime.
Note that the top-level
make llama-cpucommand is unmodified, just simplified under the hood. For direct CMake builds, here's a before and after:Before:
After:
Note to reviewers: the MLX failures are unrelated. They are due to running extended CI on a forked PR.
cc @larryliu0820