WIP [Experimental]: Release testing - #135
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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 |
|
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 | ✅ 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 |
![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
|
/test fournos llm_d llama-33-70b-rhoai-release |
🟠 Execution of
|
🔴 Submission of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
3b68fcb to
f41bf19
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
|
|
/test fournos llm_d llama-33-70b-rhoai-release |
🛑 Execution of
|
|
/test fournos llm_d llama-33-70b-rhoai-release |
|
@albertoperdomo2: 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. |
🛑 Execution of
|
🟢 Execution of
|
| # Allow time for Kubernetes to record the Job's terminal status after its deadline. | ||
| wait_guidellm_benchmark_task._retry_config["attempts"] = _wait_attempts(timeout) |
There was a problem hiding this comment.
I don't know safe this is,
most of forge run in the same Python, so this might be changing a long-live object
wait_guidellm_benchmark_task._retry_config["attempts"]
this looks like a propery to of thewait_guidellm_benchmark_tasktask, not the script specific object
| capture_guidellm_state( | ||
| artifact_dir=args.artifact_dir, | ||
| namespace=ctx.target_namespace, | ||
| benchmark_name=ctx.benchmark_name, | ||
| ) |
There was a problem hiding this comment.
capture_guidellm_state should be marked with @always
then you can remove the _raise_benchmark_timeout function and just raise
| # GitHub /var overrides can select an RWX-capable class together with | ||
| # access_mode: ReadWriteMany for multi-node model-cache mounts. |
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
needs-rebase
2 participants
Summary by CodeRabbit
llama-33-70b-rhoai-releasedeployment preset.