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.

Sign up for GitHub

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

Jump to bottom

feat: add sparse vector for PineconeIndex #355

Merged
merged 6 commits into from
Jul 16, 2024
Merged

feat: add sparse vector for PineconeIndex #355

merged 6 commits into from
Jul 16, 2024

Conversation

ashraq1455
Copy link
Contributor

@ashraq1455 ashraq1455 commented Jul 16, 2024 •
edited by github-actions bot
Loading

PR Type

Enhancement, Bug fix

Description

  • Added aquery method to local.py for asynchronous querying with route filtering and error handling.
  • Enhanced query and aquery methods in pinecone.py to support sparse_vector parameter.
  • Updated _async_query method in pinecone.py to handle sparse_vector.
  • Included kwargs in pinecone.py methods to pass additional parameters.

Changes walkthrough 📝

Relevant files
Enhancement
local.py
Add asynchronous query method with route filtering             

semantic_router/index/local.py

  • Added aquery method for asynchronous querying.
  • Implemented route filtering within aquery.
  • Added error handling for empty index or routes.
    		•  +29/-0   
    pinecone.py
    Support sparse vectors in Pinecone query methods                 

    semantic_router/index/pinecone.py

  • Added sparse_vector parameter to query and aquery methods.
  • Updated _async_query method to handle sparse_vector.
  • Included kwargs to pass additional parameters.
    		•  +6/-0     

    💡 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 enhancement Enhancement to existing features Bug fix labels Jul 16, 2024
    @github-actions GitHub Actions
    Copy link

    PR Reviewer Guide 🔍

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

    Error Handling
    The error message "No routes found matching the filter criteria." in line 121 might not be descriptive enough about the context of the error. Consider including more details about the expected input or state.

    Sparse Vector Handling
    The handling of 'sparse_vector' using kwargs.get might lead to unexpected behavior if 'sparse_vector' is not provided as a keyword argument. Consider validating 'sparse_vector' before its usage.

    @github-actions GitHub Actions
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a null check for sparse_vector before usage

    Add a check to ensure that sparse_vector is not None before using it in the
    _async_query function to prevent potential runtime errors.

    semantic_router/index/pinecone.py [529]

    -"sparse_vector": sparse_vector,
+"sparse_vector": sparse_vector if sparse_vector is not None else {},
    Suggestion importance[1-10]: 9

    Why: Adding a null check for sparse_vector before usage is crucial to prevent potential runtime errors, enhancing the robustness of the code.

    		9
    Enhancement
    Use list comprehensions to build filtered lists

    Consider using list comprehensions for building filtered_index and filtered_routes
    to improve readability and potentially enhance performance.

    semantic_router/index/local.py [114-119]

    -filtered_index = []
-filtered_routes = []
-for route, vec in zip(self.routes, self.index):
-    if route in route_filter:
-        filtered_index.append(vec)
-        filtered_routes.append(route)
+filtered_index = [vec for route, vec in zip(self.routes, self.index) if route in route_filter]
+filtered_routes = [route for route in self.routes if route in route_filter]
    Suggestion importance[1-10]: 8

    Why: The suggestion improves code readability and potentially enhances performance by using list comprehensions, which are generally more efficient and concise.

    		8
    Best practice
    Use a more specific type hint for vector

    Consider using a more specific type hint for vector in _async_query to align with
    the expected data structure, improving type safety and clarity.

    semantic_router/index/pinecone.py [520]

    -vector: list[float],
+vector: List[float],
    Suggestion importance[1-10]: 7

    Why: Using List[float] instead of list[float] aligns with the standard library's type hinting conventions, improving type safety and clarity.

    		7
    Possible issue
    Explicitly handle the sparse_vector parameter in the method signature

    Ensure that the sparse_vector parameter is properly handled in the query method to
    avoid potential issues with unhandled keyword arguments.

    semantic_router/index/pinecone.py [467]

    -**kwargs: Any,
+sparse_vector: Optional[dict] = None,
    Suggestion importance[1-10]: 5

    Why: While explicitly handling the sparse_vector parameter can improve clarity, the existing use of **kwargs is also valid and flexible. This suggestion is more about style preference.

    		5

    @codecov Codecov
    Copy link

    codecov bot commented Jul 16, 2024 •
    edited
    Loading

    Codecov Report

    Attention: Patch coverage is 10.00000% with 18 lines in your changes missing coverage. Please review.

    Project coverage is 70.00%. Comparing base (ae08fab) to head (75a1799).

    Files Patch % Lines
    semantic_router/index/local.py 5.26% 18 Missing ⚠️
    Additional details and impacted files 
    @@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
- Coverage   70.41%   70.00%   -0.41%     
==========================================
  Files          45       45              
  Lines        3005     3024      +19     
==========================================
+ Hits         2116     2117       +1     
- Misses        889      907      +18

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

    @jamescalam jamescalam merged commit a59e7d1 into main Jul 16, 2024
    6 of 8 checks passed
    @jamescalam jamescalam deleted the ashraq/sparse-vector branch July 16, 2024 07:49
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @jamescalam jamescalam jamescalam approved these changes

    Assignees
    No one assigned
    Labels
    Bug fix enhancement Enhancement to existing features Review effort [1-5]: 3
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @ashraq1455 @jamescalam