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

Fix LangChainOpenAIHandler for Azure #1102

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

MarkRx
Copy link
Contributor

@MarkRx MarkRx commented Aug 7, 2024

User description

Refactored to support delayed ChatOpenAI instantiation for Azure

Fixes #1101


PR Type

Enhancement, Bug fix


Description

  • Refactored LangChainOpenAIHandler to improve Azure support and efficiency:
    • Introduced a new _get_chat method with @functools.cache for lazy and efficient chat instance creation
    • Updated chat method to use the new _get_chat method, supporting delayed ChatOpenAI instantiation
    • Removed initialization of _chat in the constructor and added early validation
  • Updated LangChain dependencies in requirements.txt:
    • Upgraded langchain to version 0.2.0
    • Added new dependencies: langchain-core 0.2.28 and langchain-openai 0.1.20
  • These changes improve the handling of Azure OpenAI deployments and ensure compatibility with the latest LangChain versions

Changes walkthrough 📝

Relevant files
Enhancement
langchain_ai_handler.py
Refactor LangChainOpenAIHandler for Azure support               

pr_agent/algo/ai_handlers/langchain_ai_handler.py

  • Refactored LangChainOpenAIHandler to support delayed ChatOpenAI
    instantiation for Azure
  • Introduced a new _get_chat method with @functools.cache for efficient
    chat instance creation
  • Updated chat method to use the new _get_chat method
  • Removed initialization of _chat in the constructor and added early
    validation
  • +31/-27 
    Dependencies
    requirements.txt
    Update LangChain dependencies                                                       

    requirements.txt

  • Updated langchain dependency to version 0.2.0
  • Added new dependencies: langchain-core 0.2.28 and langchain-openai
    0.1.20
  • +3/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added enhancement New feature or request Bug fix labels Aug 7, 2024
    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 92
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 Multiple PR themes

    Sub-PR theme: Refactor LangChainOpenAIHandler for improved Azure support

    Relevant files:

    • pr_agent/algo/ai_handlers/langchain_ai_handler.py

    Sub-PR theme: Update LangChain dependencies

    Relevant files:

    • requirements.txt

    ⚡ Key issues to review

    Error Handling
    The new _get_chat method might benefit from more specific error handling, especially for network-related issues when interacting with Azure or OpenAI APIs.

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling for chat class initialization in the constructor

    Consider adding error handling for the case where _get_chat() fails to initialize
    the chat class. This could prevent potential runtime errors if the initialization
    fails.

    pr_agent/algo/ai_handlers/langchain_ai_handler.py [20-25]

     def __init__(self):
         # Initialize OpenAIHandler specific attributes here
         super().__init__()
         self.azure = get_settings().get("OPENAI.API_TYPE", "").lower() == "azure"
     
         # Initialize the primary chat class for early validation
    -    self._get_chat(self.deployment_id)
    +    try:
    +        self._get_chat(self.deployment_id)
    +    except Exception as e:
    +        get_logger().error(f"Failed to initialize chat class: {e}")
    +        raise
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion improves error handling and provides better logging, which is crucial for debugging and maintaining the code.

    8
    Add error checking and exception handling in the chat method

    The chat method is not handling the case where _get_chat might return None or raise
    an exception. Consider adding a check or try-except block to handle potential
    errors.

    pr_agent/algo/ai_handlers/langchain_ai_handler.py [27-29]

     def chat(self, messages: list, model: str, temperature: float):
         chat = self._get_chat(self.deployment_id)
    -    return chat.invoke(input = messages, model=model, temperature=temperature)
    +    if not chat:
    +        raise ValueError("Failed to initialize chat instance")
    +    try:
    +        return chat.invoke(input=messages, model=model, temperature=temperature)
    +    except Exception as e:
    +        get_logger().error(f"Error during chat invocation: {e}")
    +        raise
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion adds important error checking and exception handling, which can prevent silent failures and improve the robustness of the code.

    8
    Performance
    Replace unlimited caching with LRU cache for better handling of dynamic parameters

    The _get_chat method is using functools.cache, which might cause issues if the
    deployment_id changes dynamically. Consider using lru_cache with a small maxsize
    instead to allow for some caching while still being able to handle changes in
    deployment_id.

    pr_agent/algo/ai_handlers/langchain_ai_handler.py [53-54]

    -@functools.cache
    +@functools.lru_cache(maxsize=8)
     def _get_chat(self, deployment_id = None):
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using LRU cache instead of unlimited caching is a good practice for handling potentially changing parameters, improving memory management.

    7
    Dependency management
    Pin langchain package versions and uncomment them in requirements.txt

    Consider pinning the versions of the langchain packages to specific versions rather
    than using the latest versions. This can help ensure consistency and prevent
    unexpected issues from package updates.

    requirements.txt [33-35]

    -# langchain==0.2.0
    -# langchain-core==0.2.28
    -# langchain-openai==0.1.20
    +langchain==0.2.0
    +langchain-core==0.2.28
    +langchain-openai==0.1.20
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Pinning package versions is a good practice for maintaining consistency, but the suggestion to uncomment the lines may not align with the intent of the original code.

    6

    Comment on lines 53 to 54
    @functools.cache
    def _get_chat(self, deployment_id = None):
    Copy link
    Collaborator

    @mrT23 mrT23 Aug 8, 2024

    Choose a reason for hiding this comment

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

    can you explain these lines ?
    why do you add cache ? whats the benefit ? can't it cause problems when running things in parallel ?

    I would recommend removing the cache line. Seems like a false optimization to me, since I don't see a real use case where it would actually benefit

    image

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    It was a quick way to ensure a single chat object existed per LLM deployment. I assumed chat initialization and first use could have some I/O overhead though thinking more about it I agree I'll remove it.

    @mrT23 mrT23 self-requested a review August 8, 2024 05:01
    @Nichebiche
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 92
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 Multiple PR themes

    Sub-PR theme: Refactor LangChainOpenAIHandler for improved Azure support

    Relevant files:

    • pr_agent/algo/ai_handlers/langchain_ai_handler.py

    Sub-PR theme: Update LangChain dependencies

    Relevant files:

    • requirements.txt

    ⚡ Key issues to review

    Error Handling
    The new _get_chat method might benefit from more specific error handling, especially for network-related issues when interacting with Azure or OpenAI APIs.

    @MarkRx MarkRx force-pushed the feature/langchain-azure-fix branch from c610ed7 to 4201779 Compare August 8, 2024 13:55
    @mrT23 mrT23 merged commit b370cb6 into Codium-ai:main Aug 8, 2024
    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.

    LangChainOpenAIHandler fails with Azure
    3 participants