Skip to content

remove auto disable default thinking - #179

Open
jgieringer wants to merge 3 commits into
mainfrom
rm-default-disable-thinking
Open

remove auto disable default thinking#179
jgieringer wants to merge 3 commits into
mainfrom
rm-default-disable-thinking

Conversation

@jgieringer

Copy link
Copy Markdown
Collaborator

Description

v1.2 had a flag for Anthropic models that utilized thinking by default, even if thinking wasn't specified.
In favor of letting defaults be defaults, this PR removes defaults_thinking_on which would disable thinking if model parameters didn't explicitly specify to use thinking.

Copilot AI 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.

Pull request overview

This PR removes the special-casing that forcibly disabled Claude “adaptive thinking” when thinking_effort was not explicitly provided, so that model defaults remain in effect (notably for Sonnet 5–style models). It also extends the quirk registry to cover opus-5 and updates documentation/tests to reflect the new behavior.

Changes:

  • Removed the defaults_thinking_on quirk and stopped auto-injecting thinking={"type":"disabled"} when thinking_effort is unset.
  • Added opus-5 to the Claude quirk map and ensured max_tokens is defaulted appropriately where LangChain lacks a model profile.
  • Updated unit tests and docs for the adjusted “thinking” defaults and new model marker.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
llm_clients/claude_llm.py Removes defaults_thinking_on, adds opus-5 quirks, and adjusts init logging/extra-params handling around thinking.
tests/unit/llm_clients/test_claude_llm.py Updates expectations for Sonnet 5 when effort is unset and adds coverage for opus-5 max-tokens quirk.
docs/evaluating.md Removes the deprecated quirk from the documentation table and documents sparse max-tokens behavior for opus-5/sonnet-5.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/evaluating.md Outdated
Comment thread tests/unit/llm_clients/test_claude_llm.py
Comment thread docs/evaluating.md
| `adaptive_thinking` | `opus-4-7`, `opus-4-8`, `fable-5`, `sonnet-5` | Uses `thinking={"type": "adaptive"}` + `effort=<level>` instead of manual `budget_tokens` (which 400s on these models); rejects `temperature`/`top_p`/`top_k` at any non-default value. |
| `defaults_thinking_on` | `sonnet-5` | Runs adaptive thinking by default when `thinking` is omitted, so `thinking_effort` must be explicitly unset to disable it. Not needed for `fable-5`, where adaptive thinking is always on and can't be disabled at all. |
| `sparse_max_tokens_profile` | `sonnet-5` | The installed `langchain-anthropic` has no model-profile entry for this model, so it silently falls back to `max_tokens=4096` instead of the usual 64k–128k auto-set — too tight for structured output. The client sets an explicit default instead. |
| `adaptive_thinking` | `opus-4-7`, `opus-4-8`, `opus-5`, `fable-5`, `sonnet-5` | Uses `thinking={"type": "adaptive"}` + `effort=<level>` instead of manual `budget_tokens` (which 400s on these models); rejects `temperature`/`top_p`/`top_k` at any non-default value. |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if there is a way of auto populating those by looking at the code

@emily-vanark emily-vanark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

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.

4 participants