LCORE-2802: Add OKP RAG quality regression benchmarks and baseline comparison - #265
LCORE-2802: Add OKP RAG quality regression benchmarks and baseline comparison#265alessandralanz wants to merge 16 commits into
Conversation
…. Checked against a live OKP image with 85% pass rate
… Guide's dataset sizing and distribution recommendations
…s quality regression to PR code changes vs OKP data changes using pairwise A/B/C comparisons and shared test helpers have been moved to conftest.py
|
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:
WalkthroughAdds an LCORE regression PR-gate configuration, tool-calling benchmark scenarios, a baseline comparison CLI for detecting metric regressions, negative tool-call assertions, supporting fixtures and tests, and a baseline evaluation snapshot. ChangesLCORE regression gate
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as compare_against_baseline.main
participant Loader as find_and_load_summary
participant Delta as compute_metric_deltas
participant Report as generate_markdown_summary
User->>CLI: provide baseline and current directories
CLI->>Loader: load baseline and current summary JSON
Loader-->>CLI: return evaluation summaries
CLI->>Delta: compute metric deltas and statuses
Delta-->>CLI: return PASS, WARN, or FAIL results
opt markdown output requested
CLI->>Report: render markdown summary
Report-->>CLI: return report text
end
CLI-->>User: print results and return exit status
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@script/regression/compare_abc_runs.py`:
- Around line 89-110: The verdict logic in compare_abc_runs currently ignores
total cumulative critical regressions unless pr_deltas is None, so a split A→B
and B→C failure can still pass. Update the gate in the main decision block to
consider total_has_critical alongside the existing pr_has_critical and
okp_has_critical checks, using the same verdict flow in the compare_abc_runs
function so any critical total regression returns a failing result when
intended.
In `@script/regression/compare_against_baseline.py`:
- Around line 141-157: The status calculation in compare_against_baseline should
not default to PASS when a baseline metric is present but the current run is
missing it. Update the logic around score_delta/status in the comparison flow to
treat “present in baseline, missing in current” as a degraded outcome, and apply
the same handling for pass_rate_delta where relevant. Use the existing
compare_against_baseline metric handling and CRITICAL_METRICS thresholding so
missing current values are reported as WARN or FAIL instead of PASS.
- Around line 232-252: The compare script’s check-only mode is emitting the
baseline/current summary prints before the args.check_only branch, so stdout
contains more than the required single token. In compare_against_baseline.py,
update the control flow around compute_metric_deltas and the check-only handling
so the summary prints are skipped when args.check_only is set, leaving only the
final ok/regression output from the check_only path.
In `@tests/script/test_compare_against_baseline.py`:
- Around line 48-51: The missing-directory test in
test_raises_on_missing_directory is nondeterministic because it hardcodes a /tmp
path that may exist on some machines. Update the test to use pytest’s tmp_path
fixture and construct a guaranteed-missing subpath (for example via tmp_path
with a non-created child) before calling find_and_load_summary, so the
FileNotFoundError assertion always exercises the intended path.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9f84c03-82ee-4691-8e2f-cb77a7f724fc
📒 Files selected for processing (9)
baselines/lcore_regression/current_baseline_summary.jsonconfig/lcore_regression/system-config-pr-gate.yamleval_data/lcore_regression/okp_rag_quality.yamlscript/regression/__init__.pyscript/regression/compare_abc_runs.pyscript/regression/compare_against_baseline.pytests/script/conftest.pytests/script/test_compare_abc_runs.pytests/script/test_compare_against_baseline.py
…tdout, verdict logic, test determinism
…s with 4 context/retrieval metrics, and remove judge_panel config since response-quality judging no longer needed
|
Thanks!! I came across your PR, and I'm not sure if it's ready yet, but I'm wondering a few things:
cc: @asamal4 @Anxhela21 |
|
@xmican10 Thank you so much for the feedback! These are all great points, and I've addressed them just now with my latest push:
The queries and expected responses are from datasets provided by the OpenShift Installer and RHEL product groups (they are the only ones currently rated as gold) that can be found in Lightspeed Core's Evaluation Data GitLab repository. The evaluation data has been removed fro this public repo and is now cloned at pipeline runtime from a forked version of the internal Evaluation Data GitLab repo.
I agree that keeping the full Q&A datasets in the public repo is a risk, so I have removed
The baseline summary has been stripped to 3KB and now only contains aggregate metric scores (pass rates, means, confidence intervals) with no per-conversation IDs, topic names, or individual scores. The comparison scripts only need these aggregate values. The large evaluation data files have been moved to the internal GitLab repo and are no longer part of this PR. |
|
Thanks @alessandralanz for actively addressing my concerns! Thinking a bit further into the future, keeping the regression tests within ls-eval itself might expand the scope of the tooling without adding direct value for the standard ls-eval user. Wdyt @asamal4 and @Anxhela21? I'd love to get your thoughts on this from an architectural perspective. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lightspeed_evaluation/core/system/validator.py (1)
132-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
expect_no_toolscheck logic across two validator sites.The same 5-line
expect_no_toolsdetection is copy-pasted at two locations. Extracting a shared helper keeps the logic in sync and makes the metadata resolution path explicit. Additionally, both validator sites read directly fromturn_data.turn_metrics_metadata, whilecustom.pyresolves tool-eval metadata through_get_tool_eval_metadata(which may useMetricManager). This is safe today becauseexpect_no_toolsis only set per-turn, but the two resolution paths could diverge if system-level defaults ever include this flag.
src/lightspeed_evaluation/core/system/validator.py#L132-L138: Replace the inline 5-line check with a call to the extracted helper.src/lightspeed_evaluation/core/system/validator.py#L517-L523: Replace the inline 5-line check with the same helper call.♻️ Proposed helper extraction
+def _is_tool_eval_negative_assertion(turn_data: TurnData) -> bool: + """Check if tool_eval is in expect_no_tools (negative assertion) mode. + + Args: + turn_data: Turn data to inspect. + + Returns: + True if expect_no_tools is enabled for custom:tool_eval. + """ + turn_metadata = turn_data.turn_metrics_metadata or {} + tool_eval_meta = turn_metadata.get("custom:tool_eval", {}) + return tool_eval_meta.get("expect_no_tools", False)At lines 132–138:
# Negative assertion mode: no required fields needed - if metric_identifier == "custom:tool_eval": - turn_metadata = turn_data.turn_metrics_metadata or {} - tool_eval_meta = turn_metadata.get("custom:tool_eval", {}) - if tool_eval_meta.get("expect_no_tools", False): - return True, "" + if metric_identifier == "custom:tool_eval" and _is_tool_eval_negative_assertion(turn_data): + return True, ""At lines 517–523:
# Skip expected_tool_calls validation for negative assertion mode - if metric == "custom:tool_eval": - turn_metadata = turn_data.turn_metrics_metadata or {} - tool_eval_meta = turn_metadata.get("custom:tool_eval", {}) - if tool_eval_meta.get("expect_no_tools", False): - continue + if metric == "custom:tool_eval" and _is_tool_eval_negative_assertion(turn_data): + continue🤖 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 `@src/lightspeed_evaluation/core/system/validator.py` around lines 132 - 138, Extract the duplicated expect_no_tools detection from validator.py lines 132-138 and 517-523 into one shared helper that resolves custom:tool_eval metadata through the established _get_tool_eval_metadata path, then replace both inline checks with calls to that helper while preserving the existing True, "" result and default-false behavior.
🤖 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.
Nitpick comments:
In `@src/lightspeed_evaluation/core/system/validator.py`:
- Around line 132-138: Extract the duplicated expect_no_tools detection from
validator.py lines 132-138 and 517-523 into one shared helper that resolves
custom:tool_eval metadata through the established _get_tool_eval_metadata path,
then replace both inline checks with calls to that helper while preserving the
existing True, "" result and default-false behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fe83464-8f0d-4abb-a308-8d14e38fc338
📒 Files selected for processing (5)
config/lcore_regression/system-config-pr-gate.yamleval_data/lcore_regression/tool_calling_quality.yamlsrc/lightspeed_evaluation/core/metrics/custom/custom.pysrc/lightspeed_evaluation/core/system/validator.pytests/unit/core/metrics/custom/test_custom.py
|
@xmican10, I do agree. Initially we decided to keep it here because we already have an evaluation e2e test for lightspeed-stack in this repo. @alessandralanz can these scripts exist in the gitlab e2e test? These benchmarks can be saved in gitlab https://gitlab.cee.redhat.com/lightspeed-core/evaluation-data and retrieved as needed for the e2e test? |
|
Closing this PR, the baseline comparison scripts, eval data, and CI pipeline have been consolidated into a dedicated internal repo: gitlab.cee.redhat.com/lightspeed-core/quality-regression-suite. The |
3 participants
Description
Adds an evaluation framework for OKP RAG quality regression testing against the lightspeed-stack. Includes:
negative/off-topic queries
compare_against_baseline) with--check-onlymode for CI gatinggpt-4o-minias the judge LLMType of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Please provide detailed steps to perform tests related to this code change.
How were the fix/results from this change verified? Please provide relevant screenshots or results.
make pre-commitpasses all checks (pylint, pyright, ruff, black, etc.)lightspeed-stackinstance with OKP enabledSummary by CodeRabbit
custom:tool_evalwith an “expect no tool calls” mode for negative test scenarios.custom:tool_evalno-tools behavior.