fix(dspy): unify OpenAI reasoning-model classifier for non-openai provider prefixes - #89
Open
detail-app[bot] wants to merge 2 commits into
Conversation
Greptile SummaryThis PR centralizes OpenAI reasoning-model detection so LM configuration and OpenAI request formatting consistently handle provider-prefixed model identifiers.
Confidence Score: 5/5The PR appears safe to merge, with the previously reported The shared classifier now recognizes Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Provider-prefixed model ID] --> B[Shared reasoning-model classifier]
B --> C[LM configuration validation]
B --> D[OpenAI request formatter]
C --> E[Reasoning-compatible defaults]
D --> F[max_completion_tokens]
F --> G[LiteLLM provider request]
Reviews (2): Last reviewed commit: "fix: preserve o1-preview in shared reaso..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
GitHub issue creation failed
Detail attempted to publish this bug to GitHub, but the issue could not be created. This fix PR was created without that issue, and missing tracker references are shown as
Unknown issue.You can review and merge this PR normally. Please review your tracker integration settings before the next publish run.
Detail bug report: View on Detail
📝 Changes Description
Closes Unknown issue
Bug
dspy/clients/openai_format.pyanddspy/clients/lm.pyeach defined their own_is_openai_reasoning_model, and the two copies diverged:openai_format.pystripped only the literalopenai/prefix (removeprefix) and used a loosestartswith.lm.pystripped any provider prefix (split("/")[-1]) and used an anchored regex.For non-
openai/prefixes likeazure/o3, the adapter-path classifier misclassified the model as non-reasoning. On the standarddspy.Predictadapter path this caused two symptoms forazure/o3(whileopenai/o3behaved correctly):max_tokensset as a Predictor-level kwarg was sent undermax_tokensinstead ofmax_completion_tokens.temperature+reasoning_effortvalidation was skipped, letting invalid combinations reach the provider instead of raisingLMUnsupportedFeatureError.Fix
Extracted the classifier into a shared neutral module
dspy/clients/_openai_model_family.py(usinglm.py's stricter grammar — strip any provider prefix + anchored regex). Bothlm.pyandopenai_format.pynow import and call the sameis_openai_reasoning_model, so future drift is a single definition, not two. A neutral module is required becauselm.pyalready imports fromopenai_format.py, so a direct reverse import would be circular.Testing
tests/clients/test_lm.py: a single-source-of-truth guard (both modules reference the same function object), a parametrized classifier test covering provider-prefix parity and the strict-grammar footgun (e.g.azure/o3-preview,azure/o1-mini-prorejected), and an adapter-path test that captures thelitellm_completionrequest and assertsmax_completion_tokensfor bothazure/o3andopenai/o3. Existingtest_reasoning_model_token_parameterandtest_reasoning_model_requirementswere extended with azure cases.ruff check(CI-strict), pre-commit hooks, and the full default CI matrixpytest -m 'not extra and not deno' tests/(1287 passed). No regressions acrosstests/clients/,tests/adapters/,tests/predict/.azure/o3now matchesopenai/o3on both the wire key and the validation gate.max_completion_tokenscould not be run — it blocked at credential acquisition withopenai.OpenAIError: Missing credentials. Please pass one of api_key, azure_ad_token, azure_ad_token_provider, or the AZURE_OPENAI_API_KEY or AZURE_OPENAI_AD_TOKEN environment variables.(no Azure credentials in this environment). The request did reach litellm's Azureo_series_handlerbefore failing on auth, confirming the wire path is correct.✅ Contributor Checklist
Authored by Detail (AI-assisted). The fix was validated by reproducing the reported divergence, unifying the classifier, and running the test matrix above; every line is understood.
Automatic Fixes PRs can be configured here.