Make flatcc cross-compile deterministic #4312

haowhsu-quic · 2024-07-19T00:19:40Z

As mentioned in #4297, the original flow makes host / cross build happen concurrently. This change moves host build process into cmake configuring stage and refine related dependencies.

Test Plan:

cross-compile > Through running backends/qualcomm/script/build.sh --release, we could check if the compiling process successfully finished.
native-compile > Run following to check:

cmake \
    -DCMAKE_BUILD_TYPE=RelWithDebInfo \
    -DQNN_SDK_ROOT=${QNN_SDK_ROOT} \
    -DEXECUTORCH_BUILD_QNN=ON \
    -DEXECUTORCH_BUILD_SDK=ON \
    -DPYTHON_EXECUTABLE=$PYTHON_EXECUTABLE \
    -S $EXECUTORCH_ROOT \
    -B $EXECUTORCH_ROOT/build_x86_64 \

pytorch-bot · 2024-07-19T00:19:43Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4312

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 New Failures

As of commit 796a634 with merge base 711ecec ():

NEW FAILURES - The following jobs have failed:

pull / test-export-llava-linux / linux-job (gh)
RuntimeError: Command docker exec -t ad4bf1a9748a0c40b782e8f73a7e426ce19b04addbb77b8861ee33d5502c72f7 /exec failed with exit code 1
trunk / test-coreml-delegate / macos-job (gh)
RuntimeError: Command bash /Users/runner/work/_temp/exec_script failed with exit code 1
trunk / test-models-macos (cmake, mv3, xnnpack-quantization-delegation, macos-m1-stable, 90) / macos-job (gh)
Process completed with exit code 1.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

dbort · 2024-07-24T01:13:25Z

I'm sorry for the delay on this, I'm taking a look

dbort

What we really need to do is send a PR to the flatcc upstream project, adding a cmake option to put the binaries in a different location (outside of the source tree).

Another option could even be to recursively copy the flatcc tree under cmake-out and build it from there for the host tools.

dbort · 2024-07-24T01:14:38Z

sdk/CMakeLists.txt

-  # Add the host project. We build this separately so that we can generate
-  # headers on the host during the build, even if we're cross-compiling the
-  # flatcc runtime to a different architecture.


Please preserve this comment to help readers understand why this is built in a special way.

Will do, thanks.

dbort · 2024-07-24T01:21:29Z

sdk/CMakeLists.txt

-  COMMAND rm -f ${_etdump_schema_cleanup_paths}
-  WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/sdk


Even when building ahead of time, the flatcc build will still put the host binaries in _etdump_schema_cleanup_paths. If we don't delete those here, then we need to be sure that the cross-compiled binaries will always overwrite the host binaries.

Did you find that it was necessary to remove this rm -f to make things work? Could we keep it to keep things a bit safer for the cross-compiled binaries?

Yes, it's a good idea, changed. Previously I thought we only link libraries in cross-compile stage, it might not be necessary to delete artifacts.
But to make sure the deletion really happens, VERBATIM should be removed or quote marks will be added to the target which will make rm fail.
I also think WORKING_DIRECTORY could be removed since we do not build test executables.

haowhsu-quic · 2024-07-24T07:43:52Z

What we really need to do is send a PR to the flatcc upstream project, adding a cmake option to put the binaries in a different location (outside of the source tree).

Another option could even be to recursively copy the flatcc tree under cmake-out and build it from there for the host tools.

Both approaches are good! Since I do encounter failure under current scenario, this is the fastest way I could come up with to resolve issue.
If you already have plan, feel free to withdraw this PR.

dbort

Thanks for these changes. Can you also update the PR description with a "Test Plan:" section that describes how you tested that a) this fixes your problem, and b) it doesn't break things for non-cross-compiling use cases? (I added a placeholder Test Plan section to the PR description)

dbort · 2024-07-24T22:50:59Z

sdk/CMakeLists.txt

+add_custom_target(
+  etdump_schema_generated
+  DEPENDS ${_etdump_schema__outputs}
+)
+
+add_dependencies(etdump_schema etdump_schema_generated)


Could these DEPENDS be directly on etdump_schema instead of creating this extra target?

Up above we already say add_library(etdump_schema INTERFACE ${_etdump_schema__outputs}) -- is that enough to add the dependencies? Or maybe you could also add a DEPENDS section to that same add_library?

If this custom_target is necessary, please arrange the code so that it's closer to the add_library(etdump_schema, ...), and add a comment explaining why these deps can't be added directly to the library target.

Yes, it can. Thanks for the hint, that's much cleaner.

dbort · 2024-07-25T17:30:24Z

Thank you! This is much more straightforward now. I'll start the internal testing process.

facebook-github-bot · 2024-07-25T17:30:53Z

@dbort has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

dbort · 2024-07-25T23:12:25Z

sdk/CMakeLists.txt

+    -DFLATCC_TEST=OFF -DFLATCC_REFLECTION=OFF -DCMAKE_POSITION_INDEPENDENT_CODE=ON
+    -B${CMAKE_BINARY_DIR}/_host_build


One last request: please wrap this to fit in 80 columns (to satisfy an internal lint checker), and please restore the comment that was here before.

Suggested change

-DFLATCC_TEST=OFF -DFLATCC_REFLECTION=OFF -DCMAKE_POSITION_INDEPENDENT_CODE=ON

-B${CMAKE_BINARY_DIR}/_host_build

-DFLATCC_TEST=OFF -DFLATCC_REFLECTION=OFF

# See above comment about POSITION_INDEPENDENT_CODE.

-DCMAKE_POSITION_INDEPENDENT_CODE=ON

-B${CMAKE_BINARY_DIR}/_host_build

Thank you, is the cmake-format used for linter? We'll keep in mind for other changes.

If you run cmake-format, then the linters will be happy. Unfortunately, right now we have different linters in github (lintrunner) and inside of Meta, so they don't always agree, and there's no way for you to see the Meta-internal linters. We plan to disable the internal linters so that lintrunner in github is the source of truth.

Thanks for making this final fix! I'll try landing again today.

facebook-github-bot · 2024-07-26T17:50:13Z

@dbort has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

dbort · 2024-07-26T21:31:31Z

Some of the CI jobs are timing out after 90 minutes; usually they take <10 minutes. I doubt it's because of this PR, but I want to make sure they pass before landing this. Looking into it.

dbort · 2024-07-31T00:05:57Z

@haowhsu-quic could you please rebase this on top of a recent main commit? I can't update it myself because it's based on the CodeLinaro fork.

facebook-github-bot · 2024-07-31T16:53:50Z

@dbort has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

dbort · 2024-08-02T00:22:36Z

We're still trying to figure out why these jobs are failing; something is weird here.

dbort · 2024-08-02T00:28:26Z

I cloned this PR into #4515 to see if the jobs still fail there.

dbort · 2024-08-02T18:48:35Z

The cloned PR passed all tests, so I'm comfortable force-landing this PR. I'm trying again now.

facebook-github-bot · 2024-08-02T19:26:54Z

@dbort merged this pull request in 15815dd.

dbort · 2024-08-02T22:16:52Z

This broke the test-coreml-delegate build:

2024-08-02T20:11:02.3809110Z 
2024-08-02T20:11:02.3810550Z Ld /Users/runner/Library/Developer/Xcode/DerivedData/coreml_executor_runner-bkoiqshilmuvptcxjvpygyufxpnf/Build/Intermediates.noindex/InstallIntermediates/coreml_executor_runner-macosx/InstallableProducts/usr/local/bin/coreml_executor_runner normal (in target 'coreml_executor_runner' from project 'coreml_executor_runner')
2024-08-02T20:11:02.3812410Z     cd /Users/runner/work/executorch/executorch/pytorch/executorch/examples/apple/coreml/executor_runner
2024-08-02T20:11:02.3825730Z     /Applications/Xcode_15.2.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -Xlinker -reproducible -target arm64-apple-macos13.0 -isysroot /Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk -O0 -L/Users/runner/Library/Developer/Xcode/DerivedData/coreml_executor_runner-bkoiqshilmuvptcxjvpygyufxpnf/Build/Intermediates.noindex/InstallIntermediates/coreml_executor_runner-macosx/Intermediates.noindex/EagerLinkingTBDs/Debug -L/Users/runner/work/executorch/executorch/pytorch/executorch/examples/apple/coreml/xcode-build/Debug -L/Users/runner/work/executorch/executorch/pytorch/executorch/examples/apple/coreml/executor_runner/libraries -L/Users/runner/work/executorch/executorch/pytorch/executorch/examples/apple/coreml/executor_runner/libraries -F/Users/runner/Library/Developer/Xcode/DerivedData/coreml_executor_runner-bkoiqshilmuvptcxjvpygyufxpnf/Build/Intermediates.noindex/InstallIntermediates/coreml_executor_runner-macosx/Intermediates.noindex/EagerLinkingTBDs/Debug -F/Users/runner/work/executorch/executorch/pytorch/executorch/examples/apple/coreml/xcode-build/Debug -filelist /Users/runner/Library/Developer/Xcode/DerivedData/coreml_executor_runner-bkoiqshilmuvptcxjvpygyufxpnf/Build/Intermediates.noindex/InstallIntermediates/coreml_executor_runner-macosx/Intermediates.noindex/coreml_executor_runner.build/Debug/coreml_executor_runner.build/Objects-normal/arm64/coreml_executor_runner.LinkFileList -Xlinker -object_path_lto -Xlinker /Users/runner/Library/Developer/Xcode/DerivedData/coreml_executor_runner-bkoiqshilmuvptcxjvpygyufxpnf/Build/Intermediates.noindex/InstallIntermediates/coreml_executor_runner-macosx/Intermediates.noindex/coreml_executor_runner.build/Debug/coreml_executor_runner.build/Objects-normal/arm64/coreml_executor_runner_lto.o -Xlinker -export_dynamic -Xlinker -no_deduplicate -fobjc-arc -fobjc-link-runtime -all_load -letdump -lexecutorch_no_prim_ops -lflatccrt -lcoremldelegate -framework Accelerate -lprotobuf-lite -lportable_ops_lib -framework CoreML -lportable_kernels -lsqlite3 -lexecutorch -Xlinker -no_adhoc_codesign -Xlinker -dependency_info -Xlinker /Users/runner/Library/Developer/Xcode/DerivedData/coreml_executor_runner-bkoiqshilmuvptcxjvpygyufxpnf/Build/Intermediates.noindex/InstallIntermediates/coreml_executor_runner-macosx/Intermediates.noindex/coreml_executor_runner.build/Debug/coreml_executor_runner.build/Objects-normal/arm64/coreml_executor_runner_dependency_info.dat -o /Users/runner/Library/Developer/Xcode/DerivedData/coreml_executor_runner-bkoiqshilmuvptcxjvpygyufxpnf/Build/Intermediates.noindex/InstallIntermediates/coreml_executor_runner-macosx/InstallableProducts/usr/local/bin/coreml_executor_runner
2024-08-02T20:11:02.3865510Z ld: library 'flatccrt' not found
2024-08-02T20:11:02.3866120Z clang: error: linker command failed with exit code 1 (use -v to see invocation)

I will revert this for now.

This reverts commit 15815dd, which broke the test-coreml-delegate job. Details in pytorch#4312 (comment)

Summary: This reverts commit 15815dd, which broke the test-coreml-delegate job. Details in #4312 (comment) Pull Request resolved: #4528 Reviewed By: cccclai Differential Revision: D60696305 Pulled By: dbort fbshipit-source-id: 12677330b31b9fa180b79ec9daa5eef05cfd48d7

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 19, 2024

haowhsu-quic mentioned this pull request Jul 19, 2024

Qualcomm AI Engine Direct - add program validation #4297

Closed

dbort self-requested a review July 19, 2024 00:23

dbort self-assigned this Jul 19, 2024

dbort requested changes Jul 24, 2024

View reviewed changes

dbort added the ciflow/trunk label Jul 24, 2024

haowhsu-quic force-pushed the dev_cmake_flatcc branch from 3818cb0 to 455b6f8 Compare July 24, 2024 07:44

dbort requested changes Jul 24, 2024

View reviewed changes

tarun292 approved these changes Jul 25, 2024

View reviewed changes

dbort requested changes Jul 25, 2024

View reviewed changes

dbort approved these changes Jul 31, 2024

View reviewed changes

haowhsu-quic added 4 commits July 31, 2024 09:59

Make flatcc cross-compile deterministic

902cb6d

add comments back & keep rm -f

e5edc6d

remove custom_target

aa06926

fix lint error & add comment back

796a634

haowhsu-quic force-pushed the dev_cmake_flatcc branch from e5c1847 to 796a634 Compare July 31, 2024 02:00

dbort mentioned this pull request Aug 2, 2024

Make flatcc cross-compile deterministic #4515

Closed

facebook-github-bot closed this in 15815dd Aug 2, 2024

facebook-github-bot added the Merged label Aug 2, 2024

dbort added a commit to dbort/executorch that referenced this pull request Aug 2, 2024

Revert "Make flatcc cross-compile deterministic (pytorch#4312)"

5d30136

This reverts commit 15815dd, which broke the test-coreml-delegate job. Details in pytorch#4312 (comment)

dbort mentioned this pull request Aug 2, 2024

Revert "Make flatcc cross-compile deterministic (#4312)" #4528

Closed

haowhsu-quic mentioned this pull request Aug 3, 2024

cmake flatcc build issue #4541

Merged

dbort mentioned this pull request Oct 23, 2024

Add FLATCC_BUILD_INTO_SOURCE_DIR cmake option dvidelabs/flatcc#306

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make flatcc cross-compile deterministic #4312

Make flatcc cross-compile deterministic #4312

haowhsu-quic commented Jul 19, 2024 •

edited

Loading

pytorch-bot bot commented Jul 19, 2024 •

edited

Loading

dbort commented Jul 24, 2024

dbort left a comment •

edited

Loading

dbort Jul 24, 2024

haowhsu-quic Jul 24, 2024

dbort Jul 24, 2024

haowhsu-quic Jul 24, 2024

haowhsu-quic commented Jul 24, 2024

dbort left a comment

dbort Jul 24, 2024

haowhsu-quic Jul 25, 2024

dbort commented Jul 25, 2024

facebook-github-bot commented Jul 25, 2024

dbort Jul 25, 2024

haowhsu-quic Jul 26, 2024

dbort Jul 26, 2024

facebook-github-bot commented Jul 26, 2024

dbort commented Jul 26, 2024

dbort commented Jul 31, 2024

facebook-github-bot commented Jul 31, 2024

dbort commented Aug 2, 2024

dbort commented Aug 2, 2024

dbort commented Aug 2, 2024

facebook-github-bot commented Aug 2, 2024

dbort commented Aug 2, 2024

		COMMAND rm -f ${_etdump_schema_cleanup_paths}
		WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/sdk

		-DFLATCC_TEST=OFF -DFLATCC_REFLECTION=OFF -DCMAKE_POSITION_INDEPENDENT_CODE=ON
		-B${CMAKE_BINARY_DIR}/_host_build

Make flatcc cross-compile deterministic #4312

Make flatcc cross-compile deterministic #4312

Conversation

haowhsu-quic commented Jul 19, 2024 • edited Loading

pytorch-bot bot commented Jul 19, 2024 • edited Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4312

❌ 3 New Failures

dbort commented Jul 24, 2024

dbort left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haowhsu-quic commented Jul 24, 2024

dbort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbort commented Jul 25, 2024

facebook-github-bot commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 26, 2024

dbort commented Jul 26, 2024

dbort commented Jul 31, 2024

facebook-github-bot commented Jul 31, 2024

dbort commented Aug 2, 2024

dbort commented Aug 2, 2024

dbort commented Aug 2, 2024

facebook-github-bot commented Aug 2, 2024

dbort commented Aug 2, 2024

haowhsu-quic commented Jul 19, 2024 •

edited

Loading

pytorch-bot bot commented Jul 19, 2024 •

edited

Loading

dbort left a comment •

edited

Loading