-
Notifications
You must be signed in to change notification settings - Fork 461
Add OOT platform E2E test case to be run in the vllm buildkite pipeline #3154
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.
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
Conversation
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This PR adds a new E2E test case. However, it introduces a critical security vulnerability in how the configuration is fetched for the pipeline. Additionally, the new test is missing assertions to validate its behavior. Please see the detailed comments for both of these high-priority issues.
# Base docker image used to build the vllm-ascend e2e test image, which is built in the vLLM repository | ||
BASE_IMAGE_NAME="quay.io/ascend/cann:8.3.rc1.alpha002-910b-ubuntu22.04-py3.11" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the PR description, this file is intended to be downloaded and executed via source
in a CI/CD pipeline. Sourcing shell scripts from a URL, especially one pointing to a personal fork (https://raw.githubusercontent.com/leo-pony/vllm-ascend/...
), is a critical security vulnerability that can lead to arbitrary code execution. An attacker who gains control of the source repository can inject malicious commands into this file, which would then be executed by the build pipeline.
This configuration should be managed securely. For example, it could be part of the main repository, or fetched from a secure, trusted artifact storage. Please reconsider this approach.
with VllmRunner("Qwen/Qwen3-0.6B", | ||
max_model_len=4096, | ||
gpu_memory_utilization=0.7) as runner: | ||
runner.generate(example_prompts, sampling_params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case currently lacks assertions to verify the output of the runner.generate
call. A test without assertions can only catch crashes but won't detect incorrect behavior or regressions in the model's output. Please add assertions to validate the generated text.
Also, note that with temperature=0.0
, the sampling is greedy, and the top_k
and top_p
parameters will have no effect. If the goal is to test top_k
/top_p
functionality, you should use a temperature > 0
.
runner.generate(example_prompts, sampling_params) | |
outputs = runner.generate(example_prompts, sampling_params) | |
assert outputs, "The model should generate output." | |
assert len(outputs) == len(example_prompts), "Should get one output for each prompt." | |
# With temperature=0.0, output is deterministic. It's highly recommended | |
# to assert on the exact expected output for a robust test. | |
generated_text = outputs[0][1][0] | |
assert len(generated_text) > len(example_prompts[0]), "The model should generate new text." |
Signed-off-by: leo-pony <nengjunma@outlook.com>
…ne (vllm-project#3154) ### What this PR does / why we need it? Add OOT platform E2E test case to be run in the vllm buildkite pipeline. Note: added test case is not run in vllm-ascend CI. ### Does this PR introduce _any_ user-facing change? NA - vLLM version: v0.10.2 - vLLM main: vllm-project/vllm@f225ea7 Signed-off-by: leo-pony <nengjunma@outlook.com> Signed-off-by: huangdong2022 <huangdong51@huawei.com>
What this PR does / why we need it?
Add OOT platform E2E test case to be run in the vllm buildkite pipeline.
Note: added test case is not run in vllm-ascend CI.
Does this PR introduce any user-facing change?
NA
How was this patch tested?
NA
Test result:


1.Test case: has run successfully in local host:
2.Added oot_test.cfg can been successfully get and parsed:

3.Format test passed:
