OLS-2385: removing temperature param and pass threshold. - #2932
Conversation
5434039 to
fa4409f
Compare
fa4409f to
2c14818
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughJudge LLM configurations now use a 512-token limit where configured and omit temperature settings. Periodic LSEval validation prints consolidated diagnostics and requires at least one successful evaluation instead of enforcing an error-rate threshold. ChangesLSEval configuration and validation
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
🧹 Nitpick comments (1)
tests/e2e/evaluation/test_lseval_periodic.py (1)
192-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider renaming
passedtonon_erroredfor clarity.
passed = total - errorsincludes both passing and failing (non-error) evaluations, not just passing ones. The assertion message "zero successful results" correctly conveys the intent, but the variable namepassedis misleading since some of those results may have failed without erroring.♻️ Suggested rename
- passed = total - errors + non_errored = total - errorsAnd update the print/assertion references accordingly:
- f"Total={total} Passed={passed} Errors={errors} " + f"Total={total} Non-errored={non_errored} Errors={errors} "- assert passed > 0, f"All {total} evaluations errored — zero successful results." + assert non_errored > 0, f"All {total} evaluations errored — zero successful results."🤖 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 `@tests/e2e/evaluation/test_lseval_periodic.py` around lines 192 - 208, Rename the local variable passed to non_errored in the evaluation summary, preserving its calculation as total minus errors. Update every corresponding print and assertion reference so the summary accurately labels non-error evaluations rather than successful results.
🤖 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 `@tests/e2e/evaluation/test_lseval_periodic.py`:
- Around line 192-208: Rename the local variable passed to non_errored in the
evaluation summary, preserving its calculation as total minus errors. Update
every corresponding print and assertion reference so the summary accurately
labels non-error evaluations rather than successful results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 91e29b64-7a65-48ce-bfaa-335a533bfa1f
📒 Files selected for processing (7)
eval/system_azure_openai_lseval.yamleval/system_openai_lseval.yamleval/system_rhelai_vllm_lseval.yamleval/system_rhoai_vllm_lseval.yamleval/system_watsonx_lseval.yamleval/troubleshooting/system.yamltests/e2e/evaluation/test_lseval_periodic.py
💤 Files with no reviewable changes (1)
- eval/troubleshooting/system.yaml
sriroopar
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sriroopar: This pull request references OLS-2385 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
2c14818 to
207512d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/e2e/evaluation/test_lseval_periodic.py`:
- Around line 197-202: Update the judge_detail selection near
overall.get("total_judge_llm_tokens", -1) so missing or negative
total_judge_llm_tokens values report “unavailable,” while zero retains the
failed-before-judge message and positive values retain “judge was called.”
🪄 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: Enterprise
Run ID: 36993db0-506c-45e5-8e18-1abe4b3b6b82
📒 Files selected for processing (7)
eval/system_azure_openai_lseval.yamleval/system_openai_lseval.yamleval/system_rhelai_vllm_lseval.yamleval/system_rhoai_vllm_lseval.yamleval/system_watsonx_lseval.yamleval/troubleshooting/system.yamltests/e2e/evaluation/test_lseval_periodic.py
💤 Files with no reviewable changes (1)
- eval/troubleshooting/system.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- eval/system_rhoai_vllm_lseval.yaml
- eval/system_openai_lseval.yaml
- eval/system_rhelai_vllm_lseval.yaml
- eval/system_watsonx_lseval.yaml
- eval/system_azure_openai_lseval.yaml
| judge_tokens = overall.get("total_judge_llm_tokens", -1) | ||
| judge_detail = ( | ||
| "0 → OLS calls failed before judge was reached" | ||
| if judge_tokens == 0 | ||
| else "judge was called" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle unavailable judge-token data separately.
When total_judge_llm_tokens is missing, the fallback -1 is treated as evidence that the judge was called because only 0 selects the failure message. Report negative or missing values as “unavailable” instead.
🤖 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 `@tests/e2e/evaluation/test_lseval_periodic.py` around lines 197 - 202, Update
the judge_detail selection near overall.get("total_judge_llm_tokens", -1) so
missing or negative total_judge_llm_tokens values report “unavailable,” while
zero retains the failed-before-judge message and positive values retain “judge
was called.”
207512d to
f06f26f
Compare
f06f26f to
a47bc3b
Compare
|
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. |
|
/retest |
|
is this still needed? |
|
/retest |
|
@raptorsun the change is still reqired as it enables the models without the params to run, it is ready to merge as tests have passed. PTAL :) |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: onmete 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
d6779cd
into
openshift:main
4 participants
Description
gpt 5 mini doesnt support temperature param, removing it temporarily and removing threshold, as only 50 percent of evals are passing in ci
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Evaluation Updates
Testing