Add grpc unit test run, expand testing of VectorFactory (#326)

jhamon · web-flow · commit f229f45249d6 · 2024-03-28T13:17:08.000-04:00
## Problem

There is a small number of unit tests that should only get run when grpc
dependencies are installed. These were previously omitted from CI by
mistake.

## Solution

Add a step to run these grpc steps. Make it conditional on the
`use_grpc` test matrix param.

## Type of Change

- [x] Infrastructure change (CI configs, etc)
diff --git a/.github/workflows/testing-unit.yaml b/.github/workflows/testing-unit.yaml
@@ -7,6 +7,7 @@ jobs:
     name: Unit tests
     runs-on: ubuntu-latest
     strategy:
+      fail-fast: false
       matrix:
         python-version:
           - 3.8
@@ -28,8 +29,11 @@ jobs:
         with:
           include_grpc: '${{ matrix.use_grpc }}'
           include_types: true
-      - name: Run unit tests
+      - name: Run unit tests (REST)
         run: poetry run pytest --cov=pinecone --timeout=120 tests/unit
+      - name: Run unit tests (GRPC)
+        if: ${{ matrix.use_grpc == true }}
+        run: poetry run pytest --cov=pinecone/grpc --timeout=120 tests/unit_grpc
       - name: mypy check
         env:
           INCLUDE_GRPC: '${{ matrix.use_grpc }}'
diff --git a/pinecone/data/__init__.py b/pinecone/data/__init__.py
@@ -1,5 +1,5 @@
 from .index import *
-from .vector_factory import (
+from .errors import (
     VectorDictionaryMissingKeysError,
     VectorDictionaryExcessKeysError, 
     VectorTupleLengthError, 
diff --git a/pinecone/data/errors.py b/pinecone/data/errors.py
@@ -0,0 +1,37 @@
+from ..utils.constants import REQUIRED_VECTOR_FIELDS, OPTIONAL_VECTOR_FIELDS
+
+class VectorDictionaryMissingKeysError(ValueError):
+    def __init__(self, item):
+        message = f"Vector dictionary is missing required fields: {list(REQUIRED_VECTOR_FIELDS - set(item.keys()))}"
+        super().__init__(message)
+
+class VectorDictionaryExcessKeysError(ValueError):
+    def __init__(self, item):
+        invalid_keys = list(set(item.keys()) - (REQUIRED_VECTOR_FIELDS | OPTIONAL_VECTOR_FIELDS))
+        message = f"Found excess keys in the vector dictionary: {invalid_keys}. The allowed keys are: {list(REQUIRED_VECTOR_FIELDS | OPTIONAL_VECTOR_FIELDS)}"
+        super().__init__(message)
+
+class VectorTupleLengthError(ValueError):
+    def __init__(self, item):
+        message = f"Found a tuple of length {len(item)} which is not supported. Vectors can be represented as tuples either the form (id, values, metadata) or (id, values). To pass sparse values please use either dicts or Vector objects as inputs."
+        super().__init__(message)
+
+class SparseValuesTypeError(ValueError, TypeError):
+    def __init__(self):
+        message = "Found unexpected data in column `sparse_values`. Expected format is `'sparse_values': {'indices': List[int], 'values': List[float]}`."
+        super().__init__(message)
+
+class SparseValuesMissingKeysError(ValueError):
+    def __init__(self, sparse_values_dict):
+        message = f"Missing required keys in data in column `sparse_values`. Expected format is `'sparse_values': {{'indices': List[int], 'values': List[float]}}`. Found keys {list(sparse_values_dict.keys())}"
+        super().__init__(message)
+
+class SparseValuesDictionaryExpectedError(ValueError, TypeError):
+    def __init__(self, sparse_values_dict):
+        message = f"Column `sparse_values` is expected to be a dictionary, found {type(sparse_values_dict)}"
+        super().__init__(message)
+
+class MetadataDictionaryExpectedError(ValueError, TypeError):
+    def __init__(self, item):
+        message = f"Column `metadata` is expected to be a dictionary, found {type(item['metadata'])}"
+        super().__init__(message)
diff --git a/pinecone/data/sparse_vector_factory.py b/pinecone/data/sparse_vector_factory.py
@@ -0,0 +1,54 @@
+import numbers
+
+from collections.abc import Mapping
+from typing import Union, Dict
+
+from ..utils import convert_to_list
+
+from .errors import (
+    SparseValuesTypeError, 
+    SparseValuesMissingKeysError, 
+    SparseValuesDictionaryExpectedError
+) 
+
+from pinecone.core.client.models import (
+    SparseValues
+)
+
+class SparseValuesFactory:
+    @staticmethod
+    def build(input: Union[Dict, SparseValues]) -> SparseValues:
+        if input is None:
+            return input
+        if isinstance(input, SparseValues):
+            return input
+        if not isinstance(input, Mapping):
+            raise SparseValuesDictionaryExpectedError(input)
+        if not {"indices", "values"}.issubset(input):
+            raise SparseValuesMissingKeysError(input)
+
+        indices = SparseValuesFactory._convert_to_list(input.get("indices"), int)
+        values = SparseValuesFactory._convert_to_list(input.get("values"), float)
+
+        if len(indices) != len(values):
+            raise ValueError("Sparse values indices and values must have the same length")
+
+        try:
+            return SparseValues(indices=indices, values=values)
+        except TypeError as e:
+            raise SparseValuesTypeError() from e
+        
+    @staticmethod
+    def _convert_to_list(input, expected_type):
+        try:
+            converted = convert_to_list(input)
+        except TypeError as e:
+            raise SparseValuesTypeError() from e
+        
+        SparseValuesFactory._validate_list_items_type(converted, expected_type)
+        return converted
+    
+    @staticmethod
+    def _validate_list_items_type(input, expected_type):
+        if len(input) > 0 and not isinstance(input[0], expected_type):
+            raise SparseValuesTypeError()
diff --git a/pinecone/data/vector_factory.py b/pinecone/data/vector_factory.py
@@ -5,47 +5,19 @@
 
 from ..utils import fix_tuple_length, convert_to_list
 from ..utils.constants import REQUIRED_VECTOR_FIELDS, OPTIONAL_VECTOR_FIELDS
+from .sparse_vector_factory import SparseValuesFactory
 
 from pinecone.core.client.models import (
     Vector,
     SparseValues
 )
 
-class VectorDictionaryMissingKeysError(ValueError):
-    def __init__(self, item):
-        message = f"Vector dictionary is missing required fields: {list(REQUIRED_VECTOR_FIELDS - set(item.keys()))}"
-        super().__init__(message)
-
-class VectorDictionaryExcessKeysError(ValueError):
-    def __init__(self, item):
-        invalid_keys = list(set(item.keys()) - (REQUIRED_VECTOR_FIELDS | OPTIONAL_VECTOR_FIELDS))
-        message = f"Found excess keys in the vector dictionary: {invalid_keys}. The allowed keys are: {list(REQUIRED_VECTOR_FIELDS | OPTIONAL_VECTOR_FIELDS)}"
-        super().__init__(message)
-
-class VectorTupleLengthError(ValueError):
-    def __init__(self, item):
-        message = f"Found a tuple of length {len(item)} which is not supported. Vectors can be represented as tuples either the form (id, values, metadata) or (id, values). To pass sparse values please use either dicts or Vector objects as inputs."
-        super().__init__(message)
-
-class SparseValuesTypeError(ValueError, TypeError):
-    def __init__(self):
-        message = "Found unexpected data in column `sparse_values`. Expected format is `'sparse_values': {'indices': List[int], 'values': List[float]}`."
-        super().__init__(message)
-
-class SparseValuesMissingKeysError(ValueError):
-    def __init__(self, sparse_values_dict):
-        message = f"Missing required keys in data in column `sparse_values`. Expected format is `'sparse_values': {{'indices': List[int], 'values': List[float]}}`. Found keys {list(sparse_values_dict.keys())}"
-        super().__init__(message)
-
-class SparseValuesDictionaryExpectedError(ValueError, TypeError):
-    def __init__(self, sparse_values_dict):
-        message = f"Column `sparse_values` is expected to be a dictionary, found {type(sparse_values_dict)}"
-        super().__init__(message)
-
-class MetadataDictionaryExpectedError(ValueError, TypeError):
-    def __init__(self, item):
-        message = f"Column `metadata` is expected to be a dictionary, found {type(item['metadata'])}"
-        super().__init__(message)
+from .errors import (
+    VectorDictionaryMissingKeysError,
+    VectorDictionaryExcessKeysError,
+    VectorTupleLengthError,
+    MetadataDictionaryExpectedError,
+)
 
 class VectorFactory:
     @staticmethod
@@ -84,8 +56,10 @@ def _dict_to_vector(item, check_type: bool) -> Vector:
             item["values"] = convert_to_list(values)
 
         sparse_values = item.get("sparse_values")
-        if sparse_values and not isinstance(sparse_values, SparseValues):
-            item["sparse_values"] = VectorFactory._dict_to_sparse_values(sparse_values, check_type)
+        if sparse_values is None:
+            item.pop("sparse_values", None)
+        else:
+            item["sparse_values"] = SparseValuesFactory.build(sparse_values)
 
         metadata = item.get("metadata")
         if metadata and not isinstance(metadata, Mapping):
@@ -97,18 +71,3 @@ def _dict_to_vector(item, check_type: bool) -> Vector:
             if not isinstance(item["values"], Iterable) or not isinstance(item["values"].__iter__().__next__(), numbers.Real):
                 raise TypeError(f"Column `values` is expected to be a list of floats")
             raise e
-
-    @staticmethod
-    def _dict_to_sparse_values(sparse_values_dict: Dict, check_type: bool) -> SparseValues:
-        if not isinstance(sparse_values_dict, Mapping):
-            raise SparseValuesDictionaryExpectedError(sparse_values_dict)
-        if not {"indices", "values"}.issubset(sparse_values_dict):
-            raise SparseValuesMissingKeysError(sparse_values_dict)
-
-        indices = convert_to_list(sparse_values_dict.get("indices"))
-        values = convert_to_list(sparse_values_dict.get("values"))
-
-        try:
-            return SparseValues(indices=indices, values=values, _check_type=check_type)
-        except TypeError:
-            raise SparseValuesTypeError()
diff --git a/pinecone/exceptions.py b/pinecone/exceptions.py
@@ -17,6 +17,10 @@ class PineconeProtocolError(PineconeException):
 class PineconeConfigurationError(PineconeException):
     """Raised when a configuration error occurs."""
 
+class ListConversionException(PineconeException, TypeError):
+    def __init__(self, message):
+        super().__init__(message)
+
 __all__ = [
     "PineconeConfigurationError",
     "PineconeProtocolError",
@@ -30,4 +34,5 @@ class PineconeConfigurationError(PineconeException):
     "UnauthorizedException",
     "ForbiddenException",
     "ServiceException",
+    "ListConversionException"
 ]
diff --git a/pinecone/grpc/sparse_values_factory.py b/pinecone/grpc/sparse_values_factory.py
@@ -0,0 +1,59 @@
+import numbers
+
+from collections.abc import Mapping
+from typing import Union, Dict
+
+from ..utils import convert_to_list
+
+from ..data import (
+    SparseValuesTypeError, 
+    SparseValuesMissingKeysError, 
+    SparseValuesDictionaryExpectedError
+) 
+
+from pinecone.core.grpc.protos.vector_service_pb2 import (
+    SparseValues as GRPCSparseValues,
+)
+from pinecone import (
+    SparseValues as NonGRPCSparseValues
+)
+
+class SparseValuesFactory:
+    @staticmethod
+    def build(input: Union[Dict, GRPCSparseValues, NonGRPCSparseValues]) -> GRPCSparseValues:
+        if input is None:
+            return input
+        if isinstance(input, GRPCSparseValues):
+            return input
+        if isinstance(input, NonGRPCSparseValues):
+            return GRPCSparseValues(indices=input.indices, values=input.values)
+        if not isinstance(input, Mapping):
+            raise SparseValuesDictionaryExpectedError(input)
+        if not {"indices", "values"}.issubset(input):
+            raise SparseValuesMissingKeysError(input)
+
+        indices = SparseValuesFactory._convert_to_list(input.get("indices"), int)
+        values = SparseValuesFactory._convert_to_list(input.get("values"), float)
+
+        if len(indices) != len(values):
+            raise ValueError("Sparse values indices and values must have the same length")
+
+        try:
+            return GRPCSparseValues(indices=indices, values=values)
+        except TypeError as e:
+            raise SparseValuesTypeError() from e
+        
+    @staticmethod
+    def _convert_to_list(input, expected_type):
+        try:
+            converted = convert_to_list(input)
+        except TypeError as e:
+            raise SparseValuesTypeError() from e
+        
+        SparseValuesFactory._validate_list_items_type(converted, expected_type)
+        return converted
+    
+    @staticmethod
+    def _validate_list_items_type(input, expected_type):
+        if len(input) > 0 and not isinstance(input[0], expected_type):
+            raise SparseValuesTypeError()
diff --git a/pinecone/grpc/vector_factory_grpc.py b/pinecone/grpc/vector_factory_grpc.py
@@ -12,11 +12,9 @@
     VectorDictionaryMissingKeysError, 
     VectorDictionaryExcessKeysError, 
     VectorTupleLengthError, 
-    SparseValuesTypeError, 
-    SparseValuesMissingKeysError, 
-    SparseValuesDictionaryExpectedError, 
     MetadataDictionaryExpectedError 
 )
+from .sparse_values_factory import SparseValuesFactory
 
 from pinecone.core.grpc.protos.vector_service_pb2 import (
     Vector as GRPCVector,
@@ -73,8 +71,8 @@ def _dict_to_vector(item) -> GRPCVector:
                 raise TypeError(f"Column `values` is expected to be a list of floats") from e
 
         sparse_values = item.get("sparse_values")
-        if sparse_values and not isinstance(sparse_values, GRPCSparseValues):
-            item["sparse_values"] = VectorFactoryGRPC._dict_to_sparse_values(sparse_values)
+        if sparse_values != None and not isinstance(sparse_values, GRPCSparseValues):
+            item["sparse_values"] = SparseValuesFactory.build(sparse_values)
 
         metadata = item.get("metadata")
         if metadata:
@@ -90,32 +88,4 @@ def _dict_to_vector(item) -> GRPCVector:
         except TypeError as e:
             if not isinstance(item["values"], Iterable) or not isinstance(item["values"].__iter__().__next__(), numbers.Real):
                 raise TypeError(f"Column `values` is expected to be a list of floats")
-            raise e
-
-    @staticmethod
-    def _dict_to_sparse_values(sparse_values_dict: Union[Dict, GRPCSparseValues, NonGRPCSparseValues]) -> GRPCSparseValues:
-        if isinstance(sparse_values_dict, GRPCSparseValues):
-            return sparse_values_dict
-        if isinstance(sparse_values_dict, NonGRPCSparseValues):
-            return GRPCSparseValues(indices=sparse_values_dict.indices, values=sparse_values_dict.values)
-
-        if not isinstance(sparse_values_dict, Mapping):
-            raise SparseValuesDictionaryExpectedError(sparse_values_dict)
-        if not {"indices", "values"}.issubset(sparse_values_dict):
-            raise SparseValuesMissingKeysError(sparse_values_dict)
-
-    
-        try:
-            indices = convert_to_list(sparse_values_dict.get("indices"))
-        except TypeError as e:
-            raise SparseValuesTypeError() from e
-  
-        try:
-            values = convert_to_list(sparse_values_dict.get("values"))
-        except TypeError as e:
-            raise SparseValuesTypeError() from e
-
-        try:
-            return GRPCSparseValues(indices=indices, values=values)
-        except TypeError as e:
-            raise SparseValuesTypeError() from e
+            raise e
diff --git a/pinecone/utils/convert_to_list.py b/pinecone/utils/convert_to_list.py
@@ -1,9 +1,18 @@
+from ..exceptions import ListConversionException
+
 def convert_to_list(obj):
     class_name = obj.__class__.__name__
 
     if class_name == 'list':
         return obj
     elif hasattr(obj, 'tolist') and callable(getattr(obj, 'tolist')):
         return obj.tolist()
+    elif obj is None or isinstance(obj, str) or isinstance(obj, dict):
+        # The string and dictionary classes in python can be passed to list()
+        # but they're not going to yield sensible results for our use case.
+        raise ListConversionException(f"Expected a list or list-like data structure, but got: {obj}")
     else:
-        return list(obj)
+        try:
+            return list(obj)
+        except Exception as e:
+            raise ListConversionException(f"Expected a list or list-like data structure, but got: {obj}") from e
diff --git a/tests/unit/data/test_vector_factory.py b/tests/unit/data/test_vector_factory.py
diff --git a/tests/unit/utils/test_convert_to_list.py b/tests/unit/utils/test_convert_to_list.py
diff --git a/tests/unit_grpc/test_sparse_values_factory.py b/tests/unit_grpc/test_sparse_values_factory.py
diff --git a/tests/unit_grpc/test_vector_factory_grpc.py b/tests/unit_grpc/test_vector_factory_grpc.py
