Include custom_ops and XNNPACK in executor_runner if built #9248
Conversation
|
Stack from ghstack (oldest at bottom): |
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9248
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 46ae720 with merge base 82f3381 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
|
hm, apparently the linux runners have an old CMake, but I thought install_requirements.sh installed latest cmake via pip... |
|
I have a local patch for llama-runner-qnn-linux trunk job. to avoid excessive CI usage, I will wait for the other trunk jobs to come back and if they are green, I will just add the patch and merge right away. |
whoops, thought this was reviewed but it's not. should be good to go |
Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/) [ghstack-poisoned]
Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/) [ghstack-poisoned]
Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/) [ghstack-poisoned]
Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/) [ghstack-poisoned]
It is redundant with executor_runner after #9248 . Differential Revision: [D71159265](https://our.internmc.facebook.com/intern/diff/D71159265/) [ghstack-poisoned]
|
hmm, 3 test-models-macos jobs timed out, but they're all succeeding quickly on main. rerunning. |
It is redundant with executor_runner after #9248 . Differential Revision: [D71159265](https://our.internmc.facebook.com/intern/diff/D71159265/) [ghstack-poisoned]
Pull Request resolved: #9292 It is redundant with executor_runner after #9248 . Differential Revision: [D71159265](https://our.internmc.facebook.com/intern/diff/D71159265/) ghstack-source-id: 271912198
|
noting that CI is green. rebasing and merging |
| @@ -118,7 +118,7 @@ include(cmake/Dependencies.cmake) | |||
| list(TRANSFORM _xnnpack_backend__srcs PREPEND "${EXECUTORCH_ROOT}/") | |||
| add_library(xnnpack_backend STATIC ${_xnnpack_backend__srcs}) | |||
| target_link_libraries( | |||
| xnnpack_backend PUBLIC ${xnnpack_third_party} executorch_core xnnpack_schema | |||
| xnnpack_backend PUBLIC ${xnnpack_third_party} executorch_core xnnpack_schema extension_threadpool | |||
There was a problem hiding this comment.
@swolchok just checking, does it have any implications on build_apple_frameworks.sh's backend_xnnpack framework? Specifically, the fact kernels_custom already links against extension_threadpool. Can we run into duplicated symbols when both are linked with an app? All iOS CI tests that could have flagged were red by the time this change merged due to some previous changes.
There was a problem hiding this comment.
I don't think we should be requiring everyone who makes a change to the CMake dependency graph to think about build_apple_frameworks.sh. We should look into using CMake's built-in support for building Apple frameworks instead if possible.
There was a problem hiding this comment.
Can we run into duplicated symbols when both are linked with an app?
I don't know, but I would expect to see red CI jobs in that case.
There was a problem hiding this comment.
Each framework is sorta a higher level abstraction and consists of several CMake artifacts built according to the rules in build_apple_frameworks.sh. Apps can link them selectively, which may reveal missing or duplicate symbols. To catch such issues, we rely on demo and benchmark apps running tests as part of CI. However, both are currently failing due to unrelated prior changes.
There was a problem hiding this comment.
if I'm not mistaken, the build_apple_frameworks job for this diff successfully compiled and linked the demo app and it's failing in tests.
There was a problem hiding this comment.
That’s correct - it means the frameworks were linked as instructed. Now, the app tests will determine whether the frameworks contain all the required symbols, are missing any, or have duplicates. These tests don’t run on every PR but only on specific path changes and in CI (last I heard, Mac runners were costly to run every time). Since the CI is already failing for them, I’m proactively checking if there are any other potentially relevant changes we should investigate once those issues are resolved.
It is redundant with executor_runner after #9248 . Differential Revision: [D71159265](https://our.internmc.facebook.com/intern/diff/D71159265/) [ghstack-poisoned]
Pull Request resolved: #9292 It is redundant with executor_runner after #9248 . ghstack-source-id: 272251778 Differential Revision: [D71159265](https://our.internmc.facebook.com/intern/diff/D71159265/)
It is redundant with executor_runner after #9248 . Differential Revision: [D71159265](https://our.internmc.facebook.com/intern/diff/D71159265/) [ghstack-poisoned]
…tor_runner:executor_runner_opt" Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/) [ghstack-poisoned]
…tor_runner_opt" Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/) [ghstack-poisoned]
This is the minimum version `cmake_minimum_required(VERSION 3.24)` set by #9248. Android jobs are failing https://github.com/pytorch/executorch/actions/runs/13959323278/job/39077616882#step:15:753 after #9248 lands
This is the minimum version `cmake_minimum_required(VERSION 3.24)` set by pytorch#9248. Android jobs are failing https://github.com/pytorch/executorch/actions/runs/13959323278/job/39077616882#step:15:753 after pytorch#9248 lands
Pull Request resolved: #9291 Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. ghstack-source-id: 273335947 @exported-using-ghexport Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/)
…tor_runner:executor_runner_opt" Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/) [ghstack-poisoned]
…tor_runner_opt" Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only. Differential Revision: [D71220489](https://our.internmc.facebook.com/intern/diff/D71220489/) [ghstack-poisoned]
…opt (#9291) Attempt to add a Buck target that's analogous to the CMake build's executor_runner -- has all CPU ops that you need etc. The base executor_runner target is commented as intentionally having minimal deps, hence the separate target. This is sort of a companion to #9248, except that that PR is for CMake only and this PR is for Buck only.
) Somebody (@larryliu0820?) pointed out that there's no particular reason to require a separate xnn_executor_runner target, and indeed we already put several other optimized things in here if they're built.
This is the minimum version `cmake_minimum_required(VERSION 3.24)` set by pytorch#9248. Android jobs are failing https://github.com/pytorch/executorch/actions/runs/13959323278/job/39077616882#step:15:753 after pytorch#9248 lands
Somebody (@larryliu0820?) pointed out that there's no particular
reason to require a separate xnn_executor_runner target, and indeed we
already put several other optimized things in here if they're built.