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: handling non-existing local routes in sync #358

Conversation

Vits-99
Copy link
Contributor

@Vits-99 Vits-99 commented Jul 16, 2024

User description

PR for introducing a fix to issue #357: Sync feature not handling non-existing local routes present remotely

  • Added robustness checks for empty local or remote routes
  • Added possibility to pass empty list of routes at startup to sync with just remote routes
  • Fixed the bug causing not handling of non-existing local routes present remotely

Can be tested with basic code like the following and playing with local and remote routes:

from semantic_router import Route
from semantic_router.encoders import OpenAIEncoder
from semantic_router.index.pinecone import PineconeIndex
from semantic_router.layer import RouteLayer


politics = Route(
    name="politics",
    utterances=[
        "isn't politics the best thing ever",
        "why don't you tell me about your political opinions",
        "don't you just love the president",
    ],
)

chitchat = Route(
    name="chitchat",
    utterances=[
        "how's the weather today?",
        "how are things going?",
    ],
)

routes = [politics, chitchat]

encoder = OpenAIEncoder(openai_api_key=openai_api_key)

pc_index = PineconeIndex(
    api_key=pinecone_api_key,
    region="us-east-1",
    index_name="alai-clearwater-routes-input-intent",
    sync="local",
)

rl = RouteLayer(encoder=encoder, routes=routes, index=pc_index)

PR Type

Bug fix, Enhancement


Description

  • Added robustness checks for empty local or remote routes during synchronization.
  • Enhanced _sync_index method to handle various synchronization modes more effectively.
  • Modified _add_and_sync method to return the updated list of Route objects.
  • Introduced _set_layer_routes method in RouteLayer to set and override current routes.
  • Updated RouteLayer initialization to handle cases where no local routes are provided and sync is set to use remote routes.

Changes walkthrough 📝

Relevant files
Enhancement
pinecone.py
Improve sync feature robustness and handling of routes     

semantic_router/index/pinecone.py

  • Added checks for empty local or remote routes during sync.
  • Enhanced _sync_index to handle various sync modes more robustly.
  • Modified _add_and_sync to return updated list of Route objects.
  • Updated synchronization logic to include layer_routes.
  • +52/-4   
    layer.py
    Initialize index with remote routes and manage layer routes

    semantic_router/layer.py

  • Added logic to initialize index with remote routes if local routes are
    empty.
  • Introduced _set_layer_routes method to set and override current
    routes.
  • Updated _add_routes to use the new sync logic and set layer routes.
  • +23/-1   

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

    @Vits-99 Vits-99 requested a review from jamescalam July 16, 2024 20:57
    @Vits-99 Vits-99 linked an issue Jul 16, 2024 that may be closed by this pull request
    @github-actions github-actions bot added enhancement Enhancement to existing features Bug fix Review effort [1-5]: 4 labels Jul 16, 2024
    Copy link

    PR Reviewer Guide 🔍

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

    Error Handling
    The error handling in _sync_index might be too strict or unclear for users. The multiple nested conditions and checks for route presence could be simplified or better documented to enhance maintainability and readability.

    Method Complexity
    The _sync_index method has become significantly complex with multiple responsibilities, including error handling, route synchronization, and updating route lists. Consider refactoring to separate concerns and simplify the logic.

    Return Type Change
    The method _add_and_sync has an updated return type to include a list of Route objects, which might affect existing integrations or require updates in the calling code.

    Initialization Logic
    The initialization logic in RouteLayer now handles cases where no routes are provided differently based on the sync mode. This change could lead to unexpected behaviors if not properly documented or understood.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Specify which condition (local or remote) caused the error in the error message for better clarity

    The error message "No routes found in the index." is raised in multiple conditions.
    It would be more informative to specify which condition (local or remote) caused the
    error to help with debugging.

    semantic_router/index/pinecone.py [227]

    -raise ValueError("No routes found in the index.")
    +if not remote_routes:
    +    raise ValueError("No remote routes found in the index.")
    +if not local_routes["routes"]:
    +    raise ValueError("No local routes found in the index.")
     
    Suggestion importance[1-10]: 9

    Why: This suggestion enhances error messages, making debugging easier by specifying whether the local or remote condition caused the error. It significantly improves code clarity and usability.

    9
    Best practice
    Initialize the layer_routes dictionary at the start of the function to ensure it is always defined

    To ensure that the layer_routes dictionary is consistently initialized, it should be
    defined outside of the conditional blocks. This avoids potential NameError if none
    of the conditions are met before its usage.

    semantic_router/index/pinecone.py [241]

     layer_routes = {}
    +# Initialize layer_routes at the start of the function to ensure it is always defined.
     
    Suggestion importance[1-10]: 8

    Why: This suggestion ensures that the layer_routes dictionary is always defined, preventing potential NameError issues. It addresses a potential bug and improves code reliability.

    8
    Maintainability
    Refactor repeated sync setting checks into a separate method to simplify the _sync_index method

    The conditional checks for sync settings are repeated multiple times. Consider
    refactoring these checks into a separate method to simplify the _sync_index method
    and improve code maintainability.

    semantic_router/index/pinecone.py [254-295]

    -if self.sync == "remote":
    +def check_sync_setting(self, setting):
    +    return self.sync == setting
    +
    +if self.check_sync_setting("remote"):
         ...
    -elif self.sync == "local":
    +elif self.check_sync_setting("local"):
         ...
    -elif self.sync == "merge":
    +elif self.check_sync_setting("merge"):
         ...
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code maintainability by reducing repetition and simplifying the _sync_index method. However, it introduces additional methods which may add slight overhead.

    7
    Break down the _sync_index method into smaller methods to improve readability and maintainability

    The method _sync_index is complex due to multiple nested conditions. Consider
    breaking down this method into smaller, more focused methods to improve readability
    and maintainability.

    semantic_router/index/pinecone.py [205-229]

     def _sync_index(self, local_routes: dict):
    +    self.validate_routes(local_routes)
    +    self.process_sync_settings(local_routes)
    +
    +def validate_routes(self, local_routes):
    +    ...
    +def process_sync_settings(self, local_routes):
         ...
     
    Suggestion importance[1-10]: 6

    Why: This suggestion improves readability and maintainability by breaking down a complex method into smaller, focused methods. However, it requires careful implementation to ensure functionality remains intact.

    6

    @Vits-99 Vits-99 changed the title Fixed issue #357 Sync feature not handling non-existing local routes present remotely Fixes #357 Sync feature not handling non-existing local routes present remotely Jul 16, 2024
    @jamescalam jamescalam changed the title Fixes #357 Sync feature not handling non-existing local routes present remotely fix: Sync feature not handling non-existing local routes present remotely Jul 17, 2024
    @jamescalam
    Copy link
    Member

    @Vits-99 looks good but seems to have broken some things in pytest (or pytest needs updating), could we resolve those please?

    Copy link

    codecov bot commented Jul 17, 2024

    Codecov Report

    Attention: Patch coverage is 15.00000% with 34 lines in your changes missing coverage. Please review.

    Project coverage is 68.14%. Comparing base (cbb685f) to head (ac97a3c).

    Files Patch % Lines
    semantic_router/index/pinecone.py 4.16% 23 Missing ⚠️
    semantic_router/layer.py 31.25% 11 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #358      +/-   ##
    ==========================================
    - Coverage   68.93%   68.14%   -0.80%     
    ==========================================
      Files          45       45              
      Lines        3081     3117      +36     
    ==========================================
      Hits         2124     2124              
    - Misses        957      993      +36     

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

    @Vits-99
    Copy link
    Contributor Author

    Vits-99 commented Jul 17, 2024

    @jamescalam fixed broken pytests by fixing some details on modified code. Tests are ok, no update required

    @jamescalam jamescalam changed the title fix: Sync feature not handling non-existing local routes present remotely fix: handling non-existing local routes in sync Jul 31, 2024
    @jamescalam jamescalam merged commit 6da452a into main Jul 31, 2024
    6 of 8 checks passed
    @jamescalam jamescalam deleted the vittorio/357-sync-feature-not-handling-non-existing-local-routes-present-remotely branch July 31, 2024 06:24
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix enhancement Enhancement to existing features Review effort [1-5]: 4
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Sync feature not handling non-existing local routes present remotely
    2 participants