OLS-3548 Omit temperature for models that deprecate it - #3000
Conversation
|
[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.
🧹 Nitpick comments (1)
ols/src/llms/providers/provider.py (1)
382-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify
_model_supports_temperatureby removing the deadgetattr/Nonebranch.
ModelConfig.parametersis declared asparameters: ModelParameters = ModelParameters(), so it always carries a value — thegetattrfallback and theif params is Noneguard can never trigger. Direct access is cleaner and preserves full type information for mypy strict mode.♻️ Proposed refactor
def _model_supports_temperature(self) -> bool: """Check whether the current model supports the temperature parameter.""" if self.provider_config is None: return True model_config = self.provider_config.models.get(self.model) if model_config is None: return True - params = getattr(model_config, "parameters", None) - if params is None: - return True - return params.temperature_supported + return model_config.parameters.temperature_supported🤖 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 `@ols/src/llms/providers/provider.py` around lines 382 - 392, Update _model_supports_temperature to access model_config.parameters directly and return its temperature_supported value, removing the getattr fallback and params None guard while preserving the existing provider_config and model_config None handling.
🤖 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 `@ols/src/llms/providers/provider.py`:
- Around line 382-392: Update _model_supports_temperature to access
model_config.parameters directly and return its temperature_supported value,
removing the getattr fallback and params None guard while preserving the
existing provider_config and model_config None handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 30b796f3-9d81-46ad-a570-ee0316ca07bf
📒 Files selected for processing (4)
examples/olsconfig.yamlols/app/models/config.pyols/src/llms/providers/provider.pytests/unit/llms/providers/test_bedrock.py
|
Addressed the CodeRabbit nitpick — removed the dead All 22 bedrock provider tests pass. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Code reviewFound 2 issues:
lightspeed-service/ols/src/llms/providers/provider.py Lines 370 to 379 in fee421e
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
Additional note (lower priority): The new |
|
But @thoraxe - we will need the operator changes to go along with this change right - the user will have to configure this in the CR. |
Add a temperature_supported model parameter (default: true) that lets operators flag models which reject the temperature kwarg. When false, the provider strips temperature from params and logs a warning — even if dev_config overrides set it, since the model will error either way. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No operator changes are needed — This is purely a bug fix: models that reject temperature were getting it sent anyway. A user could configure it, but that's already possible through the existing parameters passthrough. |
fee421e to
efc23a8
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a ChangesTemperature capability configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Provider
participant ModelConfig
Caller->>Provider: call with temperature parameter
Provider->>ModelConfig: check temperature_supported
alt temperature_supported = true
Provider->>Provider: keep temperature in params
else temperature_supported = false
Provider->>Provider: remove temperature from params
Provider->>Provider: log warning
end
Provider->>Caller: return result
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/llms/providers/test_bedrock.py (1)
354-386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover developer-config overrides as well.
This test covers caller-supplied
temperature, butols/src/llms/providers/provider.pyalso mergesconfig.dev_config.llm_paramsbefore stripping unsupported parameters. Add a regression case setting developer-config temperature withtemperature_supported=Falseand assert it is omitted.🤖 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/unit/llms/providers/test_bedrock.py` around lines 354 - 386, Add a regression test alongside test_temperature_stripped_even_when_caller_passes_it that sets config.dev_config.llm_params temperature while temperature_supported is False, then constructs and loads Bedrock and asserts temperature is absent from both bedrock.params and ChatBedrockConverse call_kwargs. Reuse the existing provider setup and patching pattern.
🤖 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 `@examples/olsconfig.yaml`:
- Line 16: Update the OLS configuration example around temperature_supported so
it is not grouped with the reasoning-only parameters guidance. Provide a
separate example or revise the surrounding comment to clarify that
temperature_supported is a general model capability flag and may be needed for
non-reasoning models that reject temperature.
---
Nitpick comments:
In `@tests/unit/llms/providers/test_bedrock.py`:
- Around line 354-386: Add a regression test alongside
test_temperature_stripped_even_when_caller_passes_it that sets
config.dev_config.llm_params temperature while temperature_supported is False,
then constructs and loads Bedrock and asserts temperature is absent from both
bedrock.params and ChatBedrockConverse call_kwargs. Reuse the existing provider
setup and patching pattern.
🪄 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 Plus
Run ID: 31419fed-1f7a-4df5-b540-188d29c4b57a
📒 Files selected for processing (5)
examples/olsconfig.yamlols/app/models/config.pyols/src/llms/providers/provider.pytests/unit/app/models/test_config.pytests/unit/llms/providers/test_bedrock.py
- Move temperature_supported out of the reasoning-only example block in olsconfig.yaml and into a dedicated Bedrock section comment. The flag applies to any model that rejects temperature, not only reasoning models, so grouping it with reasoning_effort/reasoning_summary was misleading. - Add test_temperature_stripped_when_set_via_dev_config to test_bedrock.py to cover the case where config.dev_config.llm_params sets temperature but temperature_supported=False. Previously only caller-supplied and default temperature stripping were covered. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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/unit/llms/providers/test_bedrock.py`:
- Around line 393-422: Update test_temperature_stripped_when_set_via_dev_config
to use pytest monkeypatching (or a try/finally cleanup) when overriding
config.dev_config.llm_params, ensuring the original value is restored
automatically even if setup or assertions fail. Remove the manual reset that can
discard a pre-existing developer configuration.
🪄 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 Plus
Run ID: f9780235-cd2d-4878-a838-f76b9c67c811
📒 Files selected for processing (2)
examples/olsconfig.yamltests/unit/llms/providers/test_bedrock.py
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/olsconfig.yaml
|
/retest |
…rder Replace direct global mutation of config.dev_config.llm_params with pytest monkeypatch so the fixture is always restored after the test, even on assertion failure. Add an inline comment to _override_params explaining why temperature stripping is applied after dev_config merging: it is a hard physical capability check, not a precedence decision. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Re: xrajesh's concern about temperature stripping overriding dev_config precedence The behavior is intentional and not a bug — here's the reasoning:
The analogy: To make this intent clear to future readers, I added an inline comment in All 23 bedrock tests pass. |
|
/retest |
1 similar comment
|
/retest |
|
@thoraxe: The following test failed, say
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. |
2 participants
Summary
temperature_supportedboolean flag toModelParameters(defaults toTruefor backward compatibility)LLMProviderstripstemperaturefrom LLM params when the model's config setstemperature_supported: falseclaude-sonnet-5that have deprecated thetemperatureparameterHow to use
In
olsconfig.yaml, settemperature_supported: falseon any model that rejectstemperature:Models without this setting (or with
temperature_supported: true) continue to receivetemperatureas before.Test plan
temperature_supported=Falsetemperature_supported=True(default)Summary by CodeRabbit
New Features
temperaturefor models that do not support it; supported models retain existing behavior.Documentation
Tests