Skip to content

[CI] Try to fix test_model.sh #5361

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

Closed
larryliu0820 wants to merge 1 commit into from
Closed

[CI] Try to fix test_model.sh #5361

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 6 additions & 9 deletions .ci/scripts/test_model.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,21 @@ prepare_artifacts_upload() {

build_cmake_executor_runner() {
echo "Building executor_runner"
(rm -rf ${CMAKE_OUTPUT_DIR} \
&& mkdir ${CMAKE_OUTPUT_DIR} \
&& cd ${CMAKE_OUTPUT_DIR} \
&& retry cmake -DCMAKE_BUILD_TYPE=Release \
rm -rf ${CMAKE_OUTPUT_DIR}
cmake -DCMAKE_BUILD_TYPE=Debug \
Copy link
Contributor

@dbort dbort Sep 13, 2024

Choose a reason for hiding this comment

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

Should this use retry cmake ... to keep the logic from before? Or are you removing it intentionally?

Copy link
Contributor

@dbort dbort Sep 13, 2024

Choose a reason for hiding this comment

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

Please mention the move from Release to Debug in the PR summary. Consider adding a comment here explaining why we use this build mode here.

But besides that and removing retry, I don't see a behavior change here: it still removes the directory and generates the cmake system.

-DEXECUTORCH_BUILD_KERNELS_OPTIMIZED=ON \
-DPYTHON_EXECUTABLE="$PYTHON_EXECUTABLE" ..)
-DPYTHON_EXECUTABLE="$PYTHON_EXECUTABLE" \
-B${CMAKE_OUTPUT_DIR} .

cmake --build ${CMAKE_OUTPUT_DIR} -j4
cmake --build ${CMAKE_OUTPUT_DIR} -j4 --config Debug
}

run_portable_executor_runner() {
# Run test model
if [[ "${BUILD_TOOL}" == "buck2" ]]; then
buck2 run //examples/portable/executor_runner:executor_runner -- --model_path "./${MODEL_NAME}.pte"
elif [[ "${BUILD_TOOL}" == "cmake" ]]; then
if [[ ! -f ${CMAKE_OUTPUT_DIR}/executor_runner ]]; then
Copy link
Contributor

@swolchok swolchok Sep 13, 2024

Choose a reason for hiding this comment

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

not sure that this would've broken anything, but it does look suspicious. nice find!

Copy link
Contributor

@dbort dbort Sep 13, 2024

Choose a reason for hiding this comment

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

If this does fix it, it feels like there's a higher-level issue that this is working around. For a given commit/PR, calling build_cmake_executor_runner once or five times should create the same result, so caching should be safe. But if skipping the cache fixes things, then it implies that there's possibly an older version of CMAKE_OUTPUT_DIR sitting around. And if it's left over from a previous job run, then we have some pretty serious hermeticity issues.

But if it's not left over from a previous run, then are we calling run_portable_executor_runner multiple times in a single job? And why would one call produce a different executor_runner binary than another call? The code in the repo hasn't changed, and it's always built with the same cmake flags.

Unless calls to this are interleaved with some other "cmake" call that itself overwrites CMAKE_OUTPUT_DIR with a different build configuration.

build_cmake_executor_runner
fi
build_cmake_executor_runner
./${CMAKE_OUTPUT_DIR}/executor_runner --model_path "./${MODEL_NAME}.pte"
else
echo "Invalid build tool ${BUILD_TOOL}. Only buck2 and cmake are supported atm"
Expand Down