-
-
Notifications
You must be signed in to change notification settings - Fork 944
fix: adjust default temperature for GPT-5 and o-series models #742
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
Conversation
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.
Pull request overview
This PR adjusts the default temperature setting for new OpenAI models (GPT-5 and o-series) by automatically setting temperature to 1.0 when these models are detected. The change addresses compatibility requirements for newer model architectures.
Changes:
- Added
FIXED_TEMP_MODELSset insgpt/app.pycontaining specific model names that require temperature 1.0 - Updated temperature setting logic in 4 locations: REPL handler, chat handler, default handler, and describe-shell workflow
- Modified
comp_argshelper intests/utils.pyto adjust expected temperature based on model name for test assertions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| sgpt/app.py | Added FIXED_TEMP_MODELS set and temperature override logic in 4 code paths for models requiring temperature 1.0 |
| tests/utils.py | Updated comp_args helper to compute expected temperature based on model prefix matching |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Mirror production logic: GPT-5 and o-series models require temperature 1.0, | ||
| # while other models default to 0.0 in tests. | ||
| if model.startswith("gpt-5") or model.startswith("o"): |
Copilot
AI
Jan 21, 2026
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.
The condition model.startswith("o") is too broad and will match any model name starting with "o", not just o-series models (o1, o2, o3). For example, this would incorrectly match models like "openai-test" or "opus-model". Consider using a more specific check such as checking if the model starts with "o" followed by a digit, or better yet, use the same set-based matching approach as in sgpt/app.py for consistency.
| if model.startswith("gpt-5") or model.startswith("o"): | |
| is_o_series = len(model) > 1 and model[0] == "o" and model[1].isdigit() | |
| if model.startswith("gpt-5") or is_o_series: |
|
|
||
| FIXED_TEMP_MODELS = { | ||
| "gpt-5", | ||
| "gpt-5-mini", | ||
| "o1-mini", | ||
| "o1-preview", | ||
| "o2-mini", | ||
| "o3-mini", | ||
| "gpt-5-nano", | ||
| "gpt-5.1", | ||
| "gpt-5.2", | ||
| "gpt-5.2-pro", | ||
| "o1", | ||
| "o3", | ||
| } | ||
|
|
||
|
|
Copilot
AI
Jan 21, 2026
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.
The FIXED_TEMP_MODELS set contains speculative future model names like "gpt-5.1", "gpt-5.2", "gpt-5.2-pro", "o2-mini", "gpt-5-nano", etc. that may not exist yet or may never exist. This approach requires the code to be updated whenever OpenAI releases new models. Consider using a prefix-based matching approach (e.g., checking if model starts with "gpt-5" or matches the pattern "o[0-9]") to automatically handle future models in these series without code changes.
| FIXED_TEMP_MODELS = { | |
| "gpt-5", | |
| "gpt-5-mini", | |
| "o1-mini", | |
| "o1-preview", | |
| "o2-mini", | |
| "o3-mini", | |
| "gpt-5-nano", | |
| "gpt-5.1", | |
| "gpt-5.2", | |
| "gpt-5.2-pro", | |
| "o1", | |
| "o3", | |
| } | |
| import re | |
| class FixedTempModels(set): | |
| """ | |
| Container for models that should use a fixed temperature. | |
| Behaves like a normal set for explicitly listed model names, but also | |
| treats models whose names match known fixed-temperature patterns as | |
| members (e.g., any "gpt-5*" or "o[0-9]*" model). | |
| """ | |
| def __contains__(self, value: object) -> bool: | |
| # Preserve normal membership behavior for explicitly listed models. | |
| if super().__contains__(value): | |
| return True | |
| # Only apply pattern logic to string model names. | |
| if not isinstance(value, str): | |
| return False | |
| # Match any gpt-5* model (e.g., gpt-5.1, gpt-5.2-pro, gpt-5-nano, etc.). | |
| if value.startswith("gpt-5"): | |
| return True | |
| # Match any o[0-9]* family model (e.g., o1, o1-mini, o2-mini, o3, o3-mini). | |
| if re.match(r"^o[0-9]", value): | |
| return True | |
| return False | |
| FIXED_TEMP_MODELS = FixedTempModels( | |
| { | |
| "gpt-5", | |
| "gpt-5-mini", | |
| "o1-mini", | |
| "o1-preview", | |
| "o2-mini", | |
| "o3-mini", | |
| "o1", | |
| "o3", | |
| } | |
| ) |
| if model in FIXED_TEMP_MODELS: | ||
| temperature = 1 |
Copilot
AI
Jan 21, 2026
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.
The condition uses in for set membership but the variable is reassigned instead of checked for the default case. When a user explicitly provides a --temperature argument (e.g., --temperature 0.5), this code will silently override their choice and set it to 1.0 for models in FIXED_TEMP_MODELS. Consider checking if the temperature is at its default value (0.0) before overriding, or at minimum warn the user that their temperature setting is being ignored.
|
|
||
| if chat: | ||
| if model in FIXED_TEMP_MODELS: | ||
| temperature = 1 |
Copilot
AI
Jan 21, 2026
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.
The temperature is set to integer 1 instead of float 1.0. While Python will handle the type conversion, it's inconsistent with the default value of 0.0 defined at line 53 and could cause type confusion. Use 1.0 for consistency.
| continue | ||
| elif option == "d": | ||
| if model in FIXED_TEMP_MODELS: | ||
| temperature = 1 |
Copilot
AI
Jan 21, 2026
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.
The temperature is set to integer 1 instead of float 1.0. While Python will handle the type conversion, it's inconsistent with the default value of 0.0 defined at line 53 and could cause type confusion. Use 1.0 for consistency.
| model = kwargs.get("model", cfg.get("DEFAULT_MODEL")) | ||
|
|
||
| # Mirror production logic: GPT-5 and o-series models require temperature 1.0, | ||
| # while other models default to 0.0 in tests. | ||
| if model.startswith("gpt-5") or model.startswith("o"): | ||
| expected_temp = 1.0 | ||
| else: | ||
| expected_temp = 0.0 |
Copilot
AI
Jan 21, 2026
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.
The test logic uses prefix matching with startswith(), while the production code in sgpt/app.py uses exact set membership matching. This creates an inconsistency where tests will pass for models like "gpt-5.3" or "o4-mini" (which aren't in the FIXED_TEMP_MODELS set), but the production code won't apply the temperature adjustment for them. The test helper should use the same FIXED_TEMP_MODELS set from sgpt/app.py to ensure tests accurately reflect production behavior.
| if repl: | ||
| # Will be in infinite loop here until user exits with Ctrl+C. | ||
| if model in FIXED_TEMP_MODELS: | ||
| temperature = 1 |
Copilot
AI
Jan 21, 2026
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.
The temperature is set to integer 1 instead of float 1.0. While Python will handle the type conversion, it's inconsistent with the default value of 0.0 defined at line 53 and could cause type confusion. Use 1.0 for consistency.
| ) | ||
| else: | ||
| if model in FIXED_TEMP_MODELS: | ||
| temperature = 1 |
Copilot
AI
Jan 21, 2026
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.
The temperature is set to integer 1 instead of float 1.0. While Python will handle the type conversion, it's inconsistent with the default value of 0.0 defined at line 53 and could cause type confusion. Use 1.0 for consistency.
Description
This PR addresses an issue where new OpenAI models (specifically
gpt-5ando-serieslikeo1-mini) require or perform better with a specific temperature setting, differing from the previous hardcoded defaults.Key Changes
sgpt/client.py: Updated completion logic to automatically settemperatureto1.0for models starting withgpt-5oro(o-series). This ensures compatibility and optimal performance for these newer architectures.tests/test_code.py&tests/test_default.py: Modified thecomp_argshelper function to mirror the production logic. This ensures that the test suite correctly validates API calls based on the model being tested while maintaining backward compatibility for older models (which still default to0.0in tests).Quality Control
./scripts/tests.sh.blackandruff(via./scripts/lint.sh).gpt-4) still receive their expected default parameters (0.0).Why this is necessary
Newer OpenAI models have different constraints and recommendations regarding the
temperatureparameter. By implementing this change,shell-gptbecomes more robust and future-proof as these models become the new defaults.Related Issue:
Fixes #735
Fixed #692