Skip to content

Include custom_ops and XNNPACK in executor_runner if built #9248

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Mar 17, 2025
Merged

Include custom_ops and XNNPACK in executor_runner if built #9248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
54c3d3c
Update
swolchok Mar 13, 2025
c5d8db8
Update
swolchok Mar 14, 2025
c3579da
Update
swolchok Mar 14, 2025
86ce056
Update
swolchok Mar 14, 2025
41414b4
Update
swolchok Mar 17, 2025
d1b3664
Update
swolchok Mar 17, 2025
46ae720
Update
swolchok Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .ci/scripts/unittest-buck2.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ set -eux

# TODO: expand this to //...
# TODO: can't query cadence & vulkan backends
# TODO: can't query //kernels/prim_ops because of a cpp_unittest and
# broken code in shim to read oss.folly_cxx_tests. Sending fix but it
# needs to propagate and we need a submodule update.
# TODO: can't query //kernels/prim_ops because of non-buckified stuff in OSS.
buck2 query "//backends/apple/... + //backends/example/... + \
//backends/mediatek/... + //backends/test/... + //backends/transforms/... + \
//backends/xnnpack/... + //configurations/... + //kernels/aten/... + \
Expand All @@ -20,7 +18,9 @@ buck2 query "//backends/apple/... + //backends/example/... + \
UNBUILDABLE_OPTIMIZED_OPS_REGEX="gelu|fft_r2c|log_softmax"
BUILDABLE_OPTIMIZED_OPS=$(buck2 query //kernels/optimized/cpu/... | grep -E -v $UNBUILDABLE_OPTIMIZED_OPS_REGEX)

BUILDABLE_KERNELS_PRIM_OPS_TARGETS=$(buck2 query //kernels/prim_ops/... | grep -v prim_ops_test_py)
# TODO: build prim_ops_test_cpp again once supported_features works in
# OSS buck.
BUILDABLE_KERNELS_PRIM_OPS_TARGETS=$(buck2 query //kernels/prim_ops/... | grep -v prim_ops_test)
# TODO: expand the covered scope of Buck targets.
# //runtime/kernel/... is failing because //third-party:torchgen_files's shell script can't find python on PATH.
# //runtime/test/... requires Python torch, which we don't have in our OSS buck setup.
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/pull.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ jobs:
CONDA_ENV=$(conda env list --json | jq -r ".envs | .[-1]")
conda activate "${CONDA_ENV}"

./install_requirements.sh --use-pt-pinned-commit
# build module for executorch.extension.pybindings.portable_lib
bash test/build_size_test.sh
strip cmake-out/test/size_test
Expand Down Expand Up @@ -396,6 +397,8 @@ jobs:
CONDA_ENV=$(conda env list --json | jq -r ".envs | .[-1]")
conda activate "${CONDA_ENV}"

./install_requirements.sh --use-pt-pinned-commit

# build module for executorch.extension.pybindings.portable_lib
bash test/build_size_test.sh
strip cmake-out/test/size_test
Expand Down Expand Up @@ -510,6 +513,7 @@ jobs:
MODE=${{ matrix.mode }}
PT2E_QUANTIZE=${{ matrix.pt2e_quantize }}

./install_requirements.sh --use-pt-pinned-commit
PYTHON_EXECUTABLE=python bash .ci/scripts/setup-qnn-deps.sh
PYTHON_EXECUTABLE=python bash .ci/scripts/build-qnn-sdk.sh

Expand Down Expand Up @@ -541,6 +545,7 @@ jobs:

BUILD_TOOL="cmake"

./install_requirements.sh --use-pt-pinned-commit
PYTHON_EXECUTABLE=python bash .ci/scripts/setup-qnn-deps.sh
PYTHON_EXECUTABLE=python bash .ci/scripts/build-qnn-sdk.sh

Expand Down
1 change: 1 addition & 0 deletions .github/workflows/trunk.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,7 @@ jobs:
MODE=${{ matrix.mode }}
PT2E_QUANTIZE=${{ matrix.pt2e_quantize }}

./install_requirements.sh --use-pt-pinned-commit
PYTHON_EXECUTABLE=python bash .ci/scripts/setup-qnn-deps.sh
PYTHON_EXECUTABLE=python bash .ci/scripts/build-qnn-sdk.sh

Expand Down
10 changes: 9 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
# It should also be cmake-lint clean.
#

cmake_minimum_required(VERSION 3.19)
cmake_minimum_required(VERSION 3.24)
project(executorch)
include(build/Utils.cmake)
include(CMakeDependentOption)
Expand Down Expand Up @@ -919,6 +919,14 @@ if(EXECUTORCH_BUILD_EXECUTOR_RUNNER)
list(APPEND _executor_runner_libs quantized_ops_lib)
endif()

if(EXECUTORCH_BUILD_KERNELS_CUSTOM)
list(APPEND _executor_runner_libs $<LINK_LIBRARY:WHOLE_ARCHIVE,custom_ops>)
endif()

if(EXECUTORCH_BUILD_XNNPACK)
list(APPEND _executor_runner_libs xnnpack_backend)
endif()

if(EXECUTORCH_ENABLE_EVENT_TRACER)
if(EXECUTORCH_BUILD_DEVTOOLS)
list(APPEND _executor_runner_libs etdump flatccrt)
Expand Down
2 changes: 1 addition & 1 deletion backends/xnnpack/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

target_include_directories(
Expand Down
4 changes: 2 additions & 2 deletions kernels/test/targets.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def define_common_targets():
],
fbcode_exported_deps = [
"//common/gtest:gtest",
],
] if not runtime.is_oss else [],
xplat_exported_deps = [
"//third-party/googletest:gtest_main",
],
Expand Down Expand Up @@ -68,7 +68,7 @@ def define_common_targets():
fbcode_exported_deps = [
"//common/init:init",
"//common/gtest:gtest",
],
] if not runtime.is_oss else [],
xplat_exported_deps = [
"//xplat/folly:init_init",
"//third-party/googletest:gtest_main",
Expand Down
Loading