[cpu][ci] Add initial set of tests for Arm CPUs#28657
[cpu][ci] Add initial set of tests for Arm CPUs#28657bigPYJ1151 merged 1 commit intovllm-project:mainfrom
Conversation
ff7dd27 to
24adbd6
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces initial CI tests for Arm CPUs by adding a new test script, updating the CPU Dockerfile with necessary dependencies, and adjusting Python requirements for the ARM architecture. The changes are well-structured for enabling ARM testing. However, I've identified a critical race condition in the new test script that could cause test flakiness due to improper process termination. Additionally, the Dockerfile can be optimized by conditionally installing dependencies only on architectures where they are required, reducing the image size for ARM builds. Addressing these points will improve the robustness and efficiency of the new CI setup.
| --model meta-llama/Llama-3.2-3B-Instruct \ | ||
| --num-prompts 20 \ | ||
| --endpoint /v1/completions | ||
| kill -s SIGTERM $server_pid &' |
There was a problem hiding this comment.
The kill command is being run in the background due to the trailing &. This will cause the docker exec command to exit immediately without waiting for the server process to be terminated. This creates a race condition where the container might be removed by the trap cleanup function before the server is properly shut down. To ensure the server is terminated before the script continues, the background operator & should be removed.
| kill -s SIGTERM $server_pid &' | |
| kill -s SIGTERM $server_pid' |
There was a problem hiding this comment.
hmm that's what other cpu tests do and it works
24adbd6 to
532d585
Compare
|
@bigPYJ1151 could you please have a look at this? |
| @@ -37,6 +37,7 @@ RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \ | |||
| && update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-12 10 --slave /usr/bin/g++ g++ /usr/bin/g++-12 \ | |||
| && curl -LsSf https://astral.sh/uv/install.sh | sh | |||
|
|
|||
| ENV CC=/usr/bin/gcc-12 CXX=/usr/bin/g++-12 | |||
There was a problem hiding this comment.
Is this change necessary? As we have set gcc/g++12 as the default gcc/g++ at the start.
There was a problem hiding this comment.
yeah it's needed because some packages need to be built from src for Arm and while gcc 12 is default compiler, "CC" isn't set by default so we set it here.
Signed-off-by: Fadi Arafeh <fadi.arafeh@arm.com>
532d585 to
7b58480
Compare
|
Hi @bigPYJ1151 - thanks for the initial review. |
Signed-off-by: Fadi Arafeh <fadi.arafeh@arm.com> Signed-off-by: Runkai Tao <rt572@physics.rutgers.edu>
Signed-off-by: Fadi Arafeh <fadi.arafeh@arm.com>
Signed-off-by: Fadi Arafeh <fadi.arafeh@arm.com>
Purpose
[cpu][ci] Add initial set of tests for Arm CPUs
Test Plan
Test Result
All tests added pass.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.