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

Move all grpc stuff into pinecone.grpc subpackage #238

Merged
merged 8 commits into from
Nov 6, 2023

Conversation

jhamon
Copy link
Collaborator

@jhamon jhamon commented Nov 4, 2023

Problem

In the past this package attempted to import extra grpc deps in the root __init__.py inside a try/catch that caught ImportError when deps were missing. This is error prone because people may be attempting to use the pinecone module alongside other packages that depend on incompatible versions of these dependencies, so we shouldn't interpret successful import as a sign that a user has installed the pinecone-client[grpc] extras (with the versions we expect). When these incompatible versions are present, the ImportError doesn't happen but other runtime errors can occur even when the person is not attempting to use GRPCIndex.

Solution

The gist of this PR is to better isolate everything GRPC-related into a subpackage to minimize the changes of a grpc-related dependency clash getting in the way of using Pinecone. By doing this, issues with grpc dependencies should never impact people using the non-grpc client.

Summary of changes:

  • Moving files
    • Contents of pinecone/data/grpc moved up and over to pinecone/grpc
    • Tests moved from tests/unit/test_index_grpc.py into tests/unit_grpc/test_index_grpc.py. This is to make it easier to split grpc and non-grpc test runs.
    • Delete remaining cruft in pinecone/deprecated/legacy_utils.py because they were unused and loading grpc deps.
  • Making from pinecone import Pinecone independent of GRPC
    • References to all GRPC deps removed from pinecone/__init__.py
    • Adjusting pinecone/pinecone.py to remove IndexGRPC() method.
  • Setting up for from pinecone.grpc import Pinecone
    • Add pinecone/grpc/pinecone.yaml that extends the other Pinecone class, modifying only the Index() method to use GRPCIndex.
    • Add pinecone/grpc/__init__.py with entries for grpc-flavored Pinecone and IndexGRPC exports.
  • CI updates
    • pyproject.toml adjusted to properly indicate grpc deps as optional = true; I discovered in testing that grpc dependencies were always being installed in CI.
    • Add an input var to the setup-poetry action so that we can toggle on grpc dependencies when needed.
    • .github/workflows/testing.yaml refactored to break GRPC and non-GRPC into separate jobs, since they have different install requirements.

Usage evolution

Legacy implementation
import pinecone 
pinecone.init(api_key='foo', environment='bar')

index = pinecone.Index('index-name')
index_grpc = pinecone.GRPCIndex('index-name')
Unreleased spruce branch (before this PR)

This was an improvement over the legacy approach because we no longer relied on global state for configuration. Instead, we wrap configuration state into a class instance. However, the Pinecone class had a dependency on GRPC stuff to implement the GRPCIndex() method. The imports of these extra deps was intended to noop when they were not installed, but as I described above there are situations where you could still have runtime errors if other dependencies in your notebook / app have installed incompatible GRPC dependencies that prevent you from using the REST version of the client.

from pinecone import Pinecone

p = Pinecone(api_key='foo')
index = p.Index('index-name')
index_grpc = p.GRPCIndex('index-name')
After this PR, import from different subpackage when GRPC desired

Now the usage is exactly the same for REST vs GRPC except for the import coming from pinecone.grpc instead of pinecone when you want data operations to use grpc. GRPC deps are not loaded at all unless you specifically import from pinecone.grpc.

To use REST, import from pinecone

from pinecone import Pinecone

p = Pinecone(api_key='foo')
index = p.Index('index-name')

To use GRPC, import from pinecone.grpc subpackage

from pinecone.grpc import Pinecone

p = Pinecone(api_key='foo')
index = p.Index('index-name')

If you ever wanted to use these side by side for some reason (no real reason to ever do this unless you are writing tests for this package) you would have to alias one of the imports to avoid a name collision.

from pinecone import Pinecone
from pinecone.grpc import Pinecone as PineconeGRPC

...

Type of Change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)

Test Plan

Describe specific steps for validating this change.

@jhamon jhamon changed the title [WIP] Move all grpc stuff into pinecone.grpc subpackage Move all grpc stuff into pinecone.grpc subpackage Nov 4, 2023
@jhamon jhamon marked this pull request as ready for review November 4, 2023 06:03
Copy link
Contributor

@austin-denoble austin-denoble left a comment

Choose a reason for hiding this comment

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

Nice work, all of this makes a lot of sense. Thanks for taking this through the finish line after helping out with all the dependency problems that cropped up due to this setup. 🚢

Comment on lines -31 to -32
- name: Build docs
run: make docs
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pulling this out of here, this was a mistake on my part I think.

We cover this step in the pr workflow already so it may have been running twice.

Comment on lines +66 to +70
grpcio = { version = ">=1.44.0", optional = true }
grpc-gateway-protoc-gen-openapiv2 = { version = "0.1.0", optional = true }
googleapis-common-protos = { version = ">=1.53.0", optional = true }
lz4 = { version = ">=3.1.3", optional = true }
protobuf = { version = "~=3.20.0", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Did not know you could explicitly set optional like this. 👍

@jhamon jhamon merged commit e73726d into spruce Nov 6, 2023
@jhamon jhamon deleted the jhamon/grpc-refactor branch November 6, 2023 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants