[Refactor] Extract GrpcRunner from GRPCIndexBase class #395
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhamon
commented
Oct 15, 2024
|
|
||
| return wrapped() | ||
|
|
||
| async def run_asyncio( |
There was a problem hiding this comment.
Not using or testing run_asyncio anywhere yet, but I will. It's the same as run but with the addition of the async/await bits.
jhamon
changed the title
jhamon
changed the title
jhamon
requested review from
austin-denoble,
haruska,
rohanshah18 and
aulorbe
austin-denoble
approved these changes
Oct 15, 2024
| from pinecone.utils.constants import REQUEST_ID, CLIENT_VERSION | ||
| from pinecone.exceptions.exceptions import PineconeException | ||
|
|
||
| _logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
This wasn't being used, right?
There was a problem hiding this comment.
Currently unused. The _logger reference is now used in the GrpcChannelFactory which is logic I recently pulled out of this base class. Cleaning up this reference should have happened in that PR, but got overlooked.
| from google.protobuf.message import Message | ||
|
|
||
|
|
||
| class GrpcRunner: |
There was a problem hiding this comment.
Good idea pulling all of this out into somewhere contained. 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
This is another extractive refactoring in preparation for grpc with asyncio.
Solution
The generated stub class,
VectorServiceStub, is what knows how to call the Pinecone grpc service, but our wrapper code needs to do some work to make sure we have a consistent approach to "metadata" (grpc-speak for request headers) and handling other request params liketimeout. Previously this work was accomplished in a private method of theGRPCIndexBasebase class called_wrap_grpc_call().Since we will need to perform almost identical marshaling of metadata for requests with asyncio, I pulled this logic out into a separate class
GrpcRunnerand renamed_wrap_grpc_calltorun. You can see there is also a parallel method implementation calledrun_asyncio; currently this is unused and untested, but kind of illustrates why this refactor is useful.Type of Change
Test Plan
Tests should still be green