[llm-d] Finalize the KPI regression testing - #159
Merged
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 updates Caliper KPI filtering, analysis configuration errors, 2D KPI formatting, artifact export, orchestration, plugin contracts, and documentation. GuideLLM now tracks intended concurrency separately and uses it for six 2D KPI curves. ChangesCaliper workflow updates
GuideLLM concurrency KPI updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GuideLLMParser
participant GuideLLMBenchmark
participant PerformanceCurves
participant GuideLLMKPIs
GuideLLMParser->>GuideLLMBenchmark: store effective and intended concurrency
GuideLLMBenchmark->>PerformanceCurves: serialize intended_concurrency
GuideLLMParser->>PerformanceCurves: aggregate both concurrency series
PerformanceCurves->>GuideLLMKPIs: provide intended concurrency curve
GuideLLMKPIs-->>PerformanceCurves: return validated 2D KPI values
Possibly related PRs
🚥 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 |
Contributor
Author
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
Contributor
Author
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
…_tokens_per_request KPI
…e --upload-id and --dry-run flags
Contributor
Author
|
merging this version, will test it further 👍🏻 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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`:
- Line 165: Update the handler example’s KPI record generation around the
timestamp field so it uses the model or record timestamp, or generates the
current UTC timestamp consistent with projects/caliper/tests/stub_plugin.py,
instead of the fixed 2024-01-01T00:00:00Z value.
- Around line 385-388: Update the validation around the throughput/latency
calculation to check throughput and latency explicitly against None, allowing
throughput=0. Add a separate validation that rejects latency=0 before the
division, while preserving the existing required-value error behavior.
- Line 72: Validate that request_rates and throughputs have equal lengths before
the zip call in the curve-building logic, using the existing length-check
pattern from Lines 237-243. Raise the same type of error or validation failure
used there, and only construct the paired float values after validation
succeeds.
- Around line 314-315: Update the Schema v1 JSONL example near the throughput
record to be valid complete JSON: remove the “...” placeholder and include the
required schema_version, timestamp, labels, and source fields defined by
kpi_record.schema.json while preserving the existing KPI values.
- Around line 327-344: Update both tests to use the guide-defined request_rate
function instead of the undefined throughput_kpi symbol, and align the
missing-data pytest.raises match with request_rate’s actual error message.
- Around line 188-190: Update the exception handling around the KPI conversion
logic to catch only the expected missing-metrics exception, rather than broad
Exception. Preserve graceful handling for absent data, but log a warning or
re-raise conversion errors and programming defects so KPI failures are visible.
In `@docs/caliper/plugin_regression.md`:
- Around line 33-36: Update the comparison_keys section in plugin_regression.md
to reflect the implemented AnalysisConfig behavior: document that the field is
optional and defaults to an empty list when omitted, rather than marking it
required or promising validation the implementation does not perform.
- Around line 212-214: Correct the “higher is better” condition in the Threshold
Comparison section so it documents regression when relative_change is less than
the negative max_relative_regression threshold, while preserving the existing
lower-is-better condition.
In `@projects/caliper/cli/s3_export.py`:
- Around line 101-103: Validate that each enabled producer has a non-empty
configured output before joining paths: in projects/caliper/cli/s3_export.py
lines 101-103 for the CSV producer, lines 133-135 for the KPI JSON producer, and
lines 165-167 for the analysis producer. Reject missing values or apply an
explicit fallback before constructing csv_file_path, kpi_file_path, or
analysis_file_path.
- Around line 703-710: Update the upload-plan construction near the upload_files
handling to classify each discovered file by membership in kpi_json_files,
csv_files, or analysis_files, rather than matching legacy filename patterns.
Ensure configured KPI and analysis outputs are assigned their corresponding
upload keys and included in upload_plan, preserving the existing handling for
CSV and other files.
In `@projects/caliper/engine/kpi/analyze.py`:
- Around line 90-101: Update the analysis-config handling around AnalysisConfig
construction to validate both dictionary-created and existing AnalysisConfig
instances before returning. Validate list fields and their string elements,
ensure max_relative_regression is numeric, and require min_baseline_points to be
at least 1; route every invalid value through the existing structured
configuration-error path, preserving valid configurations.
In `@projects/caliper/engine/kpi/generate.py`:
- Around line 22-23: Update the include_label_filter and exclude_label_filter
annotations in the relevant function signature to use list[dict[str, str]] |
None, matching the structure returned by _parse_label_filters and forwarded by
the CLI.
In `@projects/guidellm/postprocess/guidellm/parsing/parsers.py`:
- Around line 489-490: Update the aggregation flow in the parser method that
currently sorts benchmarks by request_rate so benchmarks are ordered by
intended_concurrency before populating request_concurrency and
intended_concurrency curves. Preserve the existing curve-building behavior, and
add a test covering benchmarks whose request-rate order differs from their
intended-concurrency order, verifying the emitted KPI points follow
intended-concurrency order.
- Around line 346-347: Update the parser’s concurrency assignments to use the
configured streams value for both request_concurrency and intended_concurrency,
removing the effective-concurrency label/comment. Preserve the existing integer
conversion and avoid treating request_concurrency as an observed effective
metric until GuideLLM provides one.
🪄 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: 61c1caea-ce41-4e8b-a87a-c1c94462abcd
📒 Files selected for processing (22)
docs/caliper/plugin_kpis.mddocs/caliper/plugin_regression.mdprojects/caliper/cli/commands.pyprojects/caliper/cli/s3_export.pyprojects/caliper/engine/kpi/analyze.pyprojects/caliper/engine/kpi/catalog.pyprojects/caliper/engine/kpi/format.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/engine/model.pyprojects/caliper/orchestration/cli_builder.pyprojects/caliper/orchestration/postprocess.pyprojects/caliper/tests/stub_plugin.pyprojects/caliper/tests/test_kpi_analyze.pyprojects/guidellm/postprocess/guidellm/parsing/kpis.pyprojects/guidellm/postprocess/guidellm/parsing/models.pyprojects/guidellm/postprocess/guidellm/parsing/parsers.pyprojects/guidellm/postprocess/guidellm/plugin.pyprojects/guidellm/tests/test_postprocess_parser.pyprojects/mcp_gateway/postprocess/mcp_gateway/plugin.pyprojects/mcp_gateway/postprocess/tests/test_mcp_gateway_plugin.pyprojects/rhaiis/postprocess/plugin.pyprojects/skeleton/postprocess/default/plugin.py
💤 Files with no reviewable changes (6)
- projects/caliper/engine/kpi/catalog.py
- projects/caliper/engine/model.py
- projects/rhaiis/postprocess/plugin.py
- projects/mcp_gateway/postprocess/mcp_gateway/plugin.py
- projects/mcp_gateway/postprocess/tests/test_mcp_gateway_plugin.py
- projects/skeleton/postprocess/default/plugin.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation