feat(python-sdk): add MiniMax video provider routing#783
Conversation
|
|
Performance
✓ No regressions detected |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the provider implementation and test coverage of the submit/poll/retrieve flow look solid. Ran this through PR-AF (our review agent) and hand-verified its findings against the diff; nothing blocking, but four items worth addressing inline below. Separately: the CLA check is still unsigned, which will block merge regardless.
| model="minimax/video-model", | ||
| poll_interval=0, | ||
| ) | ||
| assert error_session.calls[0][1] == f"{MINIMAX_GLOBAL_BASE_URL}/video_generation" |
There was a problem hiding this comment.
This assertion makes the test environment-sensitive: the provider is constructed at line 131 without an explicit base_url, and MiniMaxProvider.__init__ falls back to os.environ["MINIMAX_BASE_URL"] before MINIMAX_GLOBAL_BASE_URL (media_providers.py lines 665-667). Since the README tells users to export exactly that variable, the test fails on any machine/CI where it's set.
Suggest isolating it at the top of the test:
monkeypatch.delenv("MINIMAX_BASE_URL", raising=False)(or pass base_url=MINIMAX_GLOBAL_BASE_URL explicitly at line 131).
| if extra: | ||
| body.update(extra) | ||
| if kwargs: | ||
| body.update(kwargs) |
There was a problem hiding this comment.
These two body.update(...) calls run after the validated fields are set, so extra or stray kwargs can silently overwrite model, prompt, duration, or resolution — e.g. extra={"duration": 3.5} bypasses the whole-number check at lines 767-770, and extra={"resolution": "1080p"} skips the .upper() normalization.
Consider guarding reserved keys before merging:
reserved = body.keys() & {**(extra or {}), **kwargs}.keys()
if reserved:
raise ValueError(f"extra/kwargs may not override: {sorted(reserved)}")(or document that extra intentionally wins, and merge it before validation so the checks still apply).
| status_code = base_resp.get("status_code") | ||
| if status_code not in (None, 0): | ||
| status_msg = base_resp.get("status_msg") or "unknown error" | ||
| raise RuntimeError( |
There was a problem hiding this comment.
Non-blocking consistency note: MiniMax wraps all API failures in bare RuntimeError (here, in _read_response, and the missing-task_id case), plus ValueError for validation and TimeoutError for polling — while FalProvider lets native client exceptions propagate. Callers of ai_generate_video end up needing provider-specific error handling.
Worth considering a shared typed exception for media-provider failures (or matching Fal's propagation behavior) so errors are catchable uniformly across providers.
| export MINIMAX_VIDEO_MODEL="..." | ||
| ``` | ||
|
|
||
| The global API base is used by default. Set `MINIMAX_BASE_URL` to select a region: |
There was a problem hiding this comment.
Worth a one-liner on precedence here: AIConfig(minimax_base_url=...) silently overrides MINIMAX_BASE_URL (agent_ai.py passes the config value as base_url, which wins in the provider's base_url or env or default chain — same for minimax_api_key vs MINIMAX_API_KEY). As written, a reader would expect the env var to take effect when both are set.
4 participants
Summary
Adds a direct MiniMax video provider to the Python SDK media router. The provider submits asynchronous generation tasks, polls their status, retrieves the generated file URL, and supports configurable global and China API bases while leaving existing video defaults unchanged.
Type of change
Test plan
sdk/python/.venv/bin/ruff check sdk/pythonsdk/python/.venv/bin/pytest -c sdk/python/pyproject.toml sdk/python/testsTest coverage
Checklist
docs/CONTRIBUTING.mdanddocs/DEVELOPMENT.md.Related issues / PRs