Fix openrouter model prefix issue by using LitellmProvider for all prefixes#2541
Fix openrouter model prefix issue by using LitellmProvider for all prefixes#2541liangzhang-keepmoving wants to merge 1 commit intoopenai:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53d72329b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| openai_use_responses: bool = False, | ||
| openai_use_responses_websocket: bool = False, |
There was a problem hiding this comment.
Setting openai_use_responses and openai_use_responses_websocket to False by default forces those values into OpenAIProvider, so MultiProvider() no longer inherits OpenAIProvider's default behavior (use_responses defaults from shared config). This silently changes the default model path for openai/... and unprefixed models from Responses to Chat Completions unless every caller opts back in, which is a broad runtime regression.
Useful? React with 👍 / 👎.
| return provider.get_model(model_name) | ||
| elif prefix: | ||
| # For unknown prefixes, pass the full model name (including prefix) to LiteLLM | ||
| return self._get_fallback_provider(prefix).get_model(f"{prefix}/{model_name}") |
There was a problem hiding this comment.
After _get_prefix_and_model_name() already strips the first segment, this branch prepends the prefix again for all prefixed models. For litellm/... inputs, LitellmProvider now receives litellm/<rest> instead of the previous <rest> payload, changing long-standing litellm/... routing semantics and breaking existing model names that depended on the stripped form.
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| # For backwards compatibility | ||
| MultiProviderMap = MultiProvider |
There was a problem hiding this comment.
Aliasing MultiProviderMap to MultiProvider is not backward compatible because MultiProvider does not implement the map API (add_provider, remove_provider, get_mapping, etc.). Existing code importing MultiProviderMap still imports successfully but fails at runtime when it uses those methods, which is a breaking public API change unrelated to the prefix fix.
Useful? React with 👍 / 👎.
| providers: list[ModelProvider] = [self.openai_provider] | ||
| if self.provider_map is not None: | ||
| providers.extend(self.provider_map.get_mapping().values()) | ||
| providers.extend(self._fallback_providers.values()) | ||
| for provider in self._fallback_providers.values(): | ||
| providers.append(provider) |
There was a problem hiding this comment.
aclose() now only walks openai_provider and fallback providers, but get_model() can still return models from provider_map. Any custom providers held there are no longer closed, so resources owned by mapped providers (sessions, websocket clients, etc.) can leak when users rely on MultiProvider.aclose() for cleanup.
Useful? React with 👍 / 👎.
|
@hshahrokni2 Thanks for the positive feedback and for sharing your insights! I'm glad the fix addresses the OpenRouter prefix issue effectively. You make a great point about LiteLLM being the right solution here since it already speaks OpenRouter's dialect. That's exactly why we chose this approach - it's clean and leverages LiteLLM's existing capabilities. Also, thanks for mentioning Komilion as an alternative solution for routing across multiple models. That's a useful option for those looking to simplify their provider-prefix management even further. @openai/openai-agents-python-maintainers This PR has received positive feedback and addresses a real issue with OpenRouter model support. Would you be able to review it when you have a chance? |
|
Thanks for sending this patch. However, this looks like a breaking change, and to me, what issue you're trying to solve is still unclear. Can you add some unit tests demonstrating the scenario plus avoid any breaking changes? |
2 participants
This PR fixes an issue where the SDK would throw an "Unknown prefix" error when using openrouter models.
The problem was in the
_create_fallback_providermethod, which only handled the "litellm" prefix and threw an error for all other prefixes. However, LiteLLM actually supports many other model providers, including openrouter, and expects model names in the format "provider/model".The fix:
_create_fallback_providerto return aLitellmProviderfor all prefixes, since LiteLLM can handle various model provider prefixesget_modelto pass the full model name (including prefix) to LiteLLM when using an unknown prefix, so LiteLLM can correctly route the requestThis change ensures that models like "openrouter/gpt-4" will work correctly through the multi-provider system.