Repository navigation
[llm-d] Almost finalize the KPI workflow - #157
Merged
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 |
📝 WalkthroughWalkthroughCaliper adds shared include/exclude label filters, renames stored test labels, changes status-file handling, fails empty parses, and extends MLflow artifact export configuration. LLM-D presets add filtering defaults and new inheritance profiles. ChangesCaliper workflow updates
LLM-D configuration updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RunParse
participant TestBaseDiscovery
participant LabelFilters
CLI->>RunParse: pass include and exclude filters
RunParse->>TestBaseDiscovery: discover test bases
TestBaseDiscovery->>LabelFilters: match test labels
LabelFilters-->>TestBaseDiscovery: return matching test bases
TestBaseDiscovery-->>RunParse: return filtered test bases
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: 3
🤖 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/commands.py`:
- Around line 1032-1038: The exception handler in run_artifacts_export must stop
emitting raw exception text and traceback to stderr: report only a generic CLI
failure, redact the traceback, and send the redacted diagnostic to the
established protected sink. Apply the same redaction and protected-sink policy
to the existing traceback.print_exception path in artifacts_export_run, ensuring
no secret values are printed, logged, or embedded in error messages under
projects/**/*.py.
- Around line 1030-1031: Update the artifacts_export flow to capture the return
value from run_artifacts_export, call ctx.exit(result) when it is non-zero, and
retain sys.exit(3) only for unexpected exceptions.
In `@projects/caliper/engine/file_export/artifacts_export_run.py`:
- Around line 195-196: Update the remaining MLflow tracking URI example near the
argument handling to use the current --mlflow-tracking-uri option instead of
obsolete --mlflow-endpoint, keeping the surrounding error message and supported
configuration alternatives unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6aa0522-377f-471a-bf86-35568442b915
📒 Files selected for processing (2)
projects/caliper/cli/commands.pyprojects/caliper/engine/file_export/artifacts_export_run.py
Contributor
Author
|
/test fournos llm_d cks |
🟢 Execution of
|
🟢 Submission of
|
Contributor
Author
|
/test fournos llm_d janus cpt |
🟢 Execution of
|
🟢 Submission of
|
kpouget
force-pushed
the
kpis
branch
2 times, most recently
from
August 4, 2026 06:50
a43e5cc to
72c0aaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/caliper/cli/commands.py (1)
540-553: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winForward label filters into KPI generation.
run_kpi_generate()callsrun_parse()withoutinclude_label_filter/exclude_label_filter, so it can discover and parse all directories even whenkpicommand filters applied during the preliminary parse. Move generation to run on the filteredmodel, or pass the filters through torun_kpi_generate().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/cli/commands.py` around lines 540 - 553, Ensure KPI generation preserves the label filters parsed by _parse_label_filters: either generate from the already filtered model created by the preliminary run_parse call, or extend run_kpi_generate and its run_parse call to receive and forward include_filter and exclude_filter. Keep discovery limited to directories matching the kpi command’s filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/caliper/engine/model.py`:
- Line 16: Update the TestBaseNode construction calls in the postprocess tests
to use the renamed test_labels keyword instead of labels, specifically in the
affected parser and MCP gateway plugin tests. Preserve the existing label values
and avoid adding constructor compatibility unless those call sites cannot be
updated.
In `@projects/caliper/engine/visualize.py`:
- Around line 73-95: Update the include/exclude filter construction before
run_parse so each pair is parsed independently into its own single-key
dictionary, preserving repeated label keys and their order instead of
aggregating through parse_filter_kv. Apply the same logic to include_pairs and
exclude_pairs, and add regression coverage confirming duplicate keys reach
run_parse as separate filters.
In `@projects/caliper/orchestration/caliper_invocation.py`:
- Around line 41-55: Update the status-file path allocation logic to reserve the
file atomically before returning it. In the existing-files/index-selection flow,
attempt exclusive creation for the candidate path, catch FileExistsError,
increment the index, and retry until successful; return the reserved path while
preserving the existing filename format and operation filtering.
---
Outside diff comments:
In `@projects/caliper/cli/commands.py`:
- Around line 540-553: Ensure KPI generation preserves the label filters parsed
by _parse_label_filters: either generate from the already filtered model created
by the preliminary run_parse call, or extend run_kpi_generate and its run_parse
call to receive and forward include_filter and exclude_filter. Keep discovery
limited to directories matching the kpi command’s filters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9a49bfb-7222-4039-b4c1-f19f908c0274
📒 Files selected for processing (20)
projects/caliper/cli/commands.pyprojects/caliper/engine/ai_eval.pyprojects/caliper/engine/file_export/artifacts_export_run.pyprojects/caliper/engine/label_filters.pyprojects/caliper/engine/model.pyprojects/caliper/engine/parameter_matrix.pyprojects/caliper/engine/parse.pyprojects/caliper/engine/traverse.pyprojects/caliper/engine/validation.pyprojects/caliper/engine/visualize.pyprojects/caliper/orchestration/caliper_invocation.pyprojects/caliper/orchestration/cli_builder.pyprojects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_config.pyprojects/caliper/tests/stub_plugin.pyprojects/caliper/tests/test_label_filters.pyprojects/guidellm/postprocess/guidellm/parsing/parsers.pyprojects/llm_d/orchestration/config.yamlprojects/mcp_gateway/postprocess/mcp_gateway/parsing/parsers.pyprojects/skeleton/postprocess/default/parsing/parsers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/caliper/engine/file_export/artifacts_export_run.py
Contributor
Author
|
/test fournos llm_d janus cpt-xks |
🔴 Submission of
|
Contributor
Author
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
kpouget
force-pushed
the
kpis
branch
2 times, most recently
from
August 4, 2026 07:59
b451f46 to
7c0c729
Compare
Contributor
Author
|
/test fournos llm_d janus cpt-xks |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
projects/llm_d/orchestration/presets.d/cpt.yaml (1)
14-31: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the S3 preset contract.
apply_preset()applies these keys viaget_config()/set_config(), andCaliperOrchestrationS3Sectionuses exactlycaliper.postprocess.s3.instanceandcaliper.postprocess.s3.directory. Add preset coverage that exercisescpt-xksforllm-d/xksandcpt-release-testing-rhoaiforllm-d/release.🤖 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/orchestration/presets.d/cpt.yaml` around lines 14 - 31, Add preset coverage for the S3 configuration contract by exercising apply_preset() with cpt-xks and verifying CaliperOrchestrationS3Section receives caliper.postprocess.s3.directory as llm-d/xks, then exercising cpt-release-testing-rhoai and verifying it resolves to llm-d/release. Confirm the caliper.postprocess.s3.instance value remains forge-cpt through get_config()/set_config().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/caliper/engine/label_filters.py`:
- Around line 35-38: Update the filter parsing logic around pair.split and
key.strip to reject empty keys after trimming, raising the existing
invalid-filter ValueError for inputs such as “=value”; continue appending
filters with trimmed non-empty keys and values unchanged.
In `@projects/caliper/engine/traverse.py`:
- Around line 49-51: Update the filtering setup around
_load_hierarchical_test_labels so a missing "labels" entry is normalized to an
empty mapping before access. Preserve the existing labels mapping when present,
allowing discovery to continue for empty marker files without raising KeyError.
In `@projects/llm_d/orchestration/presets.d/cpt.yaml`:
- Around line 43-52: Update the cpt-rhoai-release preset or its shared
extensible parent to define platform.operators.rhods-operator.channel as
cpt-rhoai-release, ensuring cpt-release-testing-rhoai and its extending profiles
inherit the required RHOAI operator channel.
---
Nitpick comments:
In `@projects/llm_d/orchestration/presets.d/cpt.yaml`:
- Around line 14-31: Add preset coverage for the S3 configuration contract by
exercising apply_preset() with cpt-xks and verifying
CaliperOrchestrationS3Section receives caliper.postprocess.s3.directory as
llm-d/xks, then exercising cpt-release-testing-rhoai and verifying it resolves
to llm-d/release. Confirm the caliper.postprocess.s3.instance value remains
forge-cpt through get_config()/set_config().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abd060c5-7374-467a-aa61-43acb1a00aba
📒 Files selected for processing (28)
projects/caliper/cli/commands.pyprojects/caliper/engine/ai_eval.pyprojects/caliper/engine/file_export/artifacts_export_run.pyprojects/caliper/engine/label_filters.pyprojects/caliper/engine/model.pyprojects/caliper/engine/parameter_matrix.pyprojects/caliper/engine/parse.pyprojects/caliper/engine/traverse.pyprojects/caliper/engine/validation.pyprojects/caliper/engine/visualize.pyprojects/caliper/orchestration/caliper_invocation.pyprojects/caliper/orchestration/cli_builder.pyprojects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_config.pyprojects/caliper/tests/stub_plugin.pyprojects/caliper/tests/test_label_filters.pyprojects/core/library/config.pyprojects/guidellm/postprocess/guidellm/parsing/parsers.pyprojects/guidellm/tests/test_postprocess_parser.pyprojects/llm_d/orchestration/config.yamlprojects/llm_d/orchestration/pr_args.pyprojects/llm_d/orchestration/presets.d/cks.yamlprojects/llm_d/orchestration/presets.d/cpt.yamlprojects/llm_d/orchestration/presets.d/janus.yamlprojects/llm_d/orchestration/presets.d/models.yamlprojects/mcp_gateway/postprocess/mcp_gateway/parsing/parsers.pyprojects/mcp_gateway/postprocess/tests/test_mcp_gateway_plugin.pyprojects/skeleton/postprocess/default/parsing/parsers.py
💤 Files with no reviewable changes (2)
- projects/llm_d/orchestration/presets.d/models.yaml
- projects/llm_d/orchestration/pr_args.py
🚧 Files skipped from review as they are similar to previous changes (18)
- projects/guidellm/postprocess/guidellm/parsing/parsers.py
- projects/mcp_gateway/postprocess/mcp_gateway/parsing/parsers.py
- projects/caliper/tests/stub_plugin.py
- projects/caliper/engine/validation.py
- projects/caliper/orchestration/postprocess.py
- projects/caliper/orchestration/postprocess_config.py
- projects/caliper/engine/model.py
- projects/caliper/orchestration/caliper_invocation.py
- projects/caliper/engine/ai_eval.py
- projects/caliper/engine/parse.py
- projects/caliper/engine/file_export/artifacts_export_run.py
- projects/skeleton/postprocess/default/parsing/parsers.py
- projects/caliper/orchestration/cli_builder.py
- projects/caliper/engine/visualize.py
- projects/llm_d/orchestration/config.yaml
- projects/caliper/engine/parameter_matrix.py
- projects/caliper/cli/commands.py
- projects/caliper/tests/test_label_filters.py
🟢 Execution of
|
🟢 Submission of
|
Contributor
Author
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
…labels -> test_labels
… for proper yaml serialization
…w test_labels name
… regression failure
Contributor
Author
|
/test fournos llm_d janus cpt-xks |
kpouget
changed the title
🟢 Execution of
|
🟢 Submission of
|
Contributor
Author
|
merging this, I'll finalize this work in another PR |
This was referenced Aug 5, 2026
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
not-setvalue.Bug Fixes