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

Config / init refactor for state encapsulation #220

Merged
merged 10 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 3 additions & 15 deletions pinecone/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,10 @@
"""
from .config import *
from .exceptions import *
from .manage import *
from .index import *
from .control.pinecone import Pinecone
from .data.index import *

try:
from .grpc.index_grpc import *
from .data.grpc.index_grpc import *
except ImportError:
pass # ignore for non-[grpc] installations
Comment on lines 9 to 12
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.


# Kept for backwards-compatibility
UpsertResult = None
"""@private"""
DeleteResult = None
"""@private"""
QueryResult = None
"""@private"""
FetchResult = None
"""@private"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

InfoResult = None
"""@private"""
260 changes: 0 additions & 260 deletions pinecone/config.py

This file was deleted.

2 changes: 2 additions & 0 deletions pinecone/config/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
from .config import Config
from .pinecone_config import PineconeConfig
69 changes: 69 additions & 0 deletions pinecone/config/config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
from typing import NamedTuple
import os

from pinecone.utils import check_kwargs
from pinecone.exceptions import PineconeConfigurationError
from pinecone.core.client.exceptions import ApiKeyError
from pinecone.config.openapi import OpenApiConfigFactory
from pinecone.core.client.configuration import Configuration as OpenApiConfiguration


class ConfigBase(NamedTuple):
api_key: str = ""
host: str = ""
openapi_config: OpenApiConfiguration = None


class Config:
"""

Configurations are resolved in the following order:

- configs passed as keyword parameters
- configs specified in environment variables
- default values (if applicable)
"""

"""Initializes the Pinecone client.

:param api_key: Required if not set in config file or by environment variable ``PINECONE_API_KEY``.
:param host: Optional. Controller host.
:param openapi_config: Optional. Set OpenAPI client configuration.
"""

def __init__(
self,
api_key: str = None,
host: str = None,
openapi_config: OpenApiConfiguration = None,
**kwargs,
):
api_key = api_key or kwargs.pop("api_key", None) or os.getenv("PINECONE_API_KEY")
host = host or kwargs.pop("host", None)
openapi_config = (
openapi_config
or kwargs.pop("openapi_config", None)
Comment on lines +44 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

or OpenApiConfigFactory.build(api_key=api_key, host=host)
)

check_kwargs(self.__init__, kwargs)
self._config = ConfigBase(api_key, host, openapi_config)
self.validate()

def validate(self):
if not self._config.api_key:
raise PineconeConfigurationError("You haven't specified an Api-Key.")
if not self._config.host:
raise PineconeConfigurationError("You haven't specified a host.")

@property
def API_KEY(self):
return self._config.api_key

@property
def HOST(self):
return self._config.host

@property
def OPENAPI_CONFIG(self):
return self._config.openapi_config
8 changes: 8 additions & 0 deletions pinecone/config/logging.py
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import logging

PARENT_LOGGER_NAME = "pinecone"
DEFAULT_PARENT_LOGGER_LEVEL = "ERROR"

_logger = logging.getLogger(__name__)
_parent_logger = logging.getLogger(PARENT_LOGGER_NAME)
_parent_logger.setLevel(DEFAULT_PARENT_LOGGER_LEVEL)
Loading