Add MiniMax LLM adapter#2156
Conversation
|
kapelame seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Summary by CodeRabbit
WalkthroughThis PR adds a MiniMax branded OpenAI-compatible LLM adapter to the Unstract SDK, including parameter validation, adapter metadata, a JSON configuration schema, package exports, and tests for routing, defaults, thinking behavior, and schema generation. ChangesMiniMax LLM adapter
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Configuration
participant MiniMaxLLMAdapter
participant LiteLLM
Configuration->>MiniMaxLLMAdapter: provide model and adapter parameters
MiniMaxLLMAdapter->>MiniMaxLLMAdapter: normalize model, API base, and thinking
MiniMaxLLMAdapter->>LiteLLM: route using the minimax provider
LiteLLM-->>MiniMaxLLMAdapter: resolve model usage costs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
for more information, see https://pre-commit.ci
|
| Filename | Overview |
|---|---|
| unstract/sdk1/src/unstract/sdk1/adapters/base1.py | Adds MiniMaxLLMParameters class with its own validate() and validate_model(); correctly uses native minimax/ LiteLLM prefix and maps enable_thinking → thinking.type. The thinking field is declared as a Pydantic field (unlike Anthropic/Bedrock/Gemini/VertexAI, which inject it post-model_dump), so model_dump() always emits thinking: None when unset. Also missing temperature=1 enforcement when thinking is enabled, which every other thinking-capable adapter enforces. |
| unstract/sdk1/src/unstract/sdk1/adapters/llm1/minimax.py | Clean adapter class wiring MiniMaxLLMParameters to the BaseAdapter interface; correct provider name, icon path, and adapter type. |
| unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/minimax.json | Well-formed JSON schema; enable_thinking defaults to true and exposes both regional endpoints. Correctly excludes reasoning_effort as noted in prior review threads. |
| unstract/sdk1/tests/test_branded_openai_adapters.py | 19 new test cases covering registration, model prefix idempotency, cost resolution, thinking toggle mapping, M2.7 guard, schema content, and api_base override; good coverage of the happy path. |
| unstract/sdk1/src/unstract/sdk1/adapters/llm1/init.py | MiniMaxLLMAdapter correctly registered in both imports and all. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI
participant MiniMaxLLMAdapter
participant MiniMaxLLMParameters
participant LiteLLM
participant MiniMaxAPI
UI->>MiniMaxLLMAdapter: get_json_schema()
MiniMaxLLMAdapter-->>UI: minimax.json (enable_thinking default: true)
UI->>MiniMaxLLMParameters: "validate({model, api_key, enable_thinking})"
MiniMaxLLMParameters->>MiniMaxLLMParameters: validate_model() → minimax/MiniMax-M3
MiniMaxLLMParameters->>MiniMaxLLMParameters: "pop enable_thinking → thinking={type:adaptive}"
MiniMaxLLMParameters->>MiniMaxLLMParameters: guard M2.7 + disabled
MiniMaxLLMParameters->>MiniMaxLLMParameters: model_dump()
MiniMaxLLMParameters-->>UI: "{model, api_key, api_base, temperature, thinking, ...}"
UI->>LiteLLM: "completion(**validated_params)"
LiteLLM->>LiteLLM: route via minimax/ provider
LiteLLM->>MiniMaxAPI: "POST /v1/chat/completions {thinking:{type:adaptive}}"
MiniMaxAPI-->>LiteLLM: response + usage
LiteLLM-->>UI: ChatCompletion
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI
participant MiniMaxLLMAdapter
participant MiniMaxLLMParameters
participant LiteLLM
participant MiniMaxAPI
UI->>MiniMaxLLMAdapter: get_json_schema()
MiniMaxLLMAdapter-->>UI: minimax.json (enable_thinking default: true)
UI->>MiniMaxLLMParameters: "validate({model, api_key, enable_thinking})"
MiniMaxLLMParameters->>MiniMaxLLMParameters: validate_model() → minimax/MiniMax-M3
MiniMaxLLMParameters->>MiniMaxLLMParameters: "pop enable_thinking → thinking={type:adaptive}"
MiniMaxLLMParameters->>MiniMaxLLMParameters: guard M2.7 + disabled
MiniMaxLLMParameters->>MiniMaxLLMParameters: model_dump()
MiniMaxLLMParameters-->>UI: "{model, api_key, api_base, temperature, thinking, ...}"
UI->>LiteLLM: "completion(**validated_params)"
LiteLLM->>LiteLLM: route via minimax/ provider
LiteLLM->>MiniMaxAPI: "POST /v1/chat/completions {thinking:{type:adaptive}}"
MiniMaxAPI-->>LiteLLM: response + usage
LiteLLM-->>UI: ChatCompletion
Reviews (2): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) | ||
|
|
||
|
|
||
| class MiniMaxLLMParameters(OpenAICompatibleLLMParameters): |
There was a problem hiding this comment.
Routing through custom_openai/ silently zeroes out cost tracking for MiniMax.
_validate_branded_openai_compatible delegates to OpenAICompatibleLLMParameters.validate(), whose validate_model() unconditionally prepends custom_openai/. cost_model is only populated when the endpoint is OpenAI's own (base1.py:483), so it stays unset here, and audit.py:98 ends up calling cost_per_token(model="custom_openai/MiniMax-M3") — which raises, gets swallowed by the bare except, and records cost_in_dollars = 0.0 at debug level.
LiteLLM already ships a native MiniMax provider, and it prices M3:
litellm.get_llm_provider("minimax/MiniMax-M3") -> ('MiniMax-M3', 'minimax', None, None)
cost_per_token("custom_openai/MiniMax-M3") -> raises => cost = 0.0
cost_per_token("minimax/MiniMax-M3") -> $0.30 / $1.20 per 1M tokens
So every MiniMax token would bill as $0 in usage tracking, silently.
This is why OpenRouterLLMParameters extends BaseChatCompletionParameters instead of OpenAICompatibleLLMParameters — its own docstring says so: "Routed through LiteLLM's native openrouter/ provider so per-token costs resolve and reasoning params map without provider-specific workarounds." MiniMax should follow OpenRouter's template, not NVIDIA Build's.
Worth noting NVIDIA Build's custom_openai/ routing is correct for NVIDIA — LiteLLM prices zero nvidia_nim/ chat models (3 entries, all rerankers, all $0.0), so there's nothing to forfeit there. That reason doesn't carry over to MiniMax.
Bonus: MinimaxChatConfig.get_api_base() already returns https://api.minimax.io/v1, so going native lets you drop the hardcoded _MINIMAX_API_BASE constant entirely.
|
|
||
| @staticmethod | ||
| def get_icon() -> str: | ||
| return "/icons/adapter-icons/MiniMax.png" |
There was a problem hiding this comment.
The shipped MiniMax.png is the OpenRouter logo.
frontend/public/icons/adapter-icons/MiniMax.png is byte-for-byte identical to OpenRouter.png:
0980b0d3102c84da9d332862c563c44f MiniMax.png
0980b0d3102c84da9d332862c563c44f OpenRouter.png
676be9d2fc5cec78d5be80af03eb9fd4 NvidiaBuild.png
Both 29953 bytes. Rendering it confirms it's the OpenRouter fork-arrow mark. NvidiaBuild.png differs, so this isn't a shared-placeholder convention — it looks like the icon was copied along with the adapter template and never swapped.
Users would see OpenRouter's branding on the MiniMax card in the adapter picker. Needs the real MiniMax logo.
|
Thanks for the review. I switched the MiniMax adapter to native routing, mapped the UI toggle to |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
unstract/sdk1/tests/test_branded_openai_adapters.py (1)
48-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComprehensive MiniMax test coverage; minor gap in thinking toggle assertion.
The test suite thoroughly covers routing, idempotency, cost resolution, thinking mapping, and M2 constraints. The cost-per-token test directly validates the fix for the previous review's cost-tracking concern.
Minor gap at line 87:
assert "enable_thinking" not in enabledonly checks theenableddict. Thedisableddict should also haveenable_thinkingremoved (viapop), but this isn't asserted.🤖 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 `@unstract/sdk1/tests/test_branded_openai_adapters.py` around lines 48 - 96, Update test_minimax_maps_thinking_toggle_to_native_parameter to also assert that "enable_thinking" is absent from the disabled validation result, confirming both toggle paths remove the source parameter after mapping.unstract/sdk1/src/unstract/sdk1/adapters/base1.py (1)
534-582: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePast concern about cost tracking is resolved; flag silent
thinkingoverwrite when bothenable_thinkingandthinkingare provided.The switch from
OpenAICompatibleLLMParameterstoBaseChatCompletionParameterswith nativeminimax/prefixing addresses the previous review's critical concern aboutcustom_openai/routing zeroing out cost tracking. The testtest_minimax_native_routing_resolves_usage_costconfirmscost_per_tokenreturns positive values.One edge case: if a caller passes both
enable_thinkingandthinkinginadapter_metadata, theenable_thinkingblock (lines 552-558) silently overwrites thethinkingvalue. Consider raising or warning when both are present to avoid surprising behavior.🤖 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 `@unstract/sdk1/src/unstract/sdk1/adapters/base1.py` around lines 534 - 582, Update MiniMaxLLMParameters.validate to detect when both enable_thinking and thinking are provided before converting enable_thinking; raise a clear ValueError (or use the project’s established warning mechanism) instead of silently overwriting thinking. Preserve the existing boolean validation and conversion behavior when only enable_thinking is supplied.
🤖 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 `@unstract/sdk1/src/unstract/sdk1/adapters/base1.py`:
- Around line 534-582: Update MiniMaxLLMParameters.validate to detect when both
enable_thinking and thinking are provided before converting enable_thinking;
raise a clear ValueError (or use the project’s established warning mechanism)
instead of silently overwriting thinking. Preserve the existing boolean
validation and conversion behavior when only enable_thinking is supplied.
In `@unstract/sdk1/tests/test_branded_openai_adapters.py`:
- Around line 48-96: Update
test_minimax_maps_thinking_toggle_to_native_parameter to also assert that
"enable_thinking" is absent from the disabled validation result, confirming both
toggle paths remove the source parameter after mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57c23626-99d4-4647-819e-a1fac0fe9251
⛔ Files ignored due to path filters (1)
frontend/public/icons/adapter-icons/MiniMax.pngis excluded by!**/*.png
📒 Files selected for processing (3)
unstract/sdk1/src/unstract/sdk1/adapters/base1.pyunstract/sdk1/src/unstract/sdk1/adapters/llm1/static/minimax.jsonunstract/sdk1/tests/test_branded_openai_adapters.py
|
|
This work continues in #2166, rebuilt from the latest |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
4 participants
What
Why
How
minimax/for native routing.thinking.typerequest field and prevents disabling thinking for MiniMax-M2.7.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
litellm==1.85.1.Notes on Testing
git diff --checkpassed.libmagiclibrary on this machine.Screenshots
Checklist