Abstract AiHandler to BaseAiHandler

[brianpham93]

In many organization, accessing directly to LLM provider is not permit. By abstracting implementation of AiHandler to an interface, this allows developer to have more freedom in customizing how each function could work with organization's own way of integrating with LLM

[brianpham93]

/review

[github-actions]

PR Analysis

(review updated until commit e37598f)

• 🎯 Main theme: Refactoring and abstraction of AI handlers
• 📝 PR summary: The PR introduces a new abstraction layer for AI handlers, allowing for more flexibility in integrating with different AI providers. It also includes the implementation of two new AI handlers: OpenAIHandler and LangChainOpenAIHandler. The PR also modifies various tools and servers to use the new AI handler abstraction.
• 📌 Type of PR: Enhancement
• 🧪 Relevant tests added: No
• ⏱️ Estimated effort to review [1-5]: 4, because the PR involves significant changes to the AI handling logic and affects multiple files. It also introduces new AI handlers which need to be reviewed for correctness and efficiency.
• 🔒 Security concerns: No security concerns found

PR Feedback

• 💡 General suggestions: The PR is well-structured and the changes are logically grouped. The abstraction of AI handlers is a good step towards making the system more flexible and adaptable. However, it would be beneficial to add tests to ensure that the new handlers and the modified logic work as expected. Additionally, error handling could be improved in some places, and some code could be made more efficient.

• 🤖 Code feedback:

  ---

  relevant file	pr_agent/agent/pr_agent.py
  suggestion	It's good to see the use of Python's built-in `inspect` module to check if a class constructor has a specific parameter. However, this approach might not be the most efficient or reliable. A more robust approach could be to use a common interface or base class for all classes that require an `ai_handler`, and then simply check if the class is a subclass of this base class. This would make the code more readable and less prone to errors. [important]
  relevant line	def has_ai_handler_param(cls: object):

  ---

  relevant file	pr_agent/algo/ai_handlers/openai_ai_handler.py
  suggestion	The `chat_completion` method includes a retry mechanism which is a good practice. However, the retry mechanism could be improved by adding a maximum delay limit to avoid very long delays in case of persistent errors. [medium]
  relevant line	@retry(exceptions=(APIError, Timeout, TryAgain, AttributeError, RateLimitError),

  ---

  relevant file	pr_agent/algo/ai_handlers/langchain_ai_handler.py
  suggestion	The `chat_completion` method seems to be missing error handling for the `self.chat` call. It would be good to add a try-except block around this call to handle potential exceptions and provide a more informative error message. [important]
  relevant line	resp = self.chat(messages, model=model, temperature=temperature)

  ---

  relevant file	pr_agent/algo/ai_handlers/base_ai_handler.py
  suggestion	The `BaseAiHandler` class is a good start for an AI handler interface. However, it could be improved by adding type hints to the method signatures. This would make the code more readable and would help developers understand what types of arguments are expected. [medium]
  relevant line	async def chat_completion(self, model: str, system: str, user: str, temperature: float = 0.2):

  ---

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

[brianpham93]

/update_changelog

[github-actions]

Changelog updates:

2023-12-09

Changed

• Abstracted AiHandler to BaseAiHandler to allow more flexibility in integrating with different LLM providers.
• Updated various tools to accept an instance of BaseAiHandler, enhancing customization options.

to commit the new content to the CHANGELOG.md file, please type:
'/update_changelog --pr_update_changelog.push_changelog_changes=true'

[brianpham93]

/generate_labels

[barnett-yuxiang]

LGTM

[Olamyy]

/review

[github-actions]

Persistent review updated to latest commit b640992

[Olamyy]

/auto_review

[github-actions]

Persistent review updated to latest commit b640992

[mrT23]

Hi @brianpham93
The base idea is solid - enabling multiple ai handlers.
However, this PR needs more work and changes before it can merged:

1. Not every tool was adapted to the new format. Hence, they will crash. This is beyond crucial to fix it fully. Triple-check that there are no instances of AiHandler() remaining, and that every tool still runs successfully.

2. with only one implementation of BaseAiHandler, it will be hard for other people to understand how to use it. You should provide at least one other implementation. The current AiHandler() should be renamed to LMAiHandler() (LM == LiteLLM), and you need to add another handler - OpenAIHandler(). Its not hard, since they have the same basic interface.
   Make sure you can actually test both handlers, and that they work.

As a side note, in contrast to the PR description, the current handler does not directly access the LLM provider ("In many organization, accessing directly to LLM provider is not permit") - it uses LiteLLM interface wrapper

[brianpham93]

thanks @mrT23 . i'll improve this PR further

[mrT23]

Persistent review updated to latest commit b640992

[mrT23]

Persistent review updated to latest commit b640992

[brianpham93]

/review

[brianpham93]

/update_changelog

[github-actions]

Changelog updates:

2023-12-13

Changed

• Abstracted AiHandler to BaseAiHandler to allow more flexibility in AI handling.
• Refactored AI handler classes and updated their instantiation in server files.
• Moved AI handlers to a specific folder for better organization.

Added

• Introduced OpenAIHandler and LangChainOpenAIHandler as new AI handlers.
• Added functionality to load default AI Handler from a util function.
• Added logging to pr_agent.py and code documentation generation for PR diffs.

to commit the new content to the CHANGELOG.md file, please type:
'/update_changelog --pr_update_changelog.push_changelog_changes=true'

[github-actions]

Persistent review updated to latest commit e37598f

[mrT23]

Hi @brianpham93 , good work.
i tested 'LangChainOpenAIHandler' and 'OpenAIHandler' (in addition to 'LiteLLMAIHandler') and they all worked.

I hade some issues with the structure and where to give the default values. In general, I don't recommend initializing things on the main path

I made some corrections, simplifications and protections, nothing that changes the behavior. Please merge this PR to yours:
#525

I did move the 'LangChainOpenAIHandler' to be optional, since this is quite an heavy installation, and currently I don't see an advantage it has over plain OpenAIHandler. if you think otherwise, let me know

after you merge my PR to yours, we can merge to main

[brianpham93]

thank @mrT23 , i've merged your PR's changes to my branch

[mrT23]

great work @brianpham93