Add SGLang and TRT-LLM engine support with multi-engine abstraction for rhaiis project - #149
Merged
Merged
Conversation
Replace all vllm-specific references with generic engine abstraction. Configuration, runtime helpers, CLI, templates, and callers now support vllm, sglang, and trtllm engines with automatic argument translation. Fix parity gaps with model-furnace: add securityContext, XDG env vars, NCCL_DEBUG for multi-GPU, 64Gi shared memory, TRT-LLM termination grace period and resource limits, and disable prometheus for SGLang. Co-authored-by: Cursor <cursoragent@cursor.com>
OpenShift SCC rejects runAsUser:0 when the service account lacks the anyuid SCC. vLLM and SGLang don't require root, so drop it. TRT-LLM retains runAsUser:0 as it needs root for flashinfer compilation. Co-authored-by: Cursor <cursoragent@cursor.com>
PyTorch's torch._inductor calls getpass.getuser() which falls back to pwd.getpwuid(). OpenShift assigns arbitrary UIDs not in /etc/passwd, causing a KeyError. Setting USER env var avoids the passwd lookup. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThe PR adds multi-engine RHAIIS orchestration for vLLM, SGLang, and TensorRT-LLM. It updates KServe deployment, benchmarking, analysis, metadata, and documentation. It also adds filesystem-group support to GuideLLM jobs and format-aware Caliper KPI processing. ChangesRHAIIS engine orchestration
GuideLLM filesystem-group support
Caliper KPI output
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TestPhase
participant ManifestBuilders
participant DeployToolbox
participant KServe
TestPhase->>ManifestBuilders: Build engine-specific manifests
ManifestBuilders-->>TestPhase: Return ServingRuntime and InferenceService
TestPhase->>DeployToolbox: Submit manifest files
DeployToolbox->>KServe: Apply YAML resources
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
kpouget
reviewed
Jul 29, 2026
kpouget
reviewed
Jul 29, 2026
Remove hardcoded ngc-secret from ServingRuntime template. Users now configure all pull secrets through rhaiis.deploy.image_pull_secrets as a list, including any NGC secrets needed for TRT-LLM. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace Jinja2 templates with Python dict construction in a new manifests.py module. The deploy_kserve_isvc script is now generic — it accepts pre-built manifest dicts and applies them, with no engine awareness. This follows the pattern established by llm_d. Co-authored-by: Cursor <cursoragent@cursor.com>
The hierarchical format transformation was overwriting kpis.json (JSONL) with a single indented JSON object, breaking the downstream csv-export step which expects JSON Lines input. Co-authored-by: Cursor <cursoragent@cursor.com>
The config override system cannot create multiple levels of nested keys at once. Pre-populate trtllm_config with sensible defaults so FournosJob overrides for kv_cache_config, cuda_graph_config, and moe_config can resolve correctly. Co-authored-by: Cursor <cursoragent@cursor.com>
The /results PVC is mounted with root ownership, but OpenShift runs the GuideLLM container as a non-root UID. Add an init container that chmod's the mount so GuideLLM can write benchmark results. Co-authored-by: Cursor <cursoragent@cursor.com>
Harshith-umesh
force-pushed
the
sglang-trtllm
branch
from
July 30, 2026 16:29
5caeefd to
802a178
Compare
fsGroup: 0 is incompatible with restricted/nonroot SCCs on shared clusters. Move it behind an optional benchmarks.guidellm.fs_group config key (disabled by default). Clusters where the CSI driver provisions root-owned volumes can opt in via configOverrides. Co-authored-by: Cursor <cursoragent@cursor.com>
Add mamba_ssm_cache_dtype, mamba_ssm_stochastic_rounding, mamba_ssm_philox_rounds, enable_chunked_prefill, and num_postprocess_workers to the base trtllm_config so they can be overridden via FournosJob configOverrides. Co-authored-by: Cursor <cursoragent@cursor.com>
The runtime_args field on the dashboard was only populated from engine_args, missing the trtllm_config entries (kv_cache, CUDA graphs, MoE, etc.). Thread trtllm_config through to _create_test_labels so it appears in the dashboard alongside engine args. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace deprecated 'python -m sglang.launch_server' with the recommended 'sglang serve' command. Co-authored-by: Cursor <cursoragent@cursor.com>
Harshith-umesh
changed the title
Add SGLang and TRT-LLM documentation: engine abstraction overview, per-engine FournosJob YAML examples, updated configOverrides table with new engine/deploy keys, and manifests.py in entrypoints list. Co-authored-by: Cursor <cursoragent@cursor.com>
Harshith-umesh
marked this pull request as ready for review
July 31, 2026 02:08
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/rhaiis/README.md (1)
467-481: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the MLflow tag names to match the code.
The table still lists
vllm_imageandvllm_version.test_phase.py's_set_mlflow_metadatanow emitsserving_imageandserving_versioninstead. Update the table so it reflects the actual tag names.📝 Proposed fix
-| `vllm_image` | vllm/vllm-openai:v0.24.0 | -| `vllm_version` | v0.24.0 | +| `serving_image` | vllm/vllm-openai:v0.24.0 | +| `serving_version` | v0.24.0 |🤖 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/rhaiis/README.md` around lines 467 - 481, Update the MLflow tags table to replace vllm_image with serving_image and vllm_version with serving_version, matching the names emitted by test_phase.py’s _set_mlflow_metadata.
🧹 Nitpick comments (1)
projects/rhaiis/orchestration/runtime_config.py (1)
104-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLog dropped, untranslated engine args.
_translate_argssilently discards any key not in_COMMON_ARG_TRANSLATIONS. Several models set vLLM-style keys such askv-cache-dtypeandenable-expert-parallelinengine_args(for examplellama-4-maverick-fp8andqwen3-235b-instruct-fp8inconfig.d/models.yaml). When the active engine issglangortrtllm, these settings disappear without any log message. Add a debug or warning log for each dropped key so users benchmarking FP8/MoE models on non-vLLM engines can see that a setting was not carried over.♻️ Proposed fix to log dropped args
def _translate_args(args: dict, engine: str) -> dict: """Translate vLLM-style engine_args to another engine's arg naming. Only shared args (TP, DP) are translated; vLLM-specific args are dropped. """ mapping = _COMMON_ARG_TRANSLATIONS.get(engine, {}) translated = {} for key, val in args.items(): if key in mapping: translated[mapping[key]] = val - # Drop vLLM-only args that have no equivalent + else: + logger.warning( + "Dropping vLLM-only arg '%s' (no %s equivalent)", key, engine + ) return translated🤖 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/rhaiis/orchestration/runtime_config.py` around lines 104 - 115, Update _translate_args to log each engine-argument key that is not present in the selected engine’s mapping before dropping it, using the module’s existing debug or warning logger. Preserve translation of mapped keys and ensure the log identifies the dropped key and target engine.
🤖 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/caliper/orchestration/postprocess.py`:
- Around line 549-553: Update the file-opening call in the hierarchical JSON
output block to explicitly use UTF-8 encoding when writing hierarchical_file.
Keep json.dump’s existing ensure_ascii=False behavior unchanged so non-ASCII
values are emitted correctly.
- Around line 551-553: Validate and constrain the output path constructed in the
caliper.postprocess.kpi.artifacts_to_kpis flow before it is used for --output or
file writes. Reject absolute configured values and any path containing parent
traversal that resolves outside env.ARTIFACT_DIR, then use the validated path
for both output_file and hierarchical_file.
In `@projects/rhaiis/orchestration/config.d/rhaiis.yaml`:
- Around line 14-57: Update the SGLang image pins under engines.sglang.images to
use aligned accelerator versions where a compatible ROCm release exists;
otherwise document the supported downstream requirement explaining why amd
remains on v0.5.4.post2-rocm700-mi30x while nvidia uses v0.5.11.
In `@projects/rhaiis/orchestration/manifests.py`:
- Around line 211-241: Update _build_vllm_sglang_container to build every vLLM
and SGLang --port argument from the engine_port parameter instead of hardcoding
8080, while preserving the existing model-path and command branching.
- Around line 292-341: The _build_trtllm_launch_script function assumes every
model uses Hugging Face storage. Add a storage_source branch to this function,
matching the existing VLLM/sglang behavior: when storage is pre-populated, use
the mounted model path directly and skip the /mnt/models snapshot check and hf
download; retain the current Hugging Face setup for HF sources, and ensure
trtllm-serve receives the correct mounted model location.
In `@projects/rhaiis/README.md`:
- Around line 484-503: The Available models table in README.md lists the wrong
TP size for nemotron3super-120b-fp8. Update the Nemotron 120B row to show TP
size 4, matching the tensor-parallel-size configured in models.yaml.
- Around line 118-153: Update the vLLM FournosJob example’s configOverrides key
from rhaiis.images.nvidia to rhaiis.engines.vllm.images.nvidia so
get_serving_image and _apply_cli_overrides use the intended serving-image
override; leave the configured image value unchanged.
---
Outside diff comments:
In `@projects/rhaiis/README.md`:
- Around line 467-481: Update the MLflow tags table to replace vllm_image with
serving_image and vllm_version with serving_version, matching the names emitted
by test_phase.py’s _set_mlflow_metadata.
---
Nitpick comments:
In `@projects/rhaiis/orchestration/runtime_config.py`:
- Around line 104-115: Update _translate_args to log each engine-argument key
that is not present in the selected engine’s mapping before dropping it, using
the module’s existing debug or warning logger. Preserve translation of mapped
keys and ensure the log identifies the dropped key and target engine.
🪄 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: 9f9b0f35-3c6b-4657-8b57-f88902f5fb77
📒 Files selected for processing (18)
projects/caliper/orchestration/postprocess.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/rhaiis/README.mdprojects/rhaiis/orchestration/analysis.pyprojects/rhaiis/orchestration/ci.pyprojects/rhaiis/orchestration/cli.pyprojects/rhaiis/orchestration/config.d/models.yamlprojects/rhaiis/orchestration/config.d/rhaiis.yamlprojects/rhaiis/orchestration/manifests.pyprojects/rhaiis/orchestration/notifications.pyprojects/rhaiis/orchestration/presets.d/presets.yamlprojects/rhaiis/orchestration/runtime_config.pyprojects/rhaiis/orchestration/test_phase.pyprojects/rhaiis/toolbox/deploy_kserve_isvc/main.pyprojects/rhaiis/toolbox/deploy_kserve_isvc/templates/inferenceservice.yaml.j2projects/rhaiis/toolbox/deploy_kserve_isvc/templates/servingruntime.yaml.j2
💤 Files with no reviewable changes (2)
- projects/rhaiis/toolbox/deploy_kserve_isvc/templates/servingruntime.yaml.j2
- projects/rhaiis/toolbox/deploy_kserve_isvc/templates/inferenceservice.yaml.j2
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
| PULL_PULL_SHA: "<commit-sha>" | ||
| ``` | ||
|
|
||
| ### FournosJob YAML (TRT-LLM) |
Contributor
There was a problem hiding this comment.
please define presets Harshith!
Member
Author
There was a problem hiding this comment.
yep I wanted to keep the presets in another PR, need to brainstorm with the rhaiis team about the presets.
- Add UTF-8 encoding to hierarchical JSON read/write - Validate output paths reject absolute/parent-traversal values - Honor configured output name for hierarchical JSON, save JSONL separately - Fix vLLM config override key in README examples - Mention release tags in FournosJob YAML examples - Accept YAML file paths instead of dicts in deploy_kserve_isvc toolbox - Write manifest files to src/ subdirectory in artifact dir Co-authored-by: Cursor <cursoragent@cursor.com>
Harshith-umesh
force-pushed
the
sglang-trtllm
branch
from
July 31, 2026 14:37
c29e130 to
89f906a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/caliper/orchestration/postprocess.py`:
- Around line 1536-1540: Validate the configured output path used to construct
kpi_json_path with the same absolute-path and parent-component checks as other
artifact outputs, and reject any resolved path outside ARTIFACT_DIR. Apply this
before build_kpi_csv_export_command uses the path as --input, including when KPI
generation is disabled.
- Around line 548-551: The JSONL companion path must remain distinct when the
configured output already ends in .jsonl. Add or reuse a helper that derives a
separate companion filename for .jsonl outputs, then use that helper
consistently in the save/rename logic and the CSV lookup path around the
postprocessing flow. Preserve the existing behavior for other output extensions
while ensuring CSV export always reads the JSONL companion rather than the
hierarchical JSON.
In `@projects/rhaiis/toolbox/deploy_kserve_isvc/main.py`:
- Around line 42-45: Validate and parse each manifest before invoking oc in both
apply paths around the ServingRuntime and line-50 resource operations. Require
exactly the expected resource kind and a non-empty metadata.name, reject Secret
manifests through the dedicated secret apply path, and abort before any
OpenShift state changes when validation fails. Reuse a shared
manifest-validation helper for both paths and use the validated name in success
messages.
- Around line 43-44: Add encoding="utf-8" parameter to the open call for
args.servingruntime_file shown in the diff, and update the second open call for
the other YAML file with the same encoding parameter. This ensures explicit
UTF-8 encoding for YAML metadata handling independent of the system locale.
🪄 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: dd347dff-2b63-4ba6-a509-f4ce54105bef
📒 Files selected for processing (4)
projects/caliper/orchestration/postprocess.pyprojects/rhaiis/README.mdprojects/rhaiis/orchestration/test_phase.pyprojects/rhaiis/toolbox/deploy_kserve_isvc/main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/rhaiis/README.md
Member
Author
|
/test fournos rhaiis nvidia |
🟢 Execution of
|
🟢 Submission of
|
csv-export now handles both hierarchical JSON (v2) and JSONL formats via read_kpis_from_file, eliminating the need to rename files or maintain separate JSONL copies. The configured output name is honored. Co-authored-by: Cursor <cursoragent@cursor.com>
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 (4)
projects/caliper/cli/commands.py (2)
690-693: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the
raise edebug artifact.
raise ere-raises the exception, so Lines 692-693 never run. The command loses the error message and the exit code 3, and it prints a raw traceback instead.🐛 Proposed fix
except Exception as e: # noqa: BLE001 - raise e click.echo(f"❌ kpi analyze failed: {e}", err=True) sys.exit(3)🤖 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/caliper/cli/commands.py` around lines 690 - 693, Remove the raise e statement from the exception handler in the KPI analyze command so the existing click.echo error message and sys.exit(3) execute when an exception occurs. Preserve the current error formatting and exit code behavior.
649-661: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not call
len()on the baseline path.
find_most_recent_baseline()returns a singlePath | None, solen(baseline_kpis)raisesTypeErrorbefore analysis runs. Assign it to a name likemost_recent_baselinewhen comparing against one file, or collect multiplekpis.jsonfiles underbaseline_kpisif the command is meant to use all baselines.🐛 Proposed fix (single most recent baseline)
- baseline_kpis = find_most_recent_baseline(baseline_dir) - if not baseline_kpis: + most_recent_baseline = find_most_recent_baseline(baseline_dir) + if not most_recent_baseline: click.echo( f"❌ No kpis.json files found in baseline directory: {baseline_dir}", err=True ) sys.exit(1) + baseline_kpis = [most_recent_baseline] click.echo(f"📊 Found {len(baseline_kpis)} baseline files to process") - # Run analysis with ALL baseline files (not just the most recent) + # Run analysis with the most recent baseline file result = run_analyze( current_path=current, baseline_kpis=baseline_kpis, output_path=output, plugin=plugin )🤖 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/caliper/cli/commands.py` around lines 649 - 661, Update the baseline handling around find_most_recent_baseline so it treats the result as a single Path or None rather than calling len() on it. Rename the variable to reflect one baseline file, adjust the missing-baseline check and status message accordingly, and pass the single baseline path to run_analyze while preserving the existing error behavior.projects/caliper/orchestration/postprocess.py (2)
1560-1561: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThe CSV input path is still built from the configured value without validation.
Line 1561 joins
self.config.kpi.artifacts_to_kpis.outputtooutput_dirwith no check._run_artifacts_to_kpisrejects absolute paths and..parts on Lines 499-502, but that check does not run whenkpi.artifacts_to_kpis.enabledis false whilekpi.kpis_to_csv.enabledis true. The value then reachesbuild_kpi_csv_export_commandas--input. Extract the check into a helper and call it at both sites.🤖 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/caliper/orchestration/postprocess.py` around lines 1560 - 1561, Extract the artifacts-to-KPIs output path validation currently performed in _run_artifacts_to_kpis into a reusable helper that rejects absolute paths and any “..” path components. Call this helper from both _run_artifacts_to_kpis and the kpis-to-CSV flow before constructing kpi_json_path or passing the value to build_kpi_csv_export_command.Source: Coding guidelines
929-952: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign
analyze_hierarchical_kpiswith the schema v2testsstructure.
_transform_kpis_to_hierarchical_format()writes{"schema_version": "2", "tests": [...]}, butanalyze_hierarchical_kpis()readscurrent_data.get("metrics")andbaseline_data.get("metrics"). Since the produced files have no top-levelmetricskey, the analyzer reports zero metrics, zero regressions, zero improvements, and returnssuccess. Move the analyzer to readtests[].kpis[]or write hierarchical KPIs under a shared expected key.🤖 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/caliper/orchestration/postprocess.py` around lines 929 - 952, Update analyze_hierarchical_kpis to consume the schema v2 structure produced by _transform_kpis_to_hierarchical_format: read KPI entries from tests[].kpis[] for both current and baseline data instead of the missing top-level metrics key. Preserve the existing comparison, regression, improvement, and result-status behavior using the extracted KPI collections.
🧹 Nitpick comments (3)
projects/caliper/orchestration/postprocess.py (2)
905-910: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the stale note and the now-unused status file.
The step no longer runs a subprocess, so the note on Line 905 describes a removed implementation.
status_filecreated earlier in this function is never written now, and the laterstatus_file.unlink()block only swallowsFileNotFoundError. Remove both to keep the function clear.♻️ Proposed refactor
- # Note: CLI command building removed in favor of direct function call - # Load plugin for KPI definitions from projects.caliper.engine.load_plugin import load_plugin🤖 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/caliper/orchestration/postprocess.py` around lines 905 - 910, Remove the stale CLI command-building note near the plugin-loading code, and remove the unused status_file creation plus its later status_file.unlink cleanup block in the surrounding postprocessing function. Keep the direct load_plugin flow unchanged.
534-558: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
read_kpis_from_fileandwrite_kpis_in_formatfor hierarchical KPI output.This block re-implements JSONL read/write around
_transform_kpis_to_hierarchical_format, which duplicates the helper flow inprojects/caliper/engine/kpi/format.py. Callread_kpis_from_file, thenwrite_kpis_in_format(..., format_type="hierarchical"), and remove the unused local_transform_kpis_to_hierarchical_format. The nestedimport jsonstatements are also unnecessary becausepostprocess.pyalready importsjson.🤖 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/caliper/orchestration/postprocess.py` around lines 534 - 558, The KPI transformation block should reuse the existing format helpers instead of manually parsing and writing JSON. Replace the local JSONL read/write and _transform_kpis_to_hierarchical_format call with read_kpis_from_file followed by write_kpis_in_format(..., format_type="hierarchical"), remove the unused local transformation helper, and delete the nested json imports.projects/caliper/engine/kpi/format.py (1)
120-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse only the first docstring line for
name.
func.__doc__can span several lines and can include indentation. The resultingnamefield then contains newlines and padding. Take the first non-empty line instead.♻️ Proposed refactor
"name": ( - func.__doc__.replace(" KPI.", "") + func.__doc__.strip().splitlines()[0].replace(" KPI.", "") if func.__doc__ else kpi_id.replace("_", " ").title() ),🤖 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/caliper/engine/kpi/format.py` around lines 120 - 127, Update the KPI metadata construction around the func.__doc__ name fallback to derive name from the first non-empty, whitespace-trimmed docstring line before removing " KPI.". Preserve the existing kpi_id-based fallback when no non-empty documentation exists.
🤖 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/caliper/engine/kpi/format.py`:
- Around line 254-317: Update the JSON parsing flow around the schema-v2 check
so a successfully parsed dictionary that is not schema version "2" falls back to
the existing JSONL record-processing path instead of raising ValueError.
Preserve hierarchical flattening for schema-v2 input and ensure the single flat
KPI record is appended through the same behavior used for JSONL lines.
---
Outside diff comments:
In `@projects/caliper/cli/commands.py`:
- Around line 690-693: Remove the raise e statement from the exception handler
in the KPI analyze command so the existing click.echo error message and
sys.exit(3) execute when an exception occurs. Preserve the current error
formatting and exit code behavior.
- Around line 649-661: Update the baseline handling around
find_most_recent_baseline so it treats the result as a single Path or None
rather than calling len() on it. Rename the variable to reflect one baseline
file, adjust the missing-baseline check and status message accordingly, and pass
the single baseline path to run_analyze while preserving the existing error
behavior.
In `@projects/caliper/orchestration/postprocess.py`:
- Around line 1560-1561: Extract the artifacts-to-KPIs output path validation
currently performed in _run_artifacts_to_kpis into a reusable helper that
rejects absolute paths and any “..” path components. Call this helper from both
_run_artifacts_to_kpis and the kpis-to-CSV flow before constructing
kpi_json_path or passing the value to build_kpi_csv_export_command.
- Around line 929-952: Update analyze_hierarchical_kpis to consume the schema v2
structure produced by _transform_kpis_to_hierarchical_format: read KPI entries
from tests[].kpis[] for both current and baseline data instead of the missing
top-level metrics key. Preserve the existing comparison, regression,
improvement, and result-status behavior using the extracted KPI collections.
---
Nitpick comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 120-127: Update the KPI metadata construction around the
func.__doc__ name fallback to derive name from the first non-empty,
whitespace-trimmed docstring line before removing " KPI.". Preserve the existing
kpi_id-based fallback when no non-empty documentation exists.
In `@projects/caliper/orchestration/postprocess.py`:
- Around line 905-910: Remove the stale CLI command-building note near the
plugin-loading code, and remove the unused status_file creation plus its later
status_file.unlink cleanup block in the surrounding postprocessing function.
Keep the direct load_plugin flow unchanged.
- Around line 534-558: The KPI transformation block should reuse the existing
format helpers instead of manually parsing and writing JSON. Replace the local
JSONL read/write and _transform_kpis_to_hierarchical_format call with
read_kpis_from_file followed by write_kpis_in_format(...,
format_type="hierarchical"), remove the unused local transformation helper, and
delete the nested json imports.
🪄 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: ef32ace2-f228-40f4-bffe-448eeffe6df7
📒 Files selected for processing (8)
projects/caliper/cli/commands.pyprojects/caliper/engine/kpi/format.pyprojects/caliper/orchestration/postprocess.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/guidellm/toolbox/run_guidellm_benchmark/utils.pyprojects/rhaiis/README.mdprojects/rhaiis/orchestration/test_phase.pyprojects/rhaiis/toolbox/deploy_kserve_isvc/main.py
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/guidellm/toolbox/run_guidellm_benchmark/main.py
- projects/guidellm/toolbox/run_guidellm_benchmark/utils.py
Labels that vary across KPIs within the same run_id (e.g. rate_index, intended_concurrency) are now stored on individual KPI records instead of being merged into test-level labels. read_kpis_from_file restores them when flattening, so csv-export produces one row per rate point. Co-authored-by: Cursor <cursoragent@cursor.com>
Member
Author
|
/test fournos rhaiis nvidia |
🟢 Execution of
|
🟢 Submission of
|
run_path was lost during hierarchical→flat conversion, causing all profiles to collapse into the same CSV group key. Now set from run_id. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
thanks Harshith |
|
[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 |
1 similar comment
|
[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
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/caliper/orchestration/postprocess.py`:
- Around line 357-383: Update the label classification logic around
run_label_values and per_kpi_label_keys to count total KPIs and label-key
presence for each run, treating any key whose presence count differs from the
run’s KPI count as per-KPI. Ensure labels present on only some KPIs remain in
each KPI’s metadata instead of being promoted to test-level labels, and add a
regression test covering a label present on only one KPI.
🪄 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: 06609193-4ac0-48fd-9478-75ffcdf9f46f
📒 Files selected for processing (2)
projects/caliper/engine/kpi/format.pyprojects/caliper/orchestration/postprocess.py
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/caliper/engine/kpi/format.py
Comment on lines
+357
to
383
| # First pass: determine which labels are common (same value for all KPIs in a run) | ||
| # vs per-KPI (vary across KPIs, e.g. rate_index, intended_concurrency) | ||
| run_label_values: dict[str, dict[str, set]] = defaultdict(lambda: defaultdict(set)) | ||
| for kpi in kpis: | ||
| run_id = kpi.get("run_id", "unknown") | ||
| for k, v in kpi.get("labels", {}).items(): | ||
| if k == "higher_is_better": | ||
| continue | ||
| run_label_values[run_id][k].add(str(v)) | ||
|
|
||
| # Labels with more than one distinct value per run are per-KPI | ||
| per_kpi_label_keys: dict[str, set[str]] = {} | ||
| for run_id, label_vals in run_label_values.items(): | ||
| per_kpi_label_keys[run_id] = {k for k, vals in label_vals.items() if len(vals) > 1} | ||
|
|
||
| for kpi in kpis: | ||
| run_id = kpi.get("run_id", "unknown") | ||
| test_data = tests_data[run_id] | ||
| varying_keys = per_kpi_label_keys.get(run_id, set()) | ||
|
|
||
| # Extract common labels (excluding KPI-specific ones) | ||
| kpi_labels = kpi.get("labels", {}) | ||
| test_labels = { | ||
| k: v for k, v in kpi_labels.items() if k not in ["higher_is_better"] | ||
| } # Exclude KPI-specific labels | ||
| k: v | ||
| for k, v in kpi_labels.items() | ||
| if k not in ("higher_is_better",) and k not in varying_keys | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Treat missing label keys as varying labels.
Line 362 records values only for present keys. If a label exists on one KPI but is absent on another KPI in the same run_id, Line 370 still classifies it as common. Lines 378-382 then move it to test-level metadata and lose the per-KPI distinction.
Count total KPIs per run and label-key presence per run. Mark a label as per-KPI when the presence counts differ. Add a regression test for a label present on only one KPI.
Proposed fix
run_label_values: dict[str, dict[str, set]] = defaultdict(lambda: defaultdict(set))
+ run_kpi_counts: dict[str, int] = defaultdict(int)
+ label_presence: dict[str, dict[str, int]] = defaultdict(lambda: defaultdict(int))
for kpi in kpis:
run_id = kpi.get("run_id", "unknown")
+ run_kpi_counts[run_id] += 1
for k, v in kpi.get("labels", {}).items():
if k == "higher_is_better":
continue
run_label_values[run_id][k].add(str(v))
+ label_presence[run_id][k] += 1
# Labels with more than one distinct value per run are per-KPI
per_kpi_label_keys: dict[str, set[str]] = {}
for run_id, label_vals in run_label_values.items():
- per_kpi_label_keys[run_id] = {k for k, vals in label_vals.items() if len(vals) > 1}
+ per_kpi_label_keys[run_id] = {
+ k
+ for k, vals in label_vals.items()
+ if len(vals) > 1 or label_presence[run_id][k] != run_kpi_counts[run_id]
+ }📝 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.
Suggested change
| # First pass: determine which labels are common (same value for all KPIs in a run) | |
| # vs per-KPI (vary across KPIs, e.g. rate_index, intended_concurrency) | |
| run_label_values: dict[str, dict[str, set]] = defaultdict(lambda: defaultdict(set)) | |
| for kpi in kpis: | |
| run_id = kpi.get("run_id", "unknown") | |
| for k, v in kpi.get("labels", {}).items(): | |
| if k == "higher_is_better": | |
| continue | |
| run_label_values[run_id][k].add(str(v)) | |
| # Labels with more than one distinct value per run are per-KPI | |
| per_kpi_label_keys: dict[str, set[str]] = {} | |
| for run_id, label_vals in run_label_values.items(): | |
| per_kpi_label_keys[run_id] = {k for k, vals in label_vals.items() if len(vals) > 1} | |
| for kpi in kpis: | |
| run_id = kpi.get("run_id", "unknown") | |
| test_data = tests_data[run_id] | |
| varying_keys = per_kpi_label_keys.get(run_id, set()) | |
| # Extract common labels (excluding KPI-specific ones) | |
| kpi_labels = kpi.get("labels", {}) | |
| test_labels = { | |
| k: v for k, v in kpi_labels.items() if k not in ["higher_is_better"] | |
| } # Exclude KPI-specific labels | |
| k: v | |
| for k, v in kpi_labels.items() | |
| if k not in ("higher_is_better",) and k not in varying_keys | |
| } | |
| # First pass: determine which labels are common (same value for all KPIs in a run) | |
| # vs per-KPI (vary across KPIs, e.g. rate_index, intended_concurrency) | |
| run_label_values: dict[str, dict[str, set]] = defaultdict(lambda: defaultdict(set)) | |
| run_kpi_counts: dict[str, int] = defaultdict(int) | |
| label_presence: dict[str, dict[str, int]] = defaultdict(lambda: defaultdict(int)) | |
| for kpi in kpis: | |
| run_id = kpi.get("run_id", "unknown") | |
| run_kpi_counts[run_id] += 1 | |
| for k, v in kpi.get("labels", {}).items(): | |
| if k == "higher_is_better": | |
| continue | |
| run_label_values[run_id][k].add(str(v)) | |
| label_presence[run_id][k] += 1 | |
| # Labels with more than one distinct value per run are per-KPI | |
| per_kpi_label_keys: dict[str, set[str]] = {} | |
| for run_id, label_vals in run_label_values.items(): | |
| per_kpi_label_keys[run_id] = { | |
| k | |
| for k, vals in label_vals.items() | |
| if len(vals) > 1 or label_presence[run_id][k] != run_kpi_counts[run_id] | |
| } | |
| for kpi in kpis: | |
| run_id = kpi.get("run_id", "unknown") | |
| test_data = tests_data[run_id] | |
| varying_keys = per_kpi_label_keys.get(run_id, set()) | |
| kpi_labels = kpi.get("labels", {}) | |
| test_labels = { | |
| k: v | |
| for k, v in kpi_labels.items() | |
| if k not in ("higher_is_better",) and k not in varying_keys | |
| } |
🤖 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/caliper/orchestration/postprocess.py` around lines 357 - 383, Update
the label classification logic around run_label_values and per_kpi_label_keys to
count total KPIs and label-key presence for each run, treating any key whose
presence count differs from the run’s KPI count as per-KPI. Ensure labels
present on only some KPIs remain in each KPI’s metadata instead of being
promoted to test-level labels, and add a regression test covering a label
present on only one KPI.
3 tasks
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 support for SGLang and TRT-LLM inference engines alongside vLLM, replacing all engine-specific references with a generic abstraction. This brings Forge RHAIIS to feature parity with model-furnace for multi-engine benchmarking.
Engine abstraction
vllm_args/vllm_imageconfig keys with genericengine_argsand per-engine config underrhaiis.engines.{vllm,sglang,trtllm}tensor-parallel-size→tp-sizefor SGLang,tp_sizefor TRT-LLM) so models can use a singleengine_argsblock--engine={vllm,sglang,trtllm}to select the runtimeManifest generation refactor
orchestration/manifests.pydeploy_kserve_isvcis now engine-agnostic — accepts pre-built manifest dictstrtllm_configJSON supportOpenShift compatibility
runAsUser: 0from vLLM/SGLang (not needed, rejected by restricted SCC)USERenv var to avoidgetpwuidcrash with OpenShift's arbitrary UIDsfsGroupopt-in viabenchmarks.guidellm.fs_groupconfig key (disabled by default)for clusters where the CSI driver provisions root-owned PVCs
runAsUser: 0(requiresanyuidSCC)Dashboard & post-processing fixes
trtllm_configentries in dashboardruntime_argsfieldfor
csv-exportOther improvements
image_pull_secret(string) withimage_pull_secrets(list) for multi-secret supporttrtllm_configdefaults (mamba SSM, chunked prefill, MoE) to base config for FournosJob overridessglang serveentrypoint instead of deprecatedpython -m sglang.launch_serverSummary by CodeRabbit
New Features
Improvements
Bug Fixes