Skip to content

fix: allow omitting temperature for models that reject it (e.g. Opus 4.7+) - #2109

Merged
moshemorad merged 1 commit into
HolmesGPT:masterfrom
yakir-shriker:fix/omit-temperature-for-unsupported-models
Jun 1, 2026
Merged

moshemorad merged 1 commit into
HolmesGPT:masterfrom
yakir-shriker:fix/omit-temperature-for-unsupported-models

Conversation

@yakir-shriker

@yakir-shriker yakir-shriker commented May 31, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Anthropic Opus 4.7 / 4.8 reject the temperature parameter:

invalid_request_error: `temperature` is deprecated for this model.

HolmesGPT always sends temperature (tool_calling_llm.py → completion(..., temperature=TEMPERATURE), default 1e-8), so these models 400 on every call. drop_params=True doesn't help — the deprecation is an Anthropic runtime behavior, not in LiteLLM's static param metadata (verified with litellm up to 1.86.2), so it isn't stripped.

Fix

Make TEMPERATURE omittable: an empty string / none / null yields None, and completion() already skips the param when it's None — so no temperature is sent. Models that accept temperature are unaffected (default unchanged).

TEMPERATURE=none   # send no temperature param

Verification

  • Unit tests added (tests/common/test_temperature_env.py).
  • End-to-end via a LiteLLM gateway: with TEMPERATURE=none, Opus 4.6 / 4.7 / 4.8 all complete successfully; without it, 4.7/4.8 return the 400 above.

Signed-off-by: Yakir Shriker yakirshr@gmail.com

Summary by CodeRabbit

  • New Features

    • Temperature setting can now be disabled or nullified by setting the environment variable to empty string, "none", or "null" values, providing more flexible configuration control.
  • Tests

    • Added tests verifying temperature configuration behavior under various conditions, including disabled states and default values.

…4.7+)

Anthropic Opus 4.7/4.8 reject the `temperature` parameter ("temperature is
deprecated for this model"), and LiteLLM's drop_params can't strip it (the
deprecation isn't in its static param metadata). Holmes always sent temperature
(TEMPERATURE default 1e-8), so these models 400'd.

Make TEMPERATURE omittable: an empty string / "none" / "null" yields None, and
completion() already skips the param when it's None — so no temperature is sent.
Verified against Opus 4.6/4.7/4.8 via a LiteLLM gateway.

Signed-off-by: Yakir Shriker <yakirshr@gmail.com>

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented May 31, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

A new _load_temperature() helper function is added to holmes/common/env_vars.py that parses the TEMPERATURE environment variable, returning None when set to empty, "none", or "null" values, and returning the float value otherwise with a default of 0.00000001. The module constant TEMPERATURE now has type Optional[float]. Test coverage verifies all three parsing behaviors.

Changes

Temperature optional parsing

Layer / File(s) Summary
Temperature parsing implementation
holmes/common/env_vars.py
New _load_temperature() function normalizes TEMPERATURE by returning None for empty/disabled strings ("", "none", "null") and converting valid numeric strings to float, with default 0.00000001 when unset. Module constant TEMPERATURE is now Optional[float] and computed via this helper.
Temperature parsing tests
tests/common/test_temperature_env.py
Three test functions patch os.environ to verify _load_temperature() returns None for disabled values, returns the parsed float for numeric values like "0.5", and returns the default 0.00000001 when unset.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • HolmesGPT/holmesgpt#1962: Updates DefaultLLM.completion to handle temperature=None values that result from the new optional temperature parsing by stripping None before forwarding to providers.
  • HolmesGPT/holmesgpt#698: Changes how TEMPERATURE is sourced and passed to LLM completion calls to work with the new optional temperature value.

Suggested reviewers

  • arikalon1
  • naomi-robusta
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: making temperature omittable for models that reject it (e.g., Opus 4.7+), which directly corresponds to the code changes that make TEMPERATURE optional and the feature to interpret specific values as None.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@netlify

netlify Bot commented May 31, 2026

Copy link
Copy Markdown

✅ Deploy Preview for holmes-docs ready!

Name Link
🔨 Latest commit 742500b
🔍 Latest deploy log https://app.netlify.com/projects/holmes-docs/deploys/6a1c5c59c7e3970008456b79
😎 Deploy Preview https://deploy-preview-2109--holmes-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
holmes/common/env_vars.py (1)

68-77: ⚡ Quick win

Consider adding error handling for invalid float values.

If TEMPERATURE is set to an invalid numeric string (e.g., "abc"), the float(raw) call will raise a ValueError at startup. While this fails fast and is easy to debug, wrapping it in a try-except with a more descriptive error message could improve the developer experience.

🛡️ Proposed error handling enhancement
 def _load_temperature() -> Optional[float]:
     # Set TEMPERATURE to an empty string / "none" / "null" to send NO temperature at
     # all. Required for models that reject the parameter (e.g. Anthropic Opus 4.7+:
     # "temperature is deprecated for this model"); LiteLLM's drop_params can't strip
     # it because the deprecation isn't in its static param metadata, so it must be
     # omitted at the source.
     raw = os.environ.get("TEMPERATURE", "0.00000001").strip()
     if raw.lower() in ("", "none", "null"):
         return None
-    return float(raw)
+    try:
+        return float(raw)
+    except ValueError as e:
+        raise ValueError(f"Invalid TEMPERATURE value '{raw}': must be a number, 'none', 'null', or empty") from e
🤖 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 `@holmes/common/env_vars.py` around lines 68 - 77, The _load_temperature
function should guard against invalid numeric strings from the TEMPERATURE env
var: wrap the float(raw) conversion in a try/except ValueError and raise a new
ValueError (or use process logging then re-raise) with a clear message that
includes the invalid raw value and that TEMPERATURE must be a float or one of
"", "none", "null"; update the function _load_temperature to perform this
validation around the float(raw) call and surface a descriptive error instead of
the raw ValueError.
🤖 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 `@holmes/common/env_vars.py`:
- Around line 68-77: The _load_temperature function should guard against invalid
numeric strings from the TEMPERATURE env var: wrap the float(raw) conversion in
a try/except ValueError and raise a new ValueError (or use process logging then
re-raise) with a clear message that includes the invalid raw value and that
TEMPERATURE must be a float or one of "", "none", "null"; update the function
_load_temperature to perform this validation around the float(raw) call and
surface a descriptive error instead of the raw ValueError.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 56e822ff-84f0-4619-a32e-788346e1d7d3

📥 Commits

Reviewing files that changed from the base of the PR and between 1bc25d9 and 742500b.

📒 Files selected for processing (2)
  • holmes/common/env_vars.py
  • tests/common/test_temperature_env.py

@moshemorad
moshemorad merged commit ceb77a3 into HolmesGPT:master Jun 1, 2026
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants