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 for o1, o1-2024-12-17 #1405

Closed
wants to merge 1 commit into from
Closed

Conversation

Shion1305
Copy link

@Shion1305 Shion1305 commented Dec 20, 2024

User description

What

Add support for o1, o1-2024-12-17

I'm aware of #1402 , but it was still WIP at this moment. Since I really need this feature asap, I've created my own PR.

Related links

https://openai.com/index/o1-and-new-tools-for-developers/
https://openai.com/api/pricing/#22wmoBV0tcLNLWL3xTd6IC


PR Type

Enhancement


Description

  • Added support for new OpenAI O1 models:
    • o1 and o1-2024-12-17 with 200K token context window
  • Fixed O1 model prefix detection in chat completion handler to properly support all O1 model variants
  • Maintains compatibility with existing O1 model handling for system and user prompts

Changes walkthrough 📝

Relevant files
Enhancement
__init__.py
Add new O1 model variants with extended context window     

pr_agent/algo/init.py

  • Added support for new OpenAI models: o1 and o1-2024-12-17 with 200K
    token context windows
  • +2/-0     
    Bug fix
    litellm_ai_handler.py
    Update O1 model prefix handling in chat completion             

    pr_agent/algo/ai_handlers/litellm_ai_handler.py

  • Modified O1 model prefix detection from 'o1-' to 'o1' to properly
    handle all O1 model variants
  • +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant 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

    Model Detection

    Verify that changing O1_MODEL_PREFIX from 'o1-' to 'o1' correctly identifies all O1 model variants without accidentally matching other model names

                O1_MODEL_PREFIX = 'o1'
                model_type = model.split('/')[-1] if '/' in model else model
                if model_type.startswith(O1_MODEL_PREFIX):

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add more precise model name validation to prevent false matches with similarly prefixed models

    The current O1 model prefix check will incorrectly match any model starting with
    'o1', including potential future models like 'o10' or 'o11'. Add a hyphen check or
    use regex to ensure exact prefix matching.

    pr_agent/algo/ai_handlers/litellm_ai_handler.py [197-199]

     O1_MODEL_PREFIX = 'o1'
     model_type = model.split('/')[-1] if '/' in model else model
    -if model_type.startswith(O1_MODEL_PREFIX):
    +if model_type.startswith(O1_MODEL_PREFIX) and (len(model_type) == 2 or model_type[2] in ['-', '/']):
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential bug where the current prefix check could incorrectly match future model names like 'o10'. The proposed solution using length check and delimiter validation would prevent such false matches, making the model detection more robust.

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

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Dec 22, 2024

    @Shion1305
    Thanks for the PR, but i am afraid #1402 contained similar content, and was opened first.

    I have deployed this to the latest docker, and you are welcome to try out the new 'o1'

    @Shion1305
    Copy link
    Author

    Thank you so much🙏 I see that o1 has been supported, I'm closing this PR!

    @Shion1305 Shion1305 closed this Dec 22, 2024
    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