Repository navigation
[Experimental]: Release testing - #135
openshift-merge-bot[bot] merged 11 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change scales llm-d deployments, adds structured vLLM and precise prefix-cache configuration, introduces a Llama release preset, enhances manifest rendering, updates Kubernetes wiring, and propagates 3600-second benchmark timeouts through workload execution. Changesllm-d deployment profiles
Guidellm benchmark timeouts
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Preset as llama-33-70b-rhoai-release
participant Renderer as render_inference_service_from_parts
participant Manifest as Kubernetes manifest
participant EPP as EndpointPickerConfig
participant KV as KV event endpoint
Preset->>Renderer: Selects model and deployment profiles
Renderer->>Renderer: Replaces profile placeholders and selects model path
Renderer->>Manifest: Applies labels, annotations, environment, and arguments
Manifest->>EPP: Configures tokenizer pool and ZMQ KV events
EPP->>KV: Publishes KV events through port 5557
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: 1
🤖 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/orchestration/render_inference_service.py`:
- Around line 118-119: The deployment profile handling should merge, rather than
replace, the existing environment variables in serving_container["env"]. Update
this block to preserve the base inference service template entries while adding
or overriding entries from deployment_profile["env"], using the existing
environment-list structure and retaining the deep-copy behavior.
🪄 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: 6fae2064-d9bc-48cb-8176-f78e9d6cc60f
📒 Files selected for processing (6)
projects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/manifests/deployments/approximate-prefix-cache.yamlprojects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yamlprojects/llm_d/orchestration/presets.d/presets.yamlprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/tests/test_profiles.py
| if "env" in deployment_profile: | ||
| serving_container["env"] = copy.deepcopy(deployment_profile["env"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Consider merging env variables instead of overwriting.
Overwriting serving_container["env"] completely removes any environment variables that might have been defined in the base inference service template (such as standard cluster proxies or sidecar configurations). Consider merging the deployment_profile["env"] with the existing env list to be safer.
💡 Proposed fix
- if "env" in deployment_profile:
- serving_container["env"] = copy.deepcopy(deployment_profile["env"])
+ if "env" in deployment_profile:
+ existing_env = {e["name"]: e for e in serving_container.get("env", [])}
+ for new_e in copy.deepcopy(deployment_profile["env"]):
+ existing_env[new_e["name"]] = new_e
+ serving_container["env"] = list(existing_env.values())📝 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.
| if "env" in deployment_profile: | |
| serving_container["env"] = copy.deepcopy(deployment_profile["env"]) | |
| if "env" in deployment_profile: | |
| existing_env = {e["name"]: e for e in serving_container.get("env", [])} | |
| for new_e in copy.deepcopy(deployment_profile["env"]): | |
| existing_env[new_e["name"]] = new_e | |
| serving_container["env"] = list(existing_env.values()) |
🤖 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/orchestration/render_inference_service.py` around lines 118 -
119, The deployment profile handling should merge, rather than replace, the
existing environment variables in serving_container["env"]. Update this block to
preserve the base inference service template entries while adding or overriding
entries from deployment_profile["env"], using the existing environment-list
structure and retaining the deep-copy behavior.
|
/test fournos llm_d llama-33-70b-rhoai-release |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d llama-33-70b-rhoai-release |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d llama-33-70b-rhoai-release |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d llama-33-70b-rhoai-release |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d llama-33-70b-rhoai-release |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/guidellm/toolbox/run_guidellm_benchmark/main.py (1)
171-183: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the Kubernetes calls on the timeout path. The job-state polls here and the
oc()calls incapture_guidellm_state()should pass a remainingtimeout_seconds; otherwise a hung API-server call can block pastctx.wait_deadlineand prevent the timeout from being raised.🤖 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 171 - 183, Bound the Kubernetes polling calls in the timeout path by computing the remaining time until ctx.wait_deadline and passing it as timeout_seconds to oc() in the active job-state check and to every oc() call within capture_guidellm_state(). Preserve the existing return-code handling while ensuring hung API calls cannot extend execution beyond the deadline.
🤖 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_guidellm_benchmark/main.py`:
- Around line 252-260: Update _raise_benchmark_timeout to write the standard
artifact_dir/FAILURE marker before raising TimeoutError, matching the existing
failed-job path. Preserve the current capture_guidellm_state call and timeout
exception behavior.
---
Outside diff comments:
In `@projects/guidellm/toolbox/run_guidellm_benchmark/main.py`:
- Around line 171-183: Bound the Kubernetes polling calls in the timeout path by
computing the remaining time until ctx.wait_deadline and passing it as
timeout_seconds to oc() in the active job-state check and to every oc() call
within capture_guidellm_state(). Preserve the existing return-code handling
while ensuring hung API calls cannot extend execution beyond the deadline.
🪄 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: 0bbe0313-eea3-4f12-bd23-ac1ab23bf438
📒 Files selected for processing (8)
projects/guidellm/tests/test_run_guidellm_benchmark.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/guidellm/toolbox/run_guidellm_benchmark/utils.pyprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/model_cache.yamlprojects/llm_d/orchestration/config.d/workloads.yamlprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/tests/test_profiles.py
💤 Files with no reviewable changes (2)
- projects/llm_d/orchestration/config.d/deployments.yaml
- projects/llm_d/orchestration/render_inference_service.py
| def _raise_benchmark_timeout(args, ctx) -> None: | ||
| capture_guidellm_state( | ||
| artifact_dir=args.artifact_dir, | ||
| namespace=ctx.target_namespace, | ||
| benchmark_name=ctx.benchmark_name, | ||
| ) | ||
| raise TimeoutError( | ||
| f"GuideLLM benchmark {ctx.benchmark_name} did not complete within {args.timeout}s" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Write the standard failure artifact for timed-out jobs.
The failed-job path writes artifact_dir/FAILURE, but _raise_benchmark_timeout() only captures state and raises. Timeout failures should produce the same failure marker so downstream artifact and CI consumers handle both terminal failure modes consistently.
Proposed fix
def _raise_benchmark_timeout(args, ctx) -> None:
capture_guidellm_state(
artifact_dir=args.artifact_dir,
namespace=ctx.target_namespace,
benchmark_name=ctx.benchmark_name,
)
+ write_text(
+ args.artifact_dir / "FAILURE",
+ f"GuideLLM benchmark job '{ctx.benchmark_name}' timed out after {args.timeout}s.\n",
+ )
raise TimeoutError(📝 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.
| def _raise_benchmark_timeout(args, ctx) -> None: | |
| capture_guidellm_state( | |
| artifact_dir=args.artifact_dir, | |
| namespace=ctx.target_namespace, | |
| benchmark_name=ctx.benchmark_name, | |
| ) | |
| raise TimeoutError( | |
| f"GuideLLM benchmark {ctx.benchmark_name} did not complete within {args.timeout}s" | |
| ) | |
| def _raise_benchmark_timeout(args, ctx) -> None: | |
| capture_guidellm_state( | |
| artifact_dir=args.artifact_dir, | |
| namespace=ctx.target_namespace, | |
| benchmark_name=ctx.benchmark_name, | |
| ) | |
| write_text( | |
| args.artifact_dir / "FAILURE", | |
| f"GuideLLM benchmark job '{ctx.benchmark_name}' timed out after {args.timeout}s.\n", | |
| ) | |
| raise TimeoutError( | |
| f"GuideLLM benchmark {ctx.benchmark_name} did not complete within {args.timeout}s" | |
| ) |
🤖 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 252 -
260, Update _raise_benchmark_timeout to write the standard artifact_dir/FAILURE
marker before raising TimeoutError, matching the existing failed-job path.
Preserve the current capture_guidellm_state call and timeout exception behavior.
🔴 Submission of
|
|
/test fournos llm_d llama-33-70b-rhoai-release |
🟠 Execution of
|
🔴 Submission of
|
a876f3c to
2da0007
Compare
|
/test fournos llm_d llama-33-70b-rhoai-release |
🟠 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d llama-33-70b-rhoai-release |
🔴 Execution of
|
🔴 Submission of
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_guidellm_benchmark/main.py`:
- Around line 186-187: Update the polling flow around _raise_benchmark_timeout
to bound each status-query subprocess by the remaining benchmark deadline. Add
or delegate timeout support through shell.run, and pass remaining_deadline() to
every relevant oc get command so stalled calls cannot exceed the configured
timeout or grace period.
🪄 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: 54d7ce9c-f765-4dc2-8456-569b408ecb98
📒 Files selected for processing (11)
projects/guidellm/tests/test_run_guidellm_benchmark.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/guidellm/toolbox/run_guidellm_benchmark/utils.pyprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/model_cache.yamlprojects/llm_d/orchestration/config.d/workloads.yamlprojects/llm_d/orchestration/manifests/deployments/approximate-prefix-cache.yamlprojects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yamlprojects/llm_d/orchestration/presets.d/presets.yamlprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/tests/test_profiles.py
🚧 Files skipped from review as they are similar to previous changes (9)
- projects/llm_d/orchestration/config.d/model_cache.yaml
- projects/llm_d/orchestration/config.d/workloads.yaml
- projects/llm_d/orchestration/presets.d/presets.yaml
- projects/guidellm/toolbox/run_guidellm_benchmark/utils.py
- projects/llm_d/orchestration/config.d/deployments.yaml
- projects/llm_d/orchestration/render_inference_service.py
- projects/guidellm/tests/test_run_guidellm_benchmark.py
- projects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yaml
- projects/llm_d/tests/test_profiles.py
albertoperdomo2
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
openshift-ci
Bot
added
the
needs-rebase
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
8c67043 to
03688e0
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test fournos llm_d smoke |
🔴 Submission of
|
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
|
thanks @albertoperdomo2 , let's merge this 🎉 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kpouget 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
llama-33-70b-rhoai-releasedeployment preset.