Ship a linkable shared runtime and make the Python bindings use it - #21514
Open
shoumikhin wants to merge 23 commits into
Open
Ship a linkable shared runtime and make the Python bindings use it#21514shoumikhin wants to merge 23 commits into
shoumikhin wants to merge 23 commits into
Conversation
Contributor
Author
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21514
Note: Links to docs will display an error until the docs builds have been completed. ❌ 48 New Failures, 41 Pending, 2 Unrelated Failures, 22 Unclassified FailuresAs of commit 3447ef5 with merge base 9789eea ( NEW FAILURES - The following jobs have failed:
UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The new shared-runtime linking logic uses GNU ld-specific flags without platform guards, which can break builds when EXECUTORCH_BUILD_SHARED is enabled on non-ELF toolchains.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR makes the ExecuTorch wheel usable as a C++ SDK by shipping a linkable shared runtime (libexecutorch.so) and exposing it via find_package(executorch) as executorch::runtime, while also updating the Python extensions to link against that shared runtime to preserve a single process-wide backend registry.
Changes:
- Add a consolidated shared runtime target (
executorch_shared/libexecutorch.so) and update multiple extension/tool targets to resolve runtime symbols from it. - Extend the wheel CMake package config to provide an imported target
executorch::runtimeresolved relative to the installed wheel for relocatable consumption. - Add CI wheel smoke coverage that verifies single-registry behavior and that a standalone C++ app can build/run via only
find_package(executorch)+executorch::runtime.
File summaries
| File | Description |
|---|---|
| tools/cmake/Utils.cmake | Adds helper to force linking against the shared runtime early on the link line. |
| tools/cmake/preset/pybind.cmake | Enables EXECUTORCH_BUILD_SHARED for the Linux pybind preset to ship the shared runtime in wheels. |
| tools/cmake/executorch-wheel-config.cmake | Exposes executorch::runtime imported target and keeps legacy _portable_lib discovery. |
| tools/cmake/Codegen.cmake | Avoids linking executorch_core when shared runtime is enabled; forces shared runtime resolution. |
| setup.py | Installs the shared runtime SONAME into the wheel and expands header install coverage for runtime-linked extensions. |
| kernels/quantized/CMakeLists.txt | Adds wheel-runtime-relative RPATH when shared runtime is enabled. |
| extension/training/CMakeLists.txt | Switches pybind training module to shared runtime path and fixes RPATH for wheel layout. |
| extension/llm/runner/CMakeLists.txt | Forces shared runtime resolution and adjusts RPATH when shared runtime is enabled. |
| extension/llm/custom_ops/CMakeLists.txt | Ensures wheel-built artifact has correct RPATH and resolves runtime from shared library. |
| devtools/etdump/CMakeLists.txt | Avoids executorch whole-archive when shared runtime is present; links shared runtime instead. |
| devtools/bundled_program/CMakeLists.txt | Avoids whole-archive duplication by linking shared runtime when enabled. |
| codegen/tools/CMakeLists.txt | Makes selective_build resolve runtime via shared library and sets wheel-relative RPATH. |
| CMakeLists.txt | Defines executorch_shared earlier and updates pybind/utility targets to avoid embedding duplicate registries. |
| .github/workflows/build-wheels-*.yml | Expands workflow path triggers to include CMakeLists and tools/cmake/. |
| .ci/scripts/wheel/test_linux.py | Runs the new C++ SDK wheel test as part of Linux wheel smoke tests. |
| .ci/scripts/wheel/test_linux_aarch64.py | Runs the new C++ SDK wheel test as part of aarch64 Linux wheel smoke tests. |
| .ci/scripts/wheel/test_cpp_sdk.py | New test that validates single-registry and that a standalone C++ app can link/run via the wheel. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
shoumikhin
added a commit
that referenced
this pull request
Jul 31, 2026
Today `pip install executorch` gives you the Python half of ExecuTorch but
nothing a C++ application can link. The wheel installs a header subset and an
`executorch-config.cmake`, but that config only locates the Python extension so
custom-op builds can compile against it: it defines no runtime library and no
CMake targets. Writing a small C++ program that loads and runs a `.pte` means
cloning the repo, syncing submodules, and building from source.
This change ships the runtime as a real library and exposes it through
`find_package`, so a standalone application needs nothing but the wheel:
find_package(executorch REQUIRED)
target_link_libraries(my_app PRIVATE executorch::runtime)
`executorch::runtime` is an imported target whose location is resolved relative
to the config file itself, so the package stays relocatable and no path from the
machine that built the wheel is baked into it. A consumer picks up the wheel's
library directory in its RUNPATH, plus `$ORIGIN`-relative entries so an
application deployed next to a copy of the runtime keeps working without
`LD_LIBRARY_PATH`.
The Python extensions have to move to the shared runtime in the same change.
Backends register themselves into a single process-wide table owned by the
runtime, and that table is only process-wide if exactly one loaded library
defines it. The Python extension statically embeds the runtime today, so adding a
shared library beside it would give a process two independent registries, and a
backend could register into the one nobody reads. The extensions therefore link
the shared runtime instead of whole-archiving the static libraries, leaving
exactly one registry owner for both the Python and C++ paths.
Getting that right needs two linker details. The shared runtime is named through
a link option rather than an ordinary dependency, because CMake orders link
libraries so a static archive precedes what it depends on, which would let the
archive satisfy the runtime symbols first. It is also wrapped in
`--no-as-needed`, because a shared library with no already-referenced symbol at
the point it appears on the link line can be dropped, and a later static archive
would then supply the registry after all.
Registration still happens through static initializers exactly as before. No new
plugin or loader ABI is introduced.
The shared runtime is Linux-only for the wheel. macOS C++ consumers are served by
the existing Swift package distribution, and the runtime has no export
annotations for a Windows DLL. Every other build keeps linking the static
libraries, because the new behavior is gated on the existing
`EXECUTORCH_BUILD_SHARED` option, so iOS, Android, and embedded builds are
unaffected.
Test plan:
The wheel smoke test now covers this on Linux, so it is checked in CI rather than
only by hand. It asserts that exactly one shipped library defines the backend
registry, builds a standalone C++ program that only calls
`find_package(executorch)` and links `executorch::runtime`, runs it without
`LD_LIBRARY_PATH`, and checks the resulting binary depends on the shipped runtime
with a relocatable RUNPATH. The check was confirmed to fail when a second
registry definition is introduced deliberately. The wheel workflows now also run
when any `CMakeLists.txt` or anything under `tools/cmake` changes, since those
files decide what the wheel contains.
Built the wheel from a clean checkout on Linux x86_64 and on Linux aarch64, then
verified each against a fresh virtual environment with a normal
dependency-resolving install:
- `nm -DC` across every shipped shared object shows exactly one definition of the
registry entry points; the Python extensions import them rather than defining
their own.
- A C++ program whose CMake only calls `find_package(executorch)` and links
`executorch::runtime` compiles against the installed wheel with no source
checkout, then loads a `.pte` and lists its methods.
- `readelf -d` on that program lists the versioned runtime in `DT_NEEDED` with
`$ORIGIN`-relative RUNPATH entries, and it runs without `LD_LIBRARY_PATH`.
- A C++ binary that links only `executorch::runtime` and loads the Python
extension sees the backends that extension registered, which is the
one-registry property stated above.
- `import executorch`, the registered backend list, and `.pte` execution through
the Python bindings are unchanged, with outputs matching eager PyTorch.
- With `EXECUTORCH_BUILD_SHARED` off, the Python extension still builds
self-contained and no shared runtime is produced, so the previous behavior is
intact.
- Configured with CMake 3.28 as well as 3.31, since the project supports 3.24 and
up and the two differ in how strictly they treat link features.
ghstack-source-id: ab9b01b
ghstack-comment-id: 5139976402
Pull-Request: #21514
Contributor
Author
|
Good catch, fixed in the latest push. Both of these were real. The whole-archive loop used to go through Worth noting the option that reaches this code is only turned on for Linux today, |
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The new executorch::runtime CMake config has correctness/robustness issues (duplicate target definition risk and unconditional Python failure path) that can break consumers during configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
tools/cmake/executorch-wheel-config.cmake:121
- With the new
executorch::runtimetarget, a pure C++ consumer should be able to configure successfully even ifpython3isn't available on PATH. Right now the unconditionalexecute_process(${PYTHON_EXECUTABLE} ...)followed byFATAL_ERRORprevents using the shared runtime without Python. Consider treating EXT_SUFFIX lookup as best-effort when the shared runtime was found, and skip_portable_libdiscovery in that case.
find_library(
_portable_lib_LIBRARY
NAMES _portable_lib${EXT_SUFFIX}
PATHS "${_executorch_package_root}/extension/pybindings/"
)
tools/cmake/executorch-wheel-config.cmake:121
find_library()will also search default system locations, which can accidentally pick up a different_portable_lib<EXT_SUFFIX>(e.g., from another install) and silently mix headers/libs. Since this config is meant to bind to the wheel it lives in, addNO_DEFAULT_PATHto restrict the search to the wheel directory.
find_library(
_portable_lib_LIBRARY
NAMES _portable_lib${EXT_SUFFIX}
PATHS "${_executorch_package_root}/extension/pybindings/"
)
tools/cmake/executorch-wheel-config.cmake:68
executorch-config.cmakecan be processed more than once in a single configure (e.g., multiplefind_package(executorch)calls via subprojects). Unconditionally callingadd_library(executorch::runtime ...)will then error with a duplicate target name. Guard theadd_librarycall withif(NOT TARGET executorch::runtime)but still apply the target properties afterward.
This issue also appears in the following locations of the same file:
- line 117
- line 117
add_library(executorch::runtime SHARED IMPORTED)
set_target_properties(
executorch::runtime
PROPERTIES IMPORTED_LOCATION "${_executorch_runtime_library}"
INTERFACE_INCLUDE_DIRECTORIES "${EXECUTORCH_INCLUDE_DIRS}"
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
shoumikhin
added a commit
that referenced
this pull request
Jul 31, 2026
Today `pip install executorch` gives you the Python half of ExecuTorch but
nothing a C++ application can link. The wheel installs a header subset and an
`executorch-config.cmake`, but that config only locates the Python extension so
custom-op builds can compile against it: it defines no runtime library and no
CMake targets. Writing a small C++ program that loads and runs a `.pte` means
cloning the repo, syncing submodules, and building from source.
This change ships the runtime as a real library and exposes it through
`find_package`, so a standalone application needs nothing but the wheel:
find_package(executorch REQUIRED)
target_link_libraries(my_app PRIVATE executorch::runtime)
`executorch::runtime` is an imported target whose location is resolved relative
to the config file itself, so the package stays relocatable and no path from the
machine that built the wheel is baked into it. A consumer picks up the wheel's
library directory in its RUNPATH, plus `$ORIGIN`-relative entries so an
application deployed next to a copy of the runtime keeps working without
`LD_LIBRARY_PATH`.
The Python extensions have to move to the shared runtime in the same change.
Backends register themselves into a single process-wide table owned by the
runtime, and that table is only process-wide if exactly one loaded library
defines it. The Python extension statically embeds the runtime today, so adding a
shared library beside it would give a process two independent registries, and a
backend could register into the one nobody reads. The extensions therefore link
the shared runtime instead of whole-archiving the static libraries, leaving
exactly one registry owner for both the Python and C++ paths.
Getting that right needs two linker details. The shared runtime is named through
a link option rather than an ordinary dependency, because CMake orders link
libraries so a static archive precedes what it depends on, which would let the
archive satisfy the runtime symbols first. It is also wrapped in
`--no-as-needed`, because a shared library with no already-referenced symbol at
the point it appears on the link line can be dropped, and a later static archive
would then supply the registry after all.
Registration still happens through static initializers exactly as before. No new
plugin or loader ABI is introduced.
The shared runtime is Linux-only for the wheel. macOS C++ consumers are served by
the existing Swift package distribution, and the runtime has no export
annotations for a Windows DLL. Every other build keeps linking the static
libraries, because the new behavior is gated on the existing
`EXECUTORCH_BUILD_SHARED` option, so iOS, Android, and embedded builds are
unaffected.
Test plan:
The wheel smoke test now covers this on Linux, so it is checked in CI rather than
only by hand. It asserts that exactly one shipped library defines the backend
registry, builds a standalone C++ program that only calls
`find_package(executorch)` and links `executorch::runtime`, runs it without
`LD_LIBRARY_PATH`, and checks the resulting binary depends on the shipped runtime
with a relocatable RUNPATH. The check was confirmed to fail when a second
registry definition is introduced deliberately. The wheel workflows now also run
when any `CMakeLists.txt` or anything under `tools/cmake` changes, since those
files decide what the wheel contains.
Built the wheel from a clean checkout on Linux x86_64 and on Linux aarch64, then
verified each against a fresh virtual environment with a normal
dependency-resolving install:
- `nm -DC` across every shipped shared object shows exactly one definition of the
registry entry points; the Python extensions import them rather than defining
their own.
- A C++ program whose CMake only calls `find_package(executorch)` and links
`executorch::runtime` compiles against the installed wheel with no source
checkout, then loads a `.pte` and lists its methods.
- `readelf -d` on that program lists the versioned runtime in `DT_NEEDED` with
`$ORIGIN`-relative RUNPATH entries, and it runs without `LD_LIBRARY_PATH`.
- A C++ binary that links only `executorch::runtime` and loads the Python
extension sees the backends that extension registered, which is the
one-registry property stated above.
- `import executorch`, the registered backend list, and `.pte` execution through
the Python bindings are unchanged, with outputs matching eager PyTorch.
- With `EXECUTORCH_BUILD_SHARED` off, the Python extension still builds
self-contained and no shared runtime is produced, so the previous behavior is
intact.
- Configured with CMake 3.28 as well as 3.31, since the project supports 3.24 and
up and the two differ in how strictly they treat link features.
ghstack-source-id: 401229a
ghstack-comment-id: 5139976402
Pull-Request: #21514
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
Several new $ORIGIN-relative RPATH adjustments appear wheel-layout-specific but are not guarded as wheel-only, which can break non-wheel shared builds/installs when EXECUTORCH_BUILD_SHARED is enabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (9)
backends/qualcomm/CMakeLists.txt:387
- This RPATH setting assumes the wheel layout (runtime under executorch/lib). For non-wheel shared installs where libexecutorch.so is in ${CMAKE_INSTALL_LIBDIR}, $ORIGIN/../../../lib may be incorrect and can break loading. Consider gating this wheel-specific RPATH on EXECUTORCH_BUILD_WHEEL_DO_NOT_USE.
if(NOT APPLE)
CMakeLists.txt:1143
- The added
$ORIGIN-relative RPATH entry assumes the wheel layout (runtime under executorch/lib). In a non-wheel shared build/install, libexecutorch.so installs to $ {CMAKE_INSTALL_LIBDIR} (e.g. /lib), so $ORIGIN/../../lib would resolve to /executorch/lib and can break runtime loading. Since this path is wheel-specific, gate it on EXECUTORCH_BUILD_WHEEL_DO_NOT_USE (or compute the correct relative path for non-wheel installs).
set(_portable_lib_rpath "$ORIGIN/../../../torch/lib")
if(EXECUTORCH_BUILD_SHARED)
string(APPEND _portable_lib_rpath ":$ORIGIN/../../lib")
endif()
codegen/tools/CMakeLists.txt:54
- This
$ORIGIN/../../lib RPATH assumes the wheel layout (runtime shipped under executorch/lib). In a normal CMake install, executorch_shared goes to $ {CMAKE_INSTALL_LIBDIR}, so this relative path will be wrong and can prevent the module from finding libexecutorch.so at runtime. Consider gating the wheel-specific RPATH on EXECUTORCH_BUILD_WHEEL_DO_NOT_USE.
if(NOT APPLE)
extension/training/CMakeLists.txt:94
- This RPATH block hardcodes the wheel’s internal layout (runtime under executorch/lib). For non-wheel shared builds, libexecutorch.so installs to ${CMAKE_INSTALL_LIBDIR}, so this can be incorrect and break loading. Since the comment explicitly describes the wheel layout, guard this block with EXECUTORCH_BUILD_WHEEL_DO_NOT_USE.
if(EXECUTORCH_BUILD_SHARED AND NOT APPLE)
extension/llm/runner/CMakeLists.txt:141
- This wheel-layout RPATH addition assumes the runtime is in executorch/lib; that is not true for a standard CMake install where libexecutorch.so goes to ${CMAKE_INSTALL_LIBDIR}. To avoid breaking non-wheel shared builds, gate this on EXECUTORCH_BUILD_WHEEL_DO_NOT_USE.
if(EXECUTORCH_BUILD_SHARED)
extension/llm/custom_ops/CMakeLists.txt:119
- This
$ORIGIN/../../../lib RPATH is described as matching the wheel layout (runtime under executorch/lib). In a standard install, executorch_shared is in $ {CMAKE_INSTALL_LIBDIR}, so this relative path can be wrong and break runtime loading. Gate this wheel-specific adjustment on EXECUTORCH_BUILD_WHEEL_DO_NOT_USE.
if(EXECUTORCH_BUILD_SHARED)
kernels/quantized/CMakeLists.txt:89
- This RPATH is described as matching the wheel layout (runtime shipped under executorch/lib). For non-wheel shared builds/install, libexecutorch.so is under ${CMAKE_INSTALL_LIBDIR}, so $ORIGIN/../../lib can be incorrect. Consider guarding this block with EXECUTORCH_BUILD_WHEEL_DO_NOT_USE to avoid breaking non-wheel shared builds.
if(EXECUTORCH_BUILD_SHARED AND NOT APPLE)
kernels/quantized/CMakeLists.txt:144
- This wheel-layout RPATH append assumes the runtime is under executorch/lib. In non-wheel shared installs, libexecutorch.so is under ${CMAKE_INSTALL_LIBDIR}, so this relative path can be wrong. Gate this on EXECUTORCH_BUILD_WHEEL_DO_NOT_USE (or compute the correct relative path for non-wheel installs).
if(EXECUTORCH_BUILD_SHARED)
backends/qualcomm/CMakeLists.txt:265
- This
$ORIGIN/../../lib RPATH assumes the wheel layout (runtime under executorch/lib). In a normal CMake install, executorch_shared is in $ {CMAKE_INSTALL_LIBDIR}, so this relative path can be incorrect and break runtime loading. Since wheel-only layout logic should be guarded, consider adding EXECUTORCH_BUILD_WHEEL_DO_NOT_USE to this condition.
This issue also appears on line 387 of the same file.
if(NOT APPLE)
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Human review recommended
It makes wide-reaching build/linker/packaging changes whose safety depends on subtle link-line and runtime loader behavior across multiple targets and needs final human verification.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
Author
|
Thanks, the RPATH observations across these files were right and I have fixed all Every So in a non-wheel shared install, Each of these now also requires
Verified on a freshly built wheel: the extension still carries both |
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The wheel CMake config has two verified correctness issues (stale cached _portable_lib_LIBRARY when skipping Python, and non-propagating C++20 requirement for _portable_lib) that can break downstream consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
tools/cmake/executorch-wheel-config.cmake:132
- When Python is not usable but the shared runtime target exists, this branch sets EXT_SUFFIX to "" and skips find_library(), but _portable_lib_LIBRARY is a cache variable. If it was found in a previous configure (or from a different wheel), it can remain set and the later
if(_portable_lib_LIBRARY)block will still run, incorrectly offering/linking the Python extension even though this code path is explicitly skipping it. Clear the cache + normal variable in this branch.
elseif(TARGET executorch::runtime)
# A C++ application linking only the shared runtime does not need Python at
# all, so a missing interpreter must not fail its configure. Skip locating the
# Python extension instead; the legacy _portable_lib target is simply not
# offered in that case.
message(
STATUS
"Python not usable, skipping the Python extension: ${SYSCONFIG_ERROR}"
)
set(EXT_SUFFIX "")
tools/cmake/executorch-wheel-config.cmake:162
- This imported
_portable_libtarget tries to enforce C++20 viaCXX_STANDARD 20, butCXX_STANDARDdoes not propagate to consumers (and imported targets have no sources to compile). Downstream custom-op builds that link_portable_libmay still compile as C++17 and fail. Use an interface requirement likeINTERFACE_COMPILE_FEATURES cxx_std_20so it applies to consumer targets.
set_target_properties(
_portable_lib
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
Author
|
Both of the latest points were correct and are now fixed. Stale cached library path. The branch that skips the Python extension when no The config was saying one thing and doing another. It now clears the cache entry C++ standard on the imported target. Also, for completeness on the earlier batch: every wheel-layout RPATH is now gated |
Contributor
There was a problem hiding this comment.
🟡 Human review recommended
It changes core build/link behavior and wheel packaging/RPATH mechanics across many targets, which warrants final human verification despite the added CI tests.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The QNN backend’s shared-runtime linkage can run before executorch_shared is defined (top-level ordering), which will break configurations that enable both EXECUTORCH_BUILD_QNN and EXECUTORCH_BUILD_SHARED.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tools/cmake/executorch-wheel-config.cmake:176
- This config sets
EXECUTORCH_FOUND, butfind_package(executorch REQUIRED)gates onexecutorch_FOUND(the package name +_FOUND). As written,find_package(... REQUIRED)can succeed even whenEXECUTORCH_FOUNDremains OFF (e.g., if neither the runtime nor_portable_libis found), which contradicts the documented usage in this file.
endif()
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+260
to
+265
| # Resolve the runtime from the shared library when one is built, so this | ||
| # delegate does not carry its own copy of the backend registry. | ||
| if(EXECUTORCH_BUILD_SHARED) | ||
| target_link_libraries(qnn_executorch_backend PRIVATE executorch_shared) | ||
| executorch_target_link_shared_runtime(qnn_executorch_backend) | ||
| if(NOT APPLE AND EXECUTORCH_BUILD_WHEEL_DO_NOT_USE) |
Contributor
There was a problem hiding this comment.
🟡 Human review recommended
It makes wide-ranging, linker-order-sensitive build/packaging changes across multiple CMake targets and platforms that merit final human validation beyond automated review.
Review details
Suppressed comments (1)
kernels/quantized/CMakeLists.txt:92
- The wheel-specific BUILD_RPATH/INSTALL_RPATH block for quantized_ops_aot_lib is later overwritten when
TARGET portable_libis true (anotherset_target_properties(... BUILD_RPATH ... INSTALL_RPATH ...)runs below). This makes the first block redundant in the common pybind+wheel case and creates two independent places to maintain the runtime rpath logic.
if(EXECUTORCH_BUILD_SHARED
AND NOT APPLE
AND EXECUTORCH_BUILD_WHEEL_DO_NOT_USE
)
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Human review recommended
It makes broad, platform- and linker-sensitive build/distribution changes (CMake linking/RPATH/runtime packaging) that warrant final human verification across supported build modes and environments.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
gen_custom_ops_aot_lib() in shared builds can lose required ExecuTorch usage requirements (include dirs/compile defs) when portable_lib is absent, which can break shared+non-pybind configurations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
263
to
+267
| target_link_libraries(${GEN_LIB_NAME} PRIVATE portable_lib) | ||
| else() | ||
| elseif(NOT EXECUTORCH_BUILD_SHARED) | ||
| target_link_libraries(${GEN_LIB_NAME} PRIVATE executorch_core) | ||
| endif() | ||
| executorch_target_link_shared_runtime(${GEN_LIB_NAME}) |
Contributor
There was a problem hiding this comment.
🟡 Human review recommended
The change spans core build/link semantics and wheel packaging across many targets/platform conditionals, and needs careful human validation despite added CI coverage.
Review details
Suppressed comments (1)
tools/cmake/executorch-wheel-config.cmake:163
_portable_libis located viafind_libraryusing the wheel’s Python extension suffix (i.e., it’s a shared object), but it’s declared asSTATIC IMPORTED. That type mismatch can cause CMake to treat it like an archive (and may affect link behavior/rpath handling for custom-op builds). Declare it as an imported shared library instead.
if(NOT TARGET _portable_lib)
add_library(_portable_lib STATIC IMPORTED)
endif()
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The wheel CMake config’s legacy _portable_lib imported target doesn’t currently propagate C10_USING_CUSTOM_GENERATED_MACROS, which can break custom-op consumers compiling against the wheel’s headers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tools/cmake/executorch-wheel-config.cmake:179
- The legacy
_portable_libimported target doesn’t propagateC10_USING_CUSTOM_GENERATED_MACROSto consumers, but the same headers are exposed viaEXECUTORCH_INCLUDE_DIRS. If the wheel doesn’t shiptorch/headeronly/macros/cmake_macros.h, custom-op builds that link_portable_libcan fail to compile withcmake_macros.hnot found (the runtime target already sets this definition).
set_target_properties(
_portable_lib
PROPERTIES IMPORTED_LOCATION "${_portable_lib_LIBRARY}"
INTERFACE_INCLUDE_DIRECTORIES "${EXECUTORCH_INCLUDE_DIRS}"
# An interface requirement rather than CXX_STANDARD: an imported
# target compiles nothing itself, and CXX_STANDARD does not reach
# consumers, so a custom-op build linking this could still
# compile
# as C++17 and fail against headers that need C++20.
INTERFACE_COMPILE_FEATURES cxx_std_20
)
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The CMake package config’s “skip Python extension” path can still leave _portable_lib_LIBRARY set in normal scope, potentially advertising a stale _portable_lib target despite intentionally skipping Python discovery.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tools/cmake/executorch-wheel-config.cmake:136
- When Python sysconfig lookup fails, this code unsets only the cache entry for
_portable_lib_LIBRARY. In CMake, a stale normal-scope value can still remain (e.g., populated from the cache at the start of configure), so the laterif(_portable_lib_LIBRARY)block can still run and incorrectly offer the_portable_libimported target even though this configure intentionally skipped locating it. Clearing the normal variable as well makes the skip reliable.
set(EXT_SUFFIX "")
# find_library caches its result, so a value left by an earlier configure
# would survive and the extension would still be offered despite being skipped
# here.
unset(_portable_lib_LIBRARY CACHE)
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The new wheel smoke test currently skips many native-import failures and can miss real ABI/linkage regressions, so the CI coverage claimed in the PR description is not fully enforced.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
.ci/scripts/wheel/test_cpp_sdk.py:265
test_python_extensions_importtreats most import failures as “unrelated” and skips them. That can hide real wheel/linkage regressions (e.g.,undefined symbol,symbol lookup error, orGLIBCXX/GLIBCversion mismatches) that indicate a native dependency/ABI problem rather than a missing Python package.
# A Python dependency that is simply not installed here, including torch,
# says nothing about how the wheel was built. Only a failure to load a
# native library does.
if "ModuleNotFoundError" in result.stderr:
print(f"- {module} needs a package this environment lacks, skipping")
continue
assert "cannot open shared object file" not in result.stderr, (
f"{module} ships in the wheel but cannot load a native dependency, "
f"which usually means a runtime path does not reach it: "
f"{result.stderr.strip()[-400:]}"
)
print(f"- {module} did not import for an unrelated reason, skipping")
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Human review recommended
It introduces broad, cross-cutting build/link/packaging behavior changes (shared runtime + link-order semantics + wheel relocatability) that warrant final human verification across platforms/toolchains.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contributor
There was a problem hiding this comment.
🟡 Human review recommended
It makes wide-reaching build/link/packaging changes across CMake, wheel contents, and multiple extension targets where subtle platform/linker behavior regressions warrant final human verification.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Today
pip install executorchgives you the Python half of ExecuTorch butnothing a C++ application can link. The wheel installs a header subset and an
executorch-config.cmake, but that config only locates the Python extension socustom-op builds can compile against it: it defines no runtime library and no
CMake targets. Writing a small C++ program that loads and runs a
.ptemeanscloning the repo, syncing submodules, and building from source.
This change ships the runtime as a real library and exposes it through
find_package, so a standalone application needs nothing but the wheel:executorch::runtimeis an imported target whose location is resolved relativeto the config file itself, so the package stays relocatable and no path from the
machine that built the wheel is baked into it. A consumer picks up the wheel's
library directory in its RUNPATH, plus
$ORIGIN-relative entries so anapplication deployed next to a copy of the runtime keeps working without
LD_LIBRARY_PATH.The Python extensions have to move to the shared runtime in the same change.
Backends register themselves into a single process-wide table owned by the
runtime, and that table is only process-wide if exactly one loaded library
defines it. The Python extension statically embeds the runtime today, so adding a
shared library beside it would give a process two independent registries, and a
backend could register into the one nobody reads. The extensions therefore link
the shared runtime instead of whole-archiving the static libraries, leaving
exactly one registry owner for both the Python and C++ paths.
Getting that right needs two linker details. The shared runtime is named through
a link option rather than an ordinary dependency, because CMake orders link
libraries so a static archive precedes what it depends on, which would let the
archive satisfy the runtime symbols first. It is also wrapped in
--no-as-needed, because a shared library with no already-referenced symbol atthe point it appears on the link line can be dropped, and a later static archive
would then supply the registry after all.
Registration still happens through static initializers exactly as before. No new
plugin or loader ABI is introduced.
The shared runtime is Linux-only for the wheel. macOS C++ consumers are served by
the existing Swift package distribution, and the runtime has no export
annotations for a Windows DLL. Every other build keeps linking the static
libraries, because the new behavior is gated on the existing
EXECUTORCH_BUILD_SHAREDoption, so iOS, Android, and embedded builds areunaffected.
Test plan:
The wheel smoke test now covers this on Linux, so it is checked in CI rather than
only by hand. It asserts that exactly one shipped library defines the backend
registry, builds a standalone C++ program that only calls
find_package(executorch)and linksexecutorch::runtime, runs it withoutLD_LIBRARY_PATH, and checks the resulting binary depends on the shipped runtimewith a relocatable RUNPATH. The check was confirmed to fail when a second
registry definition is introduced deliberately. The wheel workflows now also run
when any
CMakeLists.txtor anything undertools/cmakechanges, since thosefiles decide what the wheel contains.
Built the wheel from a clean checkout on Linux x86_64 and on Linux aarch64, then
verified each against a fresh virtual environment with a normal
dependency-resolving install:
nm -DCacross every shipped shared object shows exactly one definition of theregistry entry points; the Python extensions import them rather than defining
their own.
find_package(executorch)and linksexecutorch::runtimecompiles and runs against the installed wheel with nosource checkout. It initializes the runtime, queries the backend registry, and
compiles against the shipped program-loading header to confirm it is usable.
readelf -don that program lists the versioned runtime inDT_NEEDEDwith$ORIGIN-relative RUNPATH entries, and it runs withoutLD_LIBRARY_PATH.executorch::runtimeand loads the Pythonextension sees the backends that extension registered, which is the
one-registry property stated above.
import executorch, the registered backend list, and.pteexecution throughthe Python bindings are unchanged, with outputs matching eager PyTorch.
EXECUTORCH_BUILD_SHAREDoff, the Python extension still buildsself-contained and no shared runtime is produced, so the previous behavior is
intact.
up and the two differ in how strictly they treat link features.