Skip to content
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.

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

feat: added support for deepseek-reasoner #410

Merged
merged 1 commit into from
Jan 20, 2025
Merged

Conversation

ErikBjare
Copy link
Owner

@ErikBjare ErikBjare commented Jan 20, 2025

TODO

  • fix bug for second message after tooluse, requiring only one system message
  • fix bug about consecutive user/assistant messages (similar to anthropic)

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to aa4ab0e in 15 seconds

More details
  • Looked at 67 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. gptme/llm/llm_openai.py:116
  • Draft comment:
    Consider renaming is_o1 to is_o1_model for clarity, as it represents a boolean indicating if the base model is an 'o1' model. This applies to similar variables like is_deepseek_reasoner and is_reasoner.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The code changes in the PR are consistent with the existing code structure and logic. The addition of the 'deepseek-reasoner' model is handled correctly in both files. The logic for determining 'is_reasoner' is correctly updated to include 'deepseek-reasoner'. The model metadata is also correctly updated in 'models.py'.

Workflow ID: wflow_TcggQuAuo948TChs


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 5 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@0cd60a0). Learn more about missing BASE report.
Report is 3 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
gptme/llm/llm_openai.py 80.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #410   +/-   ##
=========================================
  Coverage          ?   69.95%           
=========================================
  Files             ?       70           
  Lines             ?     5808           
  Branches          ?        0           
=========================================
  Hits              ?     4063           
  Misses            ?     1745           
  Partials          ?        0           
Flag Coverage Δ
anthropic/claude-3-haiku-20240307 68.69% <64.00%> (?)
openai/gpt-4o-mini 68.00% <80.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ErikBjare ErikBjare force-pushed the dev/deepseek-reasoner branch from aa4ab0e to 43ed919 Compare January 20, 2025 15:25
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 43ed919 in 51 seconds

More details
  • Looked at 140 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. gptme/llm/llm_openai.py:137
  • Draft comment:
    Consider adding a check to handle empty msgs list to avoid potential IndexError.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The function does access msgs[0] without a check. However, this is an internal function only called in a context where messages are required for the API call to work. An empty message list would be invalid input for the chat API anyway. The function assumes valid input rather than defensive programming, which is reasonable for an internal implementation detail.
    I could be wrong about the API requirements - maybe there are valid cases where an empty message list should be handled gracefully rather than failing.
    Looking at OpenAI's API docs, a chat completion requires at least one message. An empty message list would be invalid input. The function is correct to assume valid input since it's an internal implementation detail.
    Delete the comment. The function reasonably assumes valid input since it's an internal implementation detail and empty message lists would be invalid for the chat API anyway.
2. gptme/llm/llm_openai.py:142
  • Draft comment:
    Use List from typing instead of list for type hinting. This applies to other instances in the file as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses list for type hinting in several places, which is not recommended in modern Python. Instead, List from typing should be used for better compatibility and readability.
3. gptme/llm/models.py:134
  • Draft comment:
    Ensure deepseek-reasoner is properly handled in get_model to avoid potential issues if the model is not found.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The deepseek-reasoner model is added to the MODELS dictionary, but there is no check for its presence in the get_model function. This could lead to issues if the model is not found.

Workflow ID: wflow_hFk3i8RDzDEZpnae


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ErikBjare ErikBjare merged commit ac3b825 into master Jan 20, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants