Consolidating executor runners in CMake - xnnpack #11239
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11239
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit dfa240b with merge base 879eee0 ( 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. |
|
@pytorchbot label "release notes: none" |
47fc752 to
a902ddd
Compare
a902ddd to
d54a72f
Compare
a6239bc to
55944a2
Compare
| @@ -153,7 +132,6 @@ endif() | |||
|
|
|||
| install( | |||
| TARGETS xnnpack_backend | |||
| DESTINATION lib | |||
There was a problem hiding this comment.
I ran cmake-lint on this file, and it reported [E1122] Duplicate keyword argument DESTINATION because this variable is overridden two lines below to what I believe is the correct destination. It's the same error and fix as in the formatting PR-L496.
.ci/scripts/test_model.sh
Outdated
| if [[ ! -f ${CMAKE_OUTPUT_DIR}/backends/xnnpack/xnn_executor_runner ]]; then | ||
| build_cmake_xnn_executor_runner | ||
| if [[ ! -f ${CMAKE_OUTPUT_DIR}/executor_runner ]]; then | ||
| build_cmake_executor_runner |
There was a problem hiding this comment.
You will have to make sure EXECUTORCH_BUILD_XNNPACK is ON, right? Maybe let build_cmake_executor_runner take a backend string and turn the option on accordingly? This can be extended to other backends.
There was a problem hiding this comment.
Sounds good, will try this out.
| @@ -150,10 +137,10 @@ test_model_with_xnnpack() { | |||
| if [[ "${BUILD_TOOL}" == "buck2" ]]; then | |||
| buck2 run //examples/xnnpack:xnn_executor_runner -- --model_path "${OUTPUT_MODEL_PATH}" | |||
There was a problem hiding this comment.
Also after this PR the internal (BUCK) behavior is different than CMake. I think it's fine for now but maybe add a TODO here?
There was a problem hiding this comment.
Added in a comment for it.
55944a2 to
8b44d68
Compare
73dd149 to
c389f68
Compare
c389f68 to
042c357
Compare
Continuing the process of consolidating the executor runners to top level CMakeLists.txt file. Completing this process for the xnnpack backend. Change also updates the CI tests to not use the generic executor_runner and removes the specific xnn_executor_runner.
Note, this change only affects the cmake build process. Similar changes will be necessary for the buck build flow.
eea93eb to
dfa240b
Compare
Summary
Continuing the process of consolidating the executor runners to top level CMakeLists.txt file. Completing this process for the xnnpack backend in the cmake build flow.
Partially Fixes #<10819>: #10819.Test plan
Successfully ran the commands in the building from source guide. Updated the github actions to not use the generic
executor_runnerand remove the invocation for the backend-specificxnn_executor_runner. Also ran the following formatinng and lint checks:cmake-format -i backends/xnnpack/CMakeLists.txtcmake-lint backends/xnnpack/CMakeLists.txtIncludes updates to documentation and tutorials (flows from examples and tutorials were verify to ensure build functionality).
Note: the buck flow side of things was unmodified, and support for that flow remains for a future change.