Treat MiniMax M2 thinking as always on - #2238
Conversation
|
| Filename | Overview |
|---|---|
| unstract/sdk1/src/unstract/sdk1/adapters/base1.py | Centralizes MiniMax thinking normalization and changes M2 handling to remove explicit thinking configuration. |
| unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/minimax.json | Updates the enable_thinking description to document the M2 always-on contract. |
| unstract/sdk1/tests/test_branded_openai_adapters.py | Replaces adaptive-thinking expectations with focused validation tests for M2 always-on behavior. |
Reviews (3): Last reviewed commit: "Strip MiniMax M2 thinking instead of rai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Thanks for the review. Commit a235538 now omits the thinking parameter for MiniMax M2 requests and aligns schema matching with runtime handling. I ran 64 adapter tests plus Ruff format and lint checks. |
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review angle, so the inline comments make sense together.
The underlying fact checks out: per MiniMax's docs, M2.x thinking is always on. But the same docs also say thinking: {"type": "disabled"} is accepted and ignored, not rejected. So the previously-injected {"type": "adaptive"} was a no-op on the wire, and this is a hygiene change rather than a bug fix. Worth being explicit about that since there's no ticket behind the PR.
Happy to take the Python refactor. Extracting _normalize_minimax_thinking() is a genuine readability win over the inline block it replaces, and no longer sending a parameter the model ignores is correct.
Asking to drop three things that add failure surface without buying behaviour: the new raise on an explicit thinking payload, the validated.pop, and the allOf conditional in the schema. Details inline. That should land around +25/-20 instead of +98/-24.
Verified locally: the suite passes on this branch (64 tests), and MiniMaxLLMParameters(model='minimax/MiniMax-M2.7', api_key='k').model_dump() returns thinking: None — which is what the pop is deleting.
| if is_m2_model: | ||
| raise ValueError( | ||
| f"{model_id} uses always-on thinking and does not accept " | ||
| "thinking configuration." | ||
| ) |
There was a problem hiding this comment.
Suggest stripping rather than raising here.
The MiniMax docs say the opposite of this message: "For M2.x models, thinking cannot be disabled; thinking: {"type": "disabled"} is accepted but thinking remains on." The API takes the parameter, it just ignores it.
Practical cost: this turns a currently-working call into a hard failure at LLM.__init__ (llm.py:250), which runs on every completion and on Test Connection — not only at save time. And the form schema has no thinking property, so it can only ever fire for SDK/API callers, who are the least served by a message that contradicts the provider.
The enable_thinking: false raise above is the one worth keeping — that's a user explicitly asking for something impossible.
| if is_m2_model: | |
| raise ValueError( | |
| f"{model_id} uses always-on thinking and does not accept " | |
| "thinking configuration." | |
| ) | |
| if is_m2_model: | |
| # M2.x always thinks; the provider accepts this parameter but ignores it. | |
| adapter_metadata.pop("thinking") | |
| return |
| if _is_minimax_m2_model(model_id): | ||
| validated.pop("thinking", None) |
There was a problem hiding this comment.
This only ever deletes a None.
By the time it runs, thinking is guaranteed absent from adapter_metadata for M2 — the helper raises if it was passed, and the enable_thinking branch skips M2 — so Pydantic emits the declared field as None:
MiniMaxLLMParameters(model='minimax/MiniMax-M2.7', api_key='k').model_dump()
→ thinking key present: True | value: None
That's the same None the M3 path keeps and has shipped with since #2166. Popping it only for M2 leaves two shapes for "no thinking" (M2: key absent, M3: key None), which is an easy thing to trip over later.
Suggest deleting both lines. Re-validation stays safe — the helper short-circuits on thinking is None, confirmed with validate({'model': 'MiniMax-M2.7', 'api_key': 'k', 'thinking': None}).
The assertion at line 192 then becomes assert validated["thinking"] is None.
| "allOf": [ | ||
| { | ||
| "if": { | ||
| "properties": { | ||
| "model": { | ||
| "pattern": "^(?:(?:minimax|anthropic)/)?[Mm][Ii][Nn][Ii][Mm][Aa][Xx]-[Mm]2(?:$|[.-])" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "model" | ||
| ] | ||
| }, | ||
| "then": { | ||
| "properties": { | ||
| "enable_thinking": { | ||
| "const": true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
Suggest dropping this block.
It encodes "what is an M2 model" a third time — _is_minimax_m2_model (base1.py:557) and the prefix-stripping in validate_model are the other two — in a different language, with hand-expanded case-insensitivity because JSON Schema has no /i flag. Nothing fails if the two drift, so the next model family silently loses the hint while tests stay green.
One thing worth knowing before merging either way: I ran this schema through @rjsf/utils@5 + validator-ajv8@5, which is what RjsfFormLayout.jsx uses. The condition does resolve correctly — M2 gets const: true and the checkbox defaults to checked — but unchecking yields two errors, and the second is raw ajv output:
[".enable_thinking must be equal to constant", "must match \"then\" schema"]
transformErrors (RjsfFormLayout.jsx:177) has no if case, so must match "then" schema reaches the user. No other adapter schema in the repo uses allOf/if, so this would become the pattern others copy.
The reworded description just above already conveys always-on, and the backend still rejects enable_thinking: false. If we want form-time feedback later, ui:widget: hidden for M2 reads better than a checkbox that accepts only one value — but it needs the same duplication solved, so it belongs in its own change.
| "standard", | ||
| "priority", | ||
| ] | ||
| assert schema["allOf"][0]["then"]["properties"]["enable_thinking"] == {"const": True} |
There was a problem hiding this comment.
allOf[0] couples this to schema layout — add a second conditional and it silently asserts the wrong branch. The Draft202012Validator loop just below already covers the same rule through behaviour, which is the version worth keeping.
Also from jsonschema import Draft202012Validator at line 329 should sit at the top of the file per our import convention.
Both go away if the allOf block is dropped.
Drop the raise on an explicit thinking payload, the M2-only validated.pop, and the allOf conditional in the adapter schema.
|
Thanks for the review. I dropped all three as requested and pushed commit 4ad8ebd.
The Checks: |
|
2 participants
Reason: Align MiniMax-M2.7 requests with its always-on thinking contract.
Checks:
uv run --project unstract/sdk1 --locked ruff check --ignore I001 unstract/sdk1/src/unstract/sdk1/adapters/base1.py unstract/sdk1/tests/test_branded_openai_adapters.pyuv run --project unstract/sdk1 --locked ruff format --check unstract/sdk1/src/unstract/sdk1/adapters/base1.py unstract/sdk1/tests/test_branded_openai_adapters.pyuv run --project unstract/sdk1 --locked pytest -q unstract/sdk1/tests/test_branded_openai_adapters.py -k 'minimax_m2 or minimax_schema_covers'git diff --check