refactor: Share GuideLLM dashboard postprocessing - #162
Conversation
|
[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 |
📝 WalkthroughWalkthroughThe PR adds shared GuideLLM dashboard parsing, metadata, KPI, and CSV helpers. It integrates them into llm-d and RHAIIS post-processing, updates orchestration labels and outputs, extends LLMInferenceService parsing, and fixes hierarchical KPI label merging. ChangesDashboard KPI integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GuideLLMParser
participant LlmDGuideLLMPlugin
participant dashboard_helpers
participant dashboard_csv
GuideLLMParser->>LlmDGuideLLMPlugin: parse benchmark and service artifacts
LlmDGuideLLMPlugin->>dashboard_helpers: enrich records and compute KPIs
dashboard_helpers->>dashboard_csv: pivot KPI records and convert units
dashboard_csv->>LlmDGuideLLMPlugin: write dashboard.csv
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
projects/guidellm/postprocess/guidellm/parsing/parsers.py (3)
132-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the docstring for the new return fields.
Line 139 states that the function returns
product_version,deployment_profile, andmodel_name. The function now also returnsreplicas,tensor_parallel_size,router_config,image_tag, andruntime_args. Line 133 and line 136 also state "YAML" only, although the function now accepts a.jsonartifact.♻️ Proposed update
""" - Extract multiple fields from LLMInferenceService YAML file. + Extract multiple fields from an LLMInferenceService YAML or JSON file. Args: - file_path: Path to llminferenceservice.yaml file + file_path: Path to the llminferenceservice.yaml, .yml, or .json file Returns: - Dictionary with extracted fields (product_version, deployment_profile, model_name) + Dictionary with extracted fields: product_version, deployment_profile, + model_name, replicas, tensor_parallel_size, router_config, image_tag, + and runtime_args. Absent fields are omitted. """🤖 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/postprocess/guidellm/parsing/parsers.py` around lines 132 - 140, Update the docstring for the relevant YAML/JSON parsing function to describe both YAML and JSON artifact inputs, and expand the Returns section to include replicas, tensor_parallel_size, router_config, image_tag, and runtime_args alongside the existing fields.
580-583: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a deterministic tiebreaker to the sort.
The sort places capture-state paths first, which matches the first-file-wins merge at lines 618-622. Python's sort is stable, so files inside each group keep the order of
node.artifact_paths. If that list comes from a directory scan, the order can vary between runs, and the extracted metadata can then vary too.Add the path as a secondary key.
♻️ Proposed change
- llmisvc_files.sort(key=lambda path: "__capture_llmisvc_state" not in str(path)) + llmisvc_files.sort( + key=lambda path: ("__capture_llmisvc_state" not in str(path), str(path)) + )🤖 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/postprocess/guidellm/parsing/parsers.py` around lines 580 - 583, Update the llmisvc_files sorting in the parser to retain capture-state paths first while using each path itself as a deterministic secondary sort key. Ensure the resulting order no longer depends on the original node.artifact_paths traversal order before the first-file-wins merge.
118-124: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winParse
llminferenceservice.jsonwithjson.load, notyaml.safe_load.
llminferenceservice.jsonis produced byoc get llminferenceservice -ojson, and a Kubernetes JSON output can use literal strings such asyes/noin annotation values. YAML 1.1 interprets those literals as booleans, so the.jsonartifact can change the extractedproduct_version/deployment_profile; updateextract_fields_from_llmisvcto choosejson.loadforllminferenceservice.jsonand keepyaml.safe_loadfor YAML artifacts.🤖 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/postprocess/guidellm/parsing/parsers.py` around lines 118 - 124, Update extract_fields_from_llmisvc to branch on the artifact name before loading content: use json.load for llminferenceservice.json and keep yaml.safe_load for llminferenceservice.yaml and llminferenceservice.yml. Anchor the change in the existing _is_llmisvc_artifact helper and the parsing logic in extract_fields_from_llmisvc so the JSON path preserves literal annotation values and does not run through the YAML parser.projects/guidellm/postprocess/guidellm/dashboard.py (1)
71-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
request_latency_exclusion.
SECONDS_TO_MS_COLUMNSselects every metric with unit"s"except therequest_latency_*columns. The reason is not stated. A reader can interpret the exclusion as an oversight, because those metrics carry the same"s"unit. Add a short comment that states the dashboard expectsrequest_latency_*in seconds.♻️ Proposed comment
+# Dashboard latency columns are milliseconds, except request_latency_*, which +# the dashboard expects in seconds. SECONDS_TO_MS_COLUMNS = frozenset( column for _, _, column, unit, _ in DASHBOARD_METRICS if unit == "s" and not column.startswith("request_latency_") )🤖 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/postprocess/guidellm/dashboard.py` around lines 71 - 75, Add a short explanatory comment immediately above SECONDS_TO_MS_COLUMNS stating that the dashboard expects request_latency_* metrics to remain in seconds, documenting why they are excluded despite having unit "s".projects/caliper/tests/test_kpi_format.py (1)
6-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a varying label.
The test locks the merge half of the contract. It does not lock the other half: a label with more than one distinct value in the same run must stay per-KPI and must not reach
output["tests"][0]["labels"]. Theupdatecall atprojects/caliper/engine/kpi/format.pyline 69 relies on the first pass to exclude those keys. A regression in the first pass would leak a varying label to test level, and this test would still pass.💚 Proposed additional test
def test_hierarchical_format_keeps_varying_labels_per_kpi(): kpis = [ { "run_id": "run-1", "kpi_id": "dashboard_ttft_median", "value": 1, "labels": {"model": "llama", "rate_index": "0"}, }, { "run_id": "run-1", "kpi_id": "dashboard_ttft_median", "value": 2, "labels": {"model": "llama", "rate_index": "1"}, }, ] model = type("Model", (), {"plugin_module": "missing.plugin"})() output = transform_kpis_to_hierarchical_format(kpis, model) test = output["tests"][0] assert test["labels"] == {"model": "llama"} assert [kpi["labels"]["rate_index"] for kpi in test["kpis"]] == ["0", "1"]🤖 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/tests/test_kpi_format.py` around lines 6 - 28, Extend the hierarchical KPI formatting tests with a varying-label case in test_hierarchical_format_keeps_varying_labels_per_kpi: use KPIs from the same run where rate_index has different values, assert output["tests"][0]["labels"] contains only the common model label, and verify each KPI retains its own rate_index label.
🤖 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/postprocess/guidellm/dashboard.py`:
- Around line 200-207: Harden _extract_dashboard_metrics against malformed
benchmark artifacts: at
projects/guidellm/postprocess/guidellm/dashboard.py#L200-L207, skip decoded
payloads that are not dictionaries; at `#L211-L218`, normalize an explicit null
mean to 0 before float conversion; and at `#L247-L253`, catch invalid
prompt_tokens/output_tokens coercion and skip or safely handle that record.
Preserve processing of valid files and metrics.
- Around line 211-218: The benchmark sorting logic in dashboard.py’s
benchmarks.sort key currently calls float() on the nested mean value, which
breaks when a benchmark reports null instead of a number. Update the sort key
path to coerce the extracted mean through a helper or inline fallback that
treats None as 0 before converting to float, while preserving the existing
nested metrics lookup and sort behavior for valid numeric means.
- Around line 254-256: Update _extract_dashboard_metrics to derive and store the
request-rate axis while iterating through benchmarks, alongside curves and
run_uuids. Change compute_dashboard_kpis to use this stored axis instead of
indexing request_rate from GuideLLMParser._create_aggregated_metrics, ensuring
skipped parser benchmarks cannot misalign KPI rate points.
- Around line 418-430: Update the group ordering in the rows-building loop
around groups and sorted(groups) so the second key element, rate_index, is
compared numerically rather than lexicographically. Preserve run_path as the
primary sort key and keep the existing row generation behavior unchanged.
In `@projects/guidellm/postprocess/guidellm/parsing/parsers.py`:
- Around line 173-179: Update the consumer condition around the
result-processing logic at line 621 to check whether field_value is not None
instead of using truthiness, preserving zero-valued replicas and
tensor_parallel_size entries extracted by the parser.
- Around line 151-155: Update the logging in the product-version extraction
block to report the normalized value assigned to result["product_version"],
rather than the raw product_version returned by
parse_product_version_from_annotation. Keep the existing extraction and storage
behavior unchanged.
- Around line 185-196: Update the serving-container extraction in the parser to
select the container with the intended serving name, falling back to index 0
when no name matches, before deriving image_tag and runtime_args. In the env
iteration, only access name and value fields for entries that are mappings,
while preserving the existing VLLM_ADDITIONAL_ARGS behavior.
In `@projects/llm_d/postprocess/plugin.py`:
- Around line 203-205: Restrict exported profile metadata to an explicit
non-sensitive allowlist in extract_kpi_labels_from_config() and
LlmDGuideLLMPlugin; remove runtime_args, env, and arbitrary
vllm_extra.args-derived values before writing KPI labels or CSV metadata. Update
projects/llm_d/postprocess/plugin.py at lines 203-205 and
projects/llm_d/orchestration/test_phase.py at lines 173-174, ensuring only
approved router_config or runtime metadata is emitted.
---
Nitpick comments:
In `@projects/caliper/tests/test_kpi_format.py`:
- Around line 6-28: Extend the hierarchical KPI formatting tests with a
varying-label case in test_hierarchical_format_keeps_varying_labels_per_kpi: use
KPIs from the same run where rate_index has different values, assert
output["tests"][0]["labels"] contains only the common model label, and verify
each KPI retains its own rate_index label.
In `@projects/guidellm/postprocess/guidellm/dashboard.py`:
- Around line 71-75: Add a short explanatory comment immediately above
SECONDS_TO_MS_COLUMNS stating that the dashboard expects request_latency_*
metrics to remain in seconds, documenting why they are excluded despite having
unit "s".
In `@projects/guidellm/postprocess/guidellm/parsing/parsers.py`:
- Around line 132-140: Update the docstring for the relevant YAML/JSON parsing
function to describe both YAML and JSON artifact inputs, and expand the Returns
section to include replicas, tensor_parallel_size, router_config, image_tag, and
runtime_args alongside the existing fields.
- Around line 580-583: Update the llmisvc_files sorting in the parser to retain
capture-state paths first while using each path itself as a deterministic
secondary sort key. Ensure the resulting order no longer depends on the original
node.artifact_paths traversal order before the first-file-wins merge.
- Around line 118-124: Update extract_fields_from_llmisvc to branch on the
artifact name before loading content: use json.load for llminferenceservice.json
and keep yaml.safe_load for llminferenceservice.yaml and
llminferenceservice.yml. Anchor the change in the existing _is_llmisvc_artifact
helper and the parsing logic in extract_fields_from_llmisvc so the JSON path
preserves literal annotation values and does not run through the YAML parser.
🪄 Autofix
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: 953b5427-265b-423c-9d03-42f096168425
📒 Files selected for processing (17)
projects/caliper/engine/kpi/format.pyprojects/caliper/tests/test_kpi_format.pyprojects/guidellm/postprocess/guidellm/dashboard.pyprojects/guidellm/postprocess/guidellm/parsing/parsers.pyprojects/llm_d/orchestration/config.d/cpt.yamlprojects/llm_d/orchestration/config.yamlprojects/llm_d/orchestration/presets.d/cks.yamlprojects/llm_d/orchestration/presets.d/cpt.yamlprojects/llm_d/orchestration/presets.d/rhoai-rc.yamlprojects/llm_d/orchestration/test_phase.pyprojects/llm_d/postprocess/__init__.pyprojects/llm_d/postprocess/plugin.pyprojects/llm_d/tests/test_postprocess_csv.pyprojects/llm_d/tests/test_profiles.pyprojects/rhaiis/postprocess/kpis.pyprojects/rhaiis/postprocess/parser.pyprojects/rhaiis/postprocess/plugin.py
| for path in files: | ||
| try: | ||
| payload = json.loads(path.read_text(encoding="utf-8")) | ||
| except (json.JSONDecodeError, OSError): | ||
| continue | ||
| benchmarks.extend(payload.get("benchmarks", [])) | ||
| metadata = metadata or payload.get("metadata", {}) | ||
| args = args or payload.get("args", {}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_extract_dashboard_metrics trusts the shape of every value it reads from benchmarks.json. The function catches json.JSONDecodeError and OSError at line 203, so it intends to tolerate an unusable artifact file. It then dereferences and coerces the decoded content without checking the shape. Each of the three sites below raises an uncaught exception that aborts the whole postprocess run instead of skipping the bad input.
projects/guidellm/postprocess/guidellm/dashboard.py#L200-L207: after the decode, skip the file whenpayloadis not adict. A valid JSON array or scalar currently makespayload.getraiseAttributeError.projects/guidellm/postprocess/guidellm/dashboard.py#L211-L218: treat an explicit"mean": nullas0in the sort key..get("mean", 0)returnsNonefor a null value, andfloat(None)raisesTypeError.projects/guidellm/postprocess/guidellm/dashboard.py#L247-L253: wrap theint(float(...))coercion ofprompt_tokensandoutput_tokens. The regex at line 240 can yield1.2.3, and a JSONdataobject can yieldnullor a non-numeric string.
📍 Affects 1 file
projects/guidellm/postprocess/guidellm/dashboard.py#L200-L207(this comment)projects/guidellm/postprocess/guidellm/dashboard.py#L211-L218projects/guidellm/postprocess/guidellm/dashboard.py#L247-L253
🤖 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/postprocess/guidellm/dashboard.py` around lines 200 - 207,
Harden _extract_dashboard_metrics against malformed benchmark artifacts: at
projects/guidellm/postprocess/guidellm/dashboard.py#L200-L207, skip decoded
payloads that are not dictionaries; at `#L211-L218`, normalize an explicit null
mean to 0 before float conversion; and at `#L247-L253`, catch invalid
prompt_tokens/output_tokens coercion and skip or safely handle that record.
Preserve processing of valid files and metrics.
| benchmarks.sort( | ||
| key=lambda benchmark: float( | ||
| benchmark.get("metrics", {}) | ||
| .get("requests_per_second", {}) | ||
| .get("successful", {}) | ||
| .get("mean", 0) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
float() fails when a benchmark reports a null mean.
The sort key calls float(...) on .get("mean", 0). The default 0 applies only when the key is absent. If a benchmark artifact contains "mean": null, the call receives None and raises TypeError. A failed or aborted benchmark rate can produce that shape.
Coerce through a helper that treats None as 0.
🐛 Proposed fix
benchmarks.sort(
key=lambda benchmark: float(
benchmark.get("metrics", {})
.get("requests_per_second", {})
.get("successful", {})
.get("mean", 0)
+ or 0
)
)📝 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.
| benchmarks.sort( | |
| key=lambda benchmark: float( | |
| benchmark.get("metrics", {}) | |
| .get("requests_per_second", {}) | |
| .get("successful", {}) | |
| .get("mean", 0) | |
| ) | |
| ) | |
| benchmarks.sort( | |
| key=lambda benchmark: float( | |
| benchmark.get("metrics", {}) | |
| .get("requests_per_second", {}) | |
| .get("successful", {}) | |
| .get("mean", 0) | |
| or 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/guidellm/postprocess/guidellm/dashboard.py` around lines 211 - 218,
The benchmark sorting logic in dashboard.py’s benchmarks.sort key currently
calls float() on the nested mean value, which breaks when a benchmark reports
null instead of a number. Update the sort key path to coerce the extracted mean
through a helper or inline fallback that treats None as 0 before converting to
float, while preserving the existing nested metrics lookup and sort behavior for
valid numeric means.
| curves = {curve_key: [] for _, curve_key, _, _, _ in DASHBOARD_METRICS} | ||
| run_uuids: list[str] = [] | ||
| for benchmark in benchmarks: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how request_rate and the dashboard curves are produced and consumed.
fd -t f 'parsers.py' projects/guidellm | while IFS= read -r f; do
rg -n -C 4 'request_rate|_create_aggregated_metrics|performance_curves' "$f"
done
rg -n -C 4 'request_rate|performance_curves' projects/guidellm/postprocess/guidellm/dashboard.pyRepository: openshift-psap/forge
Length of output: 5611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- dashboard.py relevant range ---\n'
sed -n '150,420p' projects/guidellm/postprocess/guidellm/dashboard.py
printf '\n--- parser.py relevant range ---\n'
sed -n '230,285p' projects/guidellm/postprocess/guidellm/parsing/parsers.pyRepository: openshift-psap/forge
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dashboard.py relevant range ---'
sed -n '150,420p' projects/guidellm/postprocess/guidellm/dashboard.py
printf '%s\n' ''
printf '%s\n' '--- parser.py relevant range ---'
sed -n '230,285p' projects/guidellm/postprocess/guidellm/parsing/parsers.pyRepository: openshift-psap/forge
Length of output: 14857
Keep the request-rate axis in one parse pass.
_extract_dashboard_metrics builds the dashboard curves from the raw benchmarks*.json files, while compute_dashboard_kpis indexes those curves from request_rate produced by GuideLLMParser._create_aggregated_metrics. Since parser failures skip individual benchmarks, request_rate can be shorter than the curves, and KPIs can be emitted with mismatched rate points. Derive and store the rate axis in _extract_dashboard_metrics so the same pass owns the index.
🤖 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/postprocess/guidellm/dashboard.py` around lines 254 - 256,
Update _extract_dashboard_metrics to derive and store the request-rate axis
while iterating through benchmarks, alongside curves and run_uuids. Change
compute_dashboard_kpis to use this stored axis instead of indexing request_rate
from GuideLLMParser._create_aggregated_metrics, ensuring skipped parser
benchmarks cannot misalign KPI rate points.
| groups: dict[tuple[str, str], dict[str, Any]] = {} | ||
| labels_by_group: dict[tuple[str, str], dict[str, Any]] = {} | ||
| for kpi in kpi_records: | ||
| labels = kpi.get("labels", {}) | ||
| key = (str(kpi.get("run_path", "")), str(labels.get("rate_index", "0"))) | ||
| column = kpi_to_column.get(kpi.get("kpi_id", "")) | ||
| if column: | ||
| groups.setdefault(key, {})[column] = kpi.get("value") | ||
| if key not in labels_by_group or len(labels) > len(labels_by_group[key]): | ||
| labels_by_group[key] = labels | ||
|
|
||
| rows: list[dict[str, Any]] = [] | ||
| for key in sorted(groups): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rows sort by rate_index as text, not as a number.
Line 422 stores rate_index as a string. Line 430 sorts the group keys with sorted(groups), so the comparison is lexicographic. A run with 10 or more rate points emits rows in the order 0, 1, 10, 11, 2, 3, .... The dashboard CSV then presents the rate sweep out of order.
Sort with a numeric key.
🐛 Proposed fix
+ def sort_key(key: tuple[str, str]) -> tuple[str, float, str]:
+ run_path, rate_index = key
+ try:
+ return (run_path, float(rate_index), rate_index)
+ except ValueError:
+ return (run_path, float("inf"), rate_index)
+
rows: list[dict[str, Any]] = []
- for key in sorted(groups):
+ for key in sorted(groups, key=sort_key):📝 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.
| groups: dict[tuple[str, str], dict[str, Any]] = {} | |
| labels_by_group: dict[tuple[str, str], dict[str, Any]] = {} | |
| for kpi in kpi_records: | |
| labels = kpi.get("labels", {}) | |
| key = (str(kpi.get("run_path", "")), str(labels.get("rate_index", "0"))) | |
| column = kpi_to_column.get(kpi.get("kpi_id", "")) | |
| if column: | |
| groups.setdefault(key, {})[column] = kpi.get("value") | |
| if key not in labels_by_group or len(labels) > len(labels_by_group[key]): | |
| labels_by_group[key] = labels | |
| rows: list[dict[str, Any]] = [] | |
| for key in sorted(groups): | |
| groups: dict[tuple[str, str], dict[str, Any]] = {} | |
| labels_by_group: dict[tuple[str, str], dict[str, Any]] = {} | |
| for kpi in kpi_records: | |
| labels = kpi.get("labels", {}) | |
| key = (str(kpi.get("run_path", "")), str(labels.get("rate_index", "0"))) | |
| column = kpi_to_column.get(kpi.get("kpi_id", "")) | |
| if column: | |
| groups.setdefault(key, {})[column] = kpi.get("value") | |
| if key not in labels_by_group or len(labels) > len(labels_by_group[key]): | |
| labels_by_group[key] = labels | |
| def sort_key(key: tuple[str, str]) -> tuple[str, float, str]: | |
| run_path, rate_index = key | |
| try: | |
| return (run_path, float(rate_index), rate_index) | |
| except ValueError: | |
| return (run_path, float("inf"), rate_index) | |
| rows: list[dict[str, Any]] = [] | |
| for key in sorted(groups, key=sort_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/guidellm/postprocess/guidellm/dashboard.py` around lines 418 - 430,
Update the group ordering in the rows-building loop around groups and
sorted(groups) so the second key element, rate_index, is compared numerically
rather than lexicographically. Preserve run_path as the primary sort key and
keep the existing row generation behavior unchanged.
| if annotation_value: | ||
| product_version = parse_product_version_from_annotation(annotation_value) | ||
| if product_version: | ||
| result["product_version"] = product_version | ||
| result["product_version"] = normalize_product_version(product_version) | ||
| logging.info(f"Extracted product_version '{product_version}' from {file_path}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the stored value, not the raw value.
Line 154 stores the normalized version. Line 155 logs the raw product_version. The two differ whenever normalization applies, for example v3.5.0-ea.2 becomes RHOAI-3.5-EA2. A reader who debugs a dashboard version mismatch cannot see the stored value in the log.
🔎 Proposed fix
if product_version:
- result["product_version"] = normalize_product_version(product_version)
- logging.info(f"Extracted product_version '{product_version}' from {file_path}")
+ result["product_version"] = normalize_product_version(product_version)
+ logging.info(
+ f"Extracted product_version '{product_version}' "
+ f"(normalized to '{result['product_version']}') from {file_path}"
+ )📝 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 annotation_value: | |
| product_version = parse_product_version_from_annotation(annotation_value) | |
| if product_version: | |
| result["product_version"] = product_version | |
| result["product_version"] = normalize_product_version(product_version) | |
| logging.info(f"Extracted product_version '{product_version}' from {file_path}") | |
| if annotation_value: | |
| product_version = parse_product_version_from_annotation(annotation_value) | |
| if product_version: | |
| result["product_version"] = normalize_product_version(product_version) | |
| logging.info( | |
| f"Extracted product_version '{product_version}' " | |
| f"(normalized to '{result['product_version']}') from {file_path}" | |
| ) |
🤖 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/postprocess/guidellm/parsing/parsers.py` around lines 151 -
155, Update the logging in the product-version extraction block to report the
normalized value assigned to result["product_version"], rather than the raw
product_version returned by parse_product_version_from_annotation. Keep the
existing extraction and storage behavior unchanged.
| replicas = extract_field_by_jsonpath(yaml_data, "spec.replicas") | ||
| if replicas is not None: | ||
| result["replicas"] = replicas | ||
|
|
||
| tensor_parallel_size = extract_field_by_jsonpath(yaml_data, "spec.parallelism.tensor") | ||
| if tensor_parallel_size is not None: | ||
| result["tensor_parallel_size"] = tensor_parallel_size |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A zero value is preserved here and dropped by the consumer.
Lines 174 and 178 use is not None, so replicas: 0 and tensor_parallel_size: 0 enter result. The consumer at line 621 applies if field_value and field_name not in metrics, which discards every falsy value. A scaled-to-zero deployment therefore reports no replicas at all, and the dashboard CSV shows an empty cell instead of 0.
Align the two checks. Change line 621 to test for None.
🐛 Proposed fix at line 620-622
for field_name, field_value in llmisvc_fields.items():
- if field_value and field_name not in metrics:
+ if field_value is not None and field_name not in metrics:
metrics[field_name] = field_value🤖 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/postprocess/guidellm/parsing/parsers.py` around lines 173 -
179, Update the consumer condition around the result-processing logic at line
621 to check whether field_value is not None instead of using truthiness,
preserving zero-valued replicas and tensor_parallel_size entries extracted by
the parser.
| serving_container = extract_field_by_jsonpath( | ||
| yaml_data, "spec.template.containers[0]", {} | ||
| ) | ||
| if isinstance(serving_container, dict): | ||
| image = serving_container.get("image") | ||
| if image: | ||
| result["image_tag"] = image | ||
| for env_var in serving_container.get("env", []): | ||
| if env_var.get("name") == "VLLM_ADDITIONAL_ARGS": | ||
| result["runtime_args"] = env_var.get("value", "") | ||
| break | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the signature of extract_field_by_jsonpath and its default-argument support.
rg -n -C 8 'def extract_field_by_jsonpath' --type=pyRepository: openshift-psap/forge
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo files matching parsers.py:"
fd -a 'parsers.py$' . | sed 's#^\./##'
echo
echo "target context:"
if [ -f projects/guidellm/postprocess/guidellm/parsing/parsers.py ]; then
wc -l projects/guidellm/postprocess/guidellm/parsing/parsers.py
sed -n '1,240p' projects/guidellm/postprocess/guidellm/parsing/parsers.py | cat -n
fi
echo
echo "search extract_field_by_jsonpath:"
rg -n -C 10 'extract_field_by_jsonpath|def .*parse|YAML|yaml_data|container' --type=py .Repository: openshift-psap/forge
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Standalone behavioral probe for the env loop shape concern only.
def bad_loop(env):
for env_var in env or []:
if env_var.get("name") == "VLLM_ADDITIONAL_ARGS":
return env_var.get("value", "")
return None
for value in [None, ["x"], [{"name": "A", "value": "old"}, 42], [{"name": "VLLM_ADDITIONAL_ARGS", "value": "ok"}, 42]]:
try:
print(repr(value), "=>", bad_loop(value))
except Exception as e:
print(repr(value), "raises", type(e).__name__, str(e))
PYRepository: openshift-psap/forge
Length of output: 389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
import re
target = Path("projects/guidellm/postprocess/guidellm/parsing/parsers.py")
text = target.read_text()
# Extract function bodies for references named extract_field_by_jsonpath, jsonpath, and the target block.
tree = ast.parse(text, filename=str(target))
print("target file imports and relevant definitions:")
for node in ast.walk(tree):
if isinstance(node, (ast.Import, ast.ImportFrom)) and any(
any("_jsonpath" in name.name or "jsonpath" in name.name for name in node.names)
or (isinstance(node, ast.ImportFrom) and any("jsonpath" in p for p in node.module.split(".")))
for _ in [1]
) or isinstance(node, ast.FunctionDef) and "jsonpath" in node.name.lower():
print("-", getattr(node, "name", node.module) or "<import>")
print("\nextract_field_by_jsonpath occurrences:")
for i, line in enumerate(text.splitlines(), 1):
if "extract_field_by_jsonpath" in line:
print(f"{target}:{i}:{line}")
print("\njsonpath occurrences:")
for i, line in enumerate(text.splitlines(), 1):
if "jsonpath" in line:
print(f"{target}:{i}:{line}")
print("\nextract target block if present:")
for i in range(170, 205):
try:
print(f"{target}:{i}:{text.splitlines()[i-1]}")
except IndexError:
pass
PY
echo
echo "Dependency declarations:"
for f in pyproject.toml poetry.lock requirements.txt setup.py setup.cfg requirements*.txt projects/guidellm/pyproject.toml; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
done
echo
echo "Check pyyaml/jsonpath availability and library defaults:"
python3 - <<'PY'
try:
import jsonpath_ng
print("jsonpath_ng", getattr(jsonpath_ng, "__version__", "unknown"), "available")
try:
from jsonpath_ng.ext import parse
expr = parse("containers[0]")
print(parse("containers[0]").find([{"containers":[{"name":"sidecar"}]}])[:5])
print(parse("containers[*].?(@.name==\"serving\")", jsonpath_ng.ext).find([{"containers":[{"name":"sidecar"},{"name":"serving"}]}])[:5])
except Exception as e:
print("jsonpath_ng.ext behavior failed:", type(e).__name__, e)
except Exception as e:
print("jsonpath_ng unavailable:", type(e).__name__, e)
try:
import yaml
print("yaml", getattr(yaml, "__version__", "unknown"), "available")
except Exception as e:
print("yaml unavailable:", type(e).__name__, e)
PYRepository: openshift-psap/forge
Length of output: 336
Select the serving container by name, and guard non-mapping env entries.
If spec.template.containers[0] is a sidecar, image_tag and runtime_args come from the wrong container. Select the intended container by name, with an index-0 fallback if no name match is found.
If env contains a non-mapping entry, env_var.get("name") raises before the parser cleanup path. Check isinstance(env_var, dict) before reading env-field values.
🐛 Proposed fix for the env loop
- for env_var in serving_container.get("env", []):
- if env_var.get("name") == "VLLM_ADDITIONAL_ARGS":
- result["runtime_args"] = env_var.get("value", "")
- break
+ for env_var in serving_container.get("env") or []:
+ if isinstance(env_var, dict) and env_var.get("name") == "VLLM_ADDITIONAL_ARGS":
+ result["runtime_args"] = env_var.get("value", "")
+ break🤖 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/postprocess/guidellm/parsing/parsers.py` around lines 185 -
196, Update the serving-container extraction in the parser to select the
container with the intended serving name, falling back to index 0 when no name
matches, before deriving image_tag and runtime_args. In the env iteration, only
access name and value fields for entries that are mappings, while preserving the
existing VLLM_ADDITIONAL_ARGS behavior.
| vllm_args = profile.get("vllm_extra", {}).get("args", {}) | ||
| if vllm_args: | ||
| metadata["runtime_args"] = " ".join(_build_vllm_args(vllm_args)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)projects/(llm_d|guidellm|rh_aiis|caliper)' | sed -n '1,200p'
echo "== plugin outline =="
ast-grep outline projects/llm_d/postprocess/plugin.py --view expanded || true
echo "== relevant plugin sections =="
sed -n '1,260p' projects/llm_d/postprocess/plugin.py
echo "== orchestration relevant sections =="
sed -n '1,240p' projects/llm_d/orchestration/runtime_config.py
sed -n '1,230p' projects/llm_d/orchestration/test_phase.py
sed -n '190,245p' projects/llm_d/orchestration/render_inference_service.py
echo "== tests around kpis/csv =="
sed -n '1,280p' projects/llm_d/tests/test_postprocess_csv.py
echo "== search deployment_profile/profile_name metadata path =="
rg -n "deployment_metadata_from_profile|deployment_profile|router_config|runtime_args|kpi_labels|compute_kpis|export_kpis_to_csv" projects/llm_d projects/guidellm projects/rhaiis -SRepository: openshift-psap/forge
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== export s3/export related paths =="
rg -n "s3|csv_export|export.*csv|KPI|kpi|dashboard|artifact|__test_labels__|kpi_labels|deployment_metadata_from_profile|extract_kpi_labels_from_config" projects/caliper projects/llm_d projects/guidellm projects/rhaiis -S --max-count 200
echo "== runtime config full get_deployment_profile slices =="
rg -n "def get_deployment_profile|def get_deployment_profile_name|deployments|get_config\\(\"runtime|kpi" projects/llm_d/orchestration -S -A 12 -B 4
echo "== test kpi_labels fixture/usage =="
rg -n "\"kpi_labels\"|kpi_labels=|extract_kpi_labels_from_config|deployment_profile" projects/llm_d tests projects/llm_d/tests -S --max-count 120
python3 - <<'PY'
from pathlib import Path
import ast
import re
paths = [
Path("projects/llm_d/orchestration/runtime_config.py"),
Path("projects/llm_d/orchestration/test_phase.py"),
Path("projects/llm_d/postprocess/plugin.py"),
Path("projects/guidellm/postprocess/guidellm/dashboard.py"),
]
for p in paths:
text = p.read_text()
mod = ast.parse(text)
for node in ast.walk(mod):
if isinstance(node, ast.FunctionDef) and node.name in (
"get_deployment_profile",
"get_deployment_profile_name",
"extract_kpi_labels_from_config",
"_extract_deployment_metadata",
"deployment_metadata_from_profile",
"compute_dashboard_kpis",
"metadata_row",
):
start = node.lineno
end = node.end_lineno
src = ast.get_source_segment(text, node)
print(f"\n--- {p}:{start}-{end} --- {node.name} ---")
for i,line in enumerate(text.splitlines()[start-1:end], start):
print(f"{i:5}: {line}")
PY
echo "== config.d deployment snippets =="
for f in projects/llm_d/orchestration/config.d/*.yaml projects/llm_d/orchestration/presets.d/*.yaml; do
[ -f "$f" ] || continue
echo "--- $f ---"
rg -n "vllm_extra|scheduler|router|tensor_parallelism|replicas|kpi|deployment_profile" "$f" -S --max-count 80 || true
doneRepository: openshift-psap/forge
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime_config deployment helpers =="
sed -n '80,130p' projects/llm_d/orchestration/runtime_config.py
sed -n '600,645p' projects/llm_d/orchestration/runtime_config.py
echo "== test_phase kpi_labels function =="
sed -n '158,182p' projects/llm_d/orchestration/test_phase.py
echo "== create_test_labels around kpi_labels write =="
sed -n '182,215p' projects/llm_d/orchestration/test_phase.py
echo "== deployment config files around vllm_extra/env =="
for f in projects/llm_d/orchestration/config.d/deployments.yaml projects/llm_d/orchestration/config.yaml projects/llm_d/orchestration/presets.d/rhoai-rc.yaml; do
[ -f "$f" ] || continue
echo "--- $f ---"
rg -n "vllm_extra|env:|VLLM|scheduler:|replicas|tensor_parallelism|deployment_profile" "$f" -S -A 10 -B 4 || true
done
echo "== parser path for llmisvc env/runtime_args =="
sed -n '190,255p' projects/guidellm/postprocess/guidellm/parsing/parsers.py
echo "== static semantic probe for label and csv metadata flow =="
python3 - <<'PY'
from pathlib import Path
import ast
files = [
Path("projects/llm_d/orchestration/runtime_config.py"),
Path("projects/llm_d/orchestration/test_phase.py"),
Path("projects/llm_d/postprocess/plugin.py"),
]
for file in files:
text = file.read_text()
tree = ast.parse(text)
print(f"\n--- {file} ---")
names = []
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef):
names.append((node.name, node.lineno, node.end_lineno))
for name, start, end in names:
src = ast.get_source_segment(text, next(n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef) and n.name == name))
if any(key in (name,) or any((key in src) for key in ("test_phase", "runtime_config", "plugin"))) for key in ["extract_kpi_labels_from_config", "create_test_labels", "_extract_deployment_metadata"]):
continue
for name, start, end in names:
src = ast.get_source_segment(text, next(n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef) and n.name == name))
print(f"\n## {name} ({start}-{end})")
call_lines = [i for i,line in enumerate(text.splitlines()[start-1:end], start) if any(tok in line for tok in ["write_test_labels", "deployment_metadata_from_profile", "_extract_deployment_metadata", "rtu"])
for _, _, end in names:
src = ast.get_source_segment(text, next(n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef) and n.name == name))
print(f"\n## {name} ({start}-{end})")
PYRepository: openshift-psap/forge
Length of output: 12339
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal
Reachability path
● Entry
projects/llm_d/orchestration/test_phase.py
│
▼
● Hop
projects/guidellm/postprocess/guidellm/dashboard.py:407
export_dashboard_kpis_to_csv: Pivot scalar per-rate KPIs into a dashboard-compatible CSV.
│
▼
● Sink
projects/llm_d/postprocess/plugin.py
Do not export unrestricted deployment configuration.
extract_kpi_labels_from_config() writes the resolved profile into __test_labels__.yaml under env.ARTIFACT_DIR, then LlmDGuideLLMPlugin sends the same profile metadata into dashboard CSV labels. Profiles can include arbitrary vllm_extra.args/env values; for example, dist can reach the dashboard via runtime_args.
Export only an allowlisted, non-sensitive metadata subset. Remove runtime_args/env and any vllm_extra.args values that may encode deployment secrets before adding router_config or runtime_args to kpi labels or CSV metadata.
📍 Affects 2 files
projects/llm_d/postprocess/plugin.py#L203-L205(this comment)projects/llm_d/orchestration/test_phase.py#L173-L174
🤖 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/postprocess/plugin.py` around lines 203 - 205, Restrict
exported profile metadata to an explicit non-sensitive allowlist in
extract_kpi_labels_from_config() and LlmDGuideLLMPlugin; remove runtime_args,
env, and arbitrary vllm_extra.args-derived values before writing KPI labels or
CSV metadata. Update projects/llm_d/postprocess/plugin.py at lines 203-205 and
projects/llm_d/orchestration/test_phase.py at lines 173-174, ensuring only
approved router_config or runtime metadata is emitted.
Source: Coding guidelines
| result["product_version"] = product_version | ||
| result["product_version"] = normalize_product_version(product_version) |
There was a problem hiding this comment.
this will break the current results history if it transforms
v3.5.0-ea.2 --> RHOAI-3.5-EA2
but I think that makes sense, we'll fix manually
| FIELDNAMES = [ | ||
| "run", | ||
| "accelerator", | ||
| "model", | ||
| "version", | ||
| "prompt toks", | ||
| "output toks", | ||
| "TP", | ||
| "DP", | ||
| "EP", | ||
| "replicas", | ||
| "prefill_pod_count", | ||
| "decode_pod_count", | ||
| "router_config", | ||
| "measured concurrency", | ||
| "intended concurrency", | ||
| "measured rps", | ||
| "output_tok/sec", | ||
| "total_tok/sec", | ||
| "prompt_token_count_mean", | ||
| "prompt_token_count_p99", | ||
| "output_token_count_mean", | ||
| "output_token_count_p99", | ||
| "ttft_median", | ||
| "ttft_p95", | ||
| "ttft_p1", | ||
| "ttft_p999", | ||
| "tpot_median", | ||
| "tpot_p95", | ||
| "tpot_p99", | ||
| "tpot_p999", | ||
| "tpot_p1", | ||
| "itl_median", | ||
| "itl_p95", | ||
| "itl_p999", | ||
| "itl_p1", | ||
| "request_latency_median", | ||
| "request_latency_min", | ||
| "request_latency_max", | ||
| "successful_requests", | ||
| "errored_requests", | ||
| "uuid", | ||
| "ttft_mean", | ||
| "ttft_p99", | ||
| "itl_mean", | ||
| "itl_p99", | ||
| "runtime_args", | ||
| "guidellm_start_time_ms", | ||
| "guidellm_end_time_ms", | ||
| "image_tag", | ||
| "guidellm_version", | ||
| "notes", | ||
| ] | ||
| validate_dashboard_fieldnames(FIELDNAMES) |
There was a problem hiding this comment.
this could move to the dashboard file, no?
| @@ -0,0 +1,260 @@ | |||
| """GuideLLM post-processing with the llm-d dashboard CSV schema.""" | |||
There was a problem hiding this comment.
can you move this plugin to projects/llm_d/postprocess/llm_d/plugin.py, to be able to have multiple of them side by side, if relevant?
There was a problem hiding this comment.
the idea would be to keep minimal what's in the llm_d project, versus what's in the guidellm project
- the
guidellmproject --> all the projects using guidellm should be able to consume this - the
llm_dproject --> in there we should customize what's specific to llm-d
I didn't follow this pattern so far, I know. Either put it all in the guidellm plugin for the time being, until we want to unify it and share it between RHAIIS/llm-d/..., or try to find the best way to split :)
There was a problem hiding this comment.
ah, I see with the bottom of the PR that you did start the unification
great !
2 participants
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests