[inference] Various helpers - #138
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR updates GuideLLM resource ownership, KServe deployment readiness and diagnostics, KServe state artifact layout, model-cache pod waiting, smoke-pod handling, and a reference LLMInferenceService name. ChangesGuideLLM and smoke workflow updates
KServe deployment readiness and diagnostics
KServe state artifact capture
Hugging Face cache readiness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant deploy_llmisvc
participant OpenShift
participant LLMInferenceService
deploy_llmisvc->>OpenShift: wait for pods and query pod status
deploy_llmisvc->>LLMInferenceService: query Ready condition
LLMInferenceService-->>deploy_llmisvc: readiness status and message
deploy_llmisvc->>OpenShift: resolve endpoint and capture diagnostics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
2a589b5 to
122950c
Compare
![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 (8)
projects/kserve/toolbox/deploy_llmisvc/main.py (5)
41-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
Nonesemantics ofgateway_status_address_name.Passing
Noneswitches endpoint resolution to "first address with a URL" and appends:8000when no port is present — that behavior is non-obvious from the current docstring.📝 Suggested docstring tweak
- gateway_status_address_name: Gateway status address name for endpoint resolution + gateway_status_address_name: Gateway status address name for endpoint resolution. + If None, the first status address exposing a URL is used and port 8000 is + appended when the URL has no explicit port.🤖 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/deploy_llmisvc/main.py` around lines 41 - 53, Update the deploy function’s docstring for gateway_status_address_name to document that passing None selects the first gateway status address containing a URL and appends :8000 when that URL has no port. Preserve the existing description for named gateway status addresses.
324-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
RuntimeErrorout of thetryguardingint().It works today only because
exceptcatchesValueErroronly; a future broadening of that except would swallow the abort. Parse first, then raise.♻️ Proposed refactor
for count_str in counts: - try: - if int(count_str) > 0: - raise RuntimeError( - f"Pod {pod_name} has restarted (restart count: {count_str}). Aborting wait due to pod restart." - ) - except ValueError: - # Skip non-numeric restart counts - pass + try: + restarts = int(count_str) + except ValueError: + continue # Skip non-numeric restart counts + if restarts > 0: + raise RuntimeError( + f"Pod {pod_name} has restarted (restart count: {count_str}). " + "Aborting wait due to pod restart." + )🤖 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/deploy_llmisvc/main.py` around lines 324 - 338, In the restart-count handling loop, update the logic around int(count_str) so conversion remains inside the try/except, but the RuntimeError in the positive-count condition is raised after that block. Preserve skipping non-numeric values while ensuring any detected restart always propagates the abort.
766-771: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant emptiness check.
pod_namesis derived frompod_result.stdout.strip().split(), sonot pod_namesalready coversnot pod_result.stdout.strip().♻️ Proposed tidy-up
- pod_names = pod_result.stdout.strip().split() - if not pod_names or not pod_result.stdout.strip(): + pod_names = pod_result.stdout.split() + if not pod_names:🤖 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/deploy_llmisvc/main.py` around lines 766 - 771, In the pod result handling block, simplify the condition to check only whether pod_names is empty, since it is derived from stripped and split stdout. Preserve the existing pod description file creation and return behavior.
366-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider trimming the resolution logging.
~40
logger.infocalls (with emoji status markers) for a single lookup will dominate the job log on every retry attempt. Dropping the per-branch narration todebugand keeping one summary line atinfowould keep the diagnostics without the noise.🤖 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/deploy_llmisvc/main.py` around lines 366 - 424, The endpoint resolution flow around status.address and status.addresses is excessively verbose at info level. Demote detailed branch, payload, field, and status-marker messages to debug, while retaining a single concise summary at info for the final resolution outcome, including successful URL resolution or failure.
591-608: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated capture preamble.
dry_runguard +artifacts_dircreation +getattr(ctx, "selector"/"inference_service_name", None)check is copy-pasted across five tasks. A small helper (or decorator) returning(artifacts_dir, selector)would remove ~40 duplicated lines and keep the guards consistent.Also applies to: 692-708, 734-750, 800-816
🤖 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/deploy_llmisvc/main.py` around lines 591 - 608, The repeated capture preamble should be centralized instead of duplicated across the capture tasks. Add a helper or decorator that handles the dry_run guard, creates args.artifact_dir / "artifacts", retrieves the relevant context value via getattr, and returns the artifacts directory with the selector or inference_service_name; update capture_final_llmisvc_yaml and the other referenced capture tasks to reuse it while preserving their existing no-value behavior.projects/kserve/toolbox/deploy_llmisvc/__init__.py (1)
1-5: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDon’t import
deploy_llmisvc.mainfrom the package__init__.Importing the package just to expose
try_resolve_endpoint_urlalso importsmain, which registers all@taskfunctions into the global script manager at module load time. Move the package import site torun_toolbox_command()/@entrypointor import/exportdeploy_llmisvc.main.runin another way that avoids importingmainduring package initialization.🤖 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/deploy_llmisvc/__init__.py` around lines 1 - 5, Remove the top-level deploy_llmisvc.main import and try_resolve_endpoint_url export from the package __init__.py so package initialization does not register `@task` functions. Update run_toolbox_command() or the `@entrypoint` flow to import deploy_llmisvc.main only when execution requires it, while preserving the existing command behavior.projects/kserve/toolbox/prepare_hf_model_cache/main.py (1)
289-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent retry-continuation return shape.
Line 317 returns bare
Falsewhen no pods are found yet, while line 321 and the siblingwait_for_downloadtask (Line 373) return a(False, message)tuple for the "keep retrying" case. This means the "no pods yet" retry iteration loses its diagnostic message, unlike every other retry-continuation branch in this file.♻️ Suggested fix for consistency
if not result.stdout.strip(): - return False # No pods yet, retry + return False, "No pods found yet, retrying..."Please confirm the
@task/@retryruntime treats a bareFalsethe same as a(False, message)tuple for retry purposes (only the message/logging differs), since that contract isn't fully visible in the provided context.🤖 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/prepare_hf_model_cache/main.py` around lines 289 - 325, Update wait_for_pods_running so the no-pods-yet branch returns a (False, message) tuple, matching its Pending branch and the wait_for_download retry behavior. Preserve the existing retry semantics and provide a diagnostic message indicating that no pods have appeared yet.projects/kserve/toolbox/capture_llmisvc_state/main.py (1)
165-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared per-pod log capture helper; fix success count.
capture_pod_logsandcapture_pod_previous_logs(Lines 193-217) duplicate the same pod-discovery + per-pod loop, differing only by the--previousflag and file suffix. Also,captured_countis incremented unconditionally even whenshell.runfails (check=False), so the returned message can overstate how many logs were actually captured.♻️ Proposed shared helper
+def _capture_pod_logs_impl(args, context, *, previous: bool, suffix: str, label: str): + result = shell.run( + f'oc get pods -l "app.kubernetes.io/name={args.llmisvc_name}" -n {context.target_namespace} -o jsonpath="{{.items[*].metadata.name}}"', + check=False, + log_stdout=False, + ) + pod_names = result.stdout.strip().split() + if not pod_names: + return f"No pods found to capture {label}" + + logs_dir = args.artifact_dir / "artifacts/logs" + captured_count = 0 + previous_flag = "--previous " if previous else "" + for pod_name in pod_names: + log_file = logs_dir / f"{pod_name}{suffix}" + log_result = shell.run( + f"oc logs {pod_name} -n {context.target_namespace} {previous_flag}--all-containers=true", + stdout_dest=log_file, + check=False, + ) + if log_result.returncode == 0: + captured_count += 1 + + return f"Pod {label} captured for {captured_count} pods in dedicated files" + + def capture_pod_logs(args, context): """Capture logs from LLMInferenceService pods""" - result = shell.run( - f'oc get pods -l "app.kubernetes.io/name={args.llmisvc_name}" -n {context.target_namespace} -o jsonpath="{{.items[*].metadata.name}}"', - check=False, - log_stdout=False, - ) - - pod_names = result.stdout.strip().split() - if not pod_names or not result.stdout.strip(): - return "No pods found to capture logs" - - logs_dir = args.artifact_dir / "artifacts/logs" - captured_count = 0 - - for pod_name in pod_names: - log_file = logs_dir / f"{pod_name}.log" - shell.run( - f"oc logs {pod_name} -n {context.target_namespace} --all-containers=true", - stdout_dest=log_file, - check=False, - ) - captured_count += 1 - - return f"Pod logs captured for {captured_count} pods in dedicated files" + return _capture_pod_logs_impl(args, context, previous=False, suffix=".log", label="logs")🤖 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 165 - 189, Extract the duplicated pod discovery and per-pod capture logic from capture_pod_logs and capture_pod_previous_logs into a shared helper parameterized by the previous-log flag and filename suffix. In that helper, increment the captured count only when the corresponding shell.run succeeds, while preserving the existing no-pods response and log destinations. Update both public functions to delegate to the helper and report the accurate count.
🤖 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/guidellm/toolbox/run_smoke_request/main.py`:
- Line 250: Update the Pod YAML capture around stdout_dest to avoid persisting
sensitive data: restrict or redact environment values and command arguments
before writing artifacts, and disable stdout logging by setting
log_stdout=False. Preserve the artifact generation while ensuring no raw Pod
spec or secret values are written or logged.
In `@projects/kserve/toolbox/deploy_llmisvc/main.py`:
- Around line 389-396: Replace the `url.split("/")[-1]` port heuristic in both
gateway status address call sites with a shared `_with_default_port` helper
using `urlsplit`/`urlunsplit`. Detect an existing `parts.port`, otherwise append
the default port to `netloc` so URLs with paths or trailing slashes remain
valid.
- Around line 656-689: Update capture_workload_overview to wrap its
artifact-directory creation and workload overview capture logic in the same
try/except pattern used by the sibling `@always` capture tasks. Catch failures
from mkdir or oc and return an error string instead of allowing the exception to
propagate, while preserving the existing dry-run and missing-selector returns.
- Around line 800-839: Update capture_pod_yaml and capture_pod_descriptions to
sanitize command output before persisting it under ARTIFACT_DIR. Redact literal
environment-variable values and all other sensitive data, including
secret-related fields, tokens, credentials, certificates, and pull-secret
content, while preserving useful pod debugging information. Ensure neither
pod_definitions.yaml nor the pod description artifact writes unsanitized oc
output.
- Around line 340-360: The readiness handling in both resource-check paths
cannot parse the Go map output from the current jsonpath query. Update the logic
at projects/kserve/toolbox/deploy_llmisvc/main.py:340-360 and :228-243 to fetch
the resource through oc_get_json and select the Ready condition from
status.conditions in Python, or query reason and message as separate scalar
jsonpath values; preserve the existing ready and not-ready return behavior at
both sites.
- Around line 138-153: Update the pod-check logic in the wait flow after the oc
call so an empty stdout is treated as “old pods gone” only when
result.returncode indicates success. For non-zero return codes, preserve the
retry path by returning False, even when stdout is empty.
- Around line 246-281: Update the wait_pods_scheduled retry configuration to use
a bounded attempt count consistent with the other waits in the file, such as 120
attempts with the existing 30-second delay. Replace whole-output substring
checks for "Pending" and "SchedulingGated" with per-line parsing of the pod
STATUS column, preserving the existing retry messages when any pod has either
status.
---
Nitpick comments:
In `@projects/kserve/toolbox/capture_llmisvc_state/main.py`:
- Around line 165-189: Extract the duplicated pod discovery and per-pod capture
logic from capture_pod_logs and capture_pod_previous_logs into a shared helper
parameterized by the previous-log flag and filename suffix. In that helper,
increment the captured count only when the corresponding shell.run succeeds,
while preserving the existing no-pods response and log destinations. Update both
public functions to delegate to the helper and report the accurate count.
In `@projects/kserve/toolbox/deploy_llmisvc/__init__.py`:
- Around line 1-5: Remove the top-level deploy_llmisvc.main import and
try_resolve_endpoint_url export from the package __init__.py so package
initialization does not register `@task` functions. Update run_toolbox_command()
or the `@entrypoint` flow to import deploy_llmisvc.main only when execution
requires it, while preserving the existing command behavior.
In `@projects/kserve/toolbox/deploy_llmisvc/main.py`:
- Around line 41-53: Update the deploy function’s docstring for
gateway_status_address_name to document that passing None selects the first
gateway status address containing a URL and appends :8000 when that URL has no
port. Preserve the existing description for named gateway status addresses.
- Around line 324-338: In the restart-count handling loop, update the logic
around int(count_str) so conversion remains inside the try/except, but the
RuntimeError in the positive-count condition is raised after that block.
Preserve skipping non-numeric values while ensuring any detected restart always
propagates the abort.
- Around line 766-771: In the pod result handling block, simplify the condition
to check only whether pod_names is empty, since it is derived from stripped and
split stdout. Preserve the existing pod description file creation and return
behavior.
- Around line 366-424: The endpoint resolution flow around status.address and
status.addresses is excessively verbose at info level. Demote detailed branch,
payload, field, and status-marker messages to debug, while retaining a single
concise summary at info for the final resolution outcome, including successful
URL resolution or failure.
- Around line 591-608: The repeated capture preamble should be centralized
instead of duplicated across the capture tasks. Add a helper or decorator that
handles the dry_run guard, creates args.artifact_dir / "artifacts", retrieves
the relevant context value via getattr, and returns the artifacts directory with
the selector or inference_service_name; update capture_final_llmisvc_yaml and
the other referenced capture tasks to reuse it while preserving their existing
no-value behavior.
In `@projects/kserve/toolbox/prepare_hf_model_cache/main.py`:
- Around line 289-325: Update wait_for_pods_running so the no-pods-yet branch
returns a (False, message) tuple, matching its Pending branch and the
wait_for_download retry behavior. Preserve the existing retry semantics and
provide a diagnostic message indicating that no pods have appeared yet.
🪄 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: 5b58d40e-8415-40c5-8e92-7a7619ed940b
📒 Files selected for processing (8)
projects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/guidellm/toolbox/run_guidellm_benchmark/utils.pyprojects/guidellm/toolbox/run_smoke_request/main.pyprojects/kserve/toolbox/capture_llmisvc_state/main.pyprojects/kserve/toolbox/deploy_llmisvc/__init__.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/kserve/toolbox/prepare_hf_model_cache/main.pyprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yaml
…unning in a dedicated task
that merges unrelated legend names ...
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: albertoperdomo2 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
2 participants
Summary by CodeRabbit
New Features
Bug Fixes