-
Notifications
You must be signed in to change notification settings - Fork 8.3k
fix(llms): send reasoning_effort to every openai reasoning model #7187
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,8 @@ | |
| import json | ||
| import logging | ||
| import os | ||
| from typing import TYPE_CHECKING, Any, ClassVar, Literal, TypedDict | ||
| import re | ||
| from typing import TYPE_CHECKING, Any, ClassVar, Final, Literal, TypedDict | ||
|
|
||
| import httpx | ||
| from openai import ( | ||
|
|
@@ -70,6 +71,27 @@ | |
| # per process rather than on every call. | ||
| _LEARNED_RESPONSES_ONLY_MODELS: set[str] = set() | ||
|
|
||
| # `reasoning_effort` is accepted by the o-series and by GPT generation 5 onwards. | ||
| # Matched by shape rather than by a list of names so a new member of an existing | ||
| # family works without a release here; `gpt-4o` and `gpt-4.1` parse to generation | ||
| # 4 and are excluded. An over-match is recovered rather than fatal -- the call is | ||
| # retried without the parameter when the API rejects it. | ||
| _O_SERIES_MODEL = re.compile(r"^o\d") | ||
| _GPT_GENERATION = re.compile(r"^gpt-(\d+)") | ||
| _MIN_REASONING_GPT_GENERATION: Final[int] = 5 | ||
|
|
||
|
|
||
| def _supports_reasoning_effort(model: str) -> bool: | ||
| """Whether the model accepts `reasoning_effort` on /v1/chat/completions.""" | ||
| name = model.rsplit("/", 1)[-1].lower() | ||
| if _O_SERIES_MODEL.match(name): | ||
| return True | ||
| generation = _GPT_GENERATION.match(name) | ||
| return ( | ||
| generation is not None | ||
| and int(generation.group(1)) >= _MIN_REASONING_GPT_GENERATION | ||
| ) | ||
|
|
||
|
|
||
| class WebSearchResult(TypedDict, total=False): | ||
| """Result from web search built-in tool.""" | ||
|
|
@@ -552,6 +574,16 @@ def dispatch(params: dict[str, Any]) -> str | Any: | |
| ) | ||
| return dispatch(retry_params) | ||
|
|
||
| if self._rejects_reasoning_effort_as_unsupported(cause): | ||
| retry_params = self._without_reasoning_effort(completion_params) | ||
| if retry_params is not None: | ||
| logging.debug( | ||
| "Retrying %r without reasoning_effort: the model does " | ||
| "not support the parameter.", | ||
| self.model, | ||
| ) | ||
| return dispatch(retry_params) | ||
|
|
||
| if self.custom_openai or not self._is_responses_only_error(cause): | ||
| raise | ||
| self._remember_responses_only_model() | ||
|
|
@@ -683,6 +715,16 @@ async def dispatch(params: dict[str, Any]) -> str | Any: | |
| if retry_params is not None: | ||
| return await dispatch(retry_params) | ||
|
|
||
| if self._rejects_reasoning_effort_as_unsupported(cause): | ||
| retry_params = self._without_reasoning_effort(completion_params) | ||
| if retry_params is not None: | ||
| logging.debug( | ||
| "Retrying %r without reasoning_effort: the model does " | ||
| "not support the parameter.", | ||
| self.model, | ||
| ) | ||
| return await dispatch(retry_params) | ||
|
|
||
| if self.custom_openai or not self._is_responses_only_error(cause): | ||
| raise | ||
| self._remember_responses_only_model() | ||
|
|
@@ -1765,6 +1807,49 @@ def _rejects_reasoning_effort_with_tools(error: BaseException) -> bool: | |
| message = str(source.get("message") or "").lower() | ||
| return "function tools" in message and "reasoning_effort" in message | ||
|
|
||
| @staticmethod | ||
| def _rejects_reasoning_effort_as_unsupported(error: BaseException) -> bool: | ||
| """Whether a 400 is OpenAI refusing `reasoning_effort` for this model. | ||
|
|
||
| Non-reasoning models reject the parameter itself, in one of two shapes: | ||
|
|
||
| {"code": "unsupported_parameter", "param": "reasoning_effort", | ||
| "message": "Unsupported parameter: 'reasoning_effort' is not | ||
| supported with this model."} | ||
|
|
||
| {"param": null, "message": "Unrecognized request argument | ||
| supplied: reasoning_effort"} | ||
|
|
||
| Distinct from `_rejects_reasoning_effort_with_tools`: that is a | ||
| reasoning model refusing the parameter only alongside function tools, | ||
| and it recovers by sending "none" rather than by dropping the key. Also | ||
| deliberately does not match the "Unsupported value" 400 that o1/o3 | ||
| return for a bad *value* -- the model does support the parameter, so | ||
| silently dropping it would restore the very bug this recovers from. | ||
| """ | ||
| if not isinstance(error, BadRequestError): | ||
| return False | ||
| body = getattr(error, "body", None) | ||
| source = None | ||
| if isinstance(body, dict): | ||
| inner = body.get("error") | ||
| source = inner if isinstance(inner, dict) else body | ||
| if not isinstance(source, dict): | ||
| return False | ||
| message = str(source.get("message") or "").lower() | ||
| if "reasoning_effort" not in message: | ||
| return False | ||
| if source.get("code") == "unsupported_parameter": | ||
| return True | ||
| return "unrecognized request argument" in message | ||
|
|
||
| @staticmethod | ||
| def _without_reasoning_effort(params: dict[str, Any]) -> dict[str, Any] | None: | ||
| """Params with `reasoning_effort` removed, or None if it was not set.""" | ||
| if "reasoning_effort" not in params: | ||
| return None | ||
| return {k: v for k, v in params.items() if k != "reasoning_effort"} | ||
|
|
||
| def _reasoning_effort_none_params( | ||
| self, params: dict[str, Any] | ||
| ) -> dict[str, Any] | None: | ||
|
|
@@ -1829,8 +1914,11 @@ def _prepare_completion_params( | |
| if self.top_logprobs is not None: | ||
| params["top_logprobs"] = self.top_logprobs | ||
|
|
||
| # Handle o1 model specific parameters | ||
| if self.is_o1_model and self.reasoning_effort: | ||
| # Not gated on `is_o1_model`: that is a literal "o1" substring test, so | ||
| # gpt-5, o3 and o4-mini failed it and silently thought at the server | ||
| # default. It also drives tool support and message rewriting, so it | ||
| # cannot be widened to mean "is a reasoning model". | ||
| if self.reasoning_effort and _supports_reasoning_effort(self.model): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- repository conventions and learnings ---'
find /tmp/coderabbit-repo-knowledge/crewaiinc-crewai-a921f2d1 -maxdepth 2 -type f -name '*.md' -print | sort
for f in /tmp/coderabbit-repo-knowledge/crewaiinc-crewai-a921f2d1/*/*.md; do
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
done
printf '%s\n' '--- target file outline ---'
ast-grep outline lib/crewai/src/crewai/llms/providers/openai/completion.py
printf '%s\n' '--- target hunk and directly bound definitions ---'
sed -n '1860,1960p' lib/crewai/src/crewai/llms/providers/openai/completion.py
rg -n -C 8 'def _supports_reasoning_effort|_supports_reasoning_effort|custom_openai|reasoning_effort' \
lib/crewai/src/crewai/llms/providers/openai/completion.pyRepository: crewAIInc/crewAI Length of output: 42906 Forward When Bypass the hosted model-name check for 🤖 Prompt for AI Agents |
||
| params["reasoning_effort"] = self.reasoning_effort | ||
|
|
||
| if self.response_format is not None: | ||
|
|
@@ -2043,9 +2131,11 @@ def _handle_completion( | |
| logging.error(f"Context window exceeded: {e}") | ||
| raise LLMContextLengthExceededError(str(e)) from e | ||
|
|
||
| # `_call_completions` retries this one, so reporting a failed call | ||
| # `_call_completions` retries these, so reporting a failed call | ||
| # here would surface an error the caller never experiences. | ||
| if self._rejects_reasoning_effort_with_tools(e): | ||
| if self._rejects_reasoning_effort_with_tools( | ||
| e | ||
| ) or self._rejects_reasoning_effort_as_unsupported(e): | ||
| raise | ||
|
|
||
| error_msg = f"OpenAI API call failed: {e!s}" | ||
|
|
@@ -2471,9 +2561,11 @@ async def _ahandle_completion( | |
| logging.error(f"Context window exceeded: {e}") | ||
| raise LLMContextLengthExceededError(str(e)) from e | ||
|
|
||
| # `_call_completions` retries this one, so reporting a failed call | ||
| # `_call_completions` retries these, so reporting a failed call | ||
| # here would surface an error the caller never experiences. | ||
| if self._rejects_reasoning_effort_with_tools(e): | ||
| if self._rejects_reasoning_effort_with_tools( | ||
| e | ||
| ) or self._rejects_reasoning_effort_as_unsupported(e): | ||
| raise | ||
|
|
||
| error_msg = f"OpenAI API call failed: {e!s}" | ||
|
|
||
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.
Shape matcher drops valid reasoning models
Medium Severity
_supports_reasoning_effortonly treats o-series andgpt-N(N≥5) as reasoning models, soreasoning_effortis omitted for names that do not match, includinggpt-oss-*,ft:fine-tunes, and models on the inherited OpenAI-compatible path. Those requests succeed without the setting, and the 400 retry never runs.Additional Locations (1)
lib/crewai/src/crewai/llms/providers/openai/completion.py#L1920-L1922Reviewed by Cursor Bugbot for commit 2a16198. Configure here.