[llmd] Finish the KPI regression and version comparison - #163
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 |
|
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:
📝 WalkthroughWalkthroughThe PR adds verbose parsing controls to Caliper commands and engines, refines KPI analysis and S3 export statuses, and adds GuideLLM deployment-profile reports with parser ordering and visualization-group updates. ChangesCaliper parsing and analysis
GuideLLM parsing and deployment reports
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CaliperEngine
participant ParseEngine
participant PluginParser
participant DeploymentProfileReport
CLI->>CaliperEngine: pass verbose_parsing
CaliperEngine->>ParseEngine: run parsing with verbosity
ParseEngine->>PluginParser: parse records and control diagnostics
PluginParser-->>ParseEngine: return parsed records
ParseEngine-->>CaliperEngine: return records and timing output
CaliperEngine->>DeploymentProfileReport: provide benchmark records
DeploymentProfileReport->>DeploymentProfileReport: group records and generate plots
DeploymentProfileReport-->>CLI: return HTML report path
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
84881c8 to
64eeb87
Compare
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
6f31a1b to
b6479ca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py (2)
1100-1100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
container_idis unused in this report.This function emits only the throughput plot per group and no tabbed container.
container_idis initialized at Line 1100 and incremented at Line 1163, but no generated markup references it. Remove both statements.Also applies to: 1163-1163
🤖 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/plotting/performance_analysis.py` at line 1100, Remove the unused container_id initialization and increment from the report-generating function, while leaving the throughput plot generation unchanged.
671-679: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the per-record
🐛 DEBUGlog lines, or lower them tologger.debug.These two statements log one line for every record at
infolevel. The information they carry is already summarized at Lines 686-694. In a run with many records the output becomes noisy.♻️ Proposed change
- # Debug: Print version values found - if "version" in labels: - logger.info( - f"🐛 DEBUG: Found version='{labels['version']}' in record {record.test_base_path}" - ) - else: - logger.info( - f"🐛 DEBUG: No 'version' key found in record {record.test_base_path}, keys: {list(labels.keys())}" - ) + if "version" in labels: + logger.debug( + "Found version='%s' in record %s", labels["version"], record.test_base_path + ) + else: + logger.debug( + "No 'version' key in record %s, keys: %s", + record.test_base_path, + sorted(labels.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/guidellm/postprocess/guidellm/plotting/performance_analysis.py` around lines 671 - 679, Update the per-record logging block around the version-label check to remove both `logger.info` calls or change them to `logger.debug`, while preserving the existing summarized version reporting elsewhere.
🤖 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 `@docs/caliper/plugin_kpis.md`:
- Around line 191-198: Define a module-level logger in the handler example
before the exception handling uses it, alongside the existing imports and setup.
Ensure both logger.debug and logger.warning in the KPI processing flow reference
this declared logger so the documented error handling executes without
NameError.
- Around line 191-198: Update the KPI exception handling around the KPI
invocation to catch only the dedicated missing-data exception when metrics are
absent, rather than every ValueError; preserve the existing debug-and-continue
behavior for that exception and let conversion ValueErrors, such as float
failures, reach the visible warning and re-raise path.
In `@projects/caliper/cli/s3_export.py`:
- Around line 765-773: The dry-run summary counts KPI JSON files by a fixed
filename instead of the discovered file lists. Update the summary logic near the
CSV/KPI classification and the referenced later count to use len(csv_files) and
len(kpi_json_files), while preserving the existing output-path handling for
filenames and paths.
In `@projects/caliper/engine/kpi/analyze.py`:
- Around line 134-145: Update the validation around max_relative_regression and
min_baseline_points to reject booleans explicitly. Require
max_relative_regression to be a finite numeric value greater than or equal to
zero, while preserving the existing validation error context; require
min_baseline_points to be an integer at least 1 but not a bool.
- Around line 824-855: Update the status handling in the wrapper around the
visible NO_DATA branch so engine results with status "warning" and success=True
are normalized to the same warning response instead of falling through to
failure. Preserve the existing NO_DATA behavior and ensure successful
no-baseline analyses retain status "warning" and success=True.
In `@projects/caliper/engine/visualize.py`:
- Around line 92-93: Update the show_parameter_matrix argument in run_visualize
to use verbose_parsing instead of always enabling it, so parameter-matrix output
is disabled when verbose_parsing=False and matches the AI evaluation export and
KPI generation paths.
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 1585-1587: Update both report-generator exception handlers in
projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py at lines
1585-1587 and 1187-1190 to remove {e} from logger.exception messages, then
enforce one failure policy for both generators. Ensure plugin.visualize in
projects/guidellm/postprocess/guidellm/plugin.py does not swallow
report-generation exceptions; let failures propagate to the CLI so the run fails
instead of silently producing missing output.
- Around line 738-748: Update the comparison-group construction around
comparison_groups to derive core_labels from the previously collected all_keys
set, not the leaked labels loop variable. Keep the existing filter in the record
loop, so grouping works for empty records and preserves all distinguishing keys
across heterogeneous records.
- Around line 1354-1362: Correct the five corrupted CSS property names in the
report HTML template: restore font-family, text-align, background-color,
list-style, and text-decoration within the relevant styling block. Use the
correctly formatted declarations in generate_deployment_profile_report as the
reference, leaving the surrounding CSS unchanged.
- Around line 1558-1561: Move the closing markup append and container_id
increment back inside the loadshape loop that opens each .loadshape-section and
.tabs-container. Ensure each loadshape emits its own closing div pair and
increments container_id before the next iteration so tab and container IDs
remain unique.
---
Nitpick comments:
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Line 1100: Remove the unused container_id initialization and increment from
the report-generating function, while leaving the throughput plot generation
unchanged.
- Around line 671-679: Update the per-record logging block around the
version-label check to remove both `logger.info` calls or change them to
`logger.debug`, while preserving the existing summarized version reporting
elsewhere.
🪄 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: 727496bd-ee59-437d-94cb-b73c55b7e385
📒 Files selected for processing (17)
docs/caliper/plugin_kpis.mddocs/caliper/plugin_regression.mdprojects/caliper/cli/commands.pyprojects/caliper/cli/s3_export.pyprojects/caliper/engine/ai_eval.pyprojects/caliper/engine/kpi/analyze.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/engine/parse.pyprojects/caliper/engine/visualize.pyprojects/cluster/toolbox/deploy_custom_catalog/main.pyprojects/guidellm/postprocess/guidellm/parsing/kpis.pyprojects/guidellm/postprocess/guidellm/parsing/parsers.pyprojects/guidellm/postprocess/guidellm/plotting/performance_analysis.pyprojects/guidellm/postprocess/guidellm/plugin.pyprojects/guidellm/postprocess/guidellm/visualize-groups.yamlprojects/guidellm/tests/test_postprocess_parser.pyprojects/llm_d/orchestration/config.yaml
💤 Files with no reviewable changes (1)
- projects/guidellm/postprocess/guidellm/parsing/kpis.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
9eb2c9c to
e32d52f
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py (2)
1556-1559: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThe closing markup and the
container_idincrement are still outside the loadshape loop.The earlier review marked this fixed, but Lines 1556-1559 are still at 8-space indentation while the body of the
for loadshape, plots in all_plots_data:loop at Line 1490 is at 12 spaces. Two defects follow when more than one loadshape exists:
- Only one
</div></div>pair is emitted, so every later section nests inside the previous one.container_idstays0for all loadshapes, so all tab buttons and tab contents sharetabs-container-0andtab-0-<idx>.document.getElementById(tabId)at Line 1461 resolves to the first match, so a tab click in the second loadshape switches the tab in the first.🐛 Proposed fix
- html_content += """ + html_content += """ </div> </div>""" - container_id += 1 + container_id += 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/guidellm/postprocess/guidellm/plotting/performance_analysis.py` around lines 1556 - 1559, Move the closing markup append and container_id increment into the for loadshape, plots in all_plots_data loop, matching the loop body’s indentation. Ensure each loadshape emits its own closing </div></div> pair and increments container_id before the next iteration so tab container and tab content IDs remain unique.
1352-1360: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFive CSS property names are still corrupted.
The earlier review marked this fixed, but the current code still contains
font - family(Line 1352),text - align(Line 1353),background - color(Line 1356),list - style(Line 1357), andtext - decoration(Line 1360). Browsers discard these declarations. The equivalent block ingenerate_deployment_profile_reportat Lines 963-971 has the correct names.🐛 Proposed fix
- body {{font - family: Arial, sans-serif; margin: 40px; }} - .header {{text - align: center; margin-bottom: 30px; }} + body {{ font-family: Arial, sans-serif; margin: 40px; }} + .header {{ text-align: center; margin-bottom: 30px; }} .loadshape-section {{margin: 40px 0; }} .loadshape-title {{color: `#333`; font-size: 24px; margin-bottom: 20px; border-bottom: 2px solid `#007acc`; padding-bottom: 10px; }} - .navigation {{background - color: `#f5f5f5`; padding: 15px; border-radius: 4px; margin-bottom: 20px; }} - .navigation ul {{list - style: none; padding: 0; margin: 0; }} + .navigation {{ background-color: `#f5f5f5`; padding: 15px; border-radius: 4px; margin-bottom: 20px; }} + .navigation ul {{ list-style: none; padding: 0; margin: 0; }} .navigation li {{display: inline-block; margin-right: 20px; }} .navigation a {{color: `#007acc`; text-decoration: none; font-weight: bold; }} - .navigation a:hover {{text - decoration: underline; }} + .navigation a:hover {{ text-decoration: underline; }}🤖 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/plotting/performance_analysis.py` around lines 1352 - 1360, Correct the five malformed CSS property names in the HTML template block: update font - family, text - align, background - color, list - style, and text - decoration to valid CSS property syntax. Use the corresponding declarations in generate_deployment_profile_report as the reference, while preserving the existing values and formatting.
🧹 Nitpick comments (1)
projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py (1)
840-856: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
sanitize_for_pathto module scope.The function is defined inside the
elsebranch of the per-group loop, so Python rebuilds it on every group iteration. It has no closure over loop state. A module-level helper also letsgenerate_comprehensive_performance_reportreuse it at Line 1266, where the rawloadshapevalue is used directly as a directory name.♻️ Proposed refactor
Define once near the top of the module:
_PATH_UNSAFE_CHARS = '/\\:*?|<>"' def sanitize_for_path(text: str) -> str: """Replace filesystem-unsafe characters in label values.""" return "".join("_" if c in _PATH_UNSAFE_CHARS else c for c in str(text))Then drop the nested definition:
group_desc = ", ".join(f"{k}={v}" for k, v in group_key) - - # Sanitize values for filesystem safety (replace problematic characters) - def sanitize_for_path(text: str) -> str: - """Replace filesystem-unsafe characters in label values.""" - return ( - str(text) - .replace("/", "_") - .replace("\\", "_") - .replace(":", "_") - .replace("*", "_") - .replace("?", "_") - .replace("|", "_") - .replace("<", "_") - .replace(">", "_") - .replace('"', "_") - ) - group_name = "__".join(f"{k}_{sanitize_for_path(v)}" for k, v in group_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/plotting/performance_analysis.py` around lines 840 - 856, Move sanitize_for_path from the per-group loop to module scope, defining the shared _PATH_UNSAFE_CHARS constant there and preserving its filesystem-safe replacement behavior. Remove the nested definition and use sanitize_for_path when generate_comprehensive_performance_report builds the loadshape directory name at the existing raw-value path.
🤖 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/cli/s3_export.py`:
- Around line 751-755: Update the include_ai_data branch in run_s3_export to
handle ai_data_dir being None or missing before calling list_ai_data_files,
skipping AI-data discovery and upload in that case; alternatively, update
list_ai_data_files to accept an optional path and return an empty list for None
or nonexistent directories. Preserve error_detected behavior for genuinely empty
discovered data.
- Line 863: Remove the `error_detected = False` reset before final status
evaluation so missing-input errors from the discovery branches are preserved.
Update both the no-file and dry-run return paths to derive their status from
`error_detected`, returning `failed` when it is true while retaining their
existing `skipped` or `success` statuses otherwise.
- Around line 102-117: Update the configured artifact discovery branches for
CSV, KPI JSON, and analysis paths to use is_file() instead of exists() before
appending paths to upload sets, while preserving their existing missing-path
logging. Also update explicit path validation in
run_s3_export_with_explicit_paths to reject directories using the same file
check.
- Around line 774-790: Initialize csv_files, kpi_json_files, and ai_data_files
to empty collections before the conditional discovery logic, while ensuring
analysis_files is always initialized and discovered as needed. Update the
file-classification flow around the membership checks so exports with only
analysis, KPIs, or AI data never reference an unbound discovery list.
In `@projects/caliper/engine/kpi/analyze.py`:
- Around line 400-407: Update the CLI consumer in commands.py to apply
status_dict_to_exit_code after writing the analysis status file, so the
no-baseline result from the analysis branch preserves warning exit code 2
despite success=True. Keep successful and failure exit behavior unchanged.
- Around line 93-97: Update the AnalysisConfig construction in the
raw-dictionary branch to reject unsupported keys instead of filtering them
through AnalysisConfig.__dataclass_fields__. Pass raw directly to AnalysisConfig
or explicitly validate and raise for unknown keys, while preserving normal
construction for recognized settings.
- Around line 117-145: Update _validate_analysis_config so validation errors
retain the plugin module, field name, expected type, actual type, and item index
where applicable, but omit all interpolated raw configuration values such as
field contents, invalid items, and numeric values. Ensure run_kpi_analysis
receives only sanitized error messages.
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 936-939: Update the group_labels_dict construction in the plotting
flow to also recognize the all_data fallback key used around the existing
detection near line 833, avoiding dict(group_key) for that flat string tuple.
Preserve the current empty-label behavior for unified_comparison and continue
converting normal (key, value) pair group keys with dict().
- Around line 1091-1093: Escape all group-description text before inserting it
into generated HTML and derive anchors through a shared id-safe helper. Add the
necessary module imports and _html_anchor_id(text) helper, use it wherever
group_id is generated, and apply html.escape to displayed group_desc values in
the navigation, list items, and heading while keeping href fragments and id
attributes based on the same sanitized group_id.
- Around line 764-771: Coerce every distinguishing label value to str when
extracting it for comparison tuples in the performance analysis function. Update
the initial comparison-key loop and the two later tuple-rebuilding sites to
apply the same conversion, preserving the existing "unknown" default so all
values passed to the subsequent join operations are strings.
In `@projects/rhoai/library/pr_args.py`:
- Around line 38-47: Update the directive parsing logic around `parts`, `image`,
and `channel` to reject inputs containing more than two tokens. Preserve the
existing validation and default `beta` channel behavior for one-token inputs,
while accepting only the documented `IMAGE [CHANNEL]` format.
---
Duplicate comments:
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 1556-1559: Move the closing markup append and container_id
increment into the for loadshape, plots in all_plots_data loop, matching the
loop body’s indentation. Ensure each loadshape emits its own closing
</div></div> pair and increments container_id before the next iteration so tab
container and tab content IDs remain unique.
- Around line 1352-1360: Correct the five malformed CSS property names in the
HTML template block: update font - family, text - align, background - color,
list - style, and text - decoration to valid CSS property syntax. Use the
corresponding declarations in generate_deployment_profile_report as the
reference, while preserving the existing values and formatting.
---
Nitpick comments:
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 840-856: Move sanitize_for_path from the per-group loop to module
scope, defining the shared _PATH_UNSAFE_CHARS constant there and preserving its
filesystem-safe replacement behavior. Remove the nested definition and use
sanitize_for_path when generate_comprehensive_performance_report builds the
loadshape directory name at the existing raw-value path.
🪄 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: 466c2e1a-4c8d-4291-976b-54fc95771e8e
📒 Files selected for processing (19)
docs/caliper/plugin_kpis.mddocs/caliper/plugin_regression.mdprojects/caliper/cli/commands.pyprojects/caliper/cli/s3_export.pyprojects/caliper/engine/ai_eval.pyprojects/caliper/engine/kpi/analyze.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/engine/parse.pyprojects/caliper/engine/visualize.pyprojects/cluster/toolbox/deploy_custom_catalog/main.pyprojects/cluster/toolbox/deploy_custom_catalog/templates/catalogsource.yaml.j2projects/guidellm/postprocess/guidellm/parsing/kpis.pyprojects/guidellm/postprocess/guidellm/parsing/parsers.pyprojects/guidellm/postprocess/guidellm/plotting/performance_analysis.pyprojects/guidellm/postprocess/guidellm/plugin.pyprojects/guidellm/postprocess/guidellm/visualize-groups.yamlprojects/guidellm/tests/test_postprocess_parser.pyprojects/llm_d/orchestration/config.yamlprojects/rhoai/library/pr_args.py
💤 Files with no reviewable changes (1)
- projects/guidellm/postprocess/guidellm/parsing/kpis.py
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/caliper/plugin_regression.md
- projects/caliper/engine/visualize.py
- projects/caliper/engine/ai_eval.py
- projects/cluster/toolbox/deploy_custom_catalog/main.py
- projects/guidellm/postprocess/guidellm/visualize-groups.yaml
- projects/llm_d/orchestration/config.yaml
- projects/guidellm/postprocess/guidellm/plugin.py
- projects/caliper/engine/kpi/generate.py
- projects/guidellm/postprocess/guidellm/parsing/parsers.py
- projects/guidellm/tests/test_postprocess_parser.py
- docs/caliper/plugin_kpis.md
- projects/caliper/cli/commands.py
- projects/caliper/engine/parse.py
…or the regression analyze
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt-xks |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
♻️ Duplicate comments (6)
projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py (6)
25-34: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
sanitize_for_pathstill allows.,.., and empty results.The character list omits path segments. A label value of
".."returns unchanged. Line 866 and Line 1261 then build a directory outsidereport_dir, andmkdir(exist_ok=True)succeeds. An empty or whitespace-only value collapses to the parent directory.🛡️ Proposed hardening
def sanitize_for_path(text: str) -> str: """Replace filesystem-unsafe characters in label values.""" result = str(text) for char in _PATH_UNSAFE_CHARS: result = result.replace(char, "_") - return result + result = result.strip() + if result in ("", ".", ".."): + return "_" + return result🤖 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/plotting/performance_analysis.py` around lines 25 - 34, Harden sanitize_for_path so it never returns "." or "..", nor an empty or whitespace-only path component after sanitization. Update the function to normalize whitespace and provide a safe non-empty fallback for collapsed values, while preserving replacement of the existing filesystem-unsafe characters used by its callers.
931-934: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
dict(group_key)still raisesValueErrorfor theall_datafallback key.Line 797 sets the fallback group key to
("all_data", warning_message), a flat 2-tuple of strings. Line 933 excludes only("unified_comparison",), so the fallback key reachesdict(...).dict()requires each element to be a 2-length sequence, so it raisesValueError: dictionary update sequence element#0has length 8; 2 is required. This path runs on single-version runs. Line 1183 re-raises and no report is produced.Reuse the
group_namevalue already computed at Line 847.🐛 Proposed fix
if group_plots: # Include group_key for displaying identical labels - group_labels_dict = dict(group_key) if group_key != ("unified_comparison",) else {} + if group_name in ("all_data", "unified_comparison"): + group_labels_dict = {} + else: + group_labels_dict = dict(group_key) all_plots_data.append((group_desc, group_plots, group_labels_dict))🤖 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/plotting/performance_analysis.py` around lines 931 - 934, Update the group_labels_dict construction in the plotting flow to reuse the already computed group_name value from the surrounding logic, rather than calling dict(group_key) for the flat all_data fallback key. Preserve the existing empty-label behavior for the unified_comparison key and ensure single-version all_data runs no longer raise ValueError.
1261-1261: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly the directory is sanitized, so the image links still break.
Line 1261 sanitizes the directory name. Line 1295 still passes the raw
loadshapeas the base filename, and Lines 1312-1313 still build the relative link paths from the raw value. If the value contains any character in_PATH_UNSAFE_CHARS, the link path does not match the directory on disk, and the report shows broken images.save_figurealso writes an unsafe filename inside the sanitized directory.Compute the sanitized name once. Use it for the directory, the filename, and the link paths.
🐛 Proposed fix
- loadshape_dir = report_dir / sanitize_for_path(loadshape) + safe_loadshape = sanitize_for_path(loadshape) + loadshape_dir = report_dir / safe_loadshape loadshape_dir.mkdir(exist_ok=True)At Line 1295:
- filename = f"{loadshape}_{plot_name.lower().replace(' ', '_')}" + filename = f"{safe_loadshape}_{plot_name.lower().replace(' ', '_')}"At Lines 1312-1313:
- f"{report_dir_name}/{loadshape}/{Path(png_path).name}", # PNG path - f"{report_dir_name}/{loadshape}/{Path(html_path).name}", # HTML path + f"{report_dir_name}/{safe_loadshape}/{Path(png_path).name}", # PNG path + f"{report_dir_name}/{safe_loadshape}/{Path(html_path).name}", # HTML 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/plotting/performance_analysis.py` at line 1261, Compute the sanitized loadshape name once in the relevant plotting flow, then reuse it for the loadshape_dir path, save_figure filename, and both relative image link paths. Replace all remaining raw loadshape values in these directory, filename, and link constructions so generated files and report links consistently match.
779-783: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCoerce label values to
strwhen you build comparison tuples.
UnifiedResultRecord.distinguishing_labelsis typeddict[str, Any], so a value can be anint,float,bool, orNone. Line 782 stores the raw value. Line 817 and Line 876 then call":".join(cv), andstr.joinraisesTypeErroron a non-string member.sorted(comp_values)also compares mixed types. Line 1183 re-raises, so one non-string label value aborts the report.Coerce at each extraction point.
🐛 Proposed fix
comp_values = [] for key in comparison_keys: value = record.distinguishing_labels.get(key, "unknown") - comp_values.append(value) + comp_values.append(str(value)) comparison_values.add(tuple(comp_values))Apply the same coercion at Lines 814-815 and Lines 872-874.
🤖 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/plotting/performance_analysis.py` around lines 779 - 783, Coerce each value extracted from UnifiedResultRecord.distinguishing_labels to str when building comparison tuples, including the extraction paths near the current comparison_values construction and the corresponding paths near lines 814-815 and 872-874. Preserve the existing "unknown" fallback while ensuring tuple sorting and subsequent ":".join operations only receive strings.
1086-1088: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEscape label-derived text before you interpolate it into HTML.
Line 1087 derives
group_idfromgroup_desc, which is built from label values. The replacement covers only" ","=", and",". A value that contains",<,>,&, or#breaks thehreffragment at Line 1088, theidattribute at Line 1122, the heading at Line 1123, and the list items at Line 1113. Thehrefand theidcan also diverge.Restrict
group_idto an id-safe character set, and escape displayed text.🐛 Proposed fix
Add module-level imports and a helper:
import html import re def _html_anchor_id(text: str) -> str: return re.sub(r"[^A-Za-z0-9_-]", "_", text)Then:
for group_desc, _, _ in all_plots_data: - group_id = group_desc.replace(" ", "_").replace("=", "_").replace(",", "_") - html_content += f'\n <li><a href="#{group_id}">{group_desc}</a></li>' + group_id = _html_anchor_id(group_desc) + html_content += ( + f'\n <li><a href="#{group_id}">{html.escape(group_desc)}</a></li>' + )Apply
_html_anchor_idat Line 1096, andhtml.escapeat Lines 1113 and 1123.🤖 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/plotting/performance_analysis.py` around lines 1086 - 1088, Update the plotting HTML generation around the group ID construction to derive IDs through a shared id-safe helper using only letters, digits, underscores, and hyphens, and use that same result for both links and anchors. Escape label-derived text when interpolating list items and headings, including the rendering near the group navigation and heading generation, by adding the required html and re imports and applying html.escape to displayed labels.
805-808: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winThe
("unified_comparison",)sentinel still reaches(k, v)unpacking.Line 766 stores the 1-tuple
("unified_comparison",)as a group key. Line 808 iterates that key withfor k, v in group_key, so Python unpacks the string"unified_comparison"and raisesValueError: too many values to unpack (expected 2). Line 850 and Line 851 repeat the same unpacking. This path runs whenever no record carries a non-comparison label key. Line 1183 re-raises, so no report is produced.Add a branch for the sentinel at both sites. A single helper that maps a
group_keyto(group_desc, group_name, warning_msg)removes the duplicated branching and keeps Line 933 consistent.🐛 Proposed fix
+def _describe_group_key(group_key: tuple) -> tuple[str, str, str | None]: + """Map a group key (including sentinel shapes) to description, name, and warning.""" + if group_key == ("unified_comparison",): + return "All Test Configurations", "unified_comparison", None + if len(group_key) == 2 and group_key[0] == "all_data": + return "All Available Data", "all_data", group_key[1] + group_desc = ", ".join(f"{k}={v}" for k, v in group_key) + group_name = "__".join(f"{k}_{sanitize_for_path(v)}" for k, v in group_key) + return group_desc, group_name, NoneThen at Lines 805-808:
- # Handle special case for warning message - if len(group_key) == 2 and group_key[0] == "all_data": - group_desc = "All Available Data" - else: - group_desc = ", ".join(f"{k}={v}" for k, v in group_key) + group_desc, _, _ = _describe_group_key(group_key)And at Lines 845-852:
- if len(group_key) == 2 and group_key[0] == "all_data": - group_desc = "All Available Data" - group_name = "all_data" - warning_msg = group_key[1] # Extract warning message - else: - group_desc = ", ".join(f"{k}={v}" for k, v in group_key) - group_name = "__".join(f"{k}_{sanitize_for_path(v)}" for k, v in group_key) - warning_msg = None + group_desc, group_name, warning_msg = _describe_group_key(group_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/plotting/performance_analysis.py` around lines 805 - 808, Update the group-key formatting logic around the existing description/name/warning branches to explicitly handle the one-element ("unified_comparison",) sentinel before any key/value unpacking. Extract the mapping into a shared helper returning group_desc, group_name, and warning_msg, then use it at both affected sites and the corresponding logic near the report generation path so all branches remain consistent.
🤖 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.
Duplicate comments:
In `@projects/guidellm/postprocess/guidellm/plotting/performance_analysis.py`:
- Around line 25-34: Harden sanitize_for_path so it never returns "." or "..",
nor an empty or whitespace-only path component after sanitization. Update the
function to normalize whitespace and provide a safe non-empty fallback for
collapsed values, while preserving replacement of the existing filesystem-unsafe
characters used by its callers.
- Around line 931-934: Update the group_labels_dict construction in the plotting
flow to reuse the already computed group_name value from the surrounding logic,
rather than calling dict(group_key) for the flat all_data fallback key. Preserve
the existing empty-label behavior for the unified_comparison key and ensure
single-version all_data runs no longer raise ValueError.
- Line 1261: Compute the sanitized loadshape name once in the relevant plotting
flow, then reuse it for the loadshape_dir path, save_figure filename, and both
relative image link paths. Replace all remaining raw loadshape values in these
directory, filename, and link constructions so generated files and report links
consistently match.
- Around line 779-783: Coerce each value extracted from
UnifiedResultRecord.distinguishing_labels to str when building comparison
tuples, including the extraction paths near the current comparison_values
construction and the corresponding paths near lines 814-815 and 872-874.
Preserve the existing "unknown" fallback while ensuring tuple sorting and
subsequent ":".join operations only receive strings.
- Around line 1086-1088: Update the plotting HTML generation around the group ID
construction to derive IDs through a shared id-safe helper using only letters,
digits, underscores, and hyphens, and use that same result for both links and
anchors. Escape label-derived text when interpolating list items and headings,
including the rendering near the group navigation and heading generation, by
adding the required html and re imports and applying html.escape to displayed
labels.
- Around line 805-808: Update the group-key formatting logic around the existing
description/name/warning branches to explicitly handle the one-element
("unified_comparison",) sentinel before any key/value unpacking. Extract the
mapping into a shared helper returning group_desc, group_name, and warning_msg,
then use it at both affected sites and the corresponding logic near the report
generation path so all branches remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 84e68f09-7e66-48b3-9a1c-9d98cf6a5bcc
📒 Files selected for processing (19)
docs/caliper/plugin_kpis.mddocs/caliper/plugin_regression.mdprojects/caliper/cli/commands.pyprojects/caliper/cli/kpi_analysis.pyprojects/caliper/cli/s3_export.pyprojects/caliper/engine/ai_eval.pyprojects/caliper/engine/kpi/analyze.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/engine/parse.pyprojects/caliper/engine/visualize.pyprojects/caliper/orchestration/caliper_invocation.pyprojects/caliper/orchestration/postprocess.pyprojects/guidellm/postprocess/guidellm/parsing/kpis.pyprojects/guidellm/postprocess/guidellm/parsing/parsers.pyprojects/guidellm/postprocess/guidellm/plotting/performance_analysis.pyprojects/guidellm/postprocess/guidellm/plugin.pyprojects/guidellm/postprocess/guidellm/visualize-groups.yamlprojects/guidellm/tests/test_postprocess_parser.pyprojects/llm_d/orchestration/config.yaml
💤 Files with no reviewable changes (2)
- projects/guidellm/postprocess/guidellm/parsing/kpis.py
- projects/caliper/orchestration/postprocess.py
🚧 Files skipped from review as they are similar to previous changes (16)
- projects/llm_d/orchestration/config.yaml
- projects/caliper/engine/visualize.py
- projects/caliper/orchestration/caliper_invocation.py
- projects/guidellm/postprocess/guidellm/plugin.py
- projects/guidellm/postprocess/guidellm/visualize-groups.yaml
- projects/guidellm/tests/test_postprocess_parser.py
- projects/caliper/cli/kpi_analysis.py
- projects/caliper/engine/kpi/generate.py
- projects/caliper/engine/parse.py
- projects/caliper/engine/ai_eval.py
- docs/caliper/plugin_kpis.md
- projects/guidellm/postprocess/guidellm/parsing/parsers.py
- projects/caliper/engine/kpi/analyze.py
- docs/caliper/plugin_regression.md
- projects/caliper/cli/s3_export.py
- projects/caliper/cli/commands.py
…oup to comprehensive
|
/test fournos llm_d janus cpt-xks |
🛑 Execution of
|
🔴 Execution of
|
🔴 Submission of
|
|
@kpouget: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
…m the post-processing status
|
/test fournos llm_d janus cpt-xks |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation