fix: logic for determining reasoning model#7009
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR refines the logic for detecting OpenAI reasoning models to avoid adding reasoning_effort parameters to OpenAI-compatible APIs that may not support them. The implementation now uses regex patterns to match reasoning model names (o1, o3, gpt-5 series) and checks if a custom base URL is being used to distinguish between actual OpenAI APIs and compatible proxies.
Key changes:
- Enhanced
_is_reasoning_model()method with regex-based pattern matching and base URL validation - Added comprehensive test coverage for reasoning model detection across various scenarios
- Added tests to verify OpenAI-compatible APIs don't receive
reasoning_effortparameters
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| marimo/_server/ai/providers.py | Replaced simple string prefix check with robust regex patterns and base URL validation to identify reasoning models |
| tests/_server/ai/test_providers.py | Added parametrized tests for reasoning model detection and OpenAI-compatible API parameter handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| the reasoning_effort parameter even if the model name suggests it's a | ||
| reasoning model. | ||
| """ | ||
| import re |
There was a problem hiding this comment.
The re module is imported inside the method instead of at the module level. Since this method may be called frequently during request processing, moving the import to the top of the file would improve performance by avoiding repeated import overhead.
| r"^openai/o\d", # openai/o1, openai/o3, etc. | ||
| r"^o\d", # o1, o3, etc. | ||
| r"^openai/gpt-5", # openai/gpt-5* | ||
| r"^gpt-5", # gpt-5* |
There was a problem hiding this comment.
The regex patterns use \\d which matches only a single digit, but they don't anchor the pattern or allow for continuation. This means 'o1-mini' or 'o3-mini-2024' won't match because the digit is followed by additional characters. The patterns should be r\"^openai/o\\d\" → r\"^openai/o\\d+\" or r\"^openai/o\\d.*\" to match model names like 'o1-mini', 'o1-preview', 'o3-mini', etc. The same fix applies to all four patterns.
| r"^openai/o\d", # openai/o1, openai/o3, etc. | |
| r"^o\d", # o1, o3, etc. | |
| r"^openai/gpt-5", # openai/gpt-5* | |
| r"^gpt-5", # gpt-5* | |
| r"^openai/o\d.*", # openai/o1, openai/o3, openai/o1-mini, etc. | |
| r"^o\d.*", # o1, o3, o1-mini, etc. | |
| r"^openai/gpt-5.*", # openai/gpt-5, openai/gpt-5-preview, etc. | |
| r"^gpt-5.*", # gpt-5, gpt-5-preview, etc. |
| """Test that _is_reasoning_model correctly identifies reasoning models.""" | ||
| config = AnyProviderConfig(api_key="test-key", base_url=base_url) | ||
| provider = OpenAIProvider(model_name, config) | ||
| assert provider._is_reasoning_model(model_name) == expected |
There was a problem hiding this comment.
[nitpick] The test is calling a private method _is_reasoning_model() directly. While this is acceptable for unit testing internal behavior, consider whether this logic should be tested through the public API (e.g., by verifying the parameters passed to the OpenAI client) to avoid coupling tests to implementation details.
2 participants
This fixes the logic for determing if a model is reasoning (e.g. o1, gpt-5)
Fixes #7007