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

Add support model deepseek/deepseek-reasoner #1473

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Conversation

KennyDizi
Copy link
Contributor

@KennyDizi KennyDizi commented Jan 22, 2025

PR Type

Enhancement


Description

  • Added support for the deepseek/deepseek-reasoner model with a token limit of 64K.

  • Updated logic to combine system and user prompts for deepseek-reasoner model.

  • Improved logging to specify the model name when combining prompts.


Changes walkthrough 📝

Relevant files
Enhancement
__init__.py
Add `deepseek/deepseek-reasoner` model with token limit   

pr_agent/algo/init.py

  • Added deepseek/deepseek-reasoner model with a 64K token limit.
+1/-0     
litellm_ai_handler.py
Enhance prompt handling for `deepseek-reasoner` model       

pr_agent/algo/ai_handlers/litellm_ai_handler.py

  • Included deepseek-reasoner in logic for combining system and user
    prompts.
  • Updated logging to include model name when combining prompts.
  • +2/-2     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Code Clarity

    Consider extracting the model type check into a separate function for better readability and maintainability, since it now handles both O1 and deepseek-reasoner models

    if (model_type.startswith(O1_MODEL_PREFIX)) or ("deepseek-reasoner" in model):
        user = f"{system}\n\n\n{user}"

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Improve code readability and maintainability

    The condition for combining prompts should be more explicit and separated for better
    maintainability. Create separate conditions for O1 models and deepseek-reasoner.

    pr_agent/algo/ai_handlers/litellm_ai_handler.py [203-206]

    -if (model_type.startswith(O1_MODEL_PREFIX)) or ("deepseek-reasoner" in model):
    +needs_combined_prompts = (
    +    model_type.startswith(O1_MODEL_PREFIX) or
    +    "deepseek-reasoner" in model
    +)
    +if needs_combined_prompts:
         user = f"{system}\n\n\n{user}"
         system = ""
         get_logger().info(f"Using model {model}, combining system and user prompts")
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: The suggestion improves code readability by extracting the complex condition into a descriptive variable, making the logic clearer. However, the impact is relatively minor as the original code is already functional and reasonably clear.

    4

    Comment on lines +203 to +206
    if (model_type.startswith(O1_MODEL_PREFIX)) or ("deepseek-reasoner" in model):
    user = f"{system}\n\n\n{user}"
    system = ""
    get_logger().info(f"Using O1 model, combining system and user prompts")
    get_logger().info(f"Using model {model}, combining system and user prompts")
    Copy link
    Collaborator

    @mrT23 mrT23 Jan 23, 2025

    Choose a reason for hiding this comment

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

    ok, but maybe in the future it worth further refactoring - a list of models that do not have 'system' message support, as this is getting bigger

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    That's a good point! Do you have any ideas about this? I would like to improve the code.

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    i think that in the place where we define models, we can define an extra list of 'user-message-only' models

    and use that in the handler

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    That sounds good. I will do that in the next PR.

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Jan 23, 2025

    p.s - that's also the feedback from Qodo Merge

    Improve code readability and maintainability

    The condition for combining prompts should be more explicit and separated for better
    maintainability. Create separate conditions for O1 models and deepseek-reasoner.

    pr_agent/algo/ai_handlers/litellm_ai_handler.py [203-206]

    -if (model_type.startswith(O1_MODEL_PREFIX)) or ("deepseek-reasoner" in model):
    +needs_combined_prompts = (
    +    model_type.startswith(O1_MODEL_PREFIX) or
    +    "deepseek-reasoner" in model
    +)
    +if needs_combined_prompts:
         user = f"{system}\n\n\n{user}"
         system = ""
         get_logger().info(f"Using model {model}, combining system and user prompts")```
    
        
      
    <ul class="contains-task-list">
    <li class="task-list-item enabled"><input type="checkbox" id="" class="task-list-item-checkbox"> <strong>Apply this suggestion</strong> </li>
    </ul>
    <details><summary>Suggestion importance[1-10]: 4</summary>
    <p dir="auto">Why: The suggestion improves code readability by extracting the complex condition into a descriptive variable, making the logic clearer. However, the impact is relatively minor as the original code is already functional and reasonably clear.</p>
    </details></details>

    @mrT23 mrT23 merged commit 1cb21c6 into qodo-ai:main Jan 23, 2025
    2 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants