Container image env var#94
Conversation
📝 WalkthroughWalkthroughAdds environment-variable driven container image overrides ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🧹 Nitpick comments (1)
automation/test-execution/ansible/inventory/group_vars/all/benchmark-tools.yml (1)
18-19: Consider using a versioned tag instead oflatestfor reproducibility.Using
latesttag can lead to non-reproducible benchmark results if the image is updated between runs. The role's internal default at line 46 usesv0.5.3, creating an inconsistency.Suggested change for consistency
# Using GuideLLM official container image # Can be overridden with environment variable: export GUIDELLM_CONTAINER_IMAGE=... - container_image: "{{ lookup('env', 'GUIDELLM_CONTAINER_IMAGE') | default('ghcr.io/vllm-project/guidellm:latest', true) }}" + container_image: "{{ lookup('env', 'GUIDELLM_CONTAINER_IMAGE') | default('ghcr.io/vllm-project/guidellm:v0.5.3', true) }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/inventory/group_vars/all/benchmark-tools.yml` around lines 18 - 19, The container_image variable currently defaults to the unpinned 'ghcr.io/vllm-project/guidellm:latest', which harms reproducibility; update the default in the container_image definition (while keeping the GUIDELLM_CONTAINER_IMAGE env lookup override) to use the versioned tag used by the role (e.g., 'ghcr.io/vllm-project/guidellm:v0.5.3') so container_image and the role default are consistent and benchmark runs are reproducible.
🤖 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/llm-benchmark-concurrent-load.yml`:
- Around line 94-97: The validation currently allows any key from test_configs
(via base_workload in test_configs.keys()) which lets invalid types through;
change it to only accept true base workloads (e.g., restrict to ['chat','code'])
by replacing the generic membership test with an explicit allowed list or by
deriving allowed_base_workloads = ['chat','code'] and checking base_workload
against that; also update the fail_msg to list those allowed base workloads and
ensure the later variable-workload logic that references 'chat_var' and
'code_var' remains consistent with this restriction.
---
Nitpick comments:
In
`@automation/test-execution/ansible/inventory/group_vars/all/benchmark-tools.yml`:
- Around line 18-19: The container_image variable currently defaults to the
unpinned 'ghcr.io/vllm-project/guidellm:latest', which harms reproducibility;
update the default in the container_image definition (while keeping the
GUIDELLM_CONTAINER_IMAGE env lookup override) to use the versioned tag used by
the role (e.g., 'ghcr.io/vllm-project/guidellm:v0.5.3') so container_image and
the role default are consistent and benchmark runs are reproducible.
🪄 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: 684522b9-f263-44b4-b2e8-f6a2147363b8
📒 Files selected for processing (6)
automation/test-execution/ansible/ansible.mdautomation/test-execution/ansible/inventory/group_vars/all/benchmark-tools.ymlautomation/test-execution/ansible/inventory/group_vars/all/infrastructure.ymlautomation/test-execution/ansible/llm-benchmark-concurrent-load.ymlautomation/test-execution/ansible/roles/benchmark_guidellm/tasks/main.ymldocs/getting-started.md
|
I reviewed this PR in two steps
THEN use of this syntax resulted in failure
NOTE that I made no edits to vllm-cpu-perf-eval/automation/test-execution/ansible/llm-benchmark-concurrent-load.yml:94 |
Hi John, I think I saw something similar when I just added the workload to the end of the file. But it needs to be in the test_configs section. Was your new definition in that section? |
|
yes, I added it in test_configs using this syntax
$ vi
automation/test-execution/ansible/inventory/group_vars/all/test-workloads.yml
test_configs:
# NEW Chat-lite workload
chat_lite:
workload_type: "chat_lite"
isl: 256 # Standard input: user query
osl: 128 # Standard output: assistant
response
variability: false
backend: "openai-chat"
vllm_args:
- "--dtype=auto" # Fallback dtype - overridden by
model-specific dtype
- "--no-enable-prefix-caching" # Baseline mode: no prefix caching
- "--max-model-len=2048" # Limit model context to workload
needs (2x total tokens for headroom)
kv_cache_space: "40GiB" # Fallback value - should be
overridden by model-specific kv_cache_sizes
Eventually I hijacked 'chat_var', reduced the ISL and OSL values and ran a
test using that
workload-type.
# NEW Chat-lite workload
chat_var:
workload_type: "chat_var"
isl: 256 # Standard input: user query
osl: 128 # Standard output: assistant
response
variability: false
backend: "openai-chat"
vllm_args:
- "--dtype=auto" # Fallback dtype - overridden by
model-specific dtype
- "--no-enable-prefix-caching" # Baseline mode: no prefix caching
- "--max-model-len=2048" # Limit model context to workload
needs (2x total tokens for headroom)
kv_cache_space: "40GiB" # Fallback value - should be
overridden by model-specific kv_cache_sizes
- John
…On Mon, Apr 6, 2026 at 12:26 AM Maryam Tahhan ***@***.***> wrote:
*maryamtahhan* left a comment (redhat-et/vllm-cpu-perf-eval#94)
<#94 (comment)>
I reviewed this PR in two steps
1. "-e $IMAGE" support
Using this syntax I had success
for VLLM_IMAGE in "${image_array[@]}"; do
-e "VLLM_CONTAINER_IMAGE={'image': '${VLLM_IMAGE}'}"
2. New workload_type support
I added a new workload_type to:
automation/test-execution/ansible/inventory/group_vars/all/test-workloads.yml
THEN use of this syntax resulted in failure llm-benchmark-concurrent-load.yml
\ -e "base_workload=chat_lite" \
TASK [vllm_server : Validate workload type]
************************************ fatal: [vllm-server]: FAILED! => {
"assertion": "workload_type in ['summarization', 'chat', 'code', 'rag',
'embedding', 'chat_var', 'code_var']", "changed": false, "evaluated_to":
false, "msg": "Invalid workload_type 'chat_lite'. Must be one of:
summarization, chat, code, rag, embedding, chat_var, code_var" }
NOTE that I made no edits to
vllm-cpu-perf-eval/automation/test-execution/ansible/llm-benchmark-concurrent-load.yml:94
Hi John, I think I saw something similar when I just added the workload to
the end of the file. But it needs to be in the test_configs section. Was
your new definition in that section?
—
Reply to this email directly, view it on GitHub
<#94 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFNXSAWC3FJQS5SMHEKZVI34UMWVRAVCNFSM6AAAAACXL5U7COVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCOJQGI4TCMRSHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
Retested using the changes implemented in SUCCESS. I was able to add a new workload_type 'chat_lite' and the test ran successfully. |
|
Hold on, I'm seeing an issue with the "-e $IMAGE" support. Script syntax syntax Ansible Controller DUT$ podman ps |
|
This PR looks good. I changed my script syntax to use "export" and now the testruns are using the designated ENVvar container image.
|
Add support for configuring container images via environment variables: - VLLM_CONTAINER_IMAGE: vLLM server image - GUIDELLM_CONTAINER_IMAGE: GuideLLM benchmark tool image - VLLM_BENCH_CONTAINER_IMAGE: vLLM bench tool image All variables include sensible defaults matching current configuration, allowing users to easily override images without editing config files. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add container image display to GuideLLM configuration output to match the vLLM server display, making it easier to verify which image is being used during test execution. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Replace hardcoded workload type validation with dynamic check against test_configs.keys(), matching the approach used in llm-benchmark-auto.yml. This allows users to add custom workloads to test-workloads.yml and automatically use them in concurrent load testing without modifying the playbook validation logic. Changes: - base_workload validation now checks test_configs.keys() - variable workload check now dynamic instead of hardcoded list - Updated documentation to reflect workload flexibility Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Update workload type validation in vllm_server role to dynamically check against test_configs.keys() instead of hardcoded workload list. This allows users to add custom workloads to test-workloads.yml without modifying role code. Changes: - main.yml: Validate against test_configs.keys() - start-llm.yml: Validate non-embedding workloads dynamically - Improved error messages to show available workloads Fixes issue where new workloads in test-workloads.yml were rejected by hardcoded validation, even after concurrent-load playbook was made dynamic. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
maryamtahhan
force-pushed
the
container-image-env-var
branch
from
770fc59 to
6841dfe
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
automation/test-execution/ansible/roles/vllm_server/tasks/start-llm.yml (1)
24-61: Move workload validation beforetest_configs[workload_type]access.Line 26 reads
test_configs[workload_type]before the assert at Lines 55-60. If this task file is called directly with an invalidworkload_type, execution fails early with a dict-key error and skips your clearer validation message.As per coding guidelines, "`**`: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed reordering
-- name: Get workload and core configuration - ansible.builtin.set_fact: - workload_cfg: "{{ test_configs[workload_type] }}" - core_cfg: "{{ core_configuration }}" - container_cfg: "{{ container_runtime }}" - # ============================================================================ # Caching Mode Configuration # ============================================================================ @@ # ============================================================================ # Workload Validation # ============================================================================ - name: Validate workload type ansible.builtin.assert: that: - workload_type in test_configs.keys() - workload_type != 'embedding' fail_msg: "Invalid workload_type: {{ workload_type }}. Must be a non-embedding workload from: {{ test_configs.keys() | list | select('ne', 'embedding') | sort | join(', ') }}" + +- name: Get workload and core configuration + ansible.builtin.set_fact: + workload_cfg: "{{ test_configs[workload_type] }}" + core_cfg: "{{ core_configuration }}" + container_cfg: "{{ container_runtime }}"🤖 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 24 - 61, The workload validation currently runs after the task that sets workload_cfg using test_configs[workload_type], causing a dict-key error for invalid inputs; move the "Validate workload type" ansible.builtin.assert task (the block that checks workload_type in test_configs.keys() and != 'embedding') so it appears before the "Get workload and core configuration" ansible.builtin.set_fact (which sets workload_cfg: "{{ test_configs[workload_type] }}"), ensuring the assert runs first and prevents accessing test_configs with an invalid key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@automation/test-execution/ansible/roles/vllm_server/tasks/start-llm.yml`:
- Around line 24-61: The workload validation currently runs after the task that
sets workload_cfg using test_configs[workload_type], causing a dict-key error
for invalid inputs; move the "Validate workload type" ansible.builtin.assert
task (the block that checks workload_type in test_configs.keys() and !=
'embedding') so it appears before the "Get workload and core configuration"
ansible.builtin.set_fact (which sets workload_cfg: "{{
test_configs[workload_type] }}"), ensuring the assert runs first and prevents
accessing test_configs with an invalid key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b227a5c-4c65-4688-9b9c-dd4444bc0673
📒 Files selected for processing (8)
automation/test-execution/ansible/ansible.mdautomation/test-execution/ansible/inventory/group_vars/all/benchmark-tools.ymlautomation/test-execution/ansible/inventory/group_vars/all/infrastructure.ymlautomation/test-execution/ansible/llm-benchmark-concurrent-load.ymlautomation/test-execution/ansible/roles/benchmark_guidellm/tasks/main.ymlautomation/test-execution/ansible/roles/vllm_server/tasks/main.ymlautomation/test-execution/ansible/roles/vllm_server/tasks/start-llm.ymldocs/getting-started.md
✅ Files skipped from review due to trivial changes (3)
- automation/test-execution/ansible/roles/benchmark_guidellm/tasks/main.yml
- automation/test-execution/ansible/ansible.md
- automation/test-execution/ansible/inventory/group_vars/all/infrastructure.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- automation/test-execution/ansible/llm-benchmark-concurrent-load.yml
- automation/test-execution/ansible/roles/vllm_server/tasks/main.yml
- automation/test-execution/ansible/inventory/group_vars/all/benchmark-tools.yml
- docs/getting-started.md
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores