Skip to content

Add reasoning content support (for DeepSeek format API) #840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add reasoning content support (for DeepSeek format API) #840

wants to merge 1 commit into from

Conversation

@PasserbyAlpha
Copy link

@PasserbyAlpha PasserbyAlpha commented Feb 19, 2025

For customAPI, try to load delta.reasoning_content and message.reasoning_content if it exists.

For DeepSeek-R1
reasoning_model

For GPT-4o
common_model

Currently only delta part (streaming) has been tested (I don't know how to trigger a single-request chat completion with the repo).

I'm not familiar with nodejs. Please help me to improve it if possible, thank you!

@PasserbyAlpha PasserbyAlpha mentioned this pull request Feb 19, 2025
Open
@PeterDaveHello PeterDaveHello requested a review from Copilot May 31, 2025 20:18
Copilot AI reviewed May 31, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for embedding reasoning_content segments in both streaming and single-response flows for the DeepSeek-format API.

  • Loads delta.reasoning_content and message.reasoning_content when available.
  • Wraps reasoning segments with [Think] and [Response] markers during streaming and in one-shot responses.
Comments suppressed due to low confidence (2)

src/services/apis/custom-api.mjs:42

  • [nitpick] Variable names like with_reasoning use snake_case, which differs from the project's camelCase convention; consider renaming to withReasoning, and similarly update has_reasoning_start/has_reasoning_end to hasReasoningStart/hasReasoningEnd.
let with_reasoning = false

src/services/apis/custom-api.mjs:87

  • New reasoning_content handling logic is introduced here but lacks accompanying unit tests to validate both streaming and single-response behaviors; consider adding tests for scenarios with and without reasoning_content.
const delta_reasoning = data.choices[0]?.delta?.reasoning_content

src/services/apis/custom-api.mjs
let has_reasoning_start = false
let has_reasoning_end = false

const REASONING_START_SIGN = '[Think]\n\n'
Copy link

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Magic strings for reasoning markers are declared inline; extracting these markers into a shared constants module or configuration will improve maintainability and consistency.

Copilot uses AI. Check for mistakes.
@PeterDaveHello
Copy link
Member

PeterDaveHello commented Jun 2, 2025

/review

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Jun 2, 2025

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Logic Flow

The streaming handling logic may have an issue where delta content is only added when reasoning content exists. Check if this could cause regular content to be missed when no reasoning is present.

 
if (delta) {
  if (with_reasoning && !has_reasoning_end) {
    answer += REASONING_END_SIGN
    has_reasoning_end = true
  }
  answer += delta
}
Edge Case

The code doesn't handle the case where reasoning_content exists but is empty. This could lead to unnecessary reasoning markers being added to the output.

 
if (delta_reasoning) {
  with_reasoning = true
  if (!has_reasoning_start) {
    answer += REASONING_START_SIGN
    has_reasoning_start = true
  }
  answer += delta_reasoning

@PeterDaveHello
Copy link
Member

PeterDaveHello commented Jun 2, 2025

/improve

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Jun 2, 2025

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix reasoning marker logic

The current implementation adds the reasoning end sign for every delta chunk
when reasoning is present, but it should only be added once before transitioning
to the actual response. Add a check to ensure the reasoning end marker is only
added when transitioning from reasoning to response content.

src/services/apis/custom-api.mjs [91-107]

 if (delta !== undefined) {
   // streaming handling
   if (delta_reasoning) {
     with_reasoning = true
     if (!has_reasoning_start) {
       answer += REASONING_START_SIGN
       has_reasoning_start = true
     }
     answer += delta_reasoning
   }
   if (delta) {
-    if (with_reasoning && !has_reasoning_end) {
+    if (with_reasoning && !has_reasoning_end && delta_reasoning === undefined) {
       answer += REASONING_END_SIGN
       has_reasoning_end = true
     }
     answer += delta
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a logical issue where the REASONING_END_SIGN could be added prematurely if both delta_reasoning and delta are present in the same chunk. Adding the delta_reasoning === undefined condition ensures the end marker is only added when transitioning from reasoning to response content.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PasserbyAlpha @PeterDaveHello