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

feat: integrate the OpenAILLM async into the RL #356

Merged
merged 16 commits into from
Jul 19, 2024

Conversation

tolgadevAI
Copy link
Contributor

@tolgadevAI tolgadevAI commented Jul 16, 2024

PR Type

Enhancement, Documentation


Description

  • Added support for async calls in dynamic routes within RouteLayer.
  • Enhanced OpenAILLM to support both synchronous and asynchronous clients.
  • Introduced an async acall method in OpenAILLM for handling async requests.
  • Updated dynamic routes documentation to include examples and cells demonstrating async calls.
  • Added warning and default initialization for missing LLM in dynamic routes.

Changes walkthrough 📝

Relevant files
Enhancement
layer.py
Add async support for dynamic routes in RouteLayer             

semantic_router/layer.py

  • Added support for async calls in dynamic routes.
  • Integrated OpenAILLM with async functionality.
  • Added warning and default initialization for missing LLM.
  • +8/-3     
    openai.py
    Enhance OpenAILLM to support async client and calls           

    semantic_router/llms/openai.py

  • Modified OpenAILLM to support both sync and async clients.
  • Added async acall method for handling async requests.
  • Updated client initialization to handle async client creation.
  • +61/-7   
    Documentation
    02-dynamic-routes.ipynb
    Update dynamic routes documentation with async examples   

    docs/02-dynamic-routes.ipynb

  • Added examples and cells demonstrating async calls.
  • Updated execution counts and outputs for new async examples.
  • Improved documentation for dynamic routes with async support.
  • +376/-115

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

    @github-actions github-actions bot added documentation Improvements or additions to documentation enhancement Enhancement to existing features Review effort [1-5]: 3 labels Jul 16, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The implementation in semantic_router/layer.py lines 300-307 introduces a default behavior where if no LLM is provided, it defaults to using OpenAILLM with use_async=True. This could lead to unexpected behavior if the user is unaware that no LLM was provided and the system defaults to an asynchronous LLM. It might be beneficial to either document this behavior clearly or reconsider if a silent defaulting is appropriate without explicit user consent or notification.

    Exception Handling
    In the __init__ method of OpenAILLM class, the exception handling when initializing self.client could be more specific. Currently, it catches a general Exception, which might obscure the root cause of initialization failures. It would be better to catch specific exceptions related to API client initialization to provide clearer error messages to the user.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Prevent potential IndexError by checking if completion.choices is empty before accessing it

    Add error handling for the case where completion.choices might be empty, which would
    cause an IndexError when accessing completion.choices[0].

    semantic_router/llms/openai.py [141-148]

     if function_schemas:
    +    if not completion.choices:
    +        raise ValueError("No choices available in the completion response.")
         tool_calls = completion.choices[0].message.tool_calls
         if tool_calls is None:
             raise ValueError("Invalid output, expected a tool call.")
         if len(tool_calls) < 1:
             raise ValueError(
                 "Invalid output, expected at least one tool to be specified."
             )
     
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a potential bug that could cause an IndexError, thus improving the robustness and reliability of the code.

    10
    Maintainability
    Simplify the client initialization logic to reduce redundancy and enhance code clarity

    Refactor the exception handling in the initialization of self.client to reduce code
    duplication and improve maintainability.

    semantic_router/llms/openai.py [43-56]

    -if use_async:
    -    try:
    -        self.client = openai.AsyncOpenAI(api_key=api_key)
    -    except Exception as e:
    -        raise ValueError(
    -            f"AsyncOpenAI API client failed to initialize. Error: {e}"
    -        ) from e
    -else:
    -    try:
    -        self.client = openai.OpenAI(api_key=api_key)
    -    except Exception as e:
    -        raise ValueError(
    -            f"OpenAI API client failed to initialize. Error: {e}"
    -        ) from e
    +try:
    +    self.client = openai.AsyncOpenAI(api_key=api_key) if use_async else openai.OpenAI(api_key=api_key)
    +except Exception as e:
    +    raise ValueError(
    +        f"{'Async' if use_async else ''}OpenAI API client failed to initialize. Error: {e}"
    +    ) from e
     
    Suggestion importance[1-10]: 9

    Why: This suggestion significantly improves code maintainability and readability by reducing redundancy in exception handling, making the code easier to manage and less error-prone.

    9
    Reset execution count to None for reproducibility

    To ensure that the notebook cells are reproducible and do not depend on external
    state, it is advisable to reset the execution count to None before sharing or
    exporting the notebook.

    docs/02-dynamic-routes.ipynb [73]

    -"execution_count": 63,
    +"execution_count": null,
     
    Suggestion importance[1-10]: 8

    Why: Resetting the execution count to None ensures that the notebook cells are reproducible and do not depend on external state, which is important for maintainability and sharing.

    8
    Best practice
    Add exception handling to asynchronous calls to improve robustness

    It's recommended to handle potential exceptions when using asynchronous calls to
    avoid runtime errors that could halt the execution of your notebook. You can use a
    try-except block to catch exceptions and handle them gracefully.

    docs/02-dynamic-routes.ipynb [489-490]

    -response = await rl.acall("what is the time in new york city?")
    +try:
    +    response = await rl.acall("what is the time in new york city?")
    +except Exception as e:
    +    response = f"An error occurred: {str(e)}"
     response
     
    Suggestion importance[1-10]: 9

    Why: Adding exception handling to asynchronous calls is a best practice that improves the robustness and reliability of the code by preventing runtime errors from halting execution.

    9
    Enhancement
    Improve the readability and maintainability of the acall method by extracting message transformation into a separate method

    Refactor the acall method to separate the logic of message transformation and API
    call into a private method to improve code readability and maintainability.

    semantic_router/llms/openai.py [133-138]

    +transformed_messages = self._transform_messages(messages)
     completion = await self.client.chat.completions.create(
         model=self.name,
    -    messages=[m.to_openai() for m in messages],
    +    messages=transformed_messages,
         temperature=self.temperature,
         max_tokens=self.max_tokens,
         tools=tools,  # type: ignore # We pass a list of dicts which get interpreted as Iterable[ChatCompletionToolParam].
     )
     
    +def _transform_messages(self, messages):
    +    return [m.to_openai() for m in messages]
    +
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances code readability and maintainability by separating concerns, making the acall method cleaner and easier to understand.

    8
    Add comments to clarify the context of outputs

    When displaying outputs in a notebook, it's useful to add explanatory text or
    comments to clarify the context or purpose of the output, especially when the output
    is a plain text or a simple data structure.

    docs/02-dynamic-routes.ipynb [230]

    +# Output showing the route choice for a casual conversation query
     "RouteChoice(name='chitchat', function_call=None, similarity_score=None)"
     
    Suggestion importance[1-10]: 7

    Why: Adding comments to clarify the context of outputs enhances readability and helps users understand the purpose of the output, especially when dealing with plain text or simple data structures.

    7
    Possible issue
    Ensure route.llm is not set to None by checking self.llm existence before assignment

    Consider checking for the existence of self.llm before assigning route.llm to avoid
    potential issues where route.llm could be set to None inadvertently if self.llm has
    not been initialized properly.

    semantic_router/layer.py [300-307]

     if not self.llm:
         logger.warning(
             "No LLM provided for dynamic route, will use OpenAI LLM default"
         )
         self.llm = OpenAILLM(use_async=True)
    -    route.llm = self.llm
    -else:
    -    route.llm = self.llm
    +route.llm = self.llm if self.llm else None
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the robustness of the code by ensuring route.llm is not set to None, but the likelihood of self.llm being None after initialization is low, making this a minor improvement.

    7

    @tolgadevAI tolgadevAI linked an issue Jul 16, 2024 that may be closed by this pull request
    @jamescalam jamescalam self-requested a review July 17, 2024 04:26
    Copy link

    codecov bot commented Jul 17, 2024

    Codecov Report

    Attention: Patch coverage is 18.33333% with 49 lines in your changes missing coverage. Please review.

    Project coverage is 68.93%. Comparing base (a59e7d1) to head (ce0489b).

    Files Patch % Lines
    semantic_router/llms/openai.py 20.93% 34 Missing ⚠️
    semantic_router/route.py 10.00% 9 Missing ⚠️
    semantic_router/layer.py 0.00% 6 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #356      +/-   ##
    ==========================================
    - Coverage   70.00%   68.93%   -1.07%     
    ==========================================
      Files          45       45              
      Lines        3024     3081      +57     
    ==========================================
    + Hits         2117     2124       +7     
    - Misses        907      957      +50     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @jamescalam jamescalam merged commit cbb685f into main Jul 19, 2024
    6 of 8 checks passed
    @jamescalam jamescalam deleted the tolga/async_dynamic_routes branch July 19, 2024 07:56
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    documentation Improvements or additions to documentation enhancement Enhancement to existing features Review effort [1-5]: 3
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Dynamic routes async calls
    2 participants