Skip to content

fix(mcp): omit temperature for Anthropic/Gemini/Groq when unset (#1103)#1570

Open
danielchalef wants to merge 1 commit into
mainfrom
ci/mcp-temp-default
Open

fix(mcp): omit temperature for Anthropic/Gemini/Groq when unset (#1103)#1570
danielchalef wants to merge 1 commit into
mainfrom
ci/mcp-temp-default

Conversation

@danielchalef

Copy link
Copy Markdown
Member

Summary

The MCP temperature config defaults to None. The OpenAI client omits temperature for reasoning models, but the Anthropic / Gemini / Groq clients pass config.temperature to their provider APIs verbatim — so a None is serialized as temperature: null and rejected (the Anthropic 400 in #1103).

This builds the core LLMConfig via a small _build_graphiti_llm_config helper that omits temperature (and base_url) when unset, so core's LLMConfig default (1.0) applies. An explicitly-set temperature is preserved.

Verified against the pinned graphiti-core:

  • LLMConfig().temperature == 1.0
  • AnthropicClient (anthropic_client.py:291), GroqClient (:75), GeminiClient (:294) all pass temperature=self.temperature unconditionally.

Provenance

Reimplements #1464 (rafaelreis-r/Yifan… community fix — credit to the original author) directly on main; the original was CI-red and behind. Same approach + the same test intent, extended to a parametrized suite over all three providers.

Tests

Added to mcp_server/tests/test_factories.py (TestNonOpenAITemperatureDefault): None → core default 1.0 for anthropic/gemini/groq, and an explicit value is preserved. 28/28 factory tests pass locally, ruff clean.

Fixes #1103

🤖 Generated with Claude Code

The MCP temperature defaults to None. Unlike the OpenAI client (which omits
temperature for reasoning models), the Anthropic/Gemini/Groq clients pass
config.temperature to their provider APIs verbatim, so a None is sent as
`temperature: null` and rejected (the Anthropic 400 in #1103). Build the core
LLMConfig via a helper that omits temperature (and base_url) when unset, so
core's LLMConfig default (1.0) applies; an explicit value is still preserved.

Reimplements #1464 (which was CI-red and behind) directly on main, with unit
tests covering all three providers.

Fixes #1103

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @danielchalef's task in 2m 9s —— View job


PR Review: omit temperature for Anthropic/Gemini/Groq when unset

I read the diff and cross-checked the claims against the pinned graphiti-core. The fix is correct, well-scoped, and the helper approach is clean. Verified facts:

  • LLMConfig default temperature is 1 (graphiti_core/llm_client/config.py:20,42) ✅
  • AnthropicClient (anthropic_client.py:291), GeminiClient (gemini_client.py:294), GroqClient (groq_client.py:75) all pass temperature=self.temperature unconditionally
  • OpenAIClient correctly omits None (openai_client.py:130), so leaving the OpenAI branch unchanged is justified ✅

Tests are well-structured (parametrized over all three providers + explicit-value preservation). Nice.


Findings

⚠️ Warning — the same bug still exists on the OpenAI→generic path (Ollama/LM Studio). Confidence: high (code), medium (impact).

The OpenAI branch builds its llm_config with temperature=config.temperature directly (factories.py:173-182) — not via the new helper — and can still be None. When is_non_openai_provider(api_url) is true it returns OpenAIGenericClient (factories.py:187-190), and that client passes temperature verbatim to the chat-completions API:

# graphiti_core/llm_client/openai_generic_client.py:158
temperature=self.temperature,   # unconditional, same as anthropic/gemini/groq

So a user running provider: openai against Ollama / LM Studio / vLLM with temperature unset hits the identical temperature: null class of bug this PR fixes. Whether it actually 400s depends on the server's strictness (OpenAI itself tolerates it; some compatible servers don't), which is why I rate impact medium.

This is arguably out of scope for "fix #1103", but the PR title frames it as the general "non-OpenAI clients pass temperature verbatim" problem, and the generic client is exactly that. Worth either fixing here (route the generic path through _build_graphiti_llm_config too) or noting as a known follow-up. Fix this →

💡 Suggestion — kwargs: dict (factories.py:107). Bare dict is fine but dict[str, object]/dict[str, Any] reads better under the project's pyright config. Trivial.

💡 Suggestion — base_url omission for Groq. The helper now omits base_url when None, whereas the old Groq branch passed base_url=config.providers.groq.api_url unconditionally. Benign (core's default applies, and api_url is normally set), just noting the behavior change so it's intentional.

💡 Suggestion — test assertion == 1. assert client.temperature == 1 works (int==float), but == 1.0 would read more clearly as "core's float default." Cosmetic.


Summary

The core fix is correct and verified. No blocking issues. The one thing I'd consider before merge is the OpenAIGenericClient gap — it's the same root cause and the PR's framing implies a general fix. Everything else is cosmetic.

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.

[BUG] MCP Server: Anthropic provider fails with 400 error when temperature not explicitly set

1 participant