Skip to content

Conversation

leo-pony
Copy link
Contributor

@leo-pony leo-pony commented Sep 24, 2025

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:
f013110e-6a76-45aa-a7eb-38d9faa1b8fc
946cb5f4-d339-4f46-a6b5-fadb138670c8

2.Added oot_test.cfg can been successfully get and parsed:
9cdce3f4-1d83-46d3-ba48-d2113c7376c3

# Base ubuntu image with CANN and python installed
TEST_RUN_CONFIG_URL="https://raw.githubusercontent.com/leo-pony/vllm-ascend/refs/heads/main/tests/e2e/oot_interface/oot_test.cfg"
TEST_RUN_CONFIG_FILE="oot_test.cfg"

# This function checks for and installs 'curl' on Ubuntu if it's not found.
install_curl() {
  if ! command -v curl &> /dev/null; then
    sudo yum update
    if sudo yum install -y curl; then
      echo "'curl' was installed successfully."
    else
      echo "Error: Failed to install 'curl'. Please check your network connection and package repositories."
      return 1
    fi
  fi
  return 0
}

# Downloads test run configuration file from a remote URL.
# Loads the configuration into the current script environment.
get_config() {
  # Call the helper function to ensure curl is installed.
  install_curl
  if [ $? -ne 0 ]; then
    return 1
  fi
  
  # Use curl to download the configuration file and check if the download was successful.
  if curl -s -o "$TEST_RUN_CONFIG_FILE" "$TEST_RUN_CONFIG_URL"; then
    echo "Configuration file downloaded successfully."
    source "$TEST_RUN_CONFIG_FILE"
    return 0
  else
    echo "Error: Failed to download configuration from $TEST_RUN_CONFIG_URL."
    return 1
  fi
}

# get test running configuration.
get_config
# Check if the function call was successful. If not, exit the script.
if [ $? -ne 0 ]; then
  exit 1
fi
echo "Base docker image name of test case: $BASE_IMAGE_NAME"

3.Format test passed:
97d6dc0d-382b-47fc-bf32-94a6d8267d81

Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines +1 to +2
# 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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.

Suggested change
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>
@wangxiyuan wangxiyuan merged commit 360a736 into vllm-project:main Sep 24, 2025
16 checks passed
huangdong2022 pushed a commit to huangdong2022/vllm-ascend that referenced this pull request Sep 26, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants