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: add namespace to get all for pinecone #270

Merged
merged 1 commit into from
May 9, 2024

Conversation

jamescalam
Copy link
Member

@jamescalam jamescalam commented May 9, 2024

PR Type

bug_fix


Description

  • The PR adds a condition to include the namespace in the request parameters for the _get_all method in the Pinecone router. This ensures that the namespace is considered when fetching all vectors.

Changes walkthrough 📝

Relevant files
Enhancement
pinecone.py
Include Namespace in API Request Parameters                           

semantic_router/index/pinecone.py

  • Added a condition to include namespace in the request parameters if it
    is set.
  • +2/-0     

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

    Copy link

    github-actions bot commented May 9, 2024

    PR Description updated to latest commit (80bcc4a)

    Copy link

    github-actions bot commented May 9, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR involves a small change in a single file and the logic added is straightforward. The change is limited to adding a condition to include an additional parameter in an API request.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The condition if self.namespace: assumes that self.namespace is either truthy or falsy. If self.namespace can be an empty string or zero which are valid values, this condition might incorrectly exclude them.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesemantic_router/index/pinecone.py
    suggestion      

    Consider checking for None explicitly when adding the namespace to the parameters. This avoids excluding valid falsy values like empty strings or zeros. Use if self.namespace is not None: to ensure all valid values are included. [important]

    relevant lineif self.namespace:

    Copy link

    github-actions bot commented May 9, 2024

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Ensure namespace is non-empty before adding to parameters.

    Consider checking if self.namespace is not only present but also non-empty before adding
    it to the params dictionary. This helps avoid sending unnecessary or invalid parameters in
    the request.

    semantic_router/index/pinecone.py [191-192]

    -if self.namespace:
    +if self.namespace and self.namespace.strip():
         params["namespace"] = self.namespace
     

    Copy link

    codecov bot commented May 9, 2024

    Codecov Report

    Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

    Project coverage is 80.31%. Comparing base (a4404fa) to head (80bcc4a).

    Files Patch % Lines
    semantic_router/index/pinecone.py 0.00% 2 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #270      +/-   ##
    ==========================================
    - Coverage   80.37%   80.31%   -0.07%     
    ==========================================
      Files          44       44              
      Lines        2441     2443       +2     
    ==========================================
      Hits         1962     1962              
    - Misses        479      481       +2     

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

    @jamescalam jamescalam merged commit 5d59a8b into main May 9, 2024
    6 of 8 checks passed
    @jamescalam jamescalam deleted the james/add-namespace-to-get-all branch May 9, 2024 13:51
    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.

    None yet

    1 participant