[inference] Improve kserve, guidellm and rhoai - #151
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR adds catalog-source pod polling, per-loadshape performance reports, streamed and trimmed benchmark extraction, Kubernetes-compliant inference-service names, workload diagnostics, binary shell output, and updated readiness and vault error handling. ChangesGuidellm benchmark and reporting
Inference-service naming and diagnostics
Custom catalog readiness
RHOAI readiness and deployment messages
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant run
participant CopyPod
participant shell.run
participant LocalFilesystem
run->>CopyPod: request gzip-compressed benchmark data
CopyPod-->>shell.run: return binary stream
shell.run-->>LocalFilesystem: write binary data
LocalFilesystem->>LocalFilesystem: decompress and trim JSON
LocalFilesystem-->>run: write processed result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🧹 Nitpick comments (3)
projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py (2)
932-944: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
model_infoextraction.Identical to Lines 710-721. Extract a small helper (e.g.
_extract_model_info(record)) or carry the value inall_plots_data.🤖 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/guidellm/postprocess/guidellm/plotting/performance_analysis.py` around lines 932 - 944, Deduplicate the model extraction logic currently repeated here and around the earlier model_info assignment by introducing a shared helper such as _extract_model_info(record), then use it in both locations. Preserve the existing lookup order across distinguishing_labels, run_identity, and the "Unknown" fallback.
775-783: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the orphaned helper functions.
performance_analysis.pydefines_generate_performance_summaryand_create_comprehensive_html_report_with_images, but they have no repository-wide callers, so drop them unless they are intended to be reusable entry points.🤖 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/guidellm/postprocess/guidellm/plotting/performance_analysis.py` around lines 775 - 783, Remove the unused helper functions _generate_performance_summary and _create_comprehensive_html_report_with_images from performance_analysis.py, along with any imports or supporting code used exclusively by them; preserve the active plotting flow and report-generation behavior elsewhere.projects/guidellm/toolbox/run_guidellm_benchmark/main.py (1)
414-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused
_copy_result_filehelper and its nestedgzipimport.
_copy_result_filehas no callers and repeats the result extraction flow already inlined inextract_results, leaving duplicated logic behind. Remove the helper and the redundantimport gzipif the helper is gone; otherwise, call it fromextract_results.🤖 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/guidellm/toolbox/run_guidellm_benchmark/main.py` around lines 414 - 436, Remove the unused _copy_result_file helper entirely, including its nested gzip import, since extract_results already contains the result extraction flow. Do not replace it with a call from extract_results.
🤖 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/cluster/toolbox/deploy_custom_catalog/main.py`:
- Around line 99-105: Update the pod-status handling in the deployment
status-check function to evaluate failed pods before accepting a running status,
so mixed output cannot report success when any pod has failed. At minimum, move
the `"Failed"` check ahead of the `"Running"` check while preserving the
existing error and success responses.
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 707-708: Sanitize each arbitrary loadshape label once into a
loadshape_slug, then store it alongside loadshape in all_plots_data and reuse
the slug for loadshape_dir, generated filenames, HTML href values, and anchor id
attributes. Keep the raw loadshape only as escaped display text via
html.escape(), updating the related uses around the plotting and HTML-generation
logic.
In `@projects/guidellm/toolbox/run_guidellm_benchmark/main.py`:
- Around line 348-361: Remove the unsupported text=False argument from both
gzip-stream oc() calls in
projects/guidellm/toolbox/run_guidellm_benchmark/main.py at lines 348-361 and
445-458, and ensure stdout_dest preserves the compressed payload by writing in
binary mode through oc()/shell.run. If the helper at lines 445-458 has no
callers, remove it instead.
- Around line 28-44: Update the trim_benchmark_json docstring to accurately
describe that the recursive transformation removes only the “requests” field, or
adjust the filter to remove every field currently named in the docstring; keep
the implementation and documentation consistent.
In `@projects/kserve/toolbox/capture_llmisvc_state/main.py`:
- Around line 253-282: Update the deployment, replicaset, and pod capture flow
around the `deploy_result`, `rs_result`, and `pods_result` commands so raw `oc
describe` stdout is never written to `descriptions_file` in `ARTIFACT_DIR`.
Before each `handle.write` call, emit only an allowlisted sanitized projection
or redact environment values and command arguments while preserving
non-sensitive diagnostic details.
In `@projects/kserve/toolbox/deploy_llmisvc/main.py`:
- Around line 264-275: Update the service readiness check around llmisvc_result
in the deployment flow so an existing LLMInferenceService is not considered
successful when no pods are scheduled. After validating the command return code,
inspect the later plain-text pod query result and reject output containing “No
resources found” before returning success, while preserving the existing failure
handling for a nonzero return code.
In `@projects/rhoai/toolbox/wait_datasciencecluster_ready/main.py`:
- Around line 82-97: Replace substring checks in the DataScienceCluster
readiness logic with structured status parsing from the cluster YAML or explicit
status fields/columns, including ready, error, failed, and retry states. Update
the surrounding wait flow to preserve the existing return and exception
outcomes, and add fixtures covering each terminal and retry state.
---
Nitpick comments:
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 932-944: Deduplicate the model extraction logic currently repeated
here and around the earlier model_info assignment by introducing a shared helper
such as _extract_model_info(record), then use it in both locations. Preserve the
existing lookup order across distinguishing_labels, run_identity, and the
"Unknown" fallback.
- Around line 775-783: Remove the unused helper functions
_generate_performance_summary and _create_comprehensive_html_report_with_images
from performance_analysis.py, along with any imports or supporting code used
exclusively by them; preserve the active plotting flow and report-generation
behavior elsewhere.
In `@projects/guidellm/toolbox/run_guidellm_benchmark/main.py`:
- Around line 414-436: Remove the unused _copy_result_file helper entirely,
including its nested gzip import, since extract_results already contains the
result extraction flow. Do not replace it with a call from extract_results.
🪄 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 Plus
Run ID: 6f6f6295-343a-4b4c-ae9d-3df6e9ff0b7d
📒 Files selected for processing (10)
projects/cluster/toolbox/deploy_custom_catalog/main.pyprojects/guidellm/postprocess/guidellm/plotting/performance_analysis.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/kserve/toolbox/capture_llmisvc_state/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yamlprojects/rhoai/library/deploy.pyprojects/rhoai/toolbox/wait_datasciencecluster_ready/main.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
projects/core/dsl/shell.py (1)
64-70: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBinary output is buffered in memory, not streamed.
capture_output=Trueholds the complete child output in the parent process. When a caller suppliesstdout_destwithtext=False, the payload is buffered fully and then written to the file.extract_resultsinprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyuses this path for the compressedbenchmarks.json, which can be large.If
stdout_destis set, redirect the child stdout to the open file so the data never accumulates in memory.♻️ Sketch of a streaming path for `stdout_dest`
- result = subprocess.run( - command_for_subprocess, - shell=shell, - check=False, # We handle check ourselves - capture_output=True, - text=text, - ) + if stdout_dest is not None and not text: + stdout_path = Path(stdout_dest) + stdout_path.parent.mkdir(parents=True, exist_ok=True) + with open(stdout_path, "wb") as stdout_file: + completed = subprocess.run( + command_for_subprocess, + shell=shell, + check=False, + stdout=stdout_file, + stderr=subprocess.PIPE, + ) + written_bytes = stdout_path.stat().st_size + else: + completed = subprocess.run( + command_for_subprocess, + shell=shell, + check=False, + capture_output=True, + text=text, + )The remaining branches then report
written_bytesinstead oflen(result.stdout), and the file write at lines 97-98 is skipped for the streamed case.🤖 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/core/dsl/shell.py` around lines 64 - 70, Update the subprocess execution flow around `subprocess.run` to stream child stdout directly to the open `stdout_dest` file when that destination is provided, avoiding in-memory buffering. Track and report the streamed `written_bytes` value in the result-handling branches, and skip the existing post-run write that uses `len(result.stdout)` for this case; preserve current capture behavior when `stdout_dest` is absent.projects/guidellm/toolbox/run_guidellm_benchmark/main.py (1)
414-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused
_copy_result_filehelper.
_copy_result_filehas no callers and duplicates the gzip/stream/trim/retention flow already present inextract_results. Keep the result logic inextract_resultsto avoid drift.🤖 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/guidellm/toolbox/run_guidellm_benchmark/main.py` around lines 414 - 499, Remove the unused _copy_result_file helper entirely, including its documentation and implementation. Keep the existing gzip, streaming, trimming, and full-file retention behavior in extract_results, which is the active implementation.projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py (1)
710-721: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the model-information lookup into a helper.
The same lookup chain is repeated at Lines 932-944. Two copies can diverge when a new label key is added. Compute
model_infoonce per loadshape and reuse it in the HTML section, or move the chain into a small helper.♻️ Proposed refactor
+def _extract_model_info(records) -> str: + if not records: + return "Unknown" + first_record = records[0] + return ( + first_record.distinguishing_labels.get("model") + or first_record.distinguishing_labels.get("model_name") + or first_record.distinguishing_labels.get("llm_model") + or first_record.run_identity.get("model") + or "Unknown" + )- # Extract model information for this loadshape - model_info = "Unknown" - if loadshape_records: - # Try to get model information from distinguishing labels - first_record = loadshape_records[0] - model_info = ( - first_record.distinguishing_labels.get("model") - or first_record.distinguishing_labels.get("model_name") - or first_record.distinguishing_labels.get("llm_model") - or first_record.run_identity.get("model") - or "Unknown" - ) + model_info = _extract_model_info(loadshape_records)Store
model_infoinall_plots_dataalongside the plots so the HTML section does not repeat the lookup.🤖 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/guidellm/postprocess/guidellm/plotting/performance_analysis.py` around lines 710 - 721, Store the computed model_info value (extracted from first_record.distinguishing_labels with fallback keys "model", "model_name", "llm_model", then run_identity.get("model")) inside the all_plots_data structure for each loadshape. Then retrieve model_info from all_plots_data at the second location (around lines 932-944) instead of repeating the same lookup chain, ensuring a single source of truth for the model information extraction logic and preventing divergence if new label keys are added in the future.
🤖 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/core/dsl/shell.py`:
- Around line 72-75: Update the binary-mode logging branches in the shell
execution flow to honor the caller’s log_stdout and log_stderr flags instead of
forcing them off. Guard both byte-count and decoded-stderr logging with their
respective flags, remove the unreachable decode exception handling around
bytes.decode(errors="replace"), and ensure suppressed output is never logged.
- Around line 78-91: Update the failure-handling path in the shell execution
flow to use the already computed stdout_str and stderr_str values when
constructing the exception message, including for text=False and check=True.
Preserve the binary-safe formatting established in the CommandResult
construction and avoid formatting result.stdout or result.stderr directly.
---
Nitpick comments:
In `@projects/core/dsl/shell.py`:
- Around line 64-70: Update the subprocess execution flow around
`subprocess.run` to stream child stdout directly to the open `stdout_dest` file
when that destination is provided, avoiding in-memory buffering. Track and
report the streamed `written_bytes` value in the result-handling branches, and
skip the existing post-run write that uses `len(result.stdout)` for this case;
preserve current capture behavior when `stdout_dest` is absent.
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 710-721: Store the computed model_info value (extracted from
first_record.distinguishing_labels with fallback keys "model", "model_name",
"llm_model", then run_identity.get("model")) inside the all_plots_data structure
for each loadshape. Then retrieve model_info from all_plots_data at the second
location (around lines 932-944) instead of repeating the same lookup chain,
ensuring a single source of truth for the model information extraction logic and
preventing divergence if new label keys are added in the future.
In `@projects/guidellm/toolbox/run_guidellm_benchmark/main.py`:
- Around line 414-499: Remove the unused _copy_result_file helper entirely,
including its documentation and implementation. Keep the existing gzip,
streaming, trimming, and full-file retention behavior in extract_results, which
is the active implementation.
🪄 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 Plus
Run ID: 4df8014e-9ca8-4769-8c41-fb7cae926e76
📒 Files selected for processing (11)
projects/cluster/toolbox/deploy_custom_catalog/main.pyprojects/core/dsl/shell.pyprojects/core/dsl/utils/k8s.pyprojects/guidellm/postprocess/guidellm/plotting/performance_analysis.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yamlprojects/rhoai/library/deploy.pyprojects/rhoai/toolbox/wait_datasciencecluster_ready/main.py
🚧 Files skipped from review as they are similar to previous changes (7)
- projects/llm_d/orchestration/render_inference_service.py
- projects/rhoai/library/deploy.py
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yaml
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yaml
- projects/rhoai/toolbox/wait_datasciencecluster_ready/main.py
- projects/cluster/toolbox/deploy_custom_catalog/main.py
- projects/kserve/toolbox/deploy_llmisvc/main.py
…chmarks.json file
|
/test fournos llm_d |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d |
🔴 Execution of
|
🔴 Submission of
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
projects/guidellm/toolbox/run_guidellm_benchmark/main.py (1)
460-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the module
loggerinstead oflogging.getLogger(__name__).The rest of the file logs through
logger. These two calls create a new logger inline, which breaks the consistent logging path.♻️ Proposed change
- logging.getLogger(__name__).warning("gzip extraction failed (rc=%d)", result.returncode) + logger.warning("gzip extraction failed (rc=%d)", result.returncode)- logging.getLogger(__name__).warning("Failed to extract compressed results: %s", e) + logger.warning("Failed to extract compressed results: %s", e)Also applies to: 468-468
🤖 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/guidellm/toolbox/run_guidellm_benchmark/main.py` at line 460, Replace the inline logging.getLogger(__name__) calls at both gzip extraction failure sites with the module-level logger, preserving the existing warning messages and arguments.projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py (2)
710-721: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the model-information lookup into a helper.
The same five-way fallback chain appears at lines 710-721 and again at lines 932-944. Extract one helper and call it in both places.
♻️ Proposed refactor
+def _extract_model_info(records) -> str: + if not records: + return "Unknown" + first_record = records[0] + return ( + first_record.distinguishing_labels.get("model") + or first_record.distinguishing_labels.get("model_name") + or first_record.distinguishing_labels.get("llm_model") + or first_record.run_identity.get("model") + or "Unknown" + )- model_info = "Unknown" - if loadshape_records: - # Try to get model information from distinguishing labels - first_record = loadshape_records[0] - model_info = ( - first_record.distinguishing_labels.get("model") - or first_record.distinguishing_labels.get("model_name") - or first_record.distinguishing_labels.get("llm_model") - or first_record.run_identity.get("model") - or "Unknown" - ) + model_info = _extract_model_info(loadshape_records)🤖 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/guidellm/postprocess/guidellm/plotting/performance_analysis.py` around lines 710 - 721, Extract the repeated model-information fallback chain into a shared helper near the relevant plotting utilities, preserving the existing precedence across distinguishing_labels keys and run_identity.get("model"), with "Unknown" as the final fallback. Replace the inline logic in both loadshape-processing locations around the current first-record handling and the corresponding later block with calls to that helper.
781-784: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused report helpers.
_generate_performance_summaryand_ create_comprehensive_html_report_with_imagesare only defined inprojects/guidellm/postprocess/guidellm/plotting/performance_analysis.pyand not called by the reworked inline report assembly. Remove their definitions at lines 1030-1083 and 1086-1202 to avoid dead code.🤖 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/guidellm/postprocess/guidellm/plotting/performance_analysis.py` around lines 781 - 784, Remove the unused helper definitions _generate_performance_summary and _create_comprehensive_html_report_with_images from performance_analysis.py, leaving the inline comprehensive HTML report assembly 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/core/dsl/shell.py`:
- Around line 73-76: Update the subprocess.run invocation in the shell execution
function to handle non-None input_text when text=False: encode the string to
bytes before passing it to subprocess, or reject the combination with a clear
validation error; preserve string input behavior when text=True.
In `@projects/kserve/toolbox/capture_llmisvc_state/main.py`:
- Around line 238-242: Update both oc command invocations in the workload
overview and related state-capture flows to pass arguments as a list and
explicitly use shell=False. Keep args.llmisvc_name and context.target_namespace
as separate arguments rather than interpolating them into a shell command
string.
- Around line 251-253: Update capture_workload_descriptions to create the parent
directories for descriptions_file before calling open, ensuring both artifacts
and its required logs directory exist even when absent. Preserve the existing
workload_descriptions.txt writing behavior after directory creation.
---
Nitpick comments:
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 710-721: Extract the repeated model-information fallback chain
into a shared helper near the relevant plotting utilities, preserving the
existing precedence across distinguishing_labels keys and
run_identity.get("model"), with "Unknown" as the final fallback. Replace the
inline logic in both loadshape-processing locations around the current
first-record handling and the corresponding later block with calls to that
helper.
- Around line 781-784: Remove the unused helper definitions
_generate_performance_summary and _create_comprehensive_html_report_with_images
from performance_analysis.py, leaving the inline comprehensive HTML report
assembly unchanged.
In `@projects/guidellm/toolbox/run_guidellm_benchmark/main.py`:
- Line 460: Replace the inline logging.getLogger(__name__) calls at both gzip
extraction failure sites with the module-level logger, preserving the existing
warning messages and arguments.
🪄 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 Plus
Run ID: a9e05885-c64a-4689-9230-94ba112ffdc6
📒 Files selected for processing (12)
projects/cluster/toolbox/deploy_custom_catalog/main.pyprojects/core/dsl/shell.pyprojects/core/dsl/utils/k8s.pyprojects/guidellm/postprocess/guidellm/plotting/performance_analysis.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/kserve/toolbox/capture_llmisvc_state/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yamlprojects/rhoai/library/deploy.pyprojects/rhoai/toolbox/wait_datasciencecluster_ready/main.py
🚧 Files skipped from review as they are similar to previous changes (7)
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yaml
- projects/cluster/toolbox/deploy_custom_catalog/main.py
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yaml
- projects/core/dsl/utils/k8s.py
- projects/llm_d/orchestration/render_inference_service.py
- projects/rhoai/library/deploy.py
- projects/kserve/toolbox/deploy_llmisvc/main.py
| input=input_text, | ||
| timeout=timeout_seconds, | ||
| text=text, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Locate shell.py\n'
fd -a 'shell.py' . | sed 's#^\./##'
printf '\nRelevant shell.py section\n'
wc -l projects/core/dsl/shell.py
sed -n '1,110p' projects/core/dsl/shell.py
printf '\nSubprocess behavior probe\n'
python3 - <<'PY'
import subprocess
for input_value in [None, "hello", b"hello"]:
for text in [True, False]:
try:
r = subprocess.run(["cat"], input=input_value, text=text, capture_output=True, check=True)
print(f"input={input_value!r} text={text}: ok stdout={r.stdout!r}")
except Exception as e:
print(f"input={input_value!r} text={text}: {type(e).__name__}: {e}")
PYRepository: openshift-psap/forge
Length of output: 4202
Guard input_text when binary mode is requested.
subprocess.run(input=input_text, text=text) accepts string input only when text=True. Passing a non-None str with text=False raises TypeError, so encode input_text to bytes before subprocess or reject that combination with a clear error.
🤖 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/core/dsl/shell.py` around lines 73 - 76, Update the subprocess.run
invocation in the shell execution function to handle non-None input_text when
text=False: encode the string to bytes before passing it to subprocess, or
reject the combination with a clear validation error; preserve string input
behavior when text=True.
| shell.run( | ||
| f'oc get deploy,rs,pod -l "app.kubernetes.io/name={args.llmisvc_name}" -n {context.target_namespace} -o wide', | ||
| stdout_dest=workload_overview_path, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether shell.run supports list-form commands and what shell mode it defaults to.
fd -t f 'shell.py' projects/core/dsl --exec rg -n -C6 'def run' {}Repository: openshift-psap/forge
Length of output: 469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate shell.py and inspect implementation"
fd -t f 'shell.py' projects/core/dsl -x sh -c 'echo "===== $1 ====="; cat -n "$1"' sh {}
echo
echo "Find call sites of shell.run in capture_llmisvc_state/main.py"
if [ -f projects/kserve/toolbox/capture_llmisvc_state/main.py ]; then
sed -n '210,275p' projects/kserve/toolbox/capture_llmisvc_state/main.py | cat -n
fi
echo
echo "Check for shell.run signature/docs in repository"
rg -n "shell\.run|from .*shell import run|def run\(" projects/core projects -g '*.py' | head -n 80Repository: openshift-psap/forge
Length of output: 17729
Pass the oc command as an argument list with shell=False.
shell.run defaults to shell=True, so the formatted values are interpolated by the shell before oc receives them; a value containing shell metacharacters changes the executed command. Use the list form, or shell.run(..., shell=False), including the args.llmisvc_name and context.target_namespace usages at lines 238-242 and 256-260.
🤖 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/kserve/toolbox/capture_llmisvc_state/main.py` around lines 238 -
242, Update both oc command invocations in the workload overview and related
state-capture flows to pass arguments as a list and explicitly use shell=False.
Keep args.llmisvc_name and context.target_namespace as separate arguments rather
than interpolating them into a shell command string.
| descriptions_file = args.artifact_dir / "artifacts/workload_descriptions.txt" | ||
|
|
||
| with open(descriptions_file, "w") as handle: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the artifacts directory is created before these tasks run.
fd -t f 'main.py' projects/kserve/toolbox/capture_llmisvc_state --exec rg -n 'mkdir|artifacts/' {}
# Inspect how shell.run handles stdout_dest parent directories.
fd -t f 'shell.py' projects/core/dsl --exec rg -n -C4 'stdout_dest' {}Repository: openshift-psap/forge
Length of output: 3708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
main='projects/kserve/toolbox/capture_llmisvc_state/main.py'
echo "== main outline =="
ast-grep outline "$main" --view compact || true
echo "== main task definitions around directory creation and workload description task =="
wc -l "$main"
sed -n '1,280p' "$main" | nl -ba
echo "== definitions/usages of get_task_by_name or task runs =="
rg -n 'def get_task_by_name|get_task_by_name|select_tasks|Task|run\(' "$main"Repository: openshift-psap/forge
Length of output: 511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
main='projects/kserve/toolbox/capture_llmisvc_state/main.py'
echo "== main outline =="
ast-grep outline "$main" --view names || true
echo "== task definitions and directory handling =="
python3 - <<'PY'
from pathlib import Path
p = Path("projects/kserve/toolbox/capture_llmisvc_state/main.py")
for i, line in enumerate(p.read_text().splitlines(), 1):
if "shell.mkdir" in line or "workload_descriptions" in line or "workload_overview" in line or "get_task" in line or "def main" in line or "def process" in line:
print(f"{i}: {line}")
PY
echo "== relevant source slice =="
sed -n '1,270p' "$main"Repository: openshift-psap/forge
Length of output: 10374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== execute_tasks definitions and task ordering in DSL =="
rg -n -C 6 'def execute_tasks|execute_tasks|def get_task_by_name|get_task_by_name|sorted|dependencies|depends_on|shell\.mkdir|artifacts' projects/core.dsl projects/core/dsl . --glob '!*.pyc' || true
echo "== locate module files =="
git ls-files | rg '^(projects/core|projects/core\.dsl).*\.py$'Repository: openshift-psap/forge
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate DSL files =="
git ls-files projects | rg '(^projects/core/dsl.*\.py$|(^projects/core/dsl/__init__\.py$|(^projects/core/dsl/__init__\.pyi$|(^projects/core/dsl/__init__\.pyd$|(^projects/core/dsl/__init__\.pyo$|(^projects/core/dsl/*.py$)' || true
printf '\n== files in projects/core/dsl if present ==\n'
find projects/core/dsl -maxdepth 1 -type f -print 2>/dev/null | sort || true
printf '\n== execute_tasks references in DSL package and related entrypoint package ==\n'
rg -n -C 5 'def execute_tasks|execute_tasks\(|get_task_by_name|should_run|skip|retry|fail|always|dependencies' projects/core/dsl projects/core 2>/dev/null | head -n 220Repository: openshift-psap/forge
Length of output: 14553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run.py key sections =="
wc -l projects/core/dsl/runtime.py projects/core/dsl/task.py projects/core/dsl/control_flow.py
sed -n '1,260p' projects/core/dsl/runtime.py
printf '\n--- task.py ---\n'
sed -n '1,220p' projects/core/dsl/task.py
printf '\n--- control_flow.py ---\n'
sed -n '1,140p' projects/core/dsl/control_flow.pyRepository: openshift-psap/forge
Length of output: 18651
Create the metadata directories before writing workload_descriptions.txt.
capture_workload_descriptions writes with plain open(..., "w"), which fails if artifacts/logs has been removed, skipped, or otherwise is absent. Create both needed parent directories before opening the output file.
🛠️ Proposed fix
descriptions_file = args.artifact_dir / "artifacts/workload_descriptions.txt"
+ descriptions_file.parent.mkdir(parents=True, exist_ok=True)
+ logs_dir = args.artifact_dir / "artifacts/logs"
+ logs_dir.mkdir(parents=True, exist_ok=True)📝 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.
| descriptions_file = args.artifact_dir / "artifacts/workload_descriptions.txt" | |
| with open(descriptions_file, "w") as handle: | |
| descriptions_file = args.artifact_dir / "artifacts/workload_descriptions.txt" | |
| descriptions_file.parent.mkdir(parents=True, exist_ok=True) | |
| logs_dir = args.artifact_dir / "artifacts/logs" | |
| logs_dir.mkdir(parents=True, exist_ok=True) | |
| with open(descriptions_file, "w") as handle: |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 252-252: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(descriptions_file, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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/kserve/toolbox/capture_llmisvc_state/main.py` around lines 251 -
253, Update capture_workload_descriptions to create the parent directories for
descriptions_file before calling open, ensuring both artifacts and its required
logs directory exist even when absent. Preserve the existing
workload_descriptions.txt writing behavior after directory creation.
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
|
@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. |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes