[CI] Add sglang e2e tests #1181
Conversation
luketong777
requested review from
Ann-1024,
stmatengss and
ykwd
as code owners
Summary of ChangesHello @luketong777, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's continuous integration capabilities by adding a robust end-to-end testing framework for the Mooncake project. It enables automated validation of critical functionalities, such as distributed inference with Prefill-Decode disaggregation and the integration of HiCache storage with the Mooncake backend. The changes also include comprehensive guidelines and utility scripts to facilitate the development and maintenance of future E2E tests. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a suite of end-to-end tests for sglang, leveraging the T-One platform. The changes include test scripts, Python test code, and documentation. The overall structure is good, with common logic factored out into a shared script and clear documentation for developers.
However, I've identified several critical and high-severity issues that need to be addressed:
- Critical script errors: The
common.shscript contains critical typos (gpg --dearmour, invalid shebang) that will prevent the tests from running. - High-severity correctness/portability issues: There's a hardcoded absolute path in a Python test file, an incorrect results filename that will break CI parsing, and unreliable log-grepping for determining test outcomes.
- High-severity performance issue: One of the main test scripts is highly inefficient, tearing down and recreating Docker containers for each model tested.
I've left detailed comments with suggestions for fixing these issues. Additionally, there are several medium-severity issues, such as an undefined variable in a log message and missing newlines at the end of script files. Addressing these points will significantly improve the robustness, portability, and performance of the new test suite.
scripts/tone_tests/scripts/common.sh
Outdated
| @@ -0,0 +1,267 @@ | |||
| # !/bin/bash | |||
There was a problem hiding this comment.
The shebang #! /bin/bash contains a space, which makes it invalid on many systems. This can cause the script to be executed with the wrong interpreter or fail to execute at all. Please remove the space.
| # !/bin/bash | |
| #!/bin/bash |
scripts/tone_tests/scripts/common.sh
Outdated
| fi | ||
|
|
||
| pip_cmd="" | ||
| erdm_dirver_cmd='wget -qO - http://mirrors.cloud.aliyuncs.com/erdma/GPGKEY | gpg --dearmour -o /etc/apt/trusted.gpg.d/erdma.gpg && \ |
There was a problem hiding this comment.
There are a couple of critical typos on this line that will cause the script to fail:
- The variable name
erdm_dirver_cmdis misspelled. It should likely beerdma_driver_cmd. This variable is used on lines 51 and 52, and the typo will cause a "command not found" error. - The
gpgcommand has a typo:--dearmourshould be--dearmor.
Please correct these typos at the declaration here and where the variable is used.
| erdm_dirver_cmd='wget -qO - http://mirrors.cloud.aliyuncs.com/erdma/GPGKEY | gpg --dearmour -o /etc/apt/trusted.gpg.d/erdma.gpg && \ | |
| erdma_driver_cmd='wget -qO - http://mirrors.cloud.aliyuncs.com/erdma/GPGKEY | gpg --dearmor -o /etc/apt/trusted.gpg.d/erdma.gpg && \ |
| for model in "${SUPPORT_MODELS[@]}"; do | ||
| run_single_model "$model" | ||
| done |
There was a problem hiding this comment.
The test logic recreates the Docker containers for every model in the SUPPORT_MODELS array. The run_single_model function calls setup, which in turn calls clean_container and docker_launch. This is highly inefficient and will significantly slow down the test suite. The container setup should be performed once before the loop, and the loop should only run the model-specific test logic.
| if grep -q "FAILED" "$log_file" || grep -q "ERROR" "$log_file"; then | ||
| echo "Tests failed - check log file for details" | ||
| echo "{\"test_case\": \"$test_case_name\", \"status\": \"Fail\", \"timestamp\": \"$(date -u +%Y-%m-%dT%H:%M:%SZ)\"}" > "$result_json" | ||
| echo "$test_case_name: Fail" | ||
| elif grep -q "passed" "$log_file"; then | ||
| echo "All tests passed" | ||
| echo "{\"test_case\": \"$test_case_name\", \"status\": \"Pass\", \"timestamp\": \"$(date -u +%Y-%m-%dT%H:%M:%SZ)\"}" > "$result_json" | ||
| echo "$test_case_name: Pass" | ||
| else | ||
| echo "Unable to determine test status" | ||
| echo "{\"test_case\": \"$test_case_name\", \"status\": \"Fail\", \"timestamp\": \"$(date -u +%Y-%m-%dT%H:%M:%SZ)\", \"error\": \"Unable to determine status\"}" > "$result_json" | ||
| echo "$test_case_name: Fail" | ||
| fi |
There was a problem hiding this comment.
The parse function determines test success or failure by grep-ing the log file for "FAILED" or "ERROR". This is unreliable and can lead to incorrect results. For example, a test might legitimately print the word "ERROR" in a message while still passing. The run_test function already returns the exit code of pytest. This exit code should be used to determine the test status, which is a much more robust method.
| sglang_test_path = "/sgl-workspace/sglang/test/srt/hicache" | ||
| if os.path.exists(sglang_test_path): | ||
| sys.path.insert(0, sglang_test_path) |
There was a problem hiding this comment.
The path to sglang test files is hardcoded as an absolute path (/sgl-workspace/sglang/test/srt/hicache). This makes the tests not portable and very difficult to run outside of the specific CI environment for which this path was configured. Please remove this and instead configure the PYTHONPATH environment variable in the shell script that executes these Python tests. For example, export PYTHONPATH="/path/to/sglang:$PYTHONPATH".
| ### Execute Full Test | ||
|
|
||
| ```bash | ||
| cd scripts && ./test_demo.sh |
There was a problem hiding this comment.
The example command to execute a full test is incorrect. The path scripts is not where the test scripts are located, and test_demo.sh is not one of the scripts added in this PR. This can be confusing for users trying to run the tests locally. Please update the example to reflect the correct path and a valid script name, for example test_1p1d_erdma.sh.
| cd scripts && ./test_demo.sh | |
| cd scripts/tone_tests/scripts && ./test_1p1d_erdma.sh |
| echo " main - Run complete test workflow (prepare -> run_test -> parse -> cleanup)" | ||
| exit 1 | ||
| ;; | ||
| esac No newline at end of file |
There was a problem hiding this comment.
The file is missing a final newline character. This can cause unexpected behavior, especially if the script is sourced by another script. Please add a newline at the end of the file.
| # For now, we'll assume master service is ready if process is running | ||
| # and it's been a few seconds since startup | ||
| if ( | ||
| time.time() - start_time > 5 | ||
| ): # Give master service time to initialize | ||
| master_ready = True | ||
| print("Mooncake master service is ready") |
There was a problem hiding this comment.
The readiness check for the mooncake_master service relies on the process running for more than 5 seconds. This is fragile and can lead to flaky tests if the service takes longer to initialize. It would be more robust to implement a proper health check endpoint in the mooncake_master service and poll that endpoint, similar to how the metadata service is checked.
| ### 执行完整测试 | ||
|
|
||
| ```bash | ||
| cd scripts && ./test_demo.sh |
There was a problem hiding this comment.
The example command to execute a full test is incorrect. The path scripts is not where the test scripts are located, and test_demo.sh is not one of the scripts added in this PR. This can be confusing for users trying to run the tests locally. Please update the example to reflect the correct path and a valid script name, for example test_1p1d_erdma.sh.
| cd scripts && ./test_demo.sh | |
| cd scripts/tone_tests/scripts && ./test_1p1d_erdma.sh |
| fi | ||
| } | ||
|
|
||
| main No newline at end of file |
There was a problem hiding this comment.
The file is missing a final newline character. This can cause unexpected behavior, especially if the script is sourced by another script. Please add a newline at the end of the file.
luketong777
force-pushed
the
add_e2e_tests
branch
2 times, most recently
from
f54f7d9 to
b74ec3b
Compare
Signed-off-by: lukotong-7 <shicanwei.scw@alibaba-inc.com>
Signed-off-by: lukotong-7 <shicanwei.scw@alibaba-inc.com>
luketong777
force-pushed
the
add_e2e_tests
branch
from
b74ec3b to
0886754
Compare
1 participant
Description
Type of Change
How Has This Been Tested?
Checklist