Config / init refactor for state encapsulation #220

jhamon · 2023-10-17T18:03:41Z

Problem

Singleton config limits how people can use this SDK and makes testing more difficult. Also, we no longer should fetch projectId and construct data url out of configuration parts, to keep the client agnostic to the format of the data plane url.

Solution

State encapsulation using classes
- We want to encapsulate all config-related state into a new class Config and a subclass PineconeConfig.
- Config is a vessel for api_key, host, and openapi configuration. It is host-agnostic because it is used by both the control plane (Pinecone) and the data plane (Index)
- PineconeConfig is a convenience class to set the controller host for Pinecone.
- New singleton store IndexHostStore means we won't make unnecessary and repetitious calls to describe_index if a user creates multiple Index instances referring to the same index. In the old paradigm, this was accomplished with a piece of global state storing off projectId.
Modified class init for Index
- Index class constructor now requires host url instead of an index name. If you use the helper method on a Pinecone class instance to instantiate the object, it will manage fetching the host url for you. E.g. p = Pinecone(api_key='foo'); index = p.Index('my-index').

# Old way
import pinecone
pinecone.init(api_key='foo')
index = pinecone.Index('my-index')

# New way
from pinecone import Pinecone
client = Pinecone(api_key='foo')
index = client.Index('my-index')

# New way (alternate, for data-plane only use case)
from pinecone import Index
index = Index(api_key='foo', host='https://data-url')

Moving things around
- manage.py got moved and renamed to pinecone/control/pinecone.py. The functions such as create_index, describe_index, etc that previously were top-level package exports relying on global state to access config information have been turned into class methods on a new Pinecone class defined in this file.
- pinecone/index.py got moved to pinecone/data/index.py. The pinecone/data package is a home for all classes used to access the data plane.
- Everything grpc related got shoved into pinecone/data/grpc.
- GRPC mess in one place only. Utils that were grpc-related got pulled out of pinecone/utils.py and moved to pinecone/data/grpc/utils.py.
- Leftover util junk moved into a deprecated package. Probably will be deleted soon.
Breaking down big files into small files
- The file previously at pinecone/index_grpc.py to split up into many smaller files inside pinecone/data/grpc/ package.
- Remaining util functions in pinecone/utils.py got broken up into many smaller files in pinecone/utils/
Fix existing tests
- Needed to adjust a lot of import statements and sometimes setup functions to get pre-existing tests running.

Using it

# We're going to use a new Pinecone class instead of interacting directly with the package 
# namespace as in the past
from pinecone import Pinecone

# No more init() method
p = Pinecone(api_key='foo')

# Target an index
index = p.Index('my-index')

# Use data plane methods
index.describe_index_stats()

Mostly working

describe_index (call succeeds but need to follow up on source_collection field, would ideally return different object for different capacity_modes)

Somewhat working:

create_index (API call succeeds, but client blows up when response is not the expected shape)
delete_index (only when timeout=-1, since otherwise it has dependency on list_indexes)

Broken:

list_indexes (response not expected shape)

Type of Change

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Test Plan

Automated tests will be expanded in a future PR. A bunch of code moved around to different files or got split from big files into small files and I want to merge this part of the change to unlock parallel work by Austin various follow-ups.

jhamon · 2023-10-19T17:39:38Z

pinecone/__init__.py

-QueryResult = None
-"""@private"""
-FetchResult = None
-"""@private"""


I don't have context on these, but I can't imagine any scenario where returning a bunch of None objects meaningfully preserves backwards compatibility.

Agreed, I'm not really sure what this was for. I think it's probably fine to pull them out. Otherwise it would just be someone defining behavior themselves for these things anyways.

jhamon · 2023-10-19T17:46:41Z

pinecone/__init__.py

 try:
-    from .grpc.index_grpc import *
+    from .data.grpc.index_grpc import *
 except ImportError:
    pass  # ignore for non-[grpc] installations


These try import statements led to some pretty confusing debug situations when stuff started failing due to me moving packages from one location to another since the intention is really only to ignore failures caused by people installing without grpc extras. We should revisit this pattern and make sure we're testing stuff both with and without grpc deps loaded.

jhamon · 2023-10-19T17:49:47Z

pinecone/config/logging.py

Moved these constants out of old utils.py since they were only being used in one place.

The rest of this I pulled out of We need a whole ticket to revisit and overhaul the logging approach in a holistic way.

jhamon · 2023-10-19T17:50:55Z

pinecone/config/openapi.py

Everything in here was pretty much copy/pasta out of the old config.py but wrapped inside a new factory class. I haven't done anything to verify these settings for openapi, but they worked in the past so hopefully they are sufficient for now.

jhamon · 2023-10-19T17:53:03Z

pinecone/config/pinecone_config.py

+
+class PineconeConfig(Config):
+    def __init__(self, api_key: str = None, host: str = None, **kwargs):
+        host = host or kwargs.get("host") or os.getenv("PINECONE_CONTROLLER_HOST") or DEFAULT_CONTROLLER_HOST


I don't know if we want to keep this or not, but in the past there was an env var called PINECONE_CONTROLLER_HOST being used.

But as you can see, the only difference between a Config instance and the PineconeConfig subclass is a little bit of logic here about how to set the host value.

I'm also not sure whether the env var should be maintained, but I suppose it's a nice convenience and not too much trouble to keep around.

jhamon · 2023-10-19T17:53:52Z

pinecone/control/index_host_store.py

+class SingletonMeta(type):
+    _instances = {}
+
+    def __call__(cls, *args, **kwargs):
+        if cls not in cls._instances:
+            instance = super().__call__(*args, **kwargs)
+            cls._instances[cls] = instance
+        return cls._instances[cls]
+
+
+class IndexHostStore(metaclass=SingletonMeta):


I confess, I had help from ChatGPT on this. I still need to add tests to verify the singleton is working as expected.

I'm pretty sure I was using GPT for examples of Python singletons and this is the approach it gave me as well.

jhamon · 2023-10-19T17:54:28Z

pinecone/control/models/collection_description.py

@@ -0,0 +1,7 @@
+class CollectionDescription(object):


Pulled this out of legacy manage.py. Not sure if we really want to keep it or not long term.

I was unsure of this also. I also wasn't sure why it's doing an anonymous mapping to a dict rather than having a defined set of keys like in IndexDescription(NamedTuple).

Would these classes ultimately be useful to external users?

Whether it's a dictionary or an extention of NamedTuple, the usual motivation for a presenter object like is just to have greater control over what is shown to the user and discard any unwanted fields that may be emitted by the underlying generated code. I don't see why that's needed in this case though. Probably I will delete it soon but not in this diff.

jhamon · 2023-10-19T17:54:33Z

pinecone/control/models/index_description.py

@@ -0,0 +1,13 @@
+from typing import NamedTuple


Pulled this out of legacy manage.py. Not sure if we really want to keep it or not long term.

jhamon · 2023-10-19T17:55:17Z

pinecone/control/pinecone.py

Most of the logic in this file moved from manage.py

jhamon · 2023-10-19T17:59:42Z

pinecone/control/pinecone.py

+        db = response.database
+        host = response.status.host
+
+        self.index_host_store.set_host(self.config, name, "https://" + host)


We proactively stick this host url in the singleton store if they happen to call describe_index before doing any index operations. Just to save an additional round trip.

jhamon · 2023-10-19T18:13:31Z

pinecone/core/client/api/index_operations_api.py

@@ -134,7 +134,7 @@ def configure_index_with_http_info(
        :rtype: tuple(IndexMeta, status_code(int), headers(HTTPHeaderDict))
        """

-        _hosts = ["https://controller.{environment}.pinecone.io"]
+        _hosts = ["https://api.pinecone.io"]


Changes in this file are generated, derived from changes to the servers field in our openapi spec stored in the other repository.

jhamon · 2023-10-19T18:14:05Z

pinecone/core/client/models/index_meta_database.py

@@ -30,7 +30,7 @@ class IndexMetaDatabase(BaseModel):
    """

    name: StrictStr = Field(...)
-    dimension: StrictStr = Field(...)
+    dimension: StrictInt = Field(...)


This is also a generated change, based on an updated openapi spec.

jhamon · 2023-10-19T18:14:52Z

pinecone/data/grpc/base.py

@@ -0,0 +1,192 @@
+import logging


Moved these out of the giant index.grpc.py file. No changes.

jhamon · 2023-10-19T18:16:50Z

pinecone/data/grpc/config.py

@@ -0,0 +1,33 @@
+from .retry import RetryConfig


Moved these out of the giant index.grpc.py file. No changes.

jhamon · 2023-10-19T18:16:57Z

pinecone/data/grpc/future.py

@@ -0,0 +1,34 @@
+from grpc._channel import _MultiThreadedRendezvous


Moved these out of the giant index.grpc.py file. No changes.

jhamon · 2023-10-19T18:18:23Z

pinecone/data/grpc/index_grpc.py

-from functools import wraps
-from importlib.util import find_spec
-from typing import NamedTuple, Optional, Dict, Iterable, Union, List, Tuple, Any
+from typing import Optional, Dict, Iterable, Union, List, Tuple, Any


Extracted a bunch of supporting classes and functions into smaller files. Removed dependencies no longer in use by the class defined in this file. Otherwise, no changes.

jhamon · 2023-10-19T18:21:27Z

pinecone/data/grpc/utils.py

@@ -0,0 +1,107 @@
+import uuid


Moved these grpc-related utils out of a general utils.py file so there wouldn't be any try/except import statements needed in here, and also so they are closer to where they are actually being used in the code.

jhamon · 2023-10-19T18:40:42Z

tests/unit/test_manage.py

This file got renamed and modified. Now it is tests/unit/test_control.py

jhamon · 2023-10-19T18:41:30Z

pinecone/utils/user_agent.py

@@ -0,0 +1,10 @@
+import requests


Moved from pinecone/utils.py

jhamon · 2023-10-19T18:41:35Z

pinecone/utils/fix_tuple_length.py

Moved from pinecone/utils.py

jhamon · 2023-10-19T18:41:54Z

pinecone/utils/error_handling.py

Moved from pinecone/utils.py

jhamon · 2023-10-19T18:42:01Z

pinecone/utils/environment.py

Moved from pinecone/utils.py

jhamon · 2023-10-19T18:42:08Z

pinecone/utils/deprecation_notice.py

Moved from pinecone/utils.py

jhamon · 2023-10-19T18:42:45Z

pinecone/utils/constants.py

Moved some constants closer to where they are actually being used, since they were only used in one spot.

jhamon · 2023-10-19T18:42:59Z

pinecone/utils/check_kwargs.py

Moved from pinecone/utils.py

jhamon · 2023-10-19T18:44:18Z

pinecone/deprecated/legacy_utils.py

@@ -0,0 +1,88 @@
+import re


Leftovers, hopefully to be deleted soon pending investigation on vector_column_service.proto

jhamon · 2023-10-19T18:48:08Z

pinecone/data/index.py

+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self._api_client.close()



These methods were the only thing I needed to add back as a consequence changing Index to not subclass ApiClient to satisfy one test (using with Index(...) as index syntax). The python language docs show tthese must be implemented in order to use the with syntax.

Thanks, that's very helpful.

jhamon · 2023-10-19T18:48:51Z

pinecone/data/index.py

+    def __init__(self, api_key: str, host: str, pool_threads=1, **kwargs):
+        api_key = api_key or kwargs.get("api_key")
+        host = host or kwargs.get('host')
+        pool_threads = pool_threads or kwargs.get("pool_threads")
+
+        self._config = Config(api_key=api_key, host=host, **kwargs)
+
+        api_client = ApiClient(configuration=self._config.OPENAPI_CONFIG, pool_threads=pool_threads)
+        api_client.user_agent = get_user_agent()
+        self._api_client = api_client


This is a substantive change to adopt the new Config class.

austin-denoble

Thank you so much for taking this on! Organizationally and architecturally this feels much, much better to me. I think the refactor of the singleton approach and the ability for users to be more flexible when using the client is a big win.

Overall this looks good to go I think. I'd like to do some detailed testing around the changes here but will not have time this evening.

Definitely would like to get this merged to the feature branch and start iterating on it.

austin-denoble · 2023-10-19T18:52:26Z

pinecone/control/models/collection_description.py

@@ -0,0 +1,7 @@
+class CollectionDescription(object):


I was unsure of this also. I also wasn't sure why it's doing an anonymous mapping to a dict rather than having a defined set of keys like in IndexDescription(NamedTuple).

Would these classes ultimately be useful to external users?

austin-denoble · 2023-10-19T19:54:46Z

pinecone/__init__.py

-QueryResult = None
-"""@private"""
-FetchResult = None
-"""@private"""


Agreed, I'm not really sure what this was for. I think it's probably fine to pull them out. Otherwise it would just be someone defining behavior themselves for these things anyways.

austin-denoble · 2023-10-19T20:03:21Z

pinecone/config/config.py

+            openapi_config
+            or kwargs.pop("openapi_config", None)


Is it possible that openapi_config would be passed in via method arg + also be in kwargs with the same name? I guess this question is also for other areas where we do something similar, like in pinecone/data/index.py in the __init__ and I'm just somewhat unclear on kwargs.

austin-denoble · 2023-10-19T20:11:38Z

pinecone/control/index_host_store.py

+class SingletonMeta(type):
+    _instances = {}
+
+    def __call__(cls, *args, **kwargs):
+        if cls not in cls._instances:
+            instance = super().__call__(*args, **kwargs)
+            cls._instances[cls] = instance
+        return cls._instances[cls]
+
+
+class IndexHostStore(metaclass=SingletonMeta):


I'm pretty sure I was using GPT for examples of Python singletons and this is the approach it gave me as well.

austin-denoble · 2023-10-19T20:17:49Z

pinecone/control/index_host_store.py

+        self._indexHosts = {}
+
+    def _key(self, config: Config, index_name: str) -> str:
+        return ":".join([config.API_KEY, index_name])


hyper-nit: Using a delimiter + join seems fine here, I suppose you could also use a template string for possibly easier readability, but obviously just imo:

Suggested change

return ":".join([config.API_KEY, index_name])

return f"{config.API_KEY}:{index_name}"

austin-denoble · 2023-10-19T20:38:47Z

pinecone/data/index.py

 import copy

 __all__ = [
    "Index",
    "FetchResponse",
-    "ProtobufAny",


Guessing this was removed as it doesn't seem necessarily useful being exposed for end-users, but lemme know if otherwise.

austin-denoble · 2023-10-19T20:41:13Z

pinecone/data/index.py

+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self._api_client.close()



Thanks, that's very helpful.

austin-denoble · 2023-10-19T20:53:01Z

pinecone/config/pinecone_config.py

+
+class PineconeConfig(Config):
+    def __init__(self, api_key: str = None, host: str = None, **kwargs):
+        host = host or kwargs.get("host") or os.getenv("PINECONE_CONTROLLER_HOST") or DEFAULT_CONTROLLER_HOST


I'm also not sure whether the env var should be maintained, but I suppose it's a nice convenience and not too much trouble to keep around.

austin-denoble · 2023-10-19T21:12:15Z

pinecone/utils/error_handling.py

 def validate_and_convert_errors(func):
    @wraps(func)
    def inner_func(*args, **kwargs):
-        Config.validate()  # raises exceptions in case of invalid config


This happens within the init of the Config itself, right? Not really necessary to have it here.

austin-denoble · 2023-10-19T21:23:21Z

tests/unit/test_config.py

-    assert Config.CONTROLLER_HOST == controller_host
-
-
-def test_resolution_order_kwargs_over_config_file():


It looks like we lost the tests around resolution order over a config file, but I'd assume that's fine given we'll need to add new testing around the config singleton and stuff anyways, and the config file setup isn't really documented.

jhamon added 7 commits October 17, 2023 10:44

WIP

7dda40c

Update controller host

c5c41d4

Dimension is number

b645c22

Fix subscriptable error

8b5acb0

Split grpc-related files into multiple directories

16c6432

Create data package, move grpc under it, utils reorg

18cf313

Target index by host url

aaf71fc

jhamon requested a review from austin-denoble October 19, 2023 17:28

Cleanup

38dfb14

jhamon changed the title ~~[WIP] Config / init refactor~~ Config / init refactor for state encapsulation Oct 19, 2023