Add config filter flag to test runner script - #3027
Conversation
Allows filtering which config files are run by substring match on the filename. E.g., `-f nvfp4` will only run configs containing "nvfp4" in their name, skipping the rest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
👋 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. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Require one maintainer reviewAll PRs must have at least one approving review from a maintainer before merging.
|
![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 new -f command-line option to run_tests_in_python.sh to filter configuration files by a substring pattern. The feedback suggests several improvements to the Bash script: using the more idiomatic [ -n "$FILTER" ] instead of [ ! -z "$FILTER" ], handling cases where the glob pattern does not match any files to prevent errors, and replacing the external basename command with Bash's built-in parameter expansion ${MODEL_CONFIG##*/} inside the loop to improve performance.
| if [ ! -z "$FILTER" ]; then | ||
| echo "Config filter is specified: $FILTER" | ||
| fi |
There was a problem hiding this comment.
Using [ -n "$FILTER" ] is the standard and more readable way to check if a string is non-empty in Bash, rather than negating -z with !.
| if [ ! -z "$FILTER" ]; then | |
| echo "Config filter is specified: $FILTER" | |
| fi | |
| if [ -n "$FILTER" ]; then | |
| echo "Config filter is specified: $FILTER" | |
| fi |
| # Parse list of configs. | ||
| for MODEL_CONFIG in "$CONFIG"/* | ||
| do | ||
| LOCAL_SUCCESS=0 |
There was a problem hiding this comment.
When the config directory is empty or does not exist, the glob "$CONFIG"/* will not expand and will instead evaluate to the literal string "/path/to/configs/*". This causes the loop to run once with a non-existent path, leading to misleading skip logs or cat errors.
Adding a check to ensure the file actually exists before proceeding avoids this issue.
| LOCAL_SUCCESS=0 | |
| LOCAL_SUCCESS=0 | |
| # Ensure the config file exists (handles cases where the glob doesn't match any files) | |
| [[ -e "$MODEL_CONFIG" ]] || continue |
| if [ ! -z "$FILTER" ]; then | ||
| config_basename=$(basename "$MODEL_CONFIG") | ||
| if [[ "$config_basename" != *"$FILTER"* ]]; then | ||
| echo "=== SKIPPING MODEL (filter): $MODEL_CONFIG ===" | ||
| continue | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Using the external basename command inside a loop forks a new process for every single configuration file, which is inefficient.
We can use Bash's built-in parameter expansion ${MODEL_CONFIG##*/} to extract the filename directly in-memory, which is significantly faster and more efficient. Additionally, we can use the more idiomatic [ -n "$FILTER" ] instead of [ ! -z "$FILTER" ].
| if [ ! -z "$FILTER" ]; then | |
| config_basename=$(basename "$MODEL_CONFIG") | |
| if [[ "$config_basename" != *"$FILTER"* ]]; then | |
| echo "=== SKIPPING MODEL (filter): $MODEL_CONFIG ===" | |
| continue | |
| fi | |
| fi | |
| if [ -n "$FILTER" ]; then | |
| if [[ "${MODEL_CONFIG##*/}" != *"$FILTER"* ]]; then | |
| echo "=== SKIPPING MODEL (filter): $MODEL_CONFIG ===" | |
| continue | |
| fi | |
| fi |
brian-dellabetta
left a comment
brian-dellabetta
left a comment
There was a problem hiding this comment.
cool, gemini comments seems appropriate
3 participants
Summary
-fflag totests/e2e/run_tests_in_python.shthat filters config files by substring match on filename-f nvfp4runs only configs with "nvfp4" in their name, skipping the restCompanion PR: https://github.com/neuralmagic/llm-compressor-testing/pull/332
Test plan
bash tests/e2e/run_tests_in_python.sh -c tests/lmeval/configs -f nvfp4locally and verify only nvfp4 configs are selected-fand verify all configs still run (no behavior change)🤖 Generated with Claude Code