fix(llm): route all DashScope models through native provider - #7234
Conversation
DashScope's OpenAI-compatible endpoint serves DeepSeek/Kimi/GLM/etc., not only Qwen. Stop restricting the native match to the qwen* prefix so DASHSCOPE_BASE_URL applies consistently. Fixes crewAIInc#7233.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe DashScope provider matcher now accepts all DashScope model names. Non-Qwen models route to ChangesDashScope routing
Suggested reviewers: Priority: ➖ Normal — Impact reflects medium issue severity. Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to DashScope model routing now uses the native OpenAI-compatible provider for non-Qwen models and applies custom DashScope base URLs consistently. The added DeepSeek regression coverage supports the intended routing behavior, with no current merge-blocking risk identified. 🚥 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 |
|
Hi @Alphaxiaoteng, Thanks for picking this up so quickly! The change looks right to me and matches what I had in mind when I filed the issue. Two small thoughts, both optional. The comment mentions DeepSeek/Kimi/GLM but the test only covers deepseek-v3 — parametrising over one more family would back up the comment. And it might be worth asserting llm.model == "deepseek-v3" too, just to confirm the model string is passed through correctly. One thing I left out of the issue deliberately: the hardcoded default https://dashscope-intl.aliyuncs.com/compatible-mode/v1 may also be stale, since the current docs list dashscope-us.aliyuncs.com and workspace-scoped *.maas.aliyuncs.com URLs. Probably a separate issue rather than something for this PR. Thanks! |
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Looks correct for #7233. The factory only takes the native OpenAICompatibleCompletion path when _matches_provider_pattern returns true, so the qwen* gate was sending dashscope/deepseek-v3 (and the other compatible-mode families) to LiteLLM and dropping DASHSCOPE_BASE_URL. return True matches the other OpenAI-compatible providers and Alibaba's compatible-mode docs.
The new test hits the reported case (dashscope/deepseek-v3 + custom DASHSCOPE_BASE_URL). That's enough; the matcher is now unconditional, so parametrizing Kimi/GLM is optional. Asserting llm.model == "deepseek-v3" would be a cheap extra check that the prefix is stripped.
One leftover: the comment a few lines above still says (DeepSeek, Dashscope) restrict to their own model prefixes. After this change only DeepSeek does. Please update that so it doesn't contradict the new DashScope branch.
CodeRabbit is clean. Not behind main, so no rebase needed. Leave the default dashscope-intl URL alone — that's a separate follow-up.
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Leftover comment next to the DashScope matcher.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Fixes #7233. return True matches the other OpenAI-compatible providers, so dashscope/deepseek-v3 (and Kimi/GLM) take the native path and honor DASHSCOPE_BASE_URL. The new test covers the reported case.
Please rebase onto main (mergeStateStatus is BEHIND). Non-blocking leftover: the comment a few lines above still says (DeepSeek, Dashscope) restrict to their own model prefixes — after this change only DeepSeek does.
|
thanks @Alphaxiaoteng for picking this up so quickly, and thanks @Vidit-Ostwal for the review and merge. |
Summary
_matches_provider_patternonly treated DashScope models as native when the name started withqwen, sodashscope/deepseek-v3(and other supported families) fell through to LiteLLM and ignoredDASHSCOPE_BASE_URL.return True).Test plan
test_openai_compatible.pyDashScope casesMade with Cursor