Open
Conversation
…rdening This commit improves the existing Ansible playbook infrastructure for vLLM CPU performance evaluation with enhanced AWX compatibility, security hardening, and comprehensive testing. - Fix type normalization to handle AnsibleUnsafeText from AWX - Fix allocated_nodes to return integers instead of strings - Handle empty strings and Jinja2 None conversions properly - Simplify node eligibility checking and allocation logic - Improve error messages for better validation feedback - Add no_log: true to all tasks handling HF_TOKEN - Prevent token exposure in container start operations - Secure environment variable handling in AWX jobs - Add comprehensive unit tests for cpu_utils filter plugin (598 lines) - Test coverage for: CPU range conversion, NUMA extraction, multi-NUMA allocation, OMP binding, and real-world scenarios - Support for both pytest and standalone execution - Add collections/requirements.yml for Ansible collection dependencies - Better parameter validation for AWX jobs in concurrent load testing - AWX detection for result path handling - Improved NUMA topology detection in core sweep - Enhanced result path consistency in main benchmark - Better workload configuration handling - Simplify prerequisites section - Update examples with current best practices - Clearer workflow documentation Files changed: 13 files, 772 insertions(+), 323 deletions(-) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Remove undefined 'short_codegen' workload from validation lists to prevent KeyError during test execution. Add CPU allocation validation to detect under-allocation early with clear error messages. Fix pytest.raises fallback to properly suppress exceptions when pytest is unavailable. Pin exact collection versions for reproducible AWX/CI runs. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
- Fix inconsistent output filename reference in ansible.md (benchmarks.html → benchmarks.json) - Improve requested_cores validation to handle non-numeric input safely - Document requirements files usage (AWX/production vs CLI/development) - Fix hardcoded log_collection_dest to use centralized local_results_base - Remove obsolete vLLM configuration fields from test metadata docs - Improve node capacity validation with sorting and per-node checks in cpu_utils.py - Refactor duplicate AWX_JOB_ID lookup in llm-core-sweep-auto.yml - Add NUMA node id type coercion for consistent integer types Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
The previous commit changed NUMA node ids from strings to integers, but the selectattr filters were still trying to match them as strings, causing "No first item, sequence was empty" errors. Remove the | string filter from selectattr comparisons to match integer ids correctly. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
- Fix allocate_with_fixed_tp to filter nodes by capacity (cpu_utils.py) - Update Results summary to use local_results_base (llm-benchmark-auto.yml) - Implement core_sweep_counts parameter handling (llm-benchmark-concurrent-load.yml) - Normalize core_sweep_counts to requested_cores for single-element lists - Reject multi-element lists with helpful error directing to llm-core-sweep-auto.yml - Pass effective_requested_cores to all 3 phases - Restore model configuration facts in start-llm.yml for metadata collection - Set model_kv_cache, model_dtype, model_max_len, vllm_caching_mode - Fixes compatibility with get-vllm-config-from-dut.yml assertions - Add regression tests for serialized TP inputs (test_cpu_utils.py) - Test empty string '' behaves like auto-TP - Test string 'None' behaves like auto-TP Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
…ainer start Addresses two critical issues in vLLM configuration: 1. Per-model overrides (model_kv_cache_space, model_dtype) now properly override workload defaults instead of being ignored 2. Added preflight validation of --max-model-len against workload requirements (ISL + OSL) to fail early with clear error messages Both fixes ensure configuration errors are caught early and per-model settings take precedence over workload fallbacks. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis pull request implements AWX-aware result path handling across multiple Ansible playbooks, refactors NUMA node core allocation logic with improved type normalization, expands test coverage with comprehensive unit and smoke tests including pytest configuration, and updates documentation while removing vLLM configuration collection from benchmark execution. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
automation/test-execution/ansible/roles/common/tasks/allocate-cores-from-count.yml (1)
18-27:⚠️ Potential issue | 🟠 MajorExclude string
'None'from validation to prevent premature int conversion failure.The guards at lines 24-27 don't exclude the string
'None', which commonly comes from CLI arguments. This allows string'None'to reach line 21's| intfilter, causing an unnecessary failure before the Python function's auto-TP logic can handle it. Add explicit string handling to the when conditions.Suggested fix
- name: Validate requested_tensor_parallel if provided ansible.builtin.assert: that: - requested_tensor_parallel | int in [1, 2, 4, 8] fail_msg: "Invalid tensor_parallel: {{ requested_tensor_parallel }}. Valid values: 1, 2, 4, 8" when: - requested_tensor_parallel is defined - requested_tensor_parallel != omit - - requested_tensor_parallel != None - - requested_tensor_parallel != "" + - requested_tensor_parallel is not none + - (requested_tensor_parallel | string | trim | lower) not in ["", "none"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/roles/common/tasks/allocate-cores-from-count.yml` around lines 18 - 27, The assert task validating requested_tensor_parallel (ansible.builtin.assert) can receive the literal string 'None' from CLI and fail on the `| int` conversion prematurely; update the task's when guards for the assert (the block referencing requested_tensor_parallel) to explicitly exclude the string 'None' (e.g., add a condition like requested_tensor_parallel != 'None') so that string 'None' doesn't reach the `requested_tensor_parallel | int` check and allows the downstream auto-TP logic to run.
🧹 Nitpick comments (6)
automation/test-execution/ansible/requirements.yml (1)
7-10: Align dev collection versions to AWX pins or add CI parity check.Flexible ranges (
>=1.9.0,<2.0.0for containers.podman,>=1.4.0,<1.6.0for ansible.posix) allow versions to drift from the exact AWX pins (1.9.4 and 1.5.4 respectively). This creates a risk of "works locally, fails in AWX" regressions. Either lock dev ranges to match AWX pins or add a CI check to prevent parity divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/requirements.yml` around lines 7 - 10, Update the flexible version ranges in requirements.yml so dev collection versions match AWX pins or add a CI parity check: either change the containers.podman and ansible.posix entries to exact pins 1.9.4 and 1.5.4 respectively (replace >=1.9.0,<2.0.0 and >=1.4.0,<1.6.0) or add a CI job that compares this requirements.yml to the canonical AWX pin file (collections/requirements.yml) and fails on divergence; reference the collection names containers.podman and ansible.posix and the AWX pin file when implementing the change..github/workflows/unit-tests.yml (1)
26-30: Consider using a pinned test requirements file for reproducible CI runs.Lines 29 and 58 both install floating versions (
pytest ansible pyyaml), which can cause non-deterministic failures over time. Create a shared pinnedrequirements-test.txtand use it in both the unit-tests and smoke-tests jobs for better maintainability and reproducibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yml around lines 26 - 30, Replace the floating pip installs with a pinned requirements file: add a committed requirements-test.txt listing exact versions for pytest, ansible, pyyaml (and any other test deps), then update the workflow "Install dependencies" steps (the ones currently running "pip install pytest ansible pyyaml") in both the unit-tests and smoke-tests jobs to use "pip install -r requirements-test.txt" so CI uses reproducible, pinned test dependencies.automation/test-execution/ansible/tests/smoke/test_playbook_syntax.py (1)
28-28: Remove unnecessary f-string prefixes.Multiple assertion messages use an f-string prefix without placeholders in the first part:
f"Message:\n" + "\n".join(errors). Thefprefix is unnecessary since there are no interpolations before the concatenation.💡 Proposed fix pattern (apply to all similar lines)
- assert not errors, f"YAML validation errors:\n" + "\n".join(errors) + assert not errors, "YAML validation errors:\n" + "\n".join(errors)Or use a single f-string:
- assert not errors, f"YAML validation errors:\n" + "\n".join(errors) + assert not errors, f"YAML validation errors:\n{chr(10).join(errors)}"Also applies to: 60-60, 91-91, 110-110, 137-137, 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/tests/smoke/test_playbook_syntax.py` at line 28, Remove the unnecessary f-string prefixes in the assertion messages in test_playbook_syntax (the assertions that use f"YAML validation errors:\n" + "\n".join(errors) and similar patterns); either drop the leading "f" from the first literal (making it a normal string concatenated with "\n".join(...)) or combine into a single f-string that interpolates the join (e.g., f"YAML validation errors:\n{'\n'.join(errors)}"). Update all occurrences matching that pattern (the assertion lines that build messages by concatenating a literal with "\n".join(...)) so there are no unused f-prefixes.automation/test-execution/ansible/tests/smoke/test_model_matrix.py (1)
61-61: Remove unnecessary f-string prefixes (same pattern as other test files).Same issue as
test_playbook_syntax.py- the f-string prefix adds no value when there are no placeholders before the concatenation.Also applies to: 88-88, 120-120, 136-136, 154-154, 177-177, 193-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/tests/smoke/test_model_matrix.py` at line 61, Several assertions in test_model_matrix.py use an unnecessary f-string prefix before a plain string that's concatenated with "\n".join(errors) (e.g., the assertion that starts with assert not errors, f"Model validation errors:\n" + "\n".join(errors)); remove the leading f from those string literals so they become regular strings (e.g., "Model validation errors:\n" + "\n".join(errors)). Apply the same change to the other similar assertions in this file that use the f-prefix at lines referenced (the patterns containing "Model validation errors" + "\n".join(errors) and the other error message strings) so they match the style used in other test files.automation/test-execution/ansible/roles/vllm_server/tasks/start-llm.yml (1)
302-307: Verifymodel_dtypefallback behavior when no--dtypeargument is present.If
vllm_args_mergedcontains no--dtype=*argument andmodel_dtypewas not explicitly set upstream, thedefault('--dtype=auto')ensures a safe fallback. However, this also meansmodel_dtypein metadata may showautoeven when vLLM internally selects a specific dtype. Consider documenting this behavior if downstream tooling expects the actual runtime dtype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/roles/vllm_server/tasks/start-llm.yml` around lines 302 - 307, The current set_fact sets model_dtype from vllm_args_merged with default('--dtype=auto'), which can misrepresent the actual runtime dtype when vLLM chooses dtype automatically; update the task that sets model_dtype (and related facts) so that it does not blindly record "auto" as the dtype—either set model_dtype to an empty/nullable value when no --dtype is present or add a separate fact like model_dtype_note/runtime_dtype_hint that records "auto (runtime-selected)" so downstream tooling knows this is a fallback, and keep references to vllm_args_merged and model_dtype in the updated logic and documentation.automation/test-execution/ansible/tests/smoke/test_container_config.py (1)
181-186: Incomplete port structure validation.The test loads and validates that
endpoints.ymlexists and is non-empty, but the comment indicates port structure validation is pending. Consider adding the port checks or removing the placeholder comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@automation/test-execution/ansible/ansible.md`:
- Around line 66-69: Update the Quick Start in ansible.md to instruct users to
install Ansible collections before running setup-platform.yml: add a step after
the Ansible install note that tells the user to change into the
automation/test-execution/ansible folder on the control machine and run
ansible-galaxy collection install -r requirements.yml so the collections listed
in requirements.yml are present prior to running setup-platform.yml.
In `@automation/test-execution/ansible/llm-benchmark-concurrent-load.yml`:
- Around line 59-73: Update the "Validate required parameters" task's fail_msg
to reflect the new single-entry behavior: mention that core_sweep_counts must be
a single-element list (e.g., core_sweep_counts=[16]) or recommend using
requested_cores=<N>, and remove the example suggesting multiple values (e.g.,
[16,32,64]); reference the variables requested_cores and core_sweep_counts in
the message so users know which inputs are accepted under the new validation in
the Validate required parameters assertion.
In `@automation/test-execution/ansible/roles/benchmark_guidellm/tasks/main.yml`:
- Around line 239-253: The shell task "Stream GuideLLM container logs directly
to file" is redirecting podman output to an unquoted filename built from
workload_type, core_cfg.name and test_run_id which can break if those variables
contain spaces or metacharacters; update the ansible.builtin.shell cmd to quote
the redirection target (e.g. use the Jinja2 quote filter or wrap the generated
path in single quotes) so the >/2>&1 target is treated as a single literal path,
and ensure the subsequent ansible.builtin.fetch src uses the identical
quoted/generated filename so Fetch GuideLLM logs to controller finds the file.
In `@automation/test-execution/ansible/tests/smoke/test_container_config.py`:
- Around line 108-116: The test currently checks only for the Docker-specific
"unable to find image" string in result.stderr before skipping; update the check
in the test_container_config.py test to broaden the stderr matching to include
Podman variants (e.g., "image not known", "no such image", "not found") or use a
regex/any-of-substrings approach against result.stderr.lower() before calling
pytest.skip, so the failure-to-find-image logic around result and
pytest.skip("vLLM image not available locally") correctly handles both Docker
and Podman outputs.
In `@automation/test-execution/ansible/tests/smoke/test_model_matrix.py`:
- Around line 179-193: The test test_embedding_matrix_structure asserts each
entry in embedding_matrix["matrix"]["embedding_models"] contains the fields in
required_fields (name, full_name, dimensions, max_sequence_length); currently
the model definitions lack dimensions and max_sequence_length. Fix by adding
those two keys to every embedding model definition in the embedding model matrix
(embedding_models) with appropriate integer values (e.g., dimensions: <embedding
vector size>, max_sequence_length: <token limit>) or, if those fields are not
applicable, remove them from required_fields in test_embedding_matrix_structure
to match the actual model schema.
In `@automation/test-execution/ansible/tests/unit/test_cpu_utils.py`:
- Around line 596-602: The test runner currently calls pytest.main([...]) but
does not propagate its exit code, so update the __main__ block to pass
pytest.main(...) into sys.exit so failures yield non-zero process exit;
specifically, in the if HAS_PYTEST branch replace the standalone call to
pytest.main([__file__, "-v"]) with sys.exit(pytest.main([__file__, "-v"]))
(ensure sys is imported and that the change is made inside the existing if
__name__ == "__main__" / if HAS_PYTEST block referencing HAS_PYTEST,
pytest.main, and sys.exit).
- Around line 20-37: The fallback pytest shim defines class pytest with nested
raises but misses the mark attribute used by decorators like `@pytest.mark.unit`;
add a mark object to the pytest shim (e.g., add an attribute named mark on the
pytest class or module that exposes a unit attribute usable as a decorator) so
imports that reference `@pytest.mark.unit` do not raise AttributeError; ensure the
unit attribute is a callable/decorator that returns the original function
(no-op) and keep the existing raises implementation (refer to the pytest class
and its nested raises in the current shim).
---
Outside diff comments:
In
`@automation/test-execution/ansible/roles/common/tasks/allocate-cores-from-count.yml`:
- Around line 18-27: The assert task validating requested_tensor_parallel
(ansible.builtin.assert) can receive the literal string 'None' from CLI and fail
on the `| int` conversion prematurely; update the task's when guards for the
assert (the block referencing requested_tensor_parallel) to explicitly exclude
the string 'None' (e.g., add a condition like requested_tensor_parallel !=
'None') so that string 'None' doesn't reach the `requested_tensor_parallel |
int` check and allows the downstream auto-TP logic to run.
---
Nitpick comments:
In @.github/workflows/unit-tests.yml:
- Around line 26-30: Replace the floating pip installs with a pinned
requirements file: add a committed requirements-test.txt listing exact versions
for pytest, ansible, pyyaml (and any other test deps), then update the workflow
"Install dependencies" steps (the ones currently running "pip install pytest
ansible pyyaml") in both the unit-tests and smoke-tests jobs to use "pip install
-r requirements-test.txt" so CI uses reproducible, pinned test dependencies.
In `@automation/test-execution/ansible/requirements.yml`:
- Around line 7-10: Update the flexible version ranges in requirements.yml so
dev collection versions match AWX pins or add a CI parity check: either change
the containers.podman and ansible.posix entries to exact pins 1.9.4 and 1.5.4
respectively (replace >=1.9.0,<2.0.0 and >=1.4.0,<1.6.0) or add a CI job that
compares this requirements.yml to the canonical AWX pin file
(collections/requirements.yml) and fails on divergence; reference the collection
names containers.podman and ansible.posix and the AWX pin file when implementing
the change.
In `@automation/test-execution/ansible/roles/vllm_server/tasks/start-llm.yml`:
- Around line 302-307: The current set_fact sets model_dtype from
vllm_args_merged with default('--dtype=auto'), which can misrepresent the actual
runtime dtype when vLLM chooses dtype automatically; update the task that sets
model_dtype (and related facts) so that it does not blindly record "auto" as the
dtype—either set model_dtype to an empty/nullable value when no --dtype is
present or add a separate fact like model_dtype_note/runtime_dtype_hint that
records "auto (runtime-selected)" so downstream tooling knows this is a
fallback, and keep references to vllm_args_merged and model_dtype in the updated
logic and documentation.
In `@automation/test-execution/ansible/tests/smoke/test_model_matrix.py`:
- Line 61: Several assertions in test_model_matrix.py use an unnecessary
f-string prefix before a plain string that's concatenated with "\n".join(errors)
(e.g., the assertion that starts with assert not errors, f"Model validation
errors:\n" + "\n".join(errors)); remove the leading f from those string literals
so they become regular strings (e.g., "Model validation errors:\n" +
"\n".join(errors)). Apply the same change to the other similar assertions in
this file that use the f-prefix at lines referenced (the patterns containing
"Model validation errors" + "\n".join(errors) and the other error message
strings) so they match the style used in other test files.
In `@automation/test-execution/ansible/tests/smoke/test_playbook_syntax.py`:
- Line 28: Remove the unnecessary f-string prefixes in the assertion messages in
test_playbook_syntax (the assertions that use f"YAML validation errors:\n" +
"\n".join(errors) and similar patterns); either drop the leading "f" from the
first literal (making it a normal string concatenated with "\n".join(...)) or
combine into a single f-string that interpolates the join (e.g., f"YAML
validation errors:\n{'\n'.join(errors)}"). Update all occurrences matching that
pattern (the assertion lines that build messages by concatenating a literal with
"\n".join(...)) so there are no unused f-prefixes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15043db5-287d-468d-b6b2-d6948c171ad5
📒 Files selected for processing (22)
.github/workflows/unit-tests.ymlautomation/test-execution/ansible/README.mdautomation/test-execution/ansible/ansible.mdautomation/test-execution/ansible/filter_plugins/cpu_utils.pyautomation/test-execution/ansible/llm-benchmark-auto.ymlautomation/test-execution/ansible/llm-benchmark-concurrent-load.ymlautomation/test-execution/ansible/llm-benchmark.ymlautomation/test-execution/ansible/llm-core-sweep-auto.ymlautomation/test-execution/ansible/requirements.ymlautomation/test-execution/ansible/roles/benchmark_guidellm/tasks/main.ymlautomation/test-execution/ansible/roles/common/tasks/allocate-cores-from-count.ymlautomation/test-execution/ansible/roles/common/tasks/detect-numa-topology.ymlautomation/test-execution/ansible/roles/vllm_server/tasks/start-embedding.ymlautomation/test-execution/ansible/roles/vllm_server/tasks/start-llm.ymlautomation/test-execution/ansible/tests/README.mdautomation/test-execution/ansible/tests/pytest.iniautomation/test-execution/ansible/tests/smoke/__init__.pyautomation/test-execution/ansible/tests/smoke/test_container_config.pyautomation/test-execution/ansible/tests/smoke/test_model_matrix.pyautomation/test-execution/ansible/tests/smoke/test_playbook_syntax.pyautomation/test-execution/ansible/tests/unit/test_cpu_utils.pycollections/requirements.yml
💤 Files with no reviewable changes (1)
- automation/test-execution/ansible/README.md
Comment on lines
+66
to
+69
| - **Option 2 (setup-platform.yml)**: Automatically installs Podman, Python 3, | ||
| and all performance tools on **DUT and Load Generator hosts**. Your Ansible | ||
| control machine only needs Ansible itself and the collections from | ||
| requirements.yml. |
There was a problem hiding this comment.
Add the collection install command to Quick Start.
These notes now say the control machine needs the collections from requirements.yml, but the setup steps never show how to install them. A first-time user can follow this page exactly and still hit missing collection errors on the first setup-platform.yml run.
Suggested addition near the Ansible install step
cd automation/test-execution/ansible
ansible-galaxy collection install -r requirements.yml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@automation/test-execution/ansible/ansible.md` around lines 66 - 69, Update
the Quick Start in ansible.md to instruct users to install Ansible collections
before running setup-platform.yml: add a step after the Ansible install note
that tells the user to change into the automation/test-execution/ansible folder
on the control machine and run ansible-galaxy collection install -r
requirements.yml so the collections listed in requirements.yml are present prior
to running setup-platform.yml.
Comment on lines
+59
to
+73
| - name: Validate required parameters | ||
| ansible.builtin.assert: | ||
| that: | ||
| - >- | ||
| (requested_cores | default(0) | int > 0) or | ||
| (core_sweep_counts is defined and core_sweep_counts is not none and core_sweep_counts | length > 0) | ||
| fail_msg: | | ||
| Missing required parameter: requested_cores or core_sweep_counts | ||
| Please provide either: | ||
| - Single core count: -e "requested_cores=<8|16|32|64|...>" | ||
| - Core sweep: -e "core_sweep_counts=[16,32,64]" | ||
|
|
||
| The concurrent load test requires at least one core configuration to test. | ||
| This playbook will run 3 phases with the specified core configuration(s). | ||
|
|
There was a problem hiding this comment.
Make the missing-parameter help text match the new single-entry behavior.
This message still tells users to pass core_sweep_counts=[16,32,64], but Lines 78-90 now reject more than one value. That makes the primary remediation path fail immediately.
Suggested fix
- - Core sweep: -e "core_sweep_counts=[16,32,64]"
+ - Single-entry core list: -e "core_sweep_counts=[16]"
@@
- This playbook will run 3 phases with the specified core configuration(s).
+ This playbook will run 3 phases with the specified core configuration.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Validate required parameters | |
| ansible.builtin.assert: | |
| that: | |
| - >- | |
| (requested_cores | default(0) | int > 0) or | |
| (core_sweep_counts is defined and core_sweep_counts is not none and core_sweep_counts | length > 0) | |
| fail_msg: | | |
| Missing required parameter: requested_cores or core_sweep_counts | |
| Please provide either: | |
| - Single core count: -e "requested_cores=<8|16|32|64|...>" | |
| - Core sweep: -e "core_sweep_counts=[16,32,64]" | |
| The concurrent load test requires at least one core configuration to test. | |
| This playbook will run 3 phases with the specified core configuration(s). | |
| - name: Validate required parameters | |
| ansible.builtin.assert: | |
| that: | |
| - >- | |
| (requested_cores | default(0) | int > 0) or | |
| (core_sweep_counts is defined and core_sweep_counts is not none and core_sweep_counts | length > 0) | |
| fail_msg: | | |
| Missing required parameter: requested_cores or core_sweep_counts | |
| Please provide either: | |
| - Single core count: -e "requested_cores=<8|16|32|64|...>" | |
| - Single-entry core list: -e "core_sweep_counts=[16]" | |
| The concurrent load test requires at least one core configuration to test. | |
| This playbook will run 3 phases with the specified core configuration. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@automation/test-execution/ansible/llm-benchmark-concurrent-load.yml` around
lines 59 - 73, Update the "Validate required parameters" task's fail_msg to
reflect the new single-entry behavior: mention that core_sweep_counts must be a
single-element list (e.g., core_sweep_counts=[16]) or recommend using
requested_cores=<N>, and remove the example suggesting multiple values (e.g.,
[16,32,64]); reference the variables requested_cores and core_sweep_counts in
the message so users know which inputs are accepted under the new validation in
the Validate required parameters assertion.
Comment on lines
+239
to
253
| - name: Stream GuideLLM container logs directly to file | ||
| ansible.builtin.shell: | ||
| cmd: "podman logs {{ ('guidellm-' ~ workload_type ~ '-' ~ core_cfg.name) | quote }} 2>&1" | ||
| cmd: "podman logs {{ ('guidellm-' ~ workload_type ~ '-' ~ core_cfg.name) | quote }} > /tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}-{{ test_run_id }}.log 2>&1" | ||
| args: | ||
| executable: /bin/bash | ||
| register: guidellm_full_logs | ||
| changed_when: false | ||
| failed_when: false | ||
| when: | ||
| - use_guidellm_container | bool | ||
| - container_exit_code.stdout is defined | ||
|
|
||
| - name: Save GuideLLM logs to temporary file on remote | ||
| ansible.builtin.copy: | ||
| content: "{{ guidellm_full_logs.stdout }}" | ||
| dest: "/tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}.log" | ||
| mode: "0644" | ||
| when: | ||
| - use_guidellm_container | bool | ||
| - guidellm_full_logs is defined | ||
| - guidellm_full_logs.stdout is defined | ||
|
|
||
| - name: Fetch GuideLLM logs to controller | ||
| ansible.builtin.fetch: | ||
| src: "/tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}.log" | ||
| src: "/tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}-{{ test_run_id }}.log" | ||
| dest: "{{ results_path }}/guidellm.log" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd automation/test-execution/ansible/roles/benchmark_guidellm/tasks && cat -n main.yml | sed -n '230,260p'Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 1271
🏁 Script executed:
# Check how workload_type, core_cfg.name, and test_run_id are defined/used
rg -n 'workload_type|core_cfg\.name|test_run_id' automation/test-execution/ansible/roles/benchmark_guidellm/tasks/main.yml | head -30Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 1466
🏁 Script executed:
# Check the broader context to understand variable sources
rg -n 'set_fact|register|vars:' automation/test-execution/ansible/roles/benchmark_guidellm/tasks/main.yml | head -20Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 729
Quote the redirected log filename to prevent shell interpretation of special characters.
The container name is quoted on line 241, but the output path is not. Since core_cfg.name, workload_type, and test_run_id are configuration-derived variables, they may contain spaces or shell metacharacters that would break the redirect or cause unintended file creation.
Suggested fix
- name: Stream GuideLLM container logs directly to file
ansible.builtin.shell:
- cmd: "podman logs {{ ('guidellm-' ~ workload_type ~ '-' ~ core_cfg.name) | quote }} > /tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}-{{ test_run_id }}.log 2>&1"
+ cmd: >-
+ podman logs {{ ('guidellm-' ~ workload_type ~ '-' ~ core_cfg.name) | quote }}
+ > {{ ('/tmp/guidellm-' ~ workload_type ~ '-' ~ core_cfg.name ~ '-' ~ test_run_id ~ '.log') | quote }} 2>&1
@@
- name: Fetch GuideLLM logs to controller
ansible.builtin.fetch:
- src: "/tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}-{{ test_run_id }}.log"
+ src: "{{ '/tmp/guidellm-' ~ workload_type ~ '-' ~ core_cfg.name ~ '-' ~ test_run_id ~ '.log' }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Stream GuideLLM container logs directly to file | |
| ansible.builtin.shell: | |
| cmd: "podman logs {{ ('guidellm-' ~ workload_type ~ '-' ~ core_cfg.name) | quote }} 2>&1" | |
| cmd: "podman logs {{ ('guidellm-' ~ workload_type ~ '-' ~ core_cfg.name) | quote }} > /tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}-{{ test_run_id }}.log 2>&1" | |
| args: | |
| executable: /bin/bash | |
| register: guidellm_full_logs | |
| changed_when: false | |
| failed_when: false | |
| when: | |
| - use_guidellm_container | bool | |
| - container_exit_code.stdout is defined | |
| - name: Save GuideLLM logs to temporary file on remote | |
| ansible.builtin.copy: | |
| content: "{{ guidellm_full_logs.stdout }}" | |
| dest: "/tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}.log" | |
| mode: "0644" | |
| when: | |
| - use_guidellm_container | bool | |
| - guidellm_full_logs is defined | |
| - guidellm_full_logs.stdout is defined | |
| - name: Fetch GuideLLM logs to controller | |
| ansible.builtin.fetch: | |
| src: "/tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}.log" | |
| src: "/tmp/guidellm-{{ workload_type }}-{{ core_cfg.name }}-{{ test_run_id }}.log" | |
| dest: "{{ results_path }}/guidellm.log" | |
| - name: Stream GuideLLM container logs directly to file | |
| ansible.builtin.shell: | |
| cmd: >- | |
| podman logs {{ ('guidellm-' ~ workload_type ~ '-' ~ core_cfg.name) | quote }} | |
| > {{ ('/tmp/guidellm-' ~ workload_type ~ '-' ~ core_cfg.name ~ '-' ~ test_run_id ~ '.log') | quote }} 2>&1 | |
| args: | |
| executable: /bin/bash | |
| changed_when: false | |
| failed_when: false | |
| when: | |
| - use_guidellm_container | bool | |
| - container_exit_code.stdout is defined | |
| - name: Fetch GuideLLM logs to controller | |
| ansible.builtin.fetch: | |
| src: "{{ '/tmp/guidellm-' ~ workload_type ~ '-' ~ core_cfg.name ~ '-' ~ test_run_id ~ '.log' }}" | |
| dest: "{{ results_path }}/guidellm.log" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@automation/test-execution/ansible/roles/benchmark_guidellm/tasks/main.yml`
around lines 239 - 253, The shell task "Stream GuideLLM container logs directly
to file" is redirecting podman output to an unquoted filename built from
workload_type, core_cfg.name and test_run_id which can break if those variables
contain spaces or metacharacters; update the ansible.builtin.shell cmd to quote
the redirection target (e.g. use the Jinja2 quote filter or wrap the generated
path in single quotes) so the >/2>&1 target is treated as a single literal path,
and ensure the subsequent ansible.builtin.fetch src uses the identical
quoted/generated filename so Fetch GuideLLM logs to controller finds the file.
Comment on lines
+108
to
+116
| # If image doesn't exist, skip (don't auto-pull in tests) | ||
| if "unable to find image" in result.stderr.lower(): | ||
| pytest.skip("vLLM image not available locally") | ||
|
|
||
| if result.returncode == 0: | ||
| assert ( | ||
| "vllm serve" in result.stdout | ||
| or "usage:" in result.stdout.lower() | ||
| ) |
There was a problem hiding this comment.
Docker-specific error message may not match Podman output.
The check for "unable to find image" is a Docker-specific error message. Podman typically uses different wording (e.g., "image not known" or "no such image"). This may cause the test to fail differently on Podman-only systems.
💡 Proposed fix for cross-runtime compatibility
# If image doesn't exist, skip (don't auto-pull in tests)
- if "unable to find image" in result.stderr.lower():
+ if any(msg in result.stderr.lower() for msg in [
+ "unable to find image", # docker
+ "image not known", # podman
+ "no such image", # podman alternative
+ ]):
pytest.skip("vLLM image not available locally")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # If image doesn't exist, skip (don't auto-pull in tests) | |
| if "unable to find image" in result.stderr.lower(): | |
| pytest.skip("vLLM image not available locally") | |
| if result.returncode == 0: | |
| assert ( | |
| "vllm serve" in result.stdout | |
| or "usage:" in result.stdout.lower() | |
| ) | |
| # If image doesn't exist, skip (don't auto-pull in tests) | |
| if any(msg in result.stderr.lower() for msg in [ | |
| "unable to find image", # docker | |
| "image not known", # podman | |
| "no such image", # podman alternative | |
| ]): | |
| pytest.skip("vLLM image not available locally") | |
| if result.returncode == 0: | |
| assert ( | |
| "vllm serve" in result.stdout | |
| or "usage:" in result.stdout.lower() | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@automation/test-execution/ansible/tests/smoke/test_container_config.py`
around lines 108 - 116, The test currently checks only for the Docker-specific
"unable to find image" string in result.stderr before skipping; update the check
in the test_container_config.py test to broaden the stderr matching to include
Podman variants (e.g., "image not known", "no such image", "not found") or use a
regex/any-of-substrings approach against result.stderr.lower() before calling
pytest.skip, so the failure-to-find-image logic around result and
pytest.skip("vLLM image not available locally") correctly handles both Docker
and Podman outputs.
Comment on lines
+20
to
+37
| # Mock pytest.raises for fallback tests | ||
| class pytest: | ||
|
|
||
| class raises: | ||
|
|
||
| def __init__(self, exc): | ||
| self.exc = exc | ||
|
|
||
| def __enter__(self): | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_value, traceback): | ||
| # Check if exception matches expected type | ||
| if exc_type is not None and issubclass(exc_type, self.exc): | ||
| # Suppress the exception by returning True | ||
| return True | ||
| # Let unexpected exceptions propagate | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file
fd -t f "test_cpu_utils.py" --exec wc -l {} \;Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 204
🏁 Script executed:
# Read lines 15-50 to see the fallback shim and context
cat -n automation/test-execution/ansible/tests/unit/test_cpu_utils.py | sed -n '15,50p'Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 1288
🏁 Script executed:
# Check for `@pytest.mark.unit` decorators in the file
rg "@pytest\.mark\.unit" automation/test-execution/ansible/tests/unit/test_cpu_utils.pyRepository: redhat-et/vllm-cpu-perf-eval
Length of output: 233
🏁 Script executed:
# Find the exact lines where `@pytest.mark.unit` is used
rg -n "@pytest\.mark\.unit" automation/test-execution/ansible/tests/unit/test_cpu_utils.py | head -5Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 181
🏁 Script executed:
# Show the context around the first occurrence of `@pytest.mark.unit` to confirm the issue
cat -n automation/test-execution/ansible/tests/unit/test_cpu_utils.py | sed -n '110,125p'Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 720
🏁 Script executed:
# Verify if there are any other uses of pytest.mark besides .unit
rg -n "pytest\.mark\." automation/test-execution/ansible/tests/unit/test_cpu_utils.pyRepository: redhat-et/vllm-cpu-perf-eval
Length of output: 269
Add mark.unit to the fallback pytest shim to prevent import failure.
When pytest is not installed, the module cannot be imported because @pytest.mark.unit decorators start at line 113, but the fallback shim (lines 20–37) only defines raises, not mark. This causes AttributeError during module import, so the fallback runner never runs.
Suggested fix
except ImportError:
HAS_PYTEST = False
# Mock pytest.raises for fallback tests
class pytest:
+ class mark:
+ `@staticmethod`
+ def unit(obj):
+ return obj
class raises:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@automation/test-execution/ansible/tests/unit/test_cpu_utils.py` around lines
20 - 37, The fallback pytest shim defines class pytest with nested raises but
misses the mark attribute used by decorators like `@pytest.mark.unit`; add a mark
object to the pytest shim (e.g., add an attribute named mark on the pytest class
or module that exposes a unit attribute usable as a decorator) so imports that
reference `@pytest.mark.unit` do not raise AttributeError; ensure the unit
attribute is a callable/decorator that returns the original function (no-op) and
keep the existing raises implementation (refer to the pytest class and its
nested raises in the current shim).
Comment on lines
+596
to
+602
| if __name__ == "__main__": | ||
| # Run tests if pytest not available | ||
| import sys | ||
|
|
||
| if HAS_PYTEST: | ||
| pytest.main([__file__, "-v"]) | ||
| else: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the specific file and lines
cat -n automation/test-execution/ansible/tests/unit/test_cpu_utils.py | sed -n '596,602p'Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 270
🏁 Script executed:
# Get more context around these lines to understand the full picture
cat -n automation/test-execution/ansible/tests/unit/test_cpu_utils.py | sed -n '590,610p'Repository: redhat-et/vllm-cpu-perf-eval
Length of output: 791
🌐 Web query:
pytest.main() return value exit code
💡 Result:
pytest.main returns an exit code as either an int or a pytest.ExitCode enum value (since pytest 5.0). These correspond to the standard pytest exit codes: - 0 / ExitCode.OK: All tests passed - 1 / ExitCode.TESTS_FAILED: Some tests failed - 2 / ExitCode.INTERRUPTED: Interrupted by user - 3 / ExitCode.INTERNAL_ERROR: Internal error - 4 / ExitCode.USAGE_ERROR: Command line usage error - 5 / ExitCode.NO_TESTS_COLLECTED: No tests collected Unlike command-line invocation, pytest.main does not raise SystemExit but returns the code directly for programmatic use. The enum values have integer values matching these codes and can be imported via from pytest import ExitCode.
Citations:
- 1: https://stackoverflow.com/questions/62648688/capture-exit-code-for-pytest-from-python
- 2: https://docs.pytest.org/en/4.6.x/how-to/usage.html
- 3: https://happytest.readthedocs.io/en/latest/usage/
- 4: https://github.com/pytest-dev/pytest/blob/main/doc/en/reference/exit-codes.rst
- 5: https://docs.pytest.org/en/stable/reference.html
- 6: https://happytest-apidoc.readthedocs.io/en/latest/_modules/pytest/
- 7: https://happytest-apidoc.readthedocs.io/en/latest/api/pytest/
Return pytest.main()'s exit status.
pytest.main() returns an exit code that should be passed to sys.exit(). Without it, python3 test_cpu_utils.py exits with status 0 even when tests fail, breaking CI/CD integration.
Suggested fix
if __name__ == "__main__":
- # Run tests if pytest not available
- import sys
-
if HAS_PYTEST:
- pytest.main([__file__, "-v"])
+ sys.exit(pytest.main([__file__, "-v"]))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if __name__ == "__main__": | |
| # Run tests if pytest not available | |
| import sys | |
| if HAS_PYTEST: | |
| pytest.main([__file__, "-v"]) | |
| else: | |
| if __name__ == "__main__": | |
| # Run tests if pytest not available | |
| import sys | |
| if HAS_PYTEST: | |
| sys.exit(pytest.main([__file__, "-v"])) | |
| else: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@automation/test-execution/ansible/tests/unit/test_cpu_utils.py` around lines
596 - 602, The test runner currently calls pytest.main([...]) but does not
propagate its exit code, so update the __main__ block to pass pytest.main(...)
into sys.exit so failures yield non-zero process exit; specifically, in the if
HAS_PYTEST branch replace the standalone call to pytest.main([__file__, "-v"])
with sys.exit(pytest.main([__file__, "-v"])) (ensure sys is imported and that
the change is made inside the existing if __name__ == "__main__" / if HAS_PYTEST
block referencing HAS_PYTEST, pytest.main, and sys.exit).
This commit fixes two configuration bugs found by smoke tests: Bug #1: Remove OPT models (opt-125m, opt-1.3b) - Context length (2048) incompatible with summarization workload (4096) - Legacy models causing validation failures - Reduced model count from 8 to 6 LLM models Changes: - Removed opt-125m and opt-1.3b from model-matrix.yaml - Updated README.md: 8 → 6 LLM models - Updated docs/methodology/overview.md: 8 → 6 LLM models - Removed OPT Family section from models/models.md - Removed all OPT entries from model tables and test scenarios - Removed "Decode-Heavy Models" section (was OPT-only) Bug #2: Add missing embedding model fields - granite-embedding-english-r2: added dimensions=384, max_sequence_length=512 - granite-embedding-278m-multilingual: added dimensions=768, max_sequence_length=512 These fixes ensure all model configurations pass validation tests. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
maryamtahhan
force-pushed
the
smoke-tests
branch
from
fce2e05 to
3e4fa16
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary: Add smoke tests
Summary
This PR adds a comprehensive smoke test suite that validates Ansible playbooks, model configurations, and container settings before deployment. The tests run in < 5 seconds, require no infrastructure, and have already caught 2 critical configuration bugs!
What's Changed
New Test Suite (
automation/test-execution/ansible/tests/smoke/)1. Playbook Syntax Validation (
test_playbook_syntax.py) - 12 testsansible-playbook --syntax-check)2. Model Matrix Validation (
test_model_matrix.py) - 10 tests3. Container Configuration (
test_container_config.py) - 7 testsInfrastructure Updates
GitHub Workflow: Updated
.github/workflows/unit-tests.ymlsmoke-testsjobPytest Configuration: Added
pytest.ini@pytest.mark.smoke,@pytest.mark.unit,@pytest.mark.slowDocumentation:
tests/README.md- comprehensive testing guideTESTING-ROADMAP.md- future testing phasesTest Markers: Added
@pytest.mark.unitto all existing unit tests🎉 Bugs Found Immediately!
The smoke tests caught 2 real configuration bugs on the first run:
Bug #1: OPT Models - Context Length Mismatch⚠️
Impact: These models would fail at runtime when attempting to run the summarization workload.
Bug #2: Embedding Models - Missing Required Fields⚠️
Impact: Incomplete model definitions that could cause issues during test execution.
Test Results
Before This PR
After This PR
@pytest.mark.unit)Performance
How to Test Locally
CI Integration
Tests run automatically on:
Workflow:
.github/workflows/unit-tests.ymlunit-tests(runs unit tests)smoke-tests(runs smoke tests, depends on unit-tests)Files Changed
New Files:
Modified Files:
Why This Matters
Before: Manual Validation
After: Automated Validation
Future Work
Breaking Changes
None. This PR only adds new tests and doesn't modify any runtime behavior.
Checklist
README.md,TESTING-ROADMAP.md)Demo: Smoke Tests in Action
🤖 Generated with Claude Code