Conversation
…subset of configs Signed-off-by: Dan Huang <[email protected]>
Signed-off-by: Dan Huang <[email protected]>
Signed-off-by: Dan Huang <[email protected]>
|
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
Summary of ChangesHello @dhuangnm, 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 streamlines the management and execution of end-to-end smoke tests by integrating test group identification directly into the YAML configuration files. This eliminates a previously error-prone separate list file and introduces a flexible mechanism for test scripts to filter and run tests based on specified groups, improving the overall reliability and maintainability of the testing infrastructure for both Python and RHAIIS environments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 improves the e2e smoke test identification by moving the configuration from a static file list to a test_group field within the YAML config files themselves. This is a great improvement for maintainability.
The implementation in the shell scripts, however, has some robustness issues. Specifically, there are problems with unquoted variables and fragile parsing methods (ls, grep/cut) which could cause the scripts to fail with filenames containing spaces or with slight variations in the YAML format. I've left specific comments with suggestions on how to make the scripts more robust.
|
|
||
| echo "=== RUNNING MODEL: $MODEL_CONFIG ===" | ||
| # run test if test group is not specified or the config matching the specified test group | ||
| test_group=$(cat $MODEL_CONFIG | grep 'test_group:' | cut -d'"' -f2) |
There was a problem hiding this comment.
The variable $MODEL_CONFIG is not quoted, which will cause the script to fail if a config filename contains spaces. Also, the cat command is unnecessary.
While parsing YAML with grep and cut can be fragile, a simple fix is to quote the variable and remove the cat. For a more robust solution, consider using sed or awk to handle different quoting styles or spacing in the YAML file.
| test_group=$(cat $MODEL_CONFIG | grep 'test_group:' | cut -d'"' -f2) | |
| test_group=$(grep 'test_group:' "$MODEL_CONFIG" | cut -d'"' -f2) |
| echo "Test group is specified: $GROUP" | ||
| fi | ||
|
|
||
| CONFIGS=`ls "$CONFIG"` |
There was a problem hiding this comment.
Using ls to populate a list of files is not recommended. It can lead to unexpected behavior with filenames that contain spaces or special characters. The subsequent loop that processes $CONFIGS is also likely to fail in such cases.
A safer and more robust way to iterate over files in a directory is to use a for loop with a glob pattern, like this:
for MODEL_CONFIG in "$CONFIG"/*
do
# ... your logic here ...
doneThis would require refactoring the existing loop structure but would make the script much more reliable.
| save_dir=$(cat $MODEL_CONFIG | grep 'save_dir:' | cut -d' ' -f2) | ||
| model=$(cat $MODEL_CONFIG | grep 'model:' | cut -d'/' -f2) | ||
| scheme=$(cat $MODEL_CONFIG | grep 'scheme:' | cut -d' ' -f2) | ||
| test_group=$(cat $MODEL_CONFIG | grep 'test_group:' | cut -d'"' -f2) |
There was a problem hiding this comment.
Similar to the other script, the variable $MODEL_CONFIG is not quoted here, and cat is unnecessary. This can cause issues if filenames have spaces. The root cause is the use of ls earlier in the script, but this line is also problematic.
Also, parsing YAML with grep and cut is fragile. Please consider quoting the variable and using a more robust parsing method.
| test_group=$(cat $MODEL_CONFIG | grep 'test_group:' | cut -d'"' -f2) | |
| test_group=$(grep 'test_group:' "$MODEL_CONFIG" | cut -d'"' -f2) |
There was a problem hiding this comment.
I think we want to call it "rhaiis" or something to indicate that this is also a test that runs for rhaiis e2e tests. Smoke is a bit misleading
There was a problem hiding this comment.
Good point, updated the PRs accordingly to use "rhaiis".
Signed-off-by: Dan Huang <[email protected]>
2 participants
SUMMARY:
Currently we use the file
tests/e2e/vLLM/rhaiis-e2e-smoke.listto mark the configs for smoke tests that we use to run for the RHAIIS image. This is vulnerable as we need to keep the list in this file up-to-date to any changes in the config yaml files and this is error-prone.This PR removes this
tests/e2e/vLLM/rhaiis-e2e-smoke.listfile and use the config yaml file directly to mark the smoke tests. We added a new fieldtest_groupto the yaml file and updated therun_tests_in_*.shscripts to parse this field and filter out tests if a test group (-g) is specified. This allows both python and rhaiis image testing be able to run smoke and full tests for the configs.To be more specific:
$# to run e2e tests for all configs (default)
bash tests/e2e/vLLM/run_tests_in_python.sh -c tests/e2e/vLLM/configs -t tests/e2e/vLLM/test_vllm.py$# to run e2e tests for configs with smoke only
bash tests/e2e/vLLM/run_tests_in_python.sh -c tests/e2e/vLLM/configs -t tests/e2e/vLLM/test_vllm.py -g rhaiisSimilar commands for the
run_tests_in_rhaiis.shscript.Going forward, for any newly added configs for the e2e tests, if we want to include them into the smoke tests for the RHAIIS image, we need to remember to add the
test_group: "smoke"into their yaml file under the configs/ so we can automatically pick it up for the RHAIIS image testing.TEST PLAN:
A successful run of the smoke tests is here:
https://github.com/neuralmagic/llm-compressor-testing/actions/runs/21727920814