Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds hostpath model support and hostPath HF cache wiring; centralizes tensor-parallelism and vLLM args; introduces InfiniBand/AKS hotfix and router toleration wiring; updates ISVC scheduler/nodeSelector manifests; captures HTTPRoute and benchmark artifacts; reorganizes visualization reports and plotting logic. ChangesHostpath Model Support & Infrastructure Configuration
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
|
/test jump-ci llm-d azure_h100 intelligentrouting-flavors guidellm_multiturn_eval llama-70b |
|
🟢 Test of 'llm-d test test_ci' succeeded after 00 hours 57 minutes 49 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 pd-flavors guidellm_multiturn_eval gpt-oss |
|
/test jump-ci llm-d azure_h100 llmisvc_pd guidellm_multiturn_eval gpt-oss |
|
/test jump-ci llm-d azure_h100 pd-flavors guidellm_multiturn_eval gpt-oss aks_ib |
|
🔴 Test of 'llm-d test test_ci' failed after 00 hours 08 minutes 01 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 llmisvc_pd guidellm_multiturn_eval gpt-oss aks_ib |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/llm-d/testing/test_llmd.py (1)
760-762:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply vLLM args to prefill containers as well.
Hostpath flow later expects
VLLM_ADDITIONAL_ARGSin prefill, but this function only populates main. That can fail at runtime for PD hostpath runs when prefill has no max-model-len override.Suggested fix
def apply_vllm_args_configuration(isvc_data): @@ - # Apply to main container only + # Apply to main container _apply_vllm_args_to_container_section(isvc_data, 'spec.template.containers', vllm_args, 'main') + + # Apply to prefill container for P/D deployments + if 'spec' in isvc_data and 'prefill' in isvc_data['spec']: + _apply_vllm_args_to_container_section(isvc_data, 'spec.prefill.template.containers', vllm_args, 'main')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm-d/testing/test_llmd.py` around lines 760 - 762, The code only applies vLLM args to the main container; call _apply_vllm_args_to_container_section for the prefill containers as well so VLLM_ADDITIONAL_ARGS is populated there (e.g., add a second call like _apply_vllm_args_to_container_section(isvc_data, 'spec.template.prefill.containers', vllm_args, 'prefill') or the correct path for prefill containers in this manifest) to ensure prefill gets the max-model-len override.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/llm-d/testing/test_llmd.py`:
- Around line 987-1009: The code assumes spec.prefill exists and that each
container has resources->requests/limits initialized; update the logic in
apply_to_containers and the call sites: before modifying a container ensure
container.setdefault('resources', {}) and then
container['resources'].setdefault('limits', {}) and .setdefault('requests', {}),
use those dicts when popping/setting rdma keys, and guard applying to prefill by
checking if 'prefill' in isvc_data['spec'] (only call apply_to_containers on
isvc_data['spec']['prefill']['template']['containers'] when present) while
preserving the existing infiniband_config handling in apply_to_containers.
- Around line 718-725: The code is overwriting existing
container['volumeMounts'] and isvc_data['spec']['template']['volumes'], which
can drop previously defined mounts/volumes; update the logic to merge rather
than replace: for container use a safe-get/ensure pattern (e.g.,
container.setdefault('volumeMounts', []) or check for existing list) and append
hf_cache_mount only if not already present, likewise ensure container['env'] is
initialized (already done) and for the main template use
isvc_data['spec']['template'].setdefault('volumes', []) or merge with the
existing list and append hf_cache_volume if missing so existing mounts/volumes
are preserved (refer to symbols container, hf_cache_mount, isvc_data,
hf_cache_volume, volumeMounts, volumes).
---
Outside diff comments:
In `@projects/llm-d/testing/test_llmd.py`:
- Around line 760-762: The code only applies vLLM args to the main container;
call _apply_vllm_args_to_container_section for the prefill containers as well so
VLLM_ADDITIONAL_ARGS is populated there (e.g., add a second call like
_apply_vllm_args_to_container_section(isvc_data,
'spec.template.prefill.containers', vllm_args, 'prefill') or the correct path
for prefill containers in this manifest) to ensure prefill gets the
max-model-len override.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b075169-d39a-46fb-ab08-67cc1d36a76e
📒 Files selected for processing (10)
projects/llm-d/testing/config.yamlprojects/llm-d/testing/llmisvcs/llmisvc-pd.yamlprojects/llm-d/testing/llmisvcs/llmisvc-simple.yamlprojects/llm-d/testing/prepare_llmd.pyprojects/llm-d/testing/test_llmd.pyprojects/llm-d/toolbox/llmd_capture_isvc_state/tasks/main.ymlprojects/llm-d/toolbox/llmd_deploy_llm_inference_service/tasks/main.ymlprojects/llm-d/visualizations/llmd_inference/data/plots.yamlprojects/llm-d/visualizations/llmd_inference/data/reports.yamlprojects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py
💤 Files with no reviewable changes (2)
- projects/llm-d/visualizations/llmd_inference/data/plots.yaml
- projects/llm-d/testing/llmisvcs/llmisvc-pd.yaml
| def apply_to_containers(containers): | ||
| for container in containers: | ||
| # Always remove existing rdma/ib first | ||
| container['resources']['limits'].pop('rdma/ib', None) | ||
| container['resources']['requests'].pop('rdma/ib', None) | ||
|
|
||
| # Add InfiniBand resource based on config type | ||
| if infiniband_config is True: | ||
| container['resources']['limits']['rdma/ib'] = "1" | ||
| container['resources']['requests']['rdma/ib'] = "1" | ||
| elif isinstance(infiniband_config, str): | ||
| # Use custom resource string (e.g., "rdma/shared_ib") | ||
| container['resources']['limits'][infiniband_config] = "1" | ||
| container['resources']['requests'][infiniband_config] = "1" | ||
|
|
||
| # Apply to main template containers | ||
| apply_to_containers(isvc_data['spec']['template']['containers']) | ||
|
|
||
| # Apply to prefill template containers (for P/D deployments) | ||
| if 'prefill' not in isvc_data['spec']: | ||
| raise ValueError("Trying to apply infiniband config without a prefill deployment") | ||
|
|
||
| apply_to_containers(isvc_data['spec']['prefill']['template']['containers']) |
There was a problem hiding this comment.
InfiniBand configuration incorrectly assumes prefill exists and resources are pre-initialized.
This will fail on non-PD ISVCs (no spec.prefill) and on containers missing resources.requests/limits.
Suggested fix
def apply_to_containers(containers):
for container in containers:
+ container.setdefault('resources', {})
+ container['resources'].setdefault('limits', {})
+ container['resources'].setdefault('requests', {})
# Always remove existing rdma/ib first
container['resources']['limits'].pop('rdma/ib', None)
container['resources']['requests'].pop('rdma/ib', None)
@@
- if 'prefill' not in isvc_data['spec']:
- raise ValueError("Trying to apply infiniband config without a prefill deployment")
-
- apply_to_containers(isvc_data['spec']['prefill']['template']['containers'])
+ if 'prefill' in isvc_data['spec']:
+ apply_to_containers(isvc_data['spec']['prefill']['template']['containers'])- if 'annotations' not in isvc_data['spec']['prefill']:
- isvc_data['spec']['prefill']['annotations'] = {}
- isvc_data['spec']['prefill']['annotations']['ulimits.nri.containerd.io/container.main'] = ulimit_annotation_value
+ if 'prefill' in isvc_data['spec']:
+ if 'annotations' not in isvc_data['spec']['prefill']:
+ isvc_data['spec']['prefill']['annotations'] = {}
+ isvc_data['spec']['prefill']['annotations']['ulimits.nri.containerd.io/container.main'] = ulimit_annotation_valueAlso applies to: 1148-1151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/llm-d/testing/test_llmd.py` around lines 987 - 1009, The code
assumes spec.prefill exists and that each container has
resources->requests/limits initialized; update the logic in apply_to_containers
and the call sites: before modifying a container ensure
container.setdefault('resources', {}) and then
container['resources'].setdefault('limits', {}) and .setdefault('requests', {}),
use those dicts when popping/setting rdma keys, and guard applying to prefill by
checking if 'prefill' in isvc_data['spec'] (only call apply_to_containers on
isvc_data['spec']['prefill']['template']['containers'] when present) while
preserving the existing infiniband_config handling in apply_to_containers.
|
🔴 Test of 'llm-d test test_ci' failed after 00 hours 07 minutes 39 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 llmisvc_pd guidellm_multiturn_eval gpt-oss aks_ib |
|
🟢 Test of 'llm-d test test_ci' succeeded after 00 hours 25 minutes 10 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 llmisvc_pd guidellm_multiturn_eval gpt-oss aks_ib |
|
🔴 Test of 'llm-d test test_ci' failed after 00 hours 00 minutes 40 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 llmisvc_pd guidellm_multiturn_eval gpt-oss |
|
🟢 Test of 'llm-d test test_ci' succeeded after 00 hours 46 minutes 57 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 llmisvc_pd guidellm_multiturn_eval gpt-oss aks_ib |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/llm-d/testing/test_llmd.py (1)
743-770:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
apply_vllm_args_configurationdrops user vllm_args on the prefill container.The docstring says "Applies VLLM args to both main container and prefill container (for P/D deployments)", but the implementation only writes to
spec.template.containers(main). For PD flavors, the prefill container will therefore not receive--gpu-memory-utilization=0.92,--enable-prefix-caching,--trust-remote-code,--no-disable-hybrid-kv-cache-manager, etc., even thoughapply_max_model_len_configurationandapply_prefill_tensor_parallelismdo target prefill. This is especially impactful for the new hostpath branch, whereconfigure_vllm_commandconsumesVLLM_ADDITIONAL_ARGSinto the prefill command line — those user-configured flags will silently be missing from prefill while present on decode, leading to asymmetric vLLM behavior between P and D and skewed benchmark numbers.♻️ Suggested fix: also apply to prefill in PD deployments
logging.info(f"Applying vLLM args: {final_vllm_args}") # Apply to main container only _apply_vllm_args_to_container_section(isvc_data, 'spec.template.containers', final_vllm_args, 'main') + + # Apply to prefill container if this is a P/D deployment + if 'spec' in isvc_data and 'prefill' in isvc_data['spec']: + logging.info("P/D deployment detected - applying vLLM args to prefill container") + _apply_vllm_args_to_container_section(isvc_data, 'spec.prefill.template.containers', final_vllm_args, 'main')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm-d/testing/test_llmd.py` around lines 743 - 770, apply_vllm_args_configuration currently only applies vLLM args to the main container (via _apply_vllm_args_to_container_section with 'spec.template.containers'/'main') which drops user args for prefill in PD deployments; update the function to also call _apply_vllm_args_to_container_section for the prefill container (use the same final_vllm_args and a container selector like 'spec.template.prefill.containers' and container name 'prefill' to mirror how apply_max_model_len_configuration and apply_prefill_tensor_parallelism target prefill), keeping the existing logging behavior so both main and prefill receive identical VLLM flags.
🧹 Nitpick comments (1)
projects/llm-d/testing/test_llmd.py (1)
1066-1073: 💤 Low valueStray triple-quoted string in function body.
This block isn't a docstring (the function already has one on line 1022) and isn't assigned to anything — Python evaluates and discards it. It also lists a slightly different set of files than the actual hotfix_files list (
interface.pyinstead ofplatforms/interface.py,multiproc_executor.pyinstead ofv1/executor/multiproc_executor.py, etc.), so it will drift further from reality over time. Either convert it to a#comment or drop it since the runtime error at line 1058 already produces the correctoc create cm …invocation.♻️ Suggested cleanup
- """ - oc create cm vllm-ucx-multiproc-hotfix \ - --from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/envs.py \ - --from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/interface.py \ - --from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/multiproc_executor.py \ - --from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/uniproc_executor.py \ - --from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/vllm_net_devices.py - """ -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm-d/testing/test_llmd.py` around lines 1066 - 1073, Remove the stray triple-quoted string block inside the function (the unassigned multiline literal shown between lines ~1066–1073) because it's not a docstring and duplicates outdated file names; either delete it entirely or convert it to a simple # comment if you want to keep the example, and ensure any example matches the actual hotfix_files list used elsewhere (see hotfix_files and the runtime error/oc invocation at line ~1058) so the source of truth remains the hotfix_files variable rather than this dead literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@projects/llm-d/testing/test_llmd.py`:
- Around line 743-770: apply_vllm_args_configuration currently only applies vLLM
args to the main container (via _apply_vllm_args_to_container_section with
'spec.template.containers'/'main') which drops user args for prefill in PD
deployments; update the function to also call
_apply_vllm_args_to_container_section for the prefill container (use the same
final_vllm_args and a container selector like 'spec.template.prefill.containers'
and container name 'prefill' to mirror how apply_max_model_len_configuration and
apply_prefill_tensor_parallelism target prefill), keeping the existing logging
behavior so both main and prefill receive identical VLLM flags.
---
Nitpick comments:
In `@projects/llm-d/testing/test_llmd.py`:
- Around line 1066-1073: Remove the stray triple-quoted string block inside the
function (the unassigned multiline literal shown between lines ~1066–1073)
because it's not a docstring and duplicates outdated file names; either delete
it entirely or convert it to a simple # comment if you want to keep the example,
and ensure any example matches the actual hotfix_files list used elsewhere (see
hotfix_files and the runtime error/oc invocation at line ~1058) so the source of
truth remains the hotfix_files variable rather than this dead literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4eada77-3ebc-406b-8ac1-23baedcb68fb
📒 Files selected for processing (2)
projects/llm-d/testing/config.yamlprojects/llm-d/testing/test_llmd.py
|
🔴 Test of 'llm-d test test_ci' failed after 01 hours 12 minutes 50 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 llmisvc_pd guidellm_multiturn_eval gpt-oss aks_ib |
|
🔴 Test of 'llm-d test test_ci' failed after 00 hours 04 minutes 49 seconds. 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
/test jump-ci llm-d azure_h100 llmisvc_pd guidellm_multiturn_eval gpt-oss aks_ib |
|
🔴 Test of 'llm-d test test_ci' failed after 00 hours 07 minutes 38 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 pd-flavors guidellm_multiturn_eval gpt-oss |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
projects/llm-d/testing/test_llmd.py (2)
1202-1206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnconditional prefill access will fail if prefill doesn't exist.
Lines 1203-1205 access
isvc_data['spec']['prefill']['annotations']without checking ifprefillexists, unlike the guarded access patterns at lines 1091 and 1184. If a malformed pd YAML lacksprefill, this crashes withKeyErrorbefore the parent function's more helpfulValueErrorat line 1030.Proposed fix to guard prefill access
# Set annotations for prefill pods (spec.prefill.annotations) - if 'annotations' not in isvc_data['spec']['prefill']: - isvc_data['spec']['prefill']['annotations'] = {} - isvc_data['spec']['prefill']['annotations']['ulimits.nri.containerd.io/container.main'] = ulimit_annotation_value + if 'prefill' in isvc_data['spec']: + if 'annotations' not in isvc_data['spec']['prefill']: + isvc_data['spec']['prefill']['annotations'] = {} + isvc_data['spec']['prefill']['annotations']['ulimits.nri.containerd.io/container.main'] = ulimit_annotation_value🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm-d/testing/test_llmd.py` around lines 1202 - 1206, The code unconditionally indexes isvc_data['spec']['prefill'] causing KeyError if 'prefill' is absent; update the block that sets ulimits.nri annotation to first ensure 'prefill' exists (e.g., if 'prefill' not in isvc_data['spec']: isvc_data['spec']['prefill'] = {}) and then ensure annotations exists before assigning isvc_data['spec']['prefill']['annotations']['ulimits.nri.containerd.io/container.main'] = ulimit_annotation_value so the code mirrors the guarded access used elsewhere and avoids KeyError.
1010-1024:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInfiniBand configuration assumes resources are pre-initialized.
apply_to_containersaccessescontainer['resources']['limits']andcontainer['resources']['requests']directly, but these may not exist if tensor parallelism was not configured (e.g.,tp_sizeisNone).Proposed fix to initialize resources safely
def apply_to_containers(containers): for container in containers: + container.setdefault('resources', {}) + container['resources'].setdefault('limits', {}) + container['resources'].setdefault('requests', {}) # Always remove existing rdma/ib first container['resources']['limits'].pop('rdma/ib', None) container['resources']['requests'].pop('rdma/ib', None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm-d/testing/test_llmd.py` around lines 1010 - 1024, The apply_to_containers function assumes container['resources']['limits'] and ['requests'] exist; adjust it to safely initialize missing dicts before use: ensure container has a 'resources' dict and that 'limits' and 'requests' are dicts (create empty dicts if absent) prior to popping or assigning the rdma keys, then proceed with the existing logic that removes any prior 'rdma/ib' and sets the resource based on infiniband_config (True => use 'rdma/ib', str => use that string).
🧹 Nitpick comments (2)
projects/llm-d/testing/test_llmd.py (2)
1124-1125: 💤 Low valueUse exception chaining when re-raising.
Raising a new exception without chaining loses the original traceback. Use
raise ... from eto preserve context.Proposed fix
except Exception as e: - raise RuntimeError(f"Failed to verify ConfigMap '{cm_name}' in namespace '{namespace}': {e}") + raise RuntimeError(f"Failed to verify ConfigMap '{cm_name}' in namespace '{namespace}': {e}") from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm-d/testing/test_llmd.py` around lines 1124 - 1125, The except block that currently does "raise RuntimeError(f\"Failed to verify ConfigMap '{cm_name}' in namespace '{namespace}': {e}\")" should preserve the original traceback by using exception chaining; change the re-raise to "raise RuntimeError(... ) from e" so the original exception is chained to the new RuntimeError (referencing the cm_name and namespace variables in the error message).
1157-1166: 💤 Low valueDead code:
add_hotfix_volumesis defined but never called.The function
add_hotfix_volumes(lines 1157-1166) is defined but never used. Instead,add_hotfix_to_containers(lines 1169-1177) performs the same logic and is actually called. Remove the dead function.Proposed fix to remove dead code
- # Helper function to add hotfix volume mounts to containers - def add_hotfix_volumes(containers, template_volumes): - for container in containers: - # Add volume mounts - if 'volumeMounts' not in container: - container['volumeMounts'] = [] - container['volumeMounts'].extend(hotfix_mounts) - - # Add volume to template - template_volumes.append(hotfix_volume) - # Apply hotfix volumes and complete configuration to containers def add_hotfix_to_containers(containers, template_volumes):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm-d/testing/test_llmd.py` around lines 1157 - 1166, Remove the dead helper function add_hotfix_volumes which duplicates logic already implemented and used in add_hotfix_to_containers; delete the entire add_hotfix_volumes definition (including its loop and the template_volumes.append(hotfix_volume) line) so only add_hotfix_to_containers manipulates container['volumeMounts'], hotfix_mounts and template_volumes/hotfix_volume remain in use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@projects/llm-d/testing/test_llmd.py`:
- Around line 1202-1206: The code unconditionally indexes
isvc_data['spec']['prefill'] causing KeyError if 'prefill' is absent; update the
block that sets ulimits.nri annotation to first ensure 'prefill' exists (e.g.,
if 'prefill' not in isvc_data['spec']: isvc_data['spec']['prefill'] = {}) and
then ensure annotations exists before assigning
isvc_data['spec']['prefill']['annotations']['ulimits.nri.containerd.io/container.main']
= ulimit_annotation_value so the code mirrors the guarded access used elsewhere
and avoids KeyError.
- Around line 1010-1024: The apply_to_containers function assumes
container['resources']['limits'] and ['requests'] exist; adjust it to safely
initialize missing dicts before use: ensure container has a 'resources' dict and
that 'limits' and 'requests' are dicts (create empty dicts if absent) prior to
popping or assigning the rdma keys, then proceed with the existing logic that
removes any prior 'rdma/ib' and sets the resource based on infiniband_config
(True => use 'rdma/ib', str => use that string).
---
Nitpick comments:
In `@projects/llm-d/testing/test_llmd.py`:
- Around line 1124-1125: The except block that currently does "raise
RuntimeError(f\"Failed to verify ConfigMap '{cm_name}' in namespace
'{namespace}': {e}\")" should preserve the original traceback by using exception
chaining; change the re-raise to "raise RuntimeError(... ) from e" so the
original exception is chained to the new RuntimeError (referencing the cm_name
and namespace variables in the error message).
- Around line 1157-1166: Remove the dead helper function add_hotfix_volumes
which duplicates logic already implemented and used in add_hotfix_to_containers;
delete the entire add_hotfix_volumes definition (including its loop and the
template_volumes.append(hotfix_volume) line) so only add_hotfix_to_containers
manipulates container['volumeMounts'], hotfix_mounts and
template_volumes/hotfix_volume remain in use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0159a356-3d57-4ad3-92a4-a299d8d5c8e5
📒 Files selected for processing (4)
projects/llm-d/testing/config.yamlprojects/llm-d/testing/test_llmd.pyprojects/llm-d/visualizations/llmd_inference/data/plots.yamlprojects/llm-d/visualizations/llmd_inference/data/reports.yaml
💤 Files with no reviewable changes (1)
- projects/llm-d/visualizations/llmd_inference/data/plots.yaml
|
🔴 Test of 'llm-d test test_ci' failed after 00 hours 32 minutes 23 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 pd-flavors guidellm_heterogeneous_eval gpt-oss |
|
🔴 Test of 'llm-d test test_ci' failed after 02 hours 02 minutes 49 seconds. 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
/test jump-ci llm-d azure_h100 pd-flavors guidellm_heterogeneous_eval gpt-oss |
|
🔴 Test of 'llm-d test test_ci' failed after 03 hours 28 minutes 33 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 pd-flavors guidellm_heterogeneous_eval gpt-oss |
|
🔴 Test of 'llm-d test test_ci' failed after 01 hours 08 minutes 29 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
@kpouget: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
projects/llm-d/testing/test_llmd.py (1)
1163-1182: 💤 Low valueRemove unused
add_hotfix_volumeshelper function.
add_hotfix_volumes(lines 1163-1171) is defined but never called. Onlyadd_hotfix_to_containers(lines 1174-1182) is used at lines 1186 and 1191. These two functions are identical.🧹 Proposed fix to remove dead code
- # Helper function to add hotfix volume mounts to containers - def add_hotfix_volumes(containers, template_volumes): - for container in containers: - # Add volume mounts - if 'volumeMounts' not in container: - container['volumeMounts'] = [] - container['volumeMounts'].extend(hotfix_mounts) - - # Add volume to template - template_volumes.append(hotfix_volume) - # Apply hotfix volumes and complete configuration to containers def add_hotfix_to_containers(containers, template_volumes):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm-d/testing/test_llmd.py` around lines 1163 - 1182, Remove the redundant helper add_hotfix_volumes: delete the entire add_hotfix_volumes function definition (the block that mirrors add_hotfix_to_containers) and keep only add_hotfix_to_containers; verify there are no remaining references to add_hotfix_volumes elsewhere and run tests to ensure behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/llm-d/testing/test_llmd.py`:
- Around line 1116-1130: The except block currently catches errors raised in the
try (including the intentional RuntimeError when the ConfigMap is missing) and
re-raises a new RuntimeError without preserving the original traceback; update
the handler to preserve the exception chain by using "raise
RuntimeError(f\"Failed to verify ConfigMap '{cm_name}' in namespace
'{namespace}': {e}\") from e" and (optionally) avoid double-wrapping by
re-raising when isinstance(e, RuntimeError) so the original error from the
runtime check on hotfix_files/oc output is not obscured.
In `@projects/llm-d/toolbox/llmd_run_guidellm_benchmark/tasks/main.yml`:
- Around line 138-143: The task "Copy benchmarks.json from PVC to artifacts
directory (with retry)" currently sets retries/delay but uses "when:
file_check_result.rc == 0" so it never retries; change the task to register the
copy command result (e.g., add "register: copy_result"), remove the "when:
file_check_result.rc == 0" guard, and add "until: copy_result.rc == 0" so
Ansible will actually retry the oc exec shell command (the shell line that runs
oc exec ... > "{{ artifact_extra_logs_dir }}/artifacts/results/benchmarks.json")
using the existing retries/delay settings.
In
`@projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py`:
- Line 351: The code uses an unnecessary f-string in the call to html.H4
(html.H4(f"🔧 Simple Flavor Comparison")) which triggers Ruff F541; remove the f
prefix so the literal string is passed directly (change html.H4(f"...") to
html.H4("...")) to eliminate the lint warning and avoid creating an unneeded
formatted string.
---
Nitpick comments:
In `@projects/llm-d/testing/test_llmd.py`:
- Around line 1163-1182: Remove the redundant helper add_hotfix_volumes: delete
the entire add_hotfix_volumes function definition (the block that mirrors
add_hotfix_to_containers) and keep only add_hotfix_to_containers; verify there
are no remaining references to add_hotfix_volumes elsewhere and run tests to
ensure behavior is unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0cb6103-b0a3-4911-be6b-657b7ba6d1df
📒 Files selected for processing (4)
projects/llm-d/testing/config.yamlprojects/llm-d/testing/test_llmd.pyprojects/llm-d/toolbox/llmd_run_guidellm_benchmark/tasks/main.ymlprojects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py
| try: | ||
| result = run.run(f"oc get configmap {cm_name} -n {namespace} --ignore-not-found -o name", | ||
| capture_stdout=True, check=True) | ||
| if not result.stdout.strip(): | ||
| # Extract just the filenames from hotfix_files for the error message | ||
| filenames = [file_path.split('/')[-1] for file_path in hotfix_files] | ||
| file_args = ' \\\n '.join([f"--from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/{filename}" for filename in filenames]) | ||
|
|
||
| raise RuntimeError(f"Required ConfigMap '{cm_name}' not found in namespace '{namespace}'. " | ||
| f"Please create it first:\n\n" | ||
| f"oc create cm vllm-ucx-multiproc-hotfix -n {namespace} \\\n" | ||
| f" {file_args}") | ||
| logging.info(f"Verified ConfigMap '{cm_name}' exists in namespace '{namespace}'") | ||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to verify ConfigMap '{cm_name}' in namespace '{namespace}': {e}") |
There was a problem hiding this comment.
Exception handling wraps its own RuntimeError and loses exception chain.
The try block raises RuntimeError on line 1124 when the ConfigMap is not found. The except Exception on line 1129 catches this same error, wrapping it in another RuntimeError and losing the original traceback. Additionally, raise ... from e should be used to preserve the exception chain.
🐛 Proposed fix
try:
result = run.run(f"oc get configmap {cm_name} -n {namespace} --ignore-not-found -o name",
capture_stdout=True, check=True)
if not result.stdout.strip():
# Extract just the filenames from hotfix_files for the error message
filenames = [file_path.split('/')[-1] for file_path in hotfix_files]
file_args = ' \\\n '.join([f"--from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/{filename}" for filename in filenames])
raise RuntimeError(f"Required ConfigMap '{cm_name}' not found in namespace '{namespace}'. "
f"Please create it first:\n\n"
f"oc create cm vllm-ucx-multiproc-hotfix -n {namespace} \\\n"
f" {file_args}")
logging.info(f"Verified ConfigMap '{cm_name}' exists in namespace '{namespace}'")
- except Exception as e:
- raise RuntimeError(f"Failed to verify ConfigMap '{cm_name}' in namespace '{namespace}': {e}")
+ except RuntimeError:
+ raise # Re-raise our own RuntimeError as-is
+ except Exception as e:
+ raise RuntimeError(f"Failed to verify ConfigMap '{cm_name}' in namespace '{namespace}': {e}") from e📝 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.
| try: | |
| result = run.run(f"oc get configmap {cm_name} -n {namespace} --ignore-not-found -o name", | |
| capture_stdout=True, check=True) | |
| if not result.stdout.strip(): | |
| # Extract just the filenames from hotfix_files for the error message | |
| filenames = [file_path.split('/')[-1] for file_path in hotfix_files] | |
| file_args = ' \\\n '.join([f"--from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/{filename}" for filename in filenames]) | |
| raise RuntimeError(f"Required ConfigMap '{cm_name}' not found in namespace '{namespace}'. " | |
| f"Please create it first:\n\n" | |
| f"oc create cm vllm-ucx-multiproc-hotfix -n {namespace} \\\n" | |
| f" {file_args}") | |
| logging.info(f"Verified ConfigMap '{cm_name}' exists in namespace '{namespace}'") | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to verify ConfigMap '{cm_name}' in namespace '{namespace}': {e}") | |
| try: | |
| result = run.run(f"oc get configmap {cm_name} -n {namespace} --ignore-not-found -o name", | |
| capture_stdout=True, check=True) | |
| if not result.stdout.strip(): | |
| # Extract just the filenames from hotfix_files for the error message | |
| filenames = [file_path.split('/')[-1] for file_path in hotfix_files] | |
| file_args = ' \\\n '.join([f"--from-file=guides/pd-disaggregation/ms-pd/charts/vllm-ucx-multiproc-hotfix/{filename}" for filename in filenames]) | |
| raise RuntimeError(f"Required ConfigMap '{cm_name}' not found in namespace '{namespace}'. " | |
| f"Please create it first:\n\n" | |
| f"oc create cm vllm-ucx-multiproc-hotfix -n {namespace} \\\n" | |
| f" {file_args}") | |
| logging.info(f"Verified ConfigMap '{cm_name}' exists in namespace '{namespace}'") | |
| except RuntimeError: | |
| raise # Re-raise our own RuntimeError as-is | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to verify ConfigMap '{cm_name}' in namespace '{namespace}': {e}") from e |
🧰 Tools
🪛 Ruff (0.15.14)
[warning] 1129-1129: Do not catch blind exception: Exception
(BLE001)
[warning] 1130-1130: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/llm-d/testing/test_llmd.py` around lines 1116 - 1130, The except
block currently catches errors raised in the try (including the intentional
RuntimeError when the ConfigMap is missing) and re-raises a new RuntimeError
without preserving the original traceback; update the handler to preserve the
exception chain by using "raise RuntimeError(f\"Failed to verify ConfigMap
'{cm_name}' in namespace '{namespace}': {e}\") from e" and (optionally) avoid
double-wrapping by re-raising when isinstance(e, RuntimeError) so the original
error from the runtime check on hotfix_files/oc output is not obscured.
| - name: Copy benchmarks.json from PVC to artifacts directory (with retry) | ||
| shell: | | ||
| oc exec {{ llmd_run_guidellm_benchmark_name }}-copy -n "{{ target_namespace }}" -- cat /results/benchmarks.json > "{{ artifact_extra_logs_dir }}/artifacts/results/benchmarks.json" | ||
| retries: 10 | ||
| delay: 1 | ||
| when: file_check_result.rc == 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'Copy benchmarks\.json from PVC|retries:|delay:|until:' projects/llm-d/toolbox/llmd_run_guidellm_benchmark/tasks/main.ymlRepository: openshift-psap/topsail
Length of output: 842
🏁 Script executed:
#!/bin/bash
sed -n '110,170p' projects/llm-d/toolbox/llmd_run_guidellm_benchmark/tasks/main.ymlRepository: openshift-psap/topsail
Length of output: 1983
🌐 Web query:
Ansible task retries delay without until semantics ignored
💡 Result:
In Ansible, the retries and delay parameters are functional components of the until loop mechanism; they have no effect if used in a task without the until keyword [1]. When until is omitted, Ansible ignores these settings, and the task will execute exactly once [2]. Key points regarding this behavior: - Functional Dependency: The retries and delay keywords are specifically designed to control the polling behavior of until [1]. Without until, there is no retry condition for Ansible to evaluate, so the task proceeds as a standard, single-execution task [2]. - Historical Context: Users have frequently requested that retries without until default to "retry until success," but Ansible has maintained the requirement that until must be explicitly defined to enable retry logic [2]. - Best Practices: To implement retry logic correctly, you must register the output of the task and use the until keyword to evaluate that registered variable [3][4]. For example: - name: Example task with retries ansible.builtin.command: /usr/bin/false register: result until: result is succeeded retries: 5 delay: 10 If you need to wait for a condition or a resource to become available without complex retry logic, the ansible.builtin.wait_for or ansible.builtin.wait_for_connection modules are often more appropriate alternatives [5][6].
Citations:
- 1: https://docs.ansible.com/projects/ansible/latest/playbook_guide/playbooks_loops.html
- 2: "retries" without "until": retry until success ansible/ansible#20802
- 3: https://oneuptime.com/blog/post/2026-02-21-how-to-retry-failed-ansible-playbooks/view
- 4: https://oneuptime.com/blog/post/2026-02-21-how-to-use-ansible-retries-and-delay-with-until-loop/view
- 5: https://docs.ansible.com/projects/ansible/latest/collections/ansible/builtin/wait_for_module.html
- 6: https://oneuptime.com/blog/post/2026-02-21-how-to-handle-ansible-connection-timeouts/view
Make the “with retry” task retry by adding an until condition.
In projects/llm-d/toolbox/llmd_run_guidellm_benchmark/tasks/main.yml (lines 138-143), the task sets retries/delay but omits until, so Ansible runs it only once and ignores the retry settings.
Suggested patch
- name: Copy benchmarks.json from PVC to artifacts directory (with retry)
shell: |
oc exec {{ llmd_run_guidellm_benchmark_name }}-copy -n "{{ target_namespace }}" -- cat /results/benchmarks.json > "{{ artifact_extra_logs_dir }}/artifacts/results/benchmarks.json"
+ register: copy_result
retries: 10
delay: 1
+ until: copy_result.rc == 0
when: file_check_result.rc == 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/llm-d/toolbox/llmd_run_guidellm_benchmark/tasks/main.yml` around
lines 138 - 143, The task "Copy benchmarks.json from PVC to artifacts directory
(with retry)" currently sets retries/delay but uses "when: file_check_result.rc
== 0" so it never retries; change the task to register the copy command result
(e.g., add "register: copy_result"), remove the "when: file_check_result.rc ==
0" guard, and add "until: copy_result.rc == 0" so Ansible will actually retry
the oc exec shell command (the shell line that runs oc exec ... > "{{
artifact_extra_logs_dir }}/artifacts/results/benchmarks.json") using the
existing retries/delay settings.
| baseline_settings = {"model": "llama3.3-70b", "load_shape": load_shape, "flavor": flavor} | ||
| # If only one platform, show all simple flavors together | ||
| if "platform" not in variables: | ||
| header.append(html.H4(f"🔧 Simple Flavor Comparison")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'html\.H4\(f"🔧 Simple Flavor Comparison"\)' projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.pyRepository: openshift-psap/topsail
Length of output: 141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py"
# Show the exact surrounding context for the reported line
sed -n '330,370p' "$FILE" | nl -ba -w3 -s':'
# Confirm the exact match count for the specific string
rg -n 'html\.H4\(f"🔧 Simple Flavor Comparison"\)' "$FILE"
# Broader scan: any other html.H4 f-strings without placeholders
rg -n 'html\.H4\(f"' "$FILE" || trueRepository: openshift-psap/topsail
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py"
# Show surrounding context with line numbers
awk -v start=330 -v end=370 'NR>=start && NR<=end {printf "%d:%s\n", NR, $0}' "$FILE"
# Confirm the exact match count for the specific string
rg -n 'html\.H4\(f"🔧 Simple Flavor Comparison"\)' "$FILE"
# Broader scan: any other html.H4 f-strings
rg -n 'html\.H4\(f"' "$FILE" || trueRepository: openshift-psap/topsail
Length of output: 2525
Remove unnecessary f-string prefix (Ruff F541)
projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py line 351 uses html.H4(f"🔧 Simple Flavor Comparison") with no {} placeholders—remove the f.
Suggested patch
- header.append(html.H4(f"🔧 Simple Flavor Comparison"))
+ header.append(html.H4("🔧 Simple Flavor Comparison"))📝 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.
| header.append(html.H4(f"🔧 Simple Flavor Comparison")) | |
| header.append(html.H4("🔧 Simple Flavor Comparison")) |
🧰 Tools
🪛 Ruff (0.15.14)
[error] 351-351: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py`
at line 351, The code uses an unnecessary f-string in the call to html.H4
(html.H4(f"🔧 Simple Flavor Comparison")) which triggers Ruff F541; remove the f
prefix so the literal string is passed directly (change html.H4(f"...") to
html.H4("...")) to eliminate the lint warning and avoid creating an unneeded
formatted string.
1 participant
Summary by CodeRabbit
New Features
Chores