-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix: Respect reasoning_effort config for GPT-5 models #2131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix: Respect reasoning_effort config for GPT-5 models #2131
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
|
Code review suggestions addressed: ✅ Suggestion 1 (Refactor logic): Implemented in commit 956f366
❌ Suggestion 2 (getattr defensive coding): Not implementing
All relevant suggestions have been addressed. |
|
Hey @Tyler-Rak, Can you please add tests to validate this? |
956f366 to
e965ace
Compare
|
Hi @naorpeled , sure I have added the UT for this. |
| if model.endswith('_thinking'): | ||
| effort = 'low' | ||
| else: | ||
| effort = 'none' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GPT-5 supports reasoning_effort = minimal but not none. I suspect this code will throw an error on this line when passing ‘gpt-5’ as the model.
Ref: https://platform.openai.com/chat/edit?models=gpt-5-2025-08-07
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR — and sorry if my previous comment sounded blunt. I meant it purely as a technical point. Appreciate the effort here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shun-tak
Thanks for raising this up.
Yes for GPT 5 this is the case, but OpenAI changed this for later models, and here is the list:
| Model | Default | Valid Values | Notes |
|---|---|---|---|
| GPT-5 | medium | minimal, low, medium, high | Does NOT support none |
| GPT-5.1 | none | none, low, medium, high | Does NOT support minimal |
| GPT-5.2 | none | none, minimal, low, medium, high, xhigh | Supports both none and minimal + new xhigh |
I chose to use the latest setup, but yes this is risky for anyone using the old models.
How about this: when user is not setting the reasoning parameters, instead pass a default value, we ignore that parameters and let openAI decide the parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comment. Setting OpenAI's default values may lead to unintended results for PR Agent users because the default values differ depending on the model.
- gpt-5 => medium
- gpt-5.1 => none
- gpt-5.2 => none
Looking at Codex, they set reasoning effort to medium by default because it strikes a good balance between speed and accuracy. For PR Agent as well, I feel using medium is generally preferable.
Also, I'm a bit concerned about model.endswith(‘_thinking’). This means users have to configure something like ‘gpt-5_thinking’. Is that correct? I feel like users aren't aware of this feature. If it doesn't seem to be widely used, we might consider removing it in a future version. Doing so would simplify the logic around the default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, so you recommending to simply set to medium for all case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, that make sense, I have updated accordingly, by
- Simplified the default logic(I removed the thinking branch since you mentioned and I also think it's kind of concerning, but if you want to keep it for now just let me know).
- Added the new reasoning effort type into the Enum. Since I'm actually using the newer versions of GPT.
- Slightly changed the validation mechanisms.
Please let me know if the new code looks good to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the change! Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shun-tak .
Sorry this is my first contribute and I noticed that I cannot merge the PR.
May I ask will any of the owner of this repo merge this later? Is there anything else needed from my side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review the new changes soon and see if there's anything to adjust, if there won't be any changes to be done I'll merge the PR
Previously, GPT-5 models had reasoning_effort hardcoded to 'minimal', which caused two issues: 1. 'minimal' is not supported by some models (e.g., gpt-5.1-codex) 2. User's config.reasoning_effort setting was completely ignored This fix: - Reads and respects user's reasoning_effort config value - Uses valid defaults: 'none' for non-thinking models, 'low' for thinking - Adds logging to show which value is being used Fixes qodo-ai#2120
- Store validation result in variable to avoid redundant checks - Add warning log when invalid reasoning_effort value is configured - Improve source tracking in info log - Makes code more maintainable and easier to debug
- Add 23 test cases covering all aspects of reasoning_effort feature - Test valid configuration values (none, low, medium, high) - Test invalid configuration handling with proper warnings - Test model detection logic for GPT-5 variants - Test _thinking suffix handling and defaults - Test logging behavior (info and warning messages) - Verify thinking_kwargs_gpt5 structure - Test edge cases (empty strings, case sensitivity, whitespace) - All 220 tests in test suite pass
Changes: - Add XHIGH, MINIMAL, and NONE to ReasoningEffort enum to support all GPT-5 variants - Simplify reasoning_effort validation using Pythonic try/except with enum - Remove complex conditional logic for _thinking model suffix - Default to MEDIUM for all models when config is invalid or unset - Update all 25 tests to reflect new behavior - Add tests for xhigh and minimal reasoning_effort values Benefits: - Self-maintaining validation (new enum values automatically work) - Single default value (MEDIUM) instead of multiple conditional defaults - Cleaner, more readable code with fewer lines - Consistent behavior across all model types
e965ace to
e023ffe
Compare
Ensures consistency between GPT-5 and o3/o4 reasoning_effort validation. Both branches now warn users when an invalid config value is provided, improving debuggability and user experience.
User description
Fixes #2120
Problem
GPT-5 models were hardcoded to use
reasoning_effort='minimal', ignoring the user'sconfig.reasoning_effortsetting.Solution
Modified
litellm_ai_handler.pyto:reasoning_effortfrom configuration instead of hardcodingTesting
Verified with GPT-5 model that:
reasoning_effort = "medium"is now respectedPR Type
Bug fix
Description
Respect user's
reasoning_effortconfig setting for GPT-5 modelsReplace hardcoded 'minimal'/'low' values with configurable defaults
Support reasoning effort values: 'none', 'low', 'medium', 'high'
Add logging to show which reasoning effort level is being used
Diagram Walkthrough
flowchart LR A["GPT-5 Model Request"] --> B{"Model Type?"} B -->|"Thinking Model"| C["Use config or default 'low'"] B -->|"Non-thinking Model"| D["Use config or default 'none'"] C --> E["Set reasoning_effort parameter"] D --> E E --> F["Log effort level used"]File Walkthrough
litellm_ai_handler.py
Make GPT-5 reasoning_effort configurable with smart defaultspr_agent/algo/ai_handlers/litellm_ai_handler.py
reasoning_effortfromconfiginstead of hardcoding valuesmodels