feat: add MiniMax model and endpoint support#166
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds MiniMax region and protocol configuration, native metadata for two models, provider discovery and request routing, UI/environment wiring, documentation, and automated tests for endpoint selection and Anthropic-compatible requests. ChangesMiniMax integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Starting my review of the MiniMax provider addition. I'll dig into the new endpoint routing, model metadata integration, and provider plugin to make sure everything aligns with the existing patterns. Back shortly with a bundled review. |
|
| Filename | Overview |
|---|---|
| src/rotator_library/model_info_service.py | Adds native MiniMax metadata, explicit lookup precedence, capability fields, and tiered cost calculation. |
| src/rotator_library/minimax_config.py | Defines MiniMax models, pricing, regions, protocols, and endpoint resolution. |
| src/rotator_library/providers/minimax_provider.py | Adds MiniMax model discovery and protocol-aware request routing. |
| src/rotator_library/provider_config.py | Registers MiniMax settings and preserves endpoints selected by provider hooks. |
| tests/test_minimax_provider.py | Covers model discovery, endpoint selection, request routing, native metadata, and long-context pricing. |
Reviews (3): Last reviewed commit: "docs: use ASCII MiniMax model notation" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
Solid, well-structured addition. The region/protocol endpoint selection is cleanly factored into minimax_config.py, the provider plugin follows the existing transform_request hook contract, and the native model catalog integrates nicely with the existing _native_store/priority machinery in model_info_service.py. Tests cover fallback model merging, native metadata, endpoint resolution, and an end-to-end Anthropic capture verifying /anthropic/v1/messages. Good use of graceful degradation in get_models (returns defaults on discovery failure rather than []).
Architectural Feedback
The main concern is the interaction between this provider's transform_request and the framework's convert_for_litellm step (see inline). Rewriting the model to openai/… / anthropic/… is an elegant way to reuse LiteLLM's adapters, but it also re-exposes MiniMax to those providers' _API_BASE overrides. Worth a guard so MiniMax routing can't be silently clobbered in multi-provider setups.
Key Suggestions
- Guard the post-hook
convert_for_litellmso it won't overwrite a provider hook'sapi_base/custom_llm_provider(silent misrouting risk). - Move/replace the Anthropic base-URL
ValueErrorvalidation — it's swallowed by the hook wrapper, so it currently provides no user protection. - Resolve the
MINIMAX_ANTHROPIC_BASE_URLdocumentation asymmetry.
Questions for the Author
- Are
has_tools/has_functions = Falseintentional for MiniMax-M3 given its documented tool-calling support?
This review was generated by an AI assistant.
| kwargs["model"] = f"{protocol}/{model_name}" | ||
| kwargs["api_base"] = get_minimax_endpoint(protocol=protocol) | ||
| kwargs["custom_llm_provider"] = protocol |
There was a problem hiding this comment.
Routing interaction risk: rewriting the model to openai/<name> or anthropic/<name> makes MiniMax subject to convert_for_litellm, which runs after this hook (transforms.py step 4). That step extracts the provider prefix from kwargs["model"] and, when OPENAI_API_BASE or ANTHROPIC_API_BASE is set, overwrites kwargs["api_base"] with the OpenAI/Anthropic endpoint (provider_config.py:763-768). In a multi-provider deployment where those vars are set, MiniMax requests would be silently misrouted away from the selected MiniMax endpoint, and the current tests never exercise this (they don't set those env vars).
Consider guarding against it, e.g. a sentinel so convert_for_litellm skips when the hook already set api_base/custom_llm_provider, or routing through a MiniMax-scoped provider prefix instead of borrowing openai/anthropic.
| if selected_protocol == ANTHROPIC_PROTOCOL and not endpoint.endswith("/anthropic"): | ||
| raise ValueError("MiniMax Anthropic base URL must end with /anthropic") |
There was a problem hiding this comment.
This ValueError is effectively a no-op for users: transform_request runs inside a bare except Exception in transforms.py:114-115 that only logs at debug and continues. So a misconfigured Anthropic base URL never surfaces — instead the request falls through with half-mutated kwargs (model already rewritten to anthropic/... but no api_base/custom_llm_provider), then fails opaquely at the LiteLLM layer.
Consider validating at config/startup time, or having the provider log a warning and fall back to a safe default rather than raising into a swallowed hook.
| if not override: | ||
| if selected_protocol == OPENAI_PROTOCOL: | ||
| override = os.getenv("MINIMAX_API_BASE", "").strip() | ||
| else: | ||
| override = os.getenv("MINIMAX_ANTHROPIC_BASE_URL", "").strip() |
There was a problem hiding this comment.
MINIMAX_ANTHROPIC_BASE_URL is read here but isn't documented anywhere (.env.example, README.md, DOCUMENTATION.md, or the provider_config.py extra_vars list), unlike its OpenAI counterpart MINIMAX_API_BASE. The naming is also asymmetric (MINIMAX_API_BASE vs MINIMAX_ANTHROPIC_BASE_URL).
Either document both fallback vars consistently, or drop them in favor of the four region/protocol-specific vars that are already documented.
| "has_tools": False, | ||
| "has_functions": False, |
There was a problem hiding this comment.
has_tools/has_functions are hardcoded False for both models and surfaced via the /v1/models capability metadata. MiniMax-M3 (and M2.7) advertise function/tool calling in their public docs — if so, this understates their capabilities to clients querying the proxy. Are these intentionally False, or should at least M3 report tool support?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/rotator_library/model_info_service.py`:
- Around line 1269-1274: Update the tier matching logic in the standard_tiers
loop to evaluate both optional input_tokens_gt and input_tokens_lte bounds
independently, requiring input_tokens to be greater than the lower bound when
present and less than or equal to the upper bound when present. Remove the
mutually exclusive if/else behavior while preserving open-ended range handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 014ac39d-3e00-4c0e-b6fb-2e1345e483ce
📒 Files selected for processing (5)
DOCUMENTATION.mdsrc/rotator_library/minimax_config.pysrc/rotator_library/model_info_service.pysrc/rotator_library/provider_config.pytests/test_minimax_provider.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
🪛 Ruff (0.15.21)
src/rotator_library/minimax_config.py
[warning] 10-10: typing.Dict is deprecated, use dict instead
(UP035)
🔇 Additional comments (4)
src/rotator_library/minimax_config.py (1)
144-147: LGTM!src/rotator_library/provider_config.py (1)
752-756: LGTM!tests/test_minimax_provider.py (1)
100-132: LGTM!DOCUMENTATION.md (1)
1153-1154: 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low valueCorrect typographical error.
The phrase "interleaved thinking" appears to be missing a comma, incorrectly blending two distinct capabilities ("interleaved" and "thinking").
📝 Proposed fix
-input modalities, tool use, interleaved thinking, and tiered pricing for the +input modalities, tool use, interleaved, thinking, and tiered pricing for the `/v1/models` and cost APIs.> Likely an incorrect or invalid review comment.
| for tier in standard_tiers: | ||
| if tier.get("input_tokens_lte") is not None: | ||
| matches = input_tokens <= tier["input_tokens_lte"] | ||
| else: | ||
| matches = input_tokens > tier.get("input_tokens_gt", -1) | ||
| if matches: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Make tier range intersection robust.
If a pricing tier definition ever uses both input_tokens_gt and input_tokens_lte to establish an explicit range, the current if/else logic will only enforce the upper bound (lte) and completely ignore the lower bound. Checking both bounds simultaneously ensures reliable tier resolution for future ranges.
♻️ Proposed refactor
for tier in standard_tiers:
- if tier.get("input_tokens_lte") is not None:
- matches = input_tokens <= tier["input_tokens_lte"]
- else:
- matches = input_tokens > tier.get("input_tokens_gt", -1)
+ matches = True
+ if tier.get("input_tokens_gt") is not None:
+ matches &= input_tokens > tier["input_tokens_gt"]
+ if tier.get("input_tokens_lte") is not None:
+ matches &= input_tokens <= tier["input_tokens_lte"]
if matches:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for tier in standard_tiers: | |
| if tier.get("input_tokens_lte") is not None: | |
| matches = input_tokens <= tier["input_tokens_lte"] | |
| else: | |
| matches = input_tokens > tier.get("input_tokens_gt", -1) | |
| if matches: | |
| for tier in standard_tiers: | |
| matches = True | |
| if tier.get("input_tokens_gt") is not None: | |
| matches &= input_tokens > tier["input_tokens_gt"] | |
| if tier.get("input_tokens_lte") is not None: | |
| matches &= input_tokens <= tier["input_tokens_lte"] | |
| if matches: |
🤖 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 `@src/rotator_library/model_info_service.py` around lines 1269 - 1274, Update
the tier matching logic in the standard_tiers loop to evaluate both optional
input_tokens_gt and input_tokens_lte bounds independently, requiring
input_tokens to be greater than the lower bound when present and less than or
equal to the upper bound when present. Remove the mutually exclusive if/else
behavior while preserving open-ended range handling.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
283-283: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the environment variable the runtime actually reads.
The README adds
MINIMAX_API_KEY_1, butsrc/rotator_library/litellm_providers.pyregistersMINIMAX_API_KEYas the runtime key variable. Users following this example may receive authentication failures. UseMINIMAX_API_KEYhere or update runtime support for numbered aliases.🤖 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 `@README.md` at line 283, Update the README environment-variable example to use MINIMAX_API_KEY, matching the runtime key registered by litellm_providers.py. Do not introduce numbered aliases unless runtime support is also intentionally added.
🤖 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.
Outside diff comments:
In `@README.md`:
- Line 283: Update the README environment-variable example to use
MINIMAX_API_KEY, matching the runtime key registered by litellm_providers.py. Do
not introduce numbered aliases unless runtime support is also intentionally
added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7d74c75-9930-49f5-936f-21fe24c5c4d7
📒 Files selected for processing (1)
README.md
📜 Review details
🔇 Additional comments (2)
README.md (2)
108-109: LGTM!
460-478: LGTM!
|
@octo-patch PR seems good. only problem is that i am in the middle of experimental rewrite that adds... a lot. Most likely i will merge this PR and any other PRs into that or rewrite them to work on that instead. Main is a bit behind dev, and dev is a lot behind experimental #162 . |
|
@Mirrowel, following up on your request to also assess porting effort from TL;DR
File-by-file
Recommendation
I'd suggest deciding the provider-routing approach first, since it determines whether the This analysis was generated by an AI assistant. |
2 participants
Description
Adds MiniMax provider routing with built-in
MiniMax-M3andMiniMax-M2.7model metadata, fallback model discovery, regional endpoint selection, and protocol-aware base routing. Updates environment examples and documentation, and adds native model metadata and cost coverage.The secondary compatibility base configuration remains at
/anthropic; a request capture verifies that the protocol request uses/v1/messages.Compatibility
Existing user-defined model definitions remain supported, and models discovered from the provider API are appended without replacing the built-in entries.
Testing Done
python3 -m compileall -q src testsPYTHONPATH=src .venv/bin/python -c "from rotator_library.providers import PROVIDER_PLUGINS; assert \"minimax\" in PROVIDER_PLUGINS"PYTHONPATH=src .venv/bin/python -m unittest discover -s tests -v