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: Add Pinecone Local Index support and update dependencies #522

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

theanupllm
Copy link
Contributor

@theanupllm theanupllm commented Jan 31, 2025

User description

  • Added PineconeLocalIndex to support local Pinecone testing
  • Updated GitHub Actions workflow to use Pinecone local service
  • Updated dependencies in pyproject.toml and poetry.lock
  • Modified schema and router to support PineconeLocalIndex
  • Updated test configurations to use PineconeLocalIndex

PR Type

Enhancement, Tests, Documentation


Description

  • Introduced PineconeLocalIndex for local Pinecone testing.

  • Updated test configurations and workflows to support PineconeLocalIndex.

  • Added comprehensive documentation and examples for PineconeLocalIndex.

  • Updated dependencies, including aurelio-sdk to version ^0.0.18.


Changes walkthrough 📝

Relevant files
Enhancement
4 files
__init__.py
Added `PineconeLocalIndex` to index imports.                         
+2/-0     
pinecone_local.py
Implemented `PineconeLocalIndex` for local Pinecone testing.
+1037/-0
base.py
Integrated `PineconeLocalIndex` into router initialization.
+5/-1     
schema.py
Updated schema to use `BM25SparseEmbedding` for compatibility.
+3/-2     
Tests
1 files
test_router.py
Added unit tests for `PineconeLocalIndex` integration.     
+27/-15 
Configuration changes
2 files
test.yml
Updated GitHub Actions to include Pinecone Local service.
+23/-8   
Makefile
Adjusted test commands for functional testing.                     
+2/-1     
Documentation
2 files
10-pinecone-local.ipynb
Added tutorial for `PineconeLocalIndex` usage.                     
+541/-0 
pinecone-local.ipynb
Added detailed example for `PineconeLocalIndex`.                 
+771/-0 
Dependencies
1 files
pyproject.toml
Updated dependencies, including `aurelio-sdk` to `^0.0.18`.
+1/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • - Added PineconeLocalIndex to support local Pinecone testing
    - Updated GitHub Actions workflow to use Pinecone local service
    - Updated dependencies in pyproject.toml and poetry.lock
    - Modified schema and router to support PineconeLocalIndex
    - Updated test configurations to use PineconeLocalIndex
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵
    🧪 PR contains tests
    🔒 Security concerns

    Logging Sensitive Information:
    The PineconeLocalIndex class logs the API key in its initialization method. This could lead to sensitive information being exposed in logs. Ensure that sensitive data is either not logged or is properly masked to mitigate security risks.

    ⚡ Recommended focus areas for review

    Possible Performance Bottleneck

    The _init_index method in PineconeLocalIndex includes a loop that waits for the index to be ready, which could lead to performance issues or unnecessary delays during initialization. Consider optimizing this logic or adding a timeout mechanism.

    def _init_index(self, force_create: bool = False) -> Any | None:
        """Initializing the index can be done after the object has been created
        to allow for the user to set the dimensions and other parameters.
    
        If the index doesn't exist and the dimensions are given, the index will
        be created. If the index exists, it will be returned. If the index doesn't
        exist and the dimensions are not given, the index will not be created and
        None will be returned.
    
        :param force_create: If True, the index will be created even if the
            dimensions are not given (which will raise an error).
        :type force_create: bool, optional
        """
        index_exists = self.index_name in self.client.list_indexes().names()
        dimensions_given = self.dimensions is not None
        # pinecone_base_url = os.getenv("PINECONE_API_BASE_URL")
    
        # if not pinecone_base_url:
        #     pinecone_base_url = "http://localhost:5080"
    
        if dimensions_given and not index_exists:
            # if the index doesn't exist and we have dimension value
            # we create the index
            self.client.create_index(
                name=self.index_name,
                dimension=self.dimensions,
                metric=self.metric,
                spec=self.ServerlessSpec(cloud=self.cloud, region=self.region),
            )
            # wait for index to be created
            while not self.client.describe_index(self.index_name).status["ready"]:
                time.sleep(1)
            index = self.client.Index(self.index_name)
    Logging Sensitive Information

    The __init__ method logs the Pinecone API key, which could expose sensitive information. Ensure that sensitive data is not logged or is properly masked.

    self.api_key = api_key or os.getenv("PINECONE_API_KEY")
    logger.warning(f"Pinecone API key: {self.api_key}")
    logger.warning(f"Pinecone API key os env: {os.getenv('PINECONE_API_KEY')}")
    Documentation Clarity

    The documentation for using PineconeLocalIndex is extensive but could benefit from clearer explanations of limitations and use cases, especially for users unfamiliar with Pinecone Local.

    {
     "cells": [
      {
       "cell_type": "markdown",
       "metadata": {},
       "source": [
        "# Using PineconeLocalIndex for Routes\n",
        "\n",
        "Pinecone Local is an in-memory Pinecone Database emulator available as a Docker image.\n",
        "\n",
        "It's useful for running tests using the Pinecone local server. Your data will not leave your system, which is also helpful if you want to do testing locally before committing to a Pinecone account.\n",
        "\n",
        "## Limitations\n",
        "Pinecone Local has the following limitations:\n",
        "\n",
        "- Pinecone Local is available only as a Docker image.\n",
        "- Pinecone Local is an in-memory emulator and is not suitable for production. Records loaded into Pinecone Local do not persist after it is stopped.\n",
        "- Pinecone Local does not authenticate client requests. API keys are ignored.\n",
        "- Max number of records per index: 100,000.\n",
        "\n",
        "## Getting Started\n",
        "Make sure [Docker](https://docs.docker.com/get-docker/) is installed and running on your local machine.\n",
        "\n",
        "### Download the latest pinecone-local Docker image:\n",
        "\n",
        "\n",
        "Download the latest pinecone-local Docker image:\n",
        "\n",
        "```bash\n",
        "docker pull ghcr.io/pinecone-io/pinecone-local:latest\n",
        "```\n",
        "\n",
        "### Start Pinecone Local:\n",
        "\n",
        "```bash\n",
        "docker run -d \\\n",
        "--name pinecone-local \\\n",
        "-e PORT=5080 \\\n",
        "-e PINECONE_HOST=localhost \\\n",
        "-p 5080-6000:5080-6000 \\\n",
        "--platform linux/amd64 \\\n",
        "ghcr.io/pinecone-io/pinecone-local:latest\n",
        "```\n",
        "\n"
       ]
    

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add timeout to readiness loop

    Include a timeout mechanism in the while loop waiting for the index to be ready to
    prevent infinite loops in case of an error.

    docs/10-pinecone-local.ipynb [139-140]

    +timeout = 60  # seconds
    +start_time = time.time()
     while not pc.describe_index(index_name).status["ready"]:
    +    if time.time() - start_time > timeout:
    +        raise TimeoutError("Index readiness timed out.")
         time.sleep(1)
    Suggestion importance[1-10]: 10

    Why: Including a timeout mechanism in the readiness loop prevents potential infinite loops, which could cause the program to hang indefinitely. This is a critical improvement for ensuring the reliability and stability of the code.

    10
    Add timeout to prevent infinite loops

    Add a timeout mechanism in the _init_index method's loop to prevent infinite waiting
    in case the index creation process fails or takes too long.

    semantic_router/index/pinecone_local.py [250-251]

    +timeout = 60  # Timeout in seconds
    +start_time = time.time()
     while not self.client.describe_index(self.index_name).status["ready"]:
    +    if time.time() - start_time > timeout:
    +        raise TimeoutError("Index creation timed out.")
         time.sleep(1)
    Suggestion importance[1-10]: 9

    Why: Adding a timeout mechanism to the _init_index method prevents infinite loops in case the index creation process fails or takes too long. This is a critical improvement for robustness and ensures the system does not hang indefinitely.

    9
    Ensure consistent embedding dimensions

    Validate that the embeddings list in the build_records function has consistent
    dimensions to avoid runtime errors during vector processing.

    semantic_router/index/pinecone_local.py [43-57]

    +if not all(len(embeddings[0]) == len(vec) for vec in embeddings):
    +    raise ValueError("All embeddings must have the same dimensions.")
     vectors_to_upsert = [
         PineconeRecord(
             values=vector,
             route=route,
             utterance=utterance,
             function_schema=json.dumps(function_schema),
             metadata=metadata,
         ).to_dict()
         for vector, route, utterance, function_schema, metadata in zip(
             embeddings,
             routes,
             utterances,
             function_schemas,
             metadata_list,
         )
     ]
    Suggestion importance[1-10]: 8

    Why: Validating the consistency of embedding dimensions in the build_records function is essential to avoid runtime errors during vector processing. This suggestion enhances the reliability of the function by ensuring input data integrity.

    8
    Add exception handling for index operations

    Add exception handling for the pc.create_index and pc.delete_index calls to ensure
    the program doesn't crash if these operations fail.

    docs/10-pinecone-local.ipynb [128-135]

    -pc.create_index(
    -    name=index_name,
    -    dimension=2,
    -    metric="cosine",
    -    spec=ServerlessSpec(
    -        cloud="aws",
    -        region="us-east-1",
    -    ),
    -)
    +try:
    +    pc.create_index(
    +        name=index_name,
    +        dimension=2,
    +        metric="cosine",
    +        spec=ServerlessSpec(
    +            cloud="aws",
    +            region="us-east-1",
    +        ),
    +    )
    +except Exception as e:
    +    print(f"Failed to create index: {e}")
    Suggestion importance[1-10]: 8

    Why: Adding exception handling for the pc.create_index operation ensures that the program does not crash if the index creation fails, improving the robustness of the code. This is a valuable improvement, especially in scenarios where the operation might fail due to external factors.

    8
    Fix concatenated strings in utterances

    Ensure that the utterances list in the politics route does not contain concatenated
    strings like "don't you just love the president" "don't you just hate the
    president", as this could lead to unexpected behavior during processing.

    docs/indexes/pinecone-local.ipynb [79]

    -"don't you just love the president\" \"don't you just hate the president",
    +"don't you just love the president", "don't you just hate the president",
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a clear issue where concatenated strings in the utterances list could lead to unexpected behavior during processing. The improved code correctly separates the strings, ensuring proper handling.

    8
    Security
    Avoid logging sensitive API keys

    Ensure that the api_key is securely handled and avoid logging it directly to prevent
    potential security risks.

    semantic_router/index/pinecone_local.py [147-148]

    -logger.warning(f"Pinecone API key: {self.api_key}")
    -logger.warning(f"Pinecone API key os env: {os.getenv('PINECONE_API_KEY')}")
    +logger.warning("Pinecone API key is set.")
    +logger.warning("Pinecone API key retrieved from environment variables.")
    Suggestion importance[1-10]: 10

    Why: Avoiding the logging of sensitive API keys is a critical security improvement. This change mitigates the risk of exposing sensitive information in logs, ensuring better compliance with security best practices.

    10
    General
    Replace fixed sleep with retry logic

    Ensure that the Pinecone service is fully operational by implementing a retry
    mechanism instead of a fixed 10-second sleep, to avoid potential race conditions or
    unnecessary delays.

    .github/workflows/test.yml [33-35]

     run: |
         echo "Waiting for Pinecone service to start..."
    -    sleep 10  # Wait for 10 seconds to ensure the container is up
    +    for i in {1..10}; do
    +      if nc -z localhost 5080; then
    +        echo "Pinecone service is up!"
    +        break
    +      fi
    +      sleep 1
    +    done
    Suggestion importance[1-10]: 9

    Why: Replacing the fixed sleep with a retry mechanism improves reliability by ensuring the Pinecone service is actually operational before proceeding, avoiding potential race conditions or unnecessary delays. This is a significant enhancement to the workflow's robustness.

    9
    Add error handling for missing API key

    Add error handling for cases where the PINECONE_API_KEY environment variable is not
    set, to prevent runtime errors when initializing the PineconeLocalIndex.

    docs/indexes/pinecone-local.ipynb [166-168]

    +if not os.environ.get("PINECONE_API_KEY"):
    +    raise ValueError("PINECONE_API_KEY is not set. Please provide a valid API key.")
     os.environ["PINECONE_API_KEY"] = os.environ.get("PINECONE_API_KEY") or getpass(
         "Enter Pinecone API key: "
     )
    Suggestion importance[1-10]: 9

    Why: Adding error handling for a missing PINECONE_API_KEY environment variable is crucial to prevent runtime errors. This suggestion improves robustness and ensures the application fails gracefully with a clear error message.

    9
    Add timeout for asynchronous queries

    Add a timeout or retry mechanism for the await router.acall() method to handle
    potential delays or failures in asynchronous queries gracefully.

    docs/indexes/pinecone-local.ipynb [701]

    -await router.acall("how are things going?")
    +try:
    +    result = await asyncio.wait_for(router.acall("how are things going?"), timeout=10)
    +except asyncio.TimeoutError:
    +    raise RuntimeError("Asynchronous query timed out.")
    Suggestion importance[1-10]: 8

    Why: Adding a timeout for asynchronous queries enhances the reliability of the system by handling potential delays or failures gracefully. This is a practical improvement for handling real-world scenarios.

    8
    Improve error handling in queries

    Add error handling in the query method to catch and log specific exceptions when
    querying the index fails, instead of retrying without context.

    semantic_router/index/pinecone_local.py [599-607]

    -except Exception:
    -    logger.error("retrying query with vector as str")
    -    results = self.index.query(
    -        vector=query_vector_list,
    -        sparse_vector=sparse_vector,
    -        top_k=top_k,
    -        filter=filter_query,
    -        include_metadata=True,
    -        namespace=self.namespace,
    -    )
    +except Exception as e:
    +    logger.error(f"Query failed with error: {e}")
    +    raise RuntimeError("Query execution failed. Please check the input and index status.") from e
    Suggestion importance[1-10]: 7

    Why: Adding specific error handling in the query method improves debugging and provides more informative feedback when querying the index fails. This enhances maintainability and user experience.

    7
    Validate dimensions during index initialization

    Validate the dimensions parameter when initializing PineconeLocalIndex to ensure it
    matches the expected embedding size, avoiding potential mismatches during queries.

    docs/indexes/pinecone-local.ipynb [171]

    +if dimensions != 1536:
    +    raise ValueError("Invalid dimensions. Expected 1536.")
     index = PineconeLocalIndex(index_name="route-test", dimensions=1536)
    Suggestion importance[1-10]: 7

    Why: Validating the dimensions parameter ensures that the index is initialized with the correct embedding size, preventing potential mismatches during queries. While useful, the suggestion assumes a fixed dimension size, which might not always be applicable.

    7

    - Corrected string stripping and URL construction for localhost and base URL scenarios
    - Ensured consistent URL formatting across multiple methods
    - Minor code cleanup for host URL generation
    - Replaced all references to PineconeIndex with PineconeLocalIndex in test files
    - Added dimension handling for OpenAI and Cohere encoders in index initialization
    - Updated GitHub Actions workflow to cache Poetry dependencies
    - Maintained existing test logic and retry mechanisms
    - Updated default dimensions to 1536 in test_sync.py
    - Aligns with typical embedding model dimension requirements
    - Changed default dimensions from 1536 to 3 in test_sync.py
    - Simplified index initialization parameter for testing purposes
    Copy link

    codecov bot commented Jan 31, 2025

    Codecov Report

    Attention: Patch coverage is 51.68539% with 43 lines in your changes missing coverage. Please review.

    Project coverage is 74.88%. Comparing base (a83c380) to head (eae6433).
    Report is 4 commits behind head on main.

    Files with missing lines Patch % Lines
    semantic_router/index/pinecone.py 50.57% 43 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #522      +/-   ##
    ==========================================
    - Coverage   75.74%   74.88%   -0.87%     
    ==========================================
      Files          43       43              
      Lines        3855     3930      +75     
    ==========================================
    + Hits         2920     2943      +23     
    - Misses        935      987      +52     

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

    theanupllm and others added 6 commits February 3, 2025 22:04
    …implementation
    
    - Deleted `pinecone_local.py` and `10-pinecone-local.ipynb`
    - Updated `pinecone.py` to handle both cloud and local Pinecone index scenarios
    - Modified test files to use `PineconeIndex` instead of `PineconeLocalIndex`
    - Updated GitHub Actions workflow to simplify Python version testing
    - Improved error handling and host URL management in Pinecone index methods
    - Uncomment and enable multiple Python versions (3.10, 3.11, 3.12, 3.13)
    - Enable Poetry caching in GitHub Actions
    - Remove local path addition comment
    - Simplify workflow configuration
    @@ -127,19 +137,25 @@ def __init__(
    region: str = "us-east-1",
    host: str = "",
    namespace: Optional[str] = "",
    base_url: Optional[str] = "https://api.pinecone.io",
    base_url: Optional[str] = "",
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @theanupllm wouldn't this break PineconeIndex if used normally (ie not using pinecone local)?

    Copy link
    Member

    @jamescalam jamescalam left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    one q above

    …nments
    
    - Update PineconeIndex to handle both local and cloud Pinecone index configurations
    - Add base_url default to Pinecone cloud API
    - Improve logging for base URL selection
    - Update documentation notebook to reflect new configuration approach
    - Refactor base URL assignment to prioritize environment variable
    - Ensure consistent base URL configuration for Pinecone index
    - Maintain existing logging behavior for remote API usage
    - Set X-Pinecone-API-Version header for cloud API endpoints
    - Ensure compatibility with Pinecone's latest API versioning
    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.

    2 participants