-
Notifications
You must be signed in to change notification settings - Fork 103
Fix grpc timeouts when async_req=True #510
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
Changes from all commits
1ff6df4
ce07dce
38d3d8f
07fdd9b
1cd842c
f1f2ee9
a83b7cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,15 @@ def __init__( | |
_endpoint_override: Optional[str] = None, | ||
): | ||
self.config = config | ||
self.grpc_client_config = grpc_config or GRPCClientConfig() | ||
# If grpc_config is passed, use it. Otherwise, build a new one with | ||
# default values and passing in the ssl_verify value from the config. | ||
if self.config.ssl_verify is None: | ||
default_grpc_config = GRPCClientConfig() | ||
else: | ||
default_grpc_config = GRPCClientConfig(secure=self.config.ssl_verify) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change allows me to test against a local grpc server implementation with |
||
|
||
self.grpc_client_config = grpc_config or default_grpc_config | ||
|
||
self.pool_threads = pool_threads | ||
|
||
self._endpoint_override = _endpoint_override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
from typing import Optional | ||
from typing import Optional, Union | ||
from google.protobuf import json_format | ||
from google.protobuf.message import Message | ||
|
||
|
@@ -11,6 +11,7 @@ | |
SparseValues, | ||
QueryResponse, | ||
IndexDescription as DescribeIndexStatsResponse, | ||
UpsertResponse, | ||
NamespaceSummary, | ||
) | ||
from pinecone.db_data.dataclasses import FetchResponse | ||
|
@@ -63,9 +64,28 @@ def parse_usage(usage: dict): | |
return Usage(read_units=int(usage.get("readUnits", 0))) | ||
|
||
|
||
def parse_query_response(response: dict, _check_type: bool = False): | ||
def parse_upsert_response(response: Message, _check_type: bool = False): | ||
json_response = json_format.MessageToDict(response) | ||
upserted_count = json_response.get("upsertedCount", 0) | ||
return UpsertResponse(upserted_count=int(upserted_count)) | ||
|
||
|
||
def parse_update_response(response: Union[dict, Message], _check_type: bool = False): | ||
return {} | ||
|
||
|
||
def parse_delete_response(response: Union[dict, Message], _check_type: bool = False): | ||
return {} | ||
Comment on lines
+73
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two requests don't actually return anything, so there's nothing to do here. |
||
|
||
|
||
def parse_query_response(response: Union[dict, Message], _check_type: bool = False): | ||
if isinstance(response, Message): | ||
json_response = json_format.MessageToDict(response) | ||
else: | ||
json_response = response | ||
|
||
matches = [] | ||
for item in response.get("matches", []): | ||
for item in json_response.get("matches", []): | ||
sc = ScoredVector( | ||
id=item["id"], | ||
score=item.get("score", 0.0), | ||
|
@@ -80,11 +100,11 @@ def parse_query_response(response: dict, _check_type: bool = False): | |
# creating empty `Usage` objects and then passing them into QueryResponse | ||
# when they are not actually present in the response from the server. | ||
args = { | ||
"namespace": response.get("namespace", ""), | ||
"namespace": json_response.get("namespace", ""), | ||
"matches": matches, | ||
"_check_type": _check_type, | ||
} | ||
usage = response.get("usage") | ||
usage = json_response.get("usage") | ||
if usage: | ||
args["usage"] = parse_usage(usage) | ||
return QueryResponse(**args) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
import pytest | ||
import json | ||
import uuid | ||
from ..helpers import get_environment_var, index_tags as index_tags_helper, generate_name | ||
import logging | ||
from pinecone import EmbedModel, CloudProvider, AwsRegion, IndexEmbed | ||
from pinecone.grpc import PineconeGRPC | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
RUN_ID = str(uuid.uuid4()) | ||
|
||
created_indexes = [] | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def index_tags(request): | ||
return index_tags_helper(request, RUN_ID) | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def pc(): | ||
return PineconeGRPC() | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def spec(): | ||
spec_json = get_environment_var( | ||
"SPEC", '{"serverless": {"cloud": "aws", "region": "us-east-1" }}' | ||
) | ||
return json.loads(spec_json) | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def model_idx(pc, index_tags, request): | ||
model_index_name = generate_name(request.node.name, "embed") | ||
if not pc.has_index(name=model_index_name): | ||
logger.info(f"Creating index {model_index_name}") | ||
pc.create_index_for_model( | ||
name=model_index_name, | ||
cloud=CloudProvider.AWS, | ||
region=AwsRegion.US_WEST_2, | ||
embed=IndexEmbed( | ||
model=EmbedModel.Multilingual_E5_Large, | ||
field_map={"text": "my_text_field"}, | ||
metric="cosine", | ||
), | ||
tags=index_tags, | ||
) | ||
created_indexes.append(model_index_name) | ||
else: | ||
logger.info(f"Index {model_index_name} already exists") | ||
|
||
description = pc.describe_index(name=model_index_name) | ||
return pc.Index(host=description.host) | ||
|
||
|
||
def create_index(pc, create_args): | ||
if not pc.has_index(name=create_args["name"]): | ||
logger.info(f"Creating index {create_args['name']}") | ||
pc.create_index(**create_args) | ||
else: | ||
logger.info(f"Index {create_args['name']} already exists") | ||
|
||
host = pc.describe_index(name=create_args["name"]).host | ||
|
||
return host | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def idx(pc, spec, index_tags, request): | ||
index_name = generate_name(request.node.name, "dense") | ||
logger.info(f"Request: {request.node}") | ||
create_args = { | ||
"name": index_name, | ||
"dimension": 2, | ||
"metric": "cosine", | ||
"spec": spec, | ||
"tags": index_tags, | ||
} | ||
host = create_index(pc, create_args) | ||
logger.info(f"Using index {index_name} with host {host} as idx") | ||
created_indexes.append(index_name) | ||
return pc.Index(host=host) | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def sparse_idx(pc, spec, index_tags, request): | ||
index_name = generate_name(request.node.name, "sparse") | ||
create_args = { | ||
"name": index_name, | ||
"metric": "dotproduct", | ||
"spec": spec, | ||
"vector_type": "sparse", | ||
"tags": index_tags, | ||
} | ||
host = create_index(pc, create_args) | ||
created_indexes.append(index_name) | ||
return pc.Index(host=host) | ||
|
||
|
||
def pytest_sessionfinish(session, exitstatus): | ||
for index in created_indexes: | ||
try: | ||
logger.info(f"Deleting index {index}") | ||
pc = PineconeGRPC() | ||
pc.delete_index(name=index, timeout=-1) | ||
except Exception as e: | ||
logger.error(f"Error deleting index {index}: {e}") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
import time | ||
import grpc | ||
import logging | ||
from concurrent import futures | ||
import pinecone.core.grpc.protos.db_data_2025_01_pb2 as pb2 | ||
import pinecone.core.grpc.protos.db_data_2025_01_pb2_grpc as pb2_grpc | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class TestVectorService(pb2_grpc.VectorServiceServicer): | ||
def __init__(self, sleep_seconds=5): | ||
self.sleep_seconds = sleep_seconds | ||
|
||
def Upsert(self, request, context): | ||
# Simulate a delay that will cause a timeout | ||
logger.info("Received an upsert request from test client") | ||
logger.info(f"Request: {request}") | ||
logger.info(f"Sleeping for {self.sleep_seconds} seconds to simulate a slow server call") | ||
time.sleep(self.sleep_seconds) | ||
logger.info(f"Done sleeping for {self.sleep_seconds} seconds") | ||
logger.info("Returning an upsert response from test server") | ||
return pb2.UpsertResponse(upserted_count=1) | ||
|
||
def Query(self, request, context): | ||
# Simulate a delay that will cause a timeout | ||
logger.info("Received a query request from test client") | ||
logger.info(f"Request: {request}") | ||
|
||
logger.info(f"Sleeping for {self.sleep_seconds} seconds to simulate a slow server call") | ||
time.sleep(self.sleep_seconds) | ||
logger.info(f"Done sleeping for {self.sleep_seconds} seconds") | ||
logger.info("Returning a query response from test server") | ||
return pb2.QueryResponse( | ||
results=[], | ||
matches=[pb2.ScoredVector(id="1", score=1.0, values=[1.0, 2.0, 3.0])], | ||
namespace="testnamespace", | ||
usage=pb2.Usage(read_units=1), | ||
) | ||
|
||
def Update(self, request, context): | ||
# Simulate a delay that will cause a timeout | ||
logger.info("Received an update request from test client") | ||
logger.info(f"Request: {request}") | ||
logger.info(f"Sleeping for {self.sleep_seconds} seconds to simulate a slow server call") | ||
time.sleep(self.sleep_seconds) | ||
logger.info(f"Done sleeping for {self.sleep_seconds} seconds") | ||
logger.info("Returning an update response from test server") | ||
return pb2.UpdateResponse() | ||
|
||
def Delete(self, request, context): | ||
# Simulate a delay that will cause a timeout | ||
logger.info("Received a delete request from test client") | ||
logger.info(f"Request: {request}") | ||
logger.info(f"Sleeping for {self.sleep_seconds} seconds to simulate a slow server call") | ||
time.sleep(self.sleep_seconds) | ||
logger.info(f"Done sleeping for {self.sleep_seconds} seconds") | ||
logger.info("Returning a delete response from test server") | ||
return pb2.DeleteResponse() | ||
|
||
def Fetch(self, request, context): | ||
logger.info("Received a fetch request from test client") | ||
logger.info(f"Request: {request}") | ||
logger.info(f"Sleeping for {self.sleep_seconds} seconds to simulate a slow server call") | ||
time.sleep(self.sleep_seconds) | ||
logger.info(f"Done sleeping for {self.sleep_seconds} seconds") | ||
logger.info("Returning a fetch response from test server") | ||
return pb2.FetchResponse( | ||
vectors={ | ||
"1": pb2.Vector(id="1", values=[1.0, 2.0, 3.0]), | ||
"2": pb2.Vector(id="2", values=[4.0, 5.0, 6.0]), | ||
"3": pb2.Vector(id="3", values=[7.0, 8.0, 9.0]), | ||
}, | ||
namespace="testnamespace", | ||
usage=pb2.Usage(read_units=1), | ||
) | ||
|
||
|
||
def create_sleepy_test_server(port=50051, sleep_seconds=5): | ||
"""Creates and returns a configured gRPC server for testing. | ||
|
||
Args: | ||
port (int): The port number to run the server on | ||
sleep_seconds (int): The extra latency in seconds for simulated operations | ||
|
||
Returns: | ||
grpc.Server: A configured and started gRPC server instance | ||
""" | ||
server = grpc.server(futures.ThreadPoolExecutor(max_workers=10)) | ||
pb2_grpc.add_VectorServiceServicer_to_server( | ||
TestVectorService(sleep_seconds=sleep_seconds), server | ||
) | ||
server.add_insecure_port(f"[::]:{port}") | ||
server.start() | ||
return server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little unclear on why we are checking on ssl_verify property for setting default_grpc_config = GRPCClientConfig()
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
config
object here has an optional boolean field calledssl_verify
that can have three values:None
(by default, if unset by the user),True
, orFalse
. The user sets these by passing a value to thePinecone
orPineconeGRPC
constructor like this:pc = PineconeGRPC(ssl_verify=False)
Separately, GRPC configurations can be set at the index level with
GRPCClientConfig
by passingpc.Index(name='my-index', grpc_config=grpc_config)
.GRPCClientConfig
is implemented as aNamedTuple
, an immutable data structure. This object controls the SSL behavior any many other things within the grpc code. For SSL verification, the relevant property in this object is calledsecure
. I discovered in testing that unlikessl_verify
,secure
is a true boolean (True/False only) and if you set None you will get errors.For configuring SSL verify behavior in GRPC, there are three scenarios we need to consider:
config.ssl_verify
will beNone
so we use theGRPCClientConfig()
with no modifications to configure the GRPC channel. This has SSL verification on by default.PineconeGRPC(ssl_verify=False)
but did not specify agrpc_config
object for the index client; in that scenario, we want to thread that setting through even to the index level and override the default grpc behavior for ssl verification.grpc_config
to the index constructor withpc.Index(name='foo', grpc_config=grppc_config)
. In this scenario, we use the configuration the user has provided.The lesson here is that we should endeavor to have only one way to configure things if at all possible. Merging multiple config sources, resolution order, etc gets complicated in a hurry.