feat: Isolate benchmark workloads with per-deployment lifecycle#129
Conversation
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
|
Warning Review limit reached
Next review available in: 55 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR expands ChangesBenchmark isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant RuntimeConfig
participant TestPhase
participant CleanupPhase
CI->>RuntimeConfig: get_run_specs()
CI->>RuntimeConfig: activate_run_spec(run_spec)
CI->>TestPhase: run test with isolated artifacts
TestPhase->>TestPhase: deploy with workload overrides
TestPhase->>TestPhase: run active benchmark
CI->>CleanupPhase: clean up active benchmark resources
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 fournos llm_d smoke |
🔴 Submission of
|
|
/test fournos llm_d smoke |
🔴 Submission of
|
|
/test fournos llm_d smoke |
🔴 Submission of
|
![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
🧹 Nitpick comments (2)
projects/llm_d/docs/benchmark-isolation.md (2)
57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Add a language identifier (e.g.,
textoryaml) to ensure proper syntax highlighting and avoid linter warnings.♻️ Proposed fix
-``` +```text /var runtime.model_name: Qwen/Qwen3-0.6B /var runtime.deployment_profile: [approximate-prefix-cache, distributed-default] /var runtime.benchmark_key: [short, multi-turn]</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/docs/benchmark-isolation.mdaround lines 57 - 61, Update the
fenced code block near the runtime.model_name, runtime.deployment_profile, and
runtime.benchmark_key entries to include an appropriate language identifier,
such as text, while preserving its contents.</details> <!-- cr-comment:v1:5fc7fda2abe221bf60e30b7f --> _Source: Linters/SAST tools_ --- `44-46`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Specify a language for the fenced code block.** Add a language identifier (e.g., `text`) to ensure proper syntax highlighting and avoid linter warnings. <details> <summary>♻️ Proposed fix</summary> ```diff -``` +```text model_name x deployment_profile x benchmark_key</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/docs/benchmark-isolation.mdaround lines 44 - 46, Update the
fenced code block containing “model_name x deployment_profile x benchmark_key”
to include an explicit text language identifier, such as text, while preserving
its contents.</details> <!-- cr-comment:v1:2fee857ec22396fa9d3bbd00 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/runtime_config.py:
- Around line 104-114: Update deep_merge so when both the existing merged value
and the override value are lists, it concatenates the base list with the
override list instead of replacing the base contents. Preserve recursive
dictionary merging and existing override behavior for all other value types.- Around line 353-356: The namespace construction around truncate_k8s_name must
preserve uniqueness when the combined namespace parts exceed
namespace_max_length. Add a deterministic short hash derived from the full
joined namespace parts as a suffix, truncate the readable prefix as needed to
fit the Kubernetes limit, and ensure different run specifications with distinct
trailing identifiers produce distinct namespaces.
Nitpick comments:
In@projects/llm_d/docs/benchmark-isolation.md:
- Around line 57-61: Update the fenced code block near the runtime.model_name,
runtime.deployment_profile, and runtime.benchmark_key entries to include an
appropriate language identifier, such as text, while preserving its contents.- Around line 44-46: Update the fenced code block containing “model_name x
deployment_profile x benchmark_key” to include an explicit text language
identifier, such as text, while preserving its contents.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `567096ea-f6bd-48a0-b531-d5e7cd6a51ac` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 441ea2d6d389ae31c2022954cf256b678885e832 and 4e4ad1a659e72d8abef5e0689ae065b3c5e512f7. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `projects/llm_d/docs/benchmark-isolation.md` * `projects/llm_d/orchestration/ci.py` * `projects/llm_d/orchestration/cleanup_phase.py` * `projects/llm_d/orchestration/runtime_config.py` * `projects/llm_d/orchestration/test_phase.py` * `projects/llm_d/tests/test_profiles.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| def deep_merge(base: Any, override: Any) -> Any: | ||
| if not isinstance(base, dict) or not isinstance(override, dict): | ||
| return copy.deepcopy(override) | ||
|
|
||
| merged = copy.deepcopy(base) | ||
| for key, value in override.items(): | ||
| if key in merged and isinstance(merged[key], dict) and isinstance(value, dict): | ||
| merged[key] = _deep_merge(merged[key], value) | ||
| merged[key] = deep_merge(merged[key], value) | ||
| else: | ||
| merged[key] = copy.deepcopy(value) | ||
| return merged |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
deep_merge overwrites lists instead of extending them.
Because deep_merge only recurses on dictionaries, any list in the override dictionary will completely replace the corresponding list in the base dictionary.
For instance, if a deployment profile specifies vllm_args: ["--tensor-parallel-size=4"] and the active benchmark defines deployment_overrides: {vllm_args: ["--max-model-len=32768"]}, the base vllm_args will be wiped out entirely. This can lead to unexpected deployment failures (e.g., OOM crashes) due to missing required base arguments.
Consider updating the merge logic to concatenate lists, or clearly document that list fields must be comprehensively redefined in deployment_overrides.
🤖 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/runtime_config.py` around lines 104 - 114,
Update deep_merge so when both the existing merged value and the override value
are lists, it concatenates the base list with the override list instead of
replacing the base contents. Preserve recursive dictionary merging and existing
override behavior for all other value types.
| namespace = truncate_k8s_name( | ||
| f"{base_namespace}-{model_slug}-{profile_slug}", | ||
| "-".join(ns_parts), | ||
| max_length=namespace_max_length, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Namespace truncation drops identifying slugs, causing collisions.
Because truncate_k8s_name truncates strings from the right, distinguishing trailing identifiers like bench_slug will be lost if the preceding segments approach the 63-character limit (e.g., due to long model or profile names). This will generate identical namespaces for different run specs, leading to resource collisions and failures during test executions.
Consider computing a deterministic short hash of the combined parts to ensure uniqueness when the raw string exceeds the namespace length limit.
🛡️ Proposed fix (using a hashed suffix for uniqueness)
- namespace = truncate_k8s_name(
- "-".join(ns_parts),
- max_length=namespace_max_length,
- )
+ raw_namespace = "-".join(ns_parts)
+ if len(raw_namespace) <= namespace_max_length:
+ namespace = raw_namespace
+ else:
+ import hashlib
+ # Keep the beginning and append a deterministic short hash to avoid collisions
+ prefix = truncate_k8s_name(raw_namespace, max_length=namespace_max_length - 7)
+ suffix = hashlib.md5(raw_namespace.encode()).hexdigest()[:6]
+ namespace = f"{prefix}-{suffix}"📝 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.
| namespace = truncate_k8s_name( | |
| f"{base_namespace}-{model_slug}-{profile_slug}", | |
| "-".join(ns_parts), | |
| max_length=namespace_max_length, | |
| ) | |
| raw_namespace = "-".join(ns_parts) | |
| if len(raw_namespace) <= namespace_max_length: | |
| namespace = raw_namespace | |
| else: | |
| import hashlib | |
| # Keep the beginning and append a deterministic short hash to avoid collisions | |
| prefix = truncate_k8s_name(raw_namespace, max_length=namespace_max_length - 7) | |
| suffix = hashlib.md5(raw_namespace.encode()).hexdigest()[:6] | |
| namespace = f"{prefix}-{suffix}" |
🤖 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/runtime_config.py` around lines 353 - 356, The
namespace construction around truncate_k8s_name must preserve uniqueness when
the combined namespace parts exceed namespace_max_length. Add a deterministic
short hash derived from the full joined namespace parts as a suffix, truncate
the readable prefix as needed to fit the Kubernetes limit, and ensure different
run specifications with distinct trailing identifiers produce distinct
namespaces.
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…erdomo2/forge into feat/benchmark-matrix-isolation
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/toolbox/cleanup_test_resources/main.py`:
- Line 223: Update the oc cleanup invocation to capture its CommandResult, then
log a warning when returncode is non-zero while preserving the existing
--timeout=120s and check=False behavior. Use the surrounding cleanup function’s
established logging mechanism and reference the captured result so delete
failures are no longer silently ignored.
🪄 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: 7177bd67-b5e8-48ca-b0d1-9f848705758e
📒 Files selected for processing (1)
projects/llm_d/toolbox/cleanup_test_resources/main.py
| """Best effort deletion of Kubernetes resources with timeout""" | ||
| try: | ||
| oc(*oc_args, check=False) | ||
| oc(*oc_args, "--timeout=120s", check=False) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and nearby definitions.
git ls-files 'projects/llm_d/toolbox/cleanup_test_resources/main.py' 'projects/llm_d/toolbox/cleanup_test_resources/*' | sed -n '1,20p'
echo '--- main.py around line 223 ---'
nl -ba projects/llm_d/toolbox/cleanup_test_resources/main.py | sed -n '190,250p'
echo '--- search for oc helper and callers ---'
rg -n "def oc\b|oc\(" projects/llm_d/toolbox/cleanup_test_resources -S
echo '--- broader references to subprocess timeout handling ---'
rg -n "TimeoutExpired|CalledProcessError|check=False|check=True" projects/llm_d/toolbox/cleanup_test_resources -SRepository: openshift-psap/forge
Length of output: 284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- main.py lines 190-250 ---'
sed -n '190,250p' projects/llm_d/toolbox/cleanup_test_resources/main.py
echo '--- definitions/usages of oc helper in cleanup_test_resources ---'
rg -n "def oc\b|oc_args|TimeoutExpired|CalledProcessError|check=False|check=True" projects/llm_d/toolbox/cleanup_test_resources -S
echo '--- list files in cleanup_test_resources ---'
git ls-files 'projects/llm_d/toolbox/cleanup_test_resources/*'Repository: openshift-psap/forge
Length of output: 2632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- find oc helper definitions ---'
rg -n "^def oc\b|^async def oc\b|oc =|CommandResult|returncode|raise_for_status" projects -S
echo '--- find subprocess wrapper implementations that may define check=False behavior ---'
rg -n "class .*CommandResult|def run\(|check=False|TimeoutExpired|CalledProcessError" projects -SRepository: openshift-psap/forge
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- core/dsl/utils/k8s.py (oc helper) ---'
sed -n '1,140p' projects/core/dsl/utils/k8s.py
echo '--- cleanup_test_resources/main.py around best-effort delete callers ---'
sed -n '1,240p' projects/llm_d/toolbox/cleanup_test_resources/main.py
echo '--- cleanup_phase best-effort delete pattern for comparison ---'
sed -n '120,160p' projects/llm_d/orchestration/cleanup_phase.pyRepository: openshift-psap/forge
Length of output: 11342
Warn on oc delete failures here. check=False swallows non-zero exits, and subprocess.TimeoutExpired won’t fire for the --timeout=120s flag because that only applies to oc itself. Capture the CommandResult and log on returncode != 0, otherwise cleanup failures are silently ignored.
🤖 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/cleanup_test_resources/main.py` at line 223, Update
the oc cleanup invocation to capture its CommandResult, then log a warning when
returncode is non-zero while preserving the existing --timeout=120s and
check=False behavior. Use the surrounding cleanup function’s established logging
mechanism and reference the captured result so delete failures are no longer
silently ignored.
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
projects/llm_d/toolbox/cleanup_test_resources/main.py (1)
222-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unreachable
try-exceptblock for timeouts.The
subprocess.TimeoutExpiredexception will not be raised here. The underlyingshell.runwrapper callssubprocess.runwithout atimeoutargument. The--timeout=300sargument applies directly to theocCLI, which will simply terminate and return a non-zero exit code if it times out. This timeout failure is already perfectly handled by your newif result.returncode != 0:check, making thetry...exceptblock obsolete.Additionally, as a minor caveat: if
oc_argscontainsdelete-specific flags (e.g.,--forceor--grace-period), the subsequentoc describecommand will reject them and fail silently due tocheck=False. This is generally acceptable for a best-effort debug function, but you might want to keep it in mind if you notice missing debug logs.♻️ Proposed refactor to remove the unreachable exception handling
- try: - result = oc(*oc_args, "--timeout=300s", check=False) - if result.returncode != 0: - logger.warning( - "Failed to delete %s (rc=%d), capturing describe", description, result.returncode - ) - describe_args = tuple(a for a in oc_args if a != "delete") - oc("describe", *describe_args, check=False) - except subprocess.TimeoutExpired: - logger.warning("Timed out deleting %s: oc %s", description, " ".join(oc_args)) + result = oc(*oc_args, "--timeout=300s", check=False) + if result.returncode != 0: + logger.warning( + "Failed to delete %s (rc=%d), capturing describe", description, result.returncode + ) + describe_args = tuple(a for a in oc_args if a != "delete") + oc("describe", *describe_args, check=False)🤖 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/cleanup_test_resources/main.py` around lines 222 - 231, Remove the unnecessary try-except wrapper around the oc invocation in the cleanup flow. Keep the existing result.returncode check, warning, describe capture, and timeout-related --timeout=300s argument unchanged so CLI timeouts continue through the nonzero return-code handling; do not add replacement exception handling.
🤖 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.
Nitpick comments:
In `@projects/llm_d/toolbox/cleanup_test_resources/main.py`:
- Around line 222-231: Remove the unnecessary try-except wrapper around the oc
invocation in the cleanup flow. Keep the existing result.returncode check,
warning, describe capture, and timeout-related --timeout=300s argument unchanged
so CLI timeouts continue through the nonzero return-code handling; do not add
replacement exception handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df042792-9930-4db6-bb76-4b995185ab22
📒 Files selected for processing (3)
projects/core/dsl/utils/__init__.pyprojects/llm_d/docs/benchmark-isolation.mdprojects/llm_d/toolbox/cleanup_test_resources/main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/llm_d/docs/benchmark-isolation.md
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…erdomo2/forge into feat/benchmark-matrix-isolation
|
/test fournos llm_d smoke |
🔴 Submission of
|
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
| with runtime_config.activate_run_spec(run_spec): | ||
| with env.NextArtifactDir(run_spec.artifact_dirname): | ||
| exit_code = max(exit_code, test_toolbox_run()) |
There was a problem hiding this comment.
I think you can merge the two with ..., ....:
and can you add a config flag abort_on_failure or something like that?
most of the time we want to abort on failure, instead of continuing with a broken setup. But not always
|
thanks @albertoperdomo2 , |
|
/approve |
|
[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