Repository navigation
feat: Add llm-d release profiles - #72
Merged
openshift-merge-bot[bot] merged 23 commits intoJun 26, 2026
Merged
openshift-merge-bot[bot] merged 23 commits into
openshift-merge-bot[bot] merged 23 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughAdds per-rate benchmark expansion to GuideLLM and refactors LLM_D orchestration around deployment profiles and run-spec loops, updating KServe rendering, runtime resolution, prepare/test/cleanup phases, presets, workloads, tests, and docs. Caliper only updates the ChangesGuideLLM Multi-Rate Benchmark Expansion
LLM_D Profile-Driven Deployment and RunSpec Orchestration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (1)
projects/guidellm/tests/test_run_guidellm_benchmark.py (1)
10-83: ⚡ Quick winAdd a regression test for empty
--rateswith rate expressions.Please add a negative-case test so the new validation path is locked and doesn’t regress.
Suggested test addition
+import pytest + @@ def test_expand_guidellm_runs_keeps_plain_multi_rate_benchmark_as_single_run() -> None: @@ assert runs[0].args == [ "--backend-type=openai_http", "--rate-type=concurrent", "--rates=300,200,100", "--data=prompt_tokens=1000,output_tokens=1000", "--max-seconds=600", ] + + +def test_expand_guidellm_runs_rejects_empty_rates_with_rate_expressions() -> None: + with pytest.raises(ValueError, match="--rates"): + expand_guidellm_runs( + [ + "--backend-type=openai_http", + "--rate-type=concurrent", + "--rates= , ", + "--max-requests={rate}", + ] + )🤖 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/tests/test_run_guidellm_benchmark.py` around lines 10 - 83, Add a negative test in projects/guidellm/tests/test_run_guidellm_benchmark.py that calls expand_guidellm_runs with an empty "--rates=" plus arguments that include rate expressions (e.g., "--data=prefix_count={2*rate}" or "--max-requests={rate}") and assert that expand_guidellm_runs raises a ValueError (or the same validation exception used elsewhere) to lock in the new validation path; name the test something like test_expand_guidellm_runs_rejects_empty_rates_with_expressions and reference expand_guidellm_runs in the assertion.
🤖 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/utils.py`:
- Around line 75-95: The parsing of rate_values from rates_arg can produce an
empty list and silently return zero runs; validate and fail fast by checking
after computing rate_values (the variable rate_values derived from rates_arg)
and if rates_arg contains "--rates=" or similar but rate_values is empty, raise
a clear exception (e.g., ValueError) so the caller fails rather than returning
an empty runs list; add this check before the for rate in rate_values loop that
constructs runs and reference GuideLLMRun, _substitute_rate_expressions,
_sanitize_rate_label and guidellm_args when locating the relevant code.
In `@projects/llm_d/README.md`:
- Around line 61-63: The docs currently imply all multi-rate workloads expand
into separate GuideLLM runs; update the bullet list to clarify that plain
"rates" remain a single GuideLLM invocation unless another argument contains a
"{rate}" (or other "{...*rate}" expression), in which case the workload is
expanded into one GuideLLM run per rate; mention that expressions like
"{2*rate}" and "{10*rate}" are resolved per run only when expansion occurs and
keep the note that Benchflow-specific features are not modeled yet, referencing
the "rates" key and "{rate}" expressions and the GuideLLM invocation behavior.
In `@projects/llm_d/tests/test_profiles.py`:
- Around line 40-55: The test
test_smoke_presets_inherit_deployment_scheduler_modes reuses the same config
across multiple core_config.project.apply_preset calls which can mask state
leakage; update the test to isolate each preset assertion by reinitializing the
project config (call _init_project_config or otherwise reset
core_config/project/runtime_config) before each apply_preset, then call
core_config.project.apply_preset("<preset>") and assert
runtime_config.get_scheduler_profile_key(), runtime_config.get_model_key() and
runtime_config.get_benchmark_config() for that single preset to ensure earlier
presets don't influence later checks.
---
Nitpick comments:
In `@projects/guidellm/tests/test_run_guidellm_benchmark.py`:
- Around line 10-83: Add a negative test in
projects/guidellm/tests/test_run_guidellm_benchmark.py that calls
expand_guidellm_runs with an empty "--rates=" plus arguments that include rate
expressions (e.g., "--data=prefix_count={2*rate}" or "--max-requests={rate}")
and assert that expand_guidellm_runs raises a ValueError (or the same validation
exception used elsewhere) to lock in the new validation path; name the test
something like test_expand_guidellm_runs_rejects_empty_rates_with_expressions
and reference expand_guidellm_runs in the assertion.
🪄 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: 40e0a76a-0776-4973-b711-8f466c28599f
📒 Files selected for processing (11)
projects/caliper/engine/visualize.pyprojects/guidellm/tests/test_run_guidellm_benchmark.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/guidellm/toolbox/run_guidellm_benchmark/templates/guidellm_job.yaml.j2projects/guidellm/toolbox/run_guidellm_benchmark/utils.pyprojects/llm_d/README.mdprojects/llm_d/orchestration/config.d/workloads.yamlprojects/llm_d/orchestration/presets.d/presets.yamlprojects/llm_d/orchestration/scheduler_profiles/precise.yamlprojects/llm_d/orchestration/test_phase.pyprojects/llm_d/tests/test_profiles.py
💤 Files with no reviewable changes (1)
- projects/caliper/engine/visualize.py
albertoperdomo2
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
albertoperdomo2
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
kpouget
reviewed
Jun 19, 2026
kpouget
reviewed
Jun 19, 2026
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/runtime_config.py`:
- Line 118: The benchmark name is being accessed using dot notation in the
JSONPath string passed to config.project.get_config(), but benchmark names like
concurrent-1k-1k and multi-turn contain hyphens which are not reliably
accessible via dot notation. Change the string from
f"workloads.benchmarks.{benchmark_name}" to
f"workloads.benchmarks['{benchmark_name}']" to use bracket notation, which
properly handles hyphenated keys in JSONPath.
🪄 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: 0f882457-054d-4230-aa3f-b1e3bdd33298
📒 Files selected for processing (11)
projects/guidellm/tests/test_run_guidellm_benchmark.pyprojects/guidellm/toolbox/run_guidellm_benchmark/__init__.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/guidellm/toolbox/run_guidellm_benchmark/templates/guidellm_job.yaml.j2projects/guidellm/toolbox/run_guidellm_benchmark/utils.pyprojects/llm_d/README.mdprojects/llm_d/orchestration/config.d/workloads.yamlprojects/llm_d/orchestration/presets.d/presets.yamlprojects/llm_d/orchestration/runtime_config.pyprojects/llm_d/orchestration/test_phase.pyprojects/llm_d/tests/test_profiles.py
✅ Files skipped from review due to trivial changes (2)
- projects/guidellm/toolbox/run_guidellm_benchmark/init.py
- projects/llm_d/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- projects/guidellm/toolbox/run_guidellm_benchmark/templates/guidellm_job.yaml.j2
- projects/llm_d/orchestration/presets.d/presets.yaml
- projects/guidellm/tests/test_run_guidellm_benchmark.py
- projects/llm_d/orchestration/test_phase.py
- projects/guidellm/toolbox/run_guidellm_benchmark/utils.py
- projects/guidellm/toolbox/run_guidellm_benchmark/main.py
Collaborator
Author
|
/test fournos llm_d gpt-oss-120b-inference-scheduling-release |
|
🔴 Test of 'llm_d test' failed after 00 hours 05 minutes 10 seconds 🔴 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 06 minutes 31 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
Collaborator
Author
|
/test fournos llm_d gpt-oss-120b-inference-scheduling-release |
|
🔴 Test of 'llm_d test' failed after 00 hours 01 minutes 36 seconds 🔴 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 02 minutes 58 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
Collaborator
Author
|
/test fournos llm_d gpt-oss-120b-inference-scheduling-release |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 09 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
Collaborator
Author
|
/test fournos llm_d gpt-oss-120b-inference-scheduling-release |
|
🔴 Test of 'llm_d test' failed after 00 hours 09 minutes 44 seconds 🔴 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/llm_d/orchestration/prepare_phase.py (1)
309-360: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftHandle
oci://model URIs consistently with cache rendering.
runtime_config.get_model_uri()can now returnoci://..., and the KServe renderer builds a cache-backed PVC spec for non-PVC model URIs, but this path still raisesValueErrorfor anything excepthf://. With model cache enabled, OCI-backed runs fail during prepare/test or render a PVC reference that this phase never populates. Please either add the OCI cache preparation path here or skip/disable cache for OCI in both prepare and render until that downloader exists.🤖 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/prepare_phase.py` around lines 309 - 360, The code currently raises a ValueError for any model URI scheme that is not hf://, but runtime_config.get_model_uri() can now return oci:// URIs which causes prepare phase failures. Add handling for oci:// model URIs by either inserting a skip condition early in the function (similar to the existing PVC check that returns early) to skip cache preparation for oci:// URIs, or add an elif branch before the final ValueError that checks for oci:// and logs a message before returning, preventing the unsupported scheme error from being raised.
🤖 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/kserve/toolbox/deploy_llmisvc/utils.py`:
- Around line 60-77: The model URI resolution logic at the beginning of the
function only explicitly preserves oci:// and hf:// prefixes, causing pvc:// and
pvc+hf:// URIs to be incorrectly rewritten to hf://, which breaks the cache-skip
logic later. Add explicit handling for pvc:// and pvc+hf:// prefixed URIs in the
if-elif-else block (before the final else clause that defaults to hf://) so
these PVC-based model URIs are passed through unchanged with their scheme
preserved as "pvc" or "pvc+hf" respectively.
In `@projects/llm_d/orchestration/runtime_config.py`:
- Around line 306-335: The issue is that when creating multiple RunSpec objects
in the loop over combinations, synthesized namespaces inherit the base
namespace's managed state instead of being marked as managed. For
multi-combination cases where a new namespace is synthesized (when
len(combinations) != 1), the RunSpec should have namespace_is_managed set to
True instead of the base namespace_is_managed value to ensure proper preparation
and cleanup. Additionally, the truncate_k8s_name call needs to reserve
sufficient space for the unique model_slug and profile_slug components before
truncation to prevent different matrix runs from sharing the same namespace due
to suffix truncation. Modify the RunSpec instantiation logic to conditionally
set namespace_is_managed based on whether a custom namespace was synthesized,
and ensure the truncation preserves the uniqueness of the model and profile
identifiers.
---
Outside diff comments:
In `@projects/llm_d/orchestration/prepare_phase.py`:
- Around line 309-360: The code currently raises a ValueError for any model URI
scheme that is not hf://, but runtime_config.get_model_uri() can now return
oci:// URIs which causes prepare phase failures. Add handling for oci:// model
URIs by either inserting a skip condition early in the function (similar to the
existing PVC check that returns early) to skip cache preparation for oci://
URIs, or add an elif branch before the final ValueError that checks for oci://
and logs a message before returning, preventing the unsupported scheme error
from being raised.
🪄 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: 00d992ad-a49b-4115-b212-e4ac86eecaa8
📒 Files selected for processing (17)
projects/kserve/toolbox/deploy_llmisvc/utils.pyprojects/llm_d/README.mdprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/cleanup_phase.pyprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/models.yamlprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/config.d/scheduler_profiles.yamlprojects/llm_d/orchestration/config.d/workloads.yamlprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/orchestration/prepare_sequence.pyprojects/llm_d/orchestration/presets.d/presets.yamlprojects/llm_d/orchestration/runtime_config.pyprojects/llm_d/orchestration/scheduler_profiles/approximate.yamlprojects/llm_d/orchestration/scheduler_profiles/precise.yamlprojects/llm_d/orchestration/test_phase.pyprojects/llm_d/tests/test_profiles.py
💤 Files with no reviewable changes (4)
- projects/llm_d/orchestration/config.d/models.yaml
- projects/llm_d/orchestration/scheduler_profiles/precise.yaml
- projects/llm_d/orchestration/config.d/scheduler_profiles.yaml
- projects/llm_d/orchestration/scheduler_profiles/approximate.yaml
✅ Files skipped from review due to trivial changes (1)
- projects/llm_d/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/llm_d/orchestration/config.d/workloads.yaml
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 11 minutes 04 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
albertoperdomo2
force-pushed
the
feat/add-release-profiles
branch
from
June 23, 2026 14:51
2c822c8 to
4e5d3df
Compare
Collaborator
Author
|
/test fournos llm_d gpt-oss-120b-inference-scheduling-release |
|
🔴 Test of 'llm_d test' failed after 00 hours 09 minutes 40 seconds 🔴 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 10 minutes 55 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
Collaborator
Author
|
/test fournos llm_d gpt-oss-120b-inference-scheduling-release |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 08 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
Collaborator
Author
|
/test fournos llm_d gpt-oss-120b-inference-scheduling-release |
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
albertoperdomo2
force-pushed
the
feat/add-release-profiles
branch
from
June 25, 2026 12:20
158bbdf to
bff5855
Compare
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
kpouget
reviewed
Jun 25, 2026
kpouget
reviewed
Jun 25, 2026
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Collaborator
Author
|
/test fournos llm_d gpt-oss-120b-inference-scheduling-release |
|
🟢 Test of 'llm_d test' succeeded after 00 hours 19 minutes 46 seconds 🟢 • Link to the test results. • Generated 1 Caliper report(s):
Test configuration: |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 21 minutes 05 seconds 🟢 • Link to the test results. • No reports generated... Test configuration: |
Contributor
|
nice, great work @albertoperdomo2 , let's merge this |
Contributor
|
/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
This was referenced Jul 16, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add common
llm_drelease profiles and extend the shared GuideLLM launcher to support rate-dependent benchmark expansions.This PR adapts the common Benchflow deployment and benchmark profiles to the current Forge
llm_dshape instead of copying Benchflow definitions literally.llm_dprofilesdeployment-approximate-prefix-cachedeployment-precise-prefix-cachedeployment-distributed-defaultsmoke-basellm_dbenchmark workloadsheavy-heterogeneousmulti-turnconcurrent-1k-1kworkloads.benchmarks.*entries in Forge configShared GuideLLM launcher
build_guidellm_args(...)into sharedguidellmutilsrates: [...]{rate},{N*rate}, or{rate*N}--rates=...benchmarks as a single GuideLLM invocationprefix_count={2*rate}max_requests={10*rate}Documentation and tests
llm_dREADME to reflect the current orchestration entrypoints and multi-rate{rate}expansion behaviorBehavior notes
Summary by CodeRabbit
Release Notes
{rate}expression substitution and per-rate job execution.