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

Support deepseek-chat model #1457

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Support deepseek-chat model #1457

wants to merge 4 commits into from

Conversation

KennyDizi
Copy link
Contributor

@KennyDizi KennyDizi commented Jan 17, 2025

PR Type

Enhancement, Documentation


Description

  • Added support for the deepseek/deepseek-chat model in the system.

  • Injected DEEPSEEK_API_KEY environment variable for DeepSeek integration.

  • Updated documentation to include DeepSeek model configuration details.

  • Added a template for DeepSeek API key in .secrets_template.toml.


Changes walkthrough 📝

Relevant files
Enhancement
__init__.py
Added DeepSeek model to token limits dictionary                   

pr_agent/algo/init.py

  • Added deepseek/deepseek-chat model to the model token limits
    dictionary.
  • +1/-0     
    litellm_ai_handler.py
    Added DeepSeek API key injection logic                                     

    pr_agent/algo/ai_handlers/litellm_ai_handler.py

  • Added logic to inject DEEPSEEK_API_KEY into environment variables.
  • Enabled support for DeepSeek models in the handler.
  • +4/-0     
    Documentation
    changing_a_model.md
    Updated documentation for DeepSeek model configuration     

    docs/docs/usage-guide/changing_a_model.md

  • Added instructions for configuring the DeepSeek deepseek-chat model.
  • Included details on obtaining and setting the DeepSeek API key.
  • +19/-0   
    Configuration changes
    .secrets_template.toml
    Added DeepSeek API key template                                                   

    pr_agent/settings/.secrets_template.toml

    • Added a template section for the DeepSeek API key.
    +3/-0     

    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: 2 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 No relevant tests
    🔒 Security concerns

    API Key Handling:
    The PR adds handling of a new API key (DEEPSEEK_API_KEY). While the key is stored in .secrets.toml, ensure that proper key rotation and secure storage practices are followed. Also verify that the key is not accidentally logged or exposed in error messages.

    ⚡ Recommended focus areas for review

    Error Handling

    The DeepSeek API key retrieval and environment variable setting lacks error handling or validation. Consider adding checks for empty/invalid API keys.

    if get_settings().get("DEEPSEEK.KEY", None):
        os.environ['DEEPSEEK_API_KEY'] = get_settings().get("DEEPSEEK.KEY")

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add proper error handling for missing API credentials to prevent silent failures

    Add error handling when accessing the DeepSeek API key to gracefully handle missing
    or invalid credentials, similar to how other API keys are handled in the codebase.

    pr_agent/algo/ai_handlers/litellm_ai_handler.py [94-95]

    -if get_settings().get("DEEPSEEK.KEY", None):
    -    os.environ['DEEPSEEK_API_KEY'] = get_settings().get("DEEPSEEK.KEY")
    +deepseek_key = get_settings().get("DEEPSEEK.KEY", None)
    +if not deepseek_key:
    +    logger.warning("DeepSeek API key not found in settings")
    +else:
    +    os.environ['DEEPSEEK_API_KEY'] = deepseek_key
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by adding a warning log when the DeepSeek API key is missing, which helps with debugging and follows the pattern used for other API keys in the codebase. This is a meaningful improvement for maintainability and operational reliability.

    7
    • Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

    @KennyDizi
    Copy link
    Contributor Author

    GM @mrT23
    Would you pls take a look at this PR?

    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.

    1 participant