Avoid duplicate ops registration in macOS executor_runner#19804
Conversation
The macOS preset enables both EXECUTORCH_BUILD_KERNELS_OPTIMIZED and EXECUTORCH_BUILD_EXECUTOR_RUNNER, so the top-level CMake selects optimized_native_cpu_ops_lib as the runner's ops registration library. coremldelegate is pulled into the runner's link line through executorch_backends and force-loaded, and it was also privately linking portable_ops_lib and portable_kernels whenever EXECUTORCH_COREML_BUILD_EXECUTOR_RUNNER was on. The runner therefore ended up with optimized_native_cpu_ops_lib and portable_ops_lib both registering overlapping ATen kernels at static-init time, aborting before main(). Drop the misplaced portable-kernel link from coremldelegate and remove the now-unused EXECUTORCH_COREML_BUILD_EXECUTOR_RUNNER option. The option never gated building a real CMake runner target; the CoreML executor runner under examples/apple/coreml/executor_runner is an Xcode project, and the build_executor_runner.sh script that drives it stages libportable_ops_lib.a and libportable_kernels.a into examples/apple/coreml/executor_runner/libraries/ where the Xcode project links them directly, independent of libcoremldelegate.a's link list.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19804
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 Awaiting Approval, 1 New Failure, 1 Pending, 1 Unrelated Failure, 1 Unclassified FailureAs of commit ff0ba0d with merge base a89a05a ( NEW FAILURE - The following job has failed:
UNCLASSIFIED FAILURE - DrCI could not classify the following job because the workflow did not run on the merge base. The failure may be pre-existing on trunk or introduced by this PR:
BROKEN TRUNK - The following job 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. |
|
Hi @devin-lai! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
This PR needs a
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
meta-cla
Bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
|
||
| if(EXECUTORCH_COREML_BUILD_EXECUTOR_RUNNER) | ||
| target_link_libraries( | ||
| coremldelegate PRIVATE portable_ops_lib portable_kernels |
There was a problem hiding this comment.
where else this is linked?
There was a problem hiding this comment.
In the top-level CMake configuration, coremldelegate is added to _executorch_backends in CMakeLists.txt:713–715, and then brought into _executor_runner_libs through executorch_backends at CMakeLists.txt:1355–1357. Those libraries are ultimately linked into executor_runner at CMakeLists.txt:1392.
The ops registration library is chosen separately in CMakeLists.txt:1363–1369:
- optimized_native_cpu_ops_lib when EXECUTORCH_BUILD_KERNELS_OPTIMIZED=ON
- portable_ops_lib otherwise
Because of that separation, coremldelegate itself does not need to link against either ops library directly.
For the standalone CoreML Xcode runner, the setup is also independent of coremldelegate’s CMake dependencies. The script examples/apple/coreml/scripts/build_executor_runner.sh copies libportable_ops_lib.a and libportable_kernels.a into examples/apple/coreml/executor_runner/libraries/, and the Xcode project links them explicitly.
I also checked several other CMake consumers — including examples/models/llama, examples/models/llava, and examples/devtools. In each case, the ops registration library is selected explicitly before coremldelegate is force-loaded.
digantdesai
added
the
module: build/install
nil-is-all
added
the
module: coreml
|
LGTM. |
|
Claude finished @digantdesai's task in 1m 1s —— View job PR Review: Avoid duplicate ops registration in macOS executor_runner
Verdict: LGTM — clean, well-scoped fix. The change correctly identifies the root cause: the macOS preset enables What's removed:
Verification:
No concerns. The option was effectively dead for its stated purpose (building a CoreML executor runner — that target doesn't exist in CMake), and its side effect of linking portable ops into the delegate was actively harmful on macOS. CI note: The |
|
This looks like it has broken test-coreml-delegate |
### Summary The macOS preset builds `executor_runner` with optimized kernels enabled, so the top-level runner link logic already selects `optimized_native_cpu_ops_lib` as the ops registration library. `coremldelegate` was also linking `portable_ops_lib` and `portable_kernels` through `EXECUTORCH_COREML_BUILD_EXECUTOR_RUNNER`. Since `coremldelegate` is included through `executorch_backends`, `executor_runner` could force-load both `portable_ops_lib` and `optimized_native_cpu_ops_lib`. Their generated static initializers register overlapping ATen kernels, causing `executor_runner` to abort before `main()` with a duplicate registration error. This removes the CoreML-side ops-lib link and removes the obsolete `EXECUTORCH_COREML_BUILD_EXECUTOR_RUNNER` preset option. The CoreML executor runner under `examples/apple/coreml/executor_runner` is not a CMake target, so this option was not actually building that runner. ### Compatibility The `coreml_executor_runner` built by `examples/apple/coreml/scripts/build_executor_runner.sh` is unaffected. That script builds the relevant CMake targets, then stages `libportable_ops_lib.a` and `libportable_kernels.a` into `examples/apple/coreml/executor_runner/libraries/`. The Xcode project links those archives directly, independent of `libcoremldelegate.a`'s internal link list, so the Xcode-built runner keeps working through its own link line. Other CMake consumers of `coremldelegate` already select an ops registration library independently before force-loading `coremldelegate`, so they are unaffected by removing the private portable-kernel link from the delegate. Non-Apple platforms do not build `coremldelegate` because `backends/apple/coreml/CMakeLists.txt` is gated by `if(APPLE)`. The iOS and iOS-simulator presets never set the removed option. ### Test plan ```bash cmake --preset macos cmake --build cmake-out --target executor_runner --config Debug -j ./cmake-out/Debug/executor_runner cmake --build cmake-out --target coremldelegate --config Debug -j ``` Verified the `executor_runner` link line no longer contains `libportable_ops_lib.a` or unprefixed `libportable_kernels.a`. `liboptimized_native_cpu_ops_lib.a` is still force-loaded. `liboptimized_portable_kernels.a` is still present, which is expected because it is one of `optimized_native_cpu_ops_lib`'s kernel libraries. Running without `--model_path` now reaches `main()`, resets the threadpool, and fails only on the expected missing `model.pte` path instead of aborting during static kernel registration. Authored with Claude. cc @larryliu0820 @GregoryComer @kimishpatel @YifanShenSZ @cymbalrush @metascroy Co-authored-by: Digant Desai <digantdesai@meta.com>
4 participants
Summary
The macOS preset builds
executor_runnerwith optimized kernels enabled, so the top-level runner link logic already selectsoptimized_native_cpu_ops_libas the ops registration library.coremldelegatewas also linkingportable_ops_libandportable_kernelsthroughEXECUTORCH_COREML_BUILD_EXECUTOR_RUNNER. Sincecoremldelegateis included throughexecutorch_backends,executor_runnercould force-load bothportable_ops_libandoptimized_native_cpu_ops_lib. Their generated static initializers register overlapping ATen kernels, causingexecutor_runnerto abort beforemain()with a duplicate registration error.This removes the CoreML-side ops-lib link and removes the obsolete
EXECUTORCH_COREML_BUILD_EXECUTOR_RUNNERpreset option. The CoreML executor runner underexamples/apple/coreml/executor_runneris not a CMake target, so this option was not actually building that runner.Compatibility
The
coreml_executor_runnerbuilt byexamples/apple/coreml/scripts/build_executor_runner.shis unaffected. That script builds the relevant CMake targets, then stageslibportable_ops_lib.aandlibportable_kernels.aintoexamples/apple/coreml/executor_runner/libraries/. The Xcode project links those archives directly, independent oflibcoremldelegate.a's internal link list, so the Xcode-built runner keeps working through its own link line.Other CMake consumers of
coremldelegatealready select an ops registration library independently before force-loadingcoremldelegate, so they are unaffected by removing the private portable-kernel link from the delegate. Non-Apple platforms do not buildcoremldelegatebecausebackends/apple/coreml/CMakeLists.txtis gated byif(APPLE). The iOS and iOS-simulator presets never set the removed option.Test plan
Verified the
executor_runnerlink line no longer containslibportable_ops_lib.aor unprefixedlibportable_kernels.a.liboptimized_native_cpu_ops_lib.ais still force-loaded.liboptimized_portable_kernels.ais still present, which is expected because it is one ofoptimized_native_cpu_ops_lib's kernel libraries.Running without
--model_pathnow reachesmain(), resets the threadpool, and fails only on the expected missingmodel.ptepath instead of aborting during static kernel registration.Authored with Claude.
cc @larryliu0820 @GregoryComer @kimishpatel @YifanShenSZ @cymbalrush @metascroy