Repository navigation
[llm-d] refactor: Share GuideLLM dashboard postprocessing - #162
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGuideLLM dashboard helpers now parse artifacts, enrich metrics, generate KPIs, and export CSV data. The llm-d plugin uses these helpers and recovers deployment metadata. RHAIIS delegates to the shared implementation. KPI labels and orchestration settings were updated. ChangesDashboard KPI integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GuideLLMParser
participant LlmDGuideLLMPlugin
participant dashboard_helpers
participant dashboard_csv
GuideLLMParser->>LlmDGuideLLMPlugin: parse benchmark and deployment artifacts
LlmDGuideLLMPlugin->>dashboard_helpers: enrich records and compute KPIs
dashboard_helpers-->>LlmDGuideLLMPlugin: return dashboard KPI records
LlmDGuideLLMPlugin->>dashboard_csv: export dashboard-compatible rows
dashboard_csv-->>LlmDGuideLLMPlugin: write dashboard.csv
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
openshift-ci
Bot
added
the
needs-rebase
203fffc to
acf9a54
Compare
openshift-ci
Bot
removed
the
needs-rebase
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…rge into feat/align-csv-export
🔴 Execution of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…rge into feat/align-csv-export
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…rge into feat/align-csv-export
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
🔴 Execution of
|
🔴 Execution of
|
🔴 Execution of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
🟢 Execution of
|
🟢 Execution of
|
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
|
@albertoperdomo2 since I'm not a collaborator on this PR, only you can run the above command. |
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
1 similar comment
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
🛑 Execution of
|
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d cpt-release-testing-llama-33-70b |
🟢 Execution of
|
🟢 Submission of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
🔴 Execution of
|
|
/test fournos llm_d cpt-release-testing-llama-33-70b |
🟢 Execution of
|
🟢 Submission of
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Harshith-umesh 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
🔴 Execution of
|
🟢 Execution of
|
🟢 Execution of
|
kpouget
changed the title
|
thanks @albertoperdomo2 , |
3 participants
Summary
Testing
Summary by CodeRabbit