feat: enable reasoning support for DeepSeek models on Azure#412
Conversation
afcc129 to
02ae450
Compare
DeepSeek V3 models support opt-in reasoning via a `thinking` parameter, but the Azure OpenAI adapter was stripping it as Anthropic-specific. - Add `deepseek_model?/1` helper to AdapterHelpers - Stop stripping thinking config in pre_validate_options for DeepSeek - In format_request, pass through user-provided thinking config from additional_model_request_fields for DeepSeek models (opt-in, no default change) - Fix model ID extraction in pre_validate_options to use provider_model_id when available (matching effective_model_id/1) - Fix extract_usage to infer reasoning_tokens from reasoning_content in choices when completion_tokens_details is absent (affects DeepSeek and any other model that surfaces reasoning via reasoning_content) - Add tests for all new behavior
02ae450 to
3559e2e
Compare
mikehostetler
left a comment
There was a problem hiding this comment.
Hey @shelvick — thanks so much for this contribution! Really nice work here. The test coverage is thorough, the deepseek_model?/1 helper fits perfectly alongside the existing model predicates, and the provider_model_id fix is a great catch. Love the opt-in design too — much safer than defaulting thinking on.
A couple of things I'd love your thoughts on before we merge:
infer_reasoning_from_choices over-reports reasoning tokens
When completion_tokens_details is absent and reasoning_content is present, the fallback sets reasoning_tokens = completion_tokens. Since the response also has a content field with the actual answer, this attributes all output tokens to reasoning — which could mislead downstream analytics or billing calculations.
A couple of ideas (totally open to whatever you think is best):
- Simple/honest: just leave
reasoning_tokensas0when we can't get an authoritative breakdown — at least we're not over-reporting. - Proportional estimate: use the relative text lengths of
reasoning_contentvscontentto approximate the split:round(completion_tokens * reasoning_len / (reasoning_len + answer_len)). Not perfect, but much closer to reality.
Either way would be great — the key thing is avoiding the "100% reasoning" assumption when we know there's also answer content.
Minor: inline comment in function body
On L97 of openai.ex there's a comment inside warn_and_remove_anthropic_thinking_config/2:
# DeepSeek models support thinking config natively; don't strip itThe project has a convention of no comments inside function bodies (enforced by a custom Credo rule). The code is quite readable without it — the function name and the deepseek_model? guard make the intent clear. Would you mind removing it?
That's it! Everything else looks really solid. Thanks again for putting this together 🙏
When completion_tokens_details is absent, the fallback infer_reasoning_from_choices was attributing all completion tokens to reasoning even when the response also contained answer content. Use the relative text lengths of reasoning_content vs content to approximate the split instead. Also remove inline comment in warn_and_remove_anthropic_thinking_config to follow project convention of no comments inside function bodies.
|
@mikehostetler This was fixed too, I just didn't leave a comment. (Is the comment needed? Sorry, I'm bad at GitHub.) |
|
@shelvick You're all good - I sw th e commits - I'll work these through shortly! |
Summary
deepseek_model?/1helper toAdapterHelpersfor detecting DeepSeek model IDsthinkingconfig fromadditional_model_request_fieldsfor DeepSeek models inpre_validate_options(it was being removed with an "Anthropic-specific" warning, but DeepSeek supports it natively)thinking: %{type: "enabled"}informat_requestfor DeepSeek models, enabling reasoning by defaultadditional_model_request_fields(e.g., custombudget_tokens) or suppress entirely by settingthinking: nilpre_validate_optionsto useprovider_model_idwhen available, matchingeffective_model_id/1used elsewhere in the Azure moduleNotes
thinkingparameter — confirmed via live test that sending it is harmless (ignored, no error)mai-ds-r1(Microsoft's DeepSeek R1 derivative) uses the"mai-ds"model family prefix and is correctly not matched bydeepseek_model?/1Test plan
deepseek_model?/1,pre_validate_optionsthinking preservation,format_requestdefault injection, user override, opt-out via nil, and regression checks for non-DeepSeek models