Downgrade model to gpt-4o-mini#2990
Closed
fao89 wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR switches evaluation defaults, OLSConfig fixtures, e2e assertions, shell script suite arguments, and eval data from Changesgpt-4o-mini model migration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
/lgtm |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/evaluation/test_cluster_updates.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant docstring wording after model unification.
Both target and judge models are now
gpt-4o-mini, so phrasing like "using OpenAI GPT-4o-mini and gpt-4o-mini judge" reads as if two different models are used. Consider simplifying to state a single judge/target model.Also applies to: 146-146
🤖 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_cluster_updates.py` at line 1, Simplify the redundant module docstring in the cluster-updates evaluation tests so it reflects that both target and judge now use the same unified model. Update the top-level string in test_cluster_updates.py, and also the matching docstring around the referenced test block, to describe the evaluation in a single-model/judge wording instead of listing “OpenAI GPT-4o-mini and gpt-4o-mini judge.”
🤖 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/test_api.py`:
- Around line 105-109: The assertion in test_api.py is not checking the
model/provider pair correctly in the test that validates enabled models. Update
the assertion in the affected test to compare the tuple from the variables in a
single membership check, using the same `model` and `provider` symbols already
assigned in this test, so the allowed combinations are actually verified. Keep
the expected set aligned with the current model names (`gpt-4o-mini`,
`azure_openai`, `watsonx`) while fixing the assertion syntax so it evaluates the
pair instead of only `model`.
---
Nitpick comments:
In `@tests/e2e/evaluation/test_cluster_updates.py`:
- Line 1: Simplify the redundant module docstring in the cluster-updates
evaluation tests so it reflects that both target and judge now use the same
unified model. Update the top-level string in test_cluster_updates.py, and also
the matching docstring around the referenced test block, to describe the
evaluation in a single-model/judge wording instead of listing “OpenAI
GPT-4o-mini and gpt-4o-mini judge.”
🪄 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: e26646cf-318c-46fb-a41f-810ee6316db2
📒 Files selected for processing (21)
Makefileeval/system.yamleval/system_azure_openai_lseval.yamleval/system_cluster_updates.yamleval/troubleshooting/olsconfig/olsconfig-azure-openai.yamltests/config/operator_install/olsconfig.crd.azure_openai.yamltests/config/operator_install/olsconfig.crd.azure_openai_lseval.yamltests/config/operator_install/olsconfig.crd.azure_openai_tool_calling.yamltests/config/operator_install/olsconfig.crd.evaluation.yamltests/config/operator_install/olsconfig.crd.openai.yamltests/config/operator_install/olsconfig.crd.openai_data_export.yamltests/config/operator_install/olsconfig.crd.openai_mcp.yamltests/config/operator_install/olsconfig.crd.openai_quota.yamltests/config/operator_install/olsconfig.crd.openai_tool_calling.yamltests/e2e/conftest.pytests/e2e/evaluation/test_cluster_updates.pytests/e2e/test_api.pytests/scripts/test-cluster-updates.shtests/scripts/test-e2e-cluster-periodics.shtests/scripts/test-e2e-cluster.shtests/scripts/test-evaluation.sh
|
New changes are detected. LGTM label has been removed. |
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 `@scripts/evaluation/utils/constants.py`:
- Line 7: The mapping for the `openai+gpt-4o-mini` entry is inconsistent because
the key was renamed but the tuple in `constants.py` still points to
`gpt-4.1-mini`. Update that tuple to use the matching `gpt-4o-mini` model so
downstream consumers like `response_evaluation.py` resolve the correct model and
answer_id. Use the existing `azure_openai+gpt-4o-mini` entry as the reference
for the intended key/value pairing.
🪄 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: 6f1b9505-3d4a-46bf-825f-4171af36463e
📒 Files selected for processing (22)
Makefileeval/system.yamleval/system_azure_openai_lseval.yamleval/system_cluster_updates.yamleval/troubleshooting/olsconfig/olsconfig-azure-openai.yamlscripts/evaluation/utils/constants.pytests/config/operator_install/olsconfig.crd.azure_openai.yamltests/config/operator_install/olsconfig.crd.azure_openai_lseval.yamltests/config/operator_install/olsconfig.crd.azure_openai_tool_calling.yamltests/config/operator_install/olsconfig.crd.evaluation.yamltests/config/operator_install/olsconfig.crd.openai.yamltests/config/operator_install/olsconfig.crd.openai_data_export.yamltests/config/operator_install/olsconfig.crd.openai_mcp.yamltests/config/operator_install/olsconfig.crd.openai_quota.yamltests/config/operator_install/olsconfig.crd.openai_tool_calling.yamltests/e2e/conftest.pytests/e2e/evaluation/test_cluster_updates.pytests/e2e/test_api.pytests/scripts/test-cluster-updates.shtests/scripts/test-e2e-cluster-periodics.shtests/scripts/test-e2e-cluster.shtests/scripts/test-evaluation.sh
✅ Files skipped from review due to trivial changes (9)
- tests/config/operator_install/olsconfig.crd.evaluation.yaml
- eval/troubleshooting/olsconfig/olsconfig-azure-openai.yaml
- tests/scripts/test-cluster-updates.sh
- tests/e2e/evaluation/test_cluster_updates.py
- tests/config/operator_install/olsconfig.crd.openai_tool_calling.yaml
- tests/config/operator_install/olsconfig.crd.openai.yaml
- tests/config/operator_install/olsconfig.crd.openai_mcp.yaml
- Makefile
- tests/e2e/conftest.py
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/config/operator_install/olsconfig.crd.azure_openai_tool_calling.yaml
- eval/system.yaml
- tests/config/operator_install/olsconfig.crd.openai_data_export.yaml
- tests/config/operator_install/olsconfig.crd.azure_openai.yaml
- eval/system_azure_openai_lseval.yaml
- eval/system_cluster_updates.yaml
- tests/config/operator_install/olsconfig.crd.azure_openai_lseval.yaml
- tests/scripts/test-evaluation.sh
- tests/config/operator_install/olsconfig.crd.openai_quota.yaml
- tests/scripts/test-e2e-cluster-periodics.sh
- tests/scripts/test-e2e-cluster.sh
- tests/e2e/test_api.py
Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/evaluation/eval_data/question_answer_pair.json (1)
288-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompressed/malformed YAML text in
openai+gpt-4o-mini+with_raganswer.The new text strings collapse newlines and lose spacing (e.g.
key:MONGODB_DATABASE,- name :MONGODB_ADMIN_PASSWORD), unlike the properly formattedazure_openai+gpt-4o-mini+with_rag(lines 295-301) andwatsonx(lines 282-287) entries for the same eval case. Since this is captured model output used as scoring reference, it's likely faithful to the actual response, but the inconsistent formatting across providers for the same question makes the dataset harder to read/maintain.🤖 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 `@scripts/evaluation/eval_data/question_answer_pair.json` around lines 288 - 294, The `openai+gpt-4o-mini+with_rag` answer strings are collapsed and malformed, unlike the other provider entries for the same eval case. Update the `question_answer_pair.json` entry to preserve the YAML formatting in `text` with proper newlines and spacing, using the same readable structure as the neighboring `azure_openai+gpt-4o-mini+with_rag` and `watsonx` entries so the stored model output remains consistent and maintainable.
🤖 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 `@scripts/evaluation/eval_data/question_answer_pair.json`:
- Around line 288-294: The `openai+gpt-4o-mini+with_rag` answer strings are
collapsed and malformed, unlike the other provider entries for the same eval
case. Update the `question_answer_pair.json` entry to preserve the YAML
formatting in `text` with proper newlines and spacing, using the same readable
structure as the neighboring `azure_openai+gpt-4o-mini+with_rag` and `watsonx`
entries so the stored model output remains consistent and maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3fc4078-862d-400f-8abb-d31c12cee042
📒 Files selected for processing (23)
Makefileeval/system.yamleval/system_azure_openai_lseval.yamleval/system_cluster_updates.yamleval/troubleshooting/olsconfig/olsconfig-azure-openai.yamlscripts/evaluation/eval_data/question_answer_pair.jsonscripts/evaluation/utils/constants.pytests/config/operator_install/olsconfig.crd.azure_openai.yamltests/config/operator_install/olsconfig.crd.azure_openai_lseval.yamltests/config/operator_install/olsconfig.crd.azure_openai_tool_calling.yamltests/config/operator_install/olsconfig.crd.evaluation.yamltests/config/operator_install/olsconfig.crd.openai.yamltests/config/operator_install/olsconfig.crd.openai_data_export.yamltests/config/operator_install/olsconfig.crd.openai_mcp.yamltests/config/operator_install/olsconfig.crd.openai_quota.yamltests/config/operator_install/olsconfig.crd.openai_tool_calling.yamltests/e2e/conftest.pytests/e2e/evaluation/test_cluster_updates.pytests/e2e/test_api.pytests/scripts/test-cluster-updates.shtests/scripts/test-e2e-cluster-periodics.shtests/scripts/test-e2e-cluster.shtests/scripts/test-evaluation.sh
✅ Files skipped from review due to trivial changes (9)
- eval/system_azure_openai_lseval.yaml
- tests/config/operator_install/olsconfig.crd.openai_data_export.yaml
- tests/config/operator_install/olsconfig.crd.openai_tool_calling.yaml
- tests/e2e/conftest.py
- tests/e2e/evaluation/test_cluster_updates.py
- tests/config/operator_install/olsconfig.crd.evaluation.yaml
- tests/config/operator_install/olsconfig.crd.openai_quota.yaml
- tests/scripts/test-cluster-updates.sh
- tests/e2e/test_api.py
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/config/operator_install/olsconfig.crd.azure_openai.yaml
- tests/config/operator_install/olsconfig.crd.openai.yaml
- Makefile
- tests/config/operator_install/olsconfig.crd.openai_mcp.yaml
- tests/config/operator_install/olsconfig.crd.azure_openai_lseval.yaml
- eval/troubleshooting/olsconfig/olsconfig-azure-openai.yaml
- eval/system.yaml
- tests/config/operator_install/olsconfig.crd.azure_openai_tool_calling.yaml
- tests/scripts/test-e2e-cluster.sh
- tests/scripts/test-evaluation.sh
- scripts/evaluation/utils/constants.py
- eval/system_cluster_updates.yaml
- tests/scripts/test-e2e-cluster-periodics.sh
|
@fao89: all tests passed! 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. |
Member
Author
|
/cc @sriroopar |
Member
Author
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
2 participants
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.
Signed-off-by: Fabricio Aguiar fabricio.aguiar@gmail.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit