-
Notifications
You must be signed in to change notification settings - Fork 94
Add describe, delete, and list namespaces (REST) #507
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
base: main
Are you sure you want to change the base?
Conversation
…t idx.<namespace_op>
…y and not idx.<namespace_op>" This reverts commit 6a011e8.
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.
LGTM overall, left some comments and a few questions. Nice work, I'll defer to @jhamon for now on a lot of this as I'm still getting familiar with the client refactor.
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.
A couple things:
Require kwargs for new methods
I would add the @require_kwargs
decorator on all new methods. Search the repo for other examples of this. We haven't added it in 100% of places because it would be a breaking change to add it to methods that never had it before if people are successfully calling methods using positional arguments, but we want to use this on all new methods going forward.
Why? Well, python is flexible in how methods are called. If you had a function like this:
def mymethod(foo):
print(foo)
then you could call the function with positional arguments like mymethod('bar')
or kwarg-style as mymethod(foo='bar')
. Relying on positional arguments can cause problems down the road when new arguments are added, the args list gets long, if arguments transition from required to optional, etc. If everyone uses the keyword form from the beginning, a lot of these changes can be made without disrupting existing code.
Testing docs change
Unfortunately testing these docs changes is slightly involved because there's a problem preventing us from directly having sphinx as a dev-dependency. If you want to try building the docs, you need to follow the steps in this github action but do not commit any changes to the python version or poetry.lock when you are done.
Basically, you need to
poetry env use 3.11
to make sure you're using python 3.11- (Temporarily, do not commit) edit the pyproject.toml to require python
^3.11
poetry install -E asyncio -E grpc
- Install docs deps
poetry add sphinx myst-parser --group dev
- Build the docs
poetry run sphinx-build -b html docs docsbuild
- Inspect the output
open docsbuild/index.html
You will need to edit docs/rest.rst
and docs/asyncio.rst
to explicitly expose docs for your new methods, and then you'll want to look at them to see whether the generated text looks correct from a formatting standpoint.
args = NamespaceRequestFactory.delete_namespace_args(namespace=namespace) | ||
return self.__namespace_operations_api.delete_namespace(**args) | ||
|
||
def list(self, **kwargs) -> Iterator[ListNamespacesResponse]: |
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 would adjust the method signature to spell out the parameters you described in the docstrings. This will better support code completion and type hints in peoples code editors because it gives a better idea of what arguments are expected.
def list(self, **kwargs) -> Iterator[ListNamespacesResponse]: | |
def list(self, limit: Optional[int] = None, pagination_token: Optional[str] = None, **kwargs) -> Iterator[ListNamespacesResponse]: |
To use Optional
type hint, import it like from typing import Optional
. If these aren't passed, they will get a default value of None
(basically the same as null in python)
As for **kwargs
, we use **kwargs to gather up all other keyword arguments passed into a dictionary. For example list(limit=10, foo='bar')
would put 'foo'
into the kwargs
dictionary but not limit
. We typically only rely on this for secret / undocumented behaviors, or when we need some way to pass arguments through to somewhere else without creating a tight coupling to the list() signature.
A common example of how this might be used, in theory anyway, is to pass kwargs to the underlying openapi API method. But we don't want openapi junk leaking out into public view and polluting our method interface, so we don't want to declare all those properties on every method we implement.
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 have followed the same pattern as list vectors and list imports, where we only pass all arguments as optional in the list_paginated method.
list: https://github.com/pinecone-io/pinecone-python-client/blob/main/pinecone/db_data/index.py#L472
list imports: https://github.com/pinecone-io/pinecone-python-client/blob/main/pinecone/db_data/index.py#L516
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 left a ton of comments (sorry lol), but overall I think you did a nice job.
for m in [ListNamespacesResponse, NamespaceDescription]: | ||
install_json_repr_override(m) |
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 would do some manual testing with poetry run repl
to make sure the output looks good for these methods. The goal of these overrides is to massage the output for notebook-style interactive use, but since it's relying on jsonifying other objects we need to make sure that's working well each time we add something new since the json dump may not understand all object types that exist.
Problem
Add support for describe, delete, and and list namespaces for index and async index classes.
Solution
Following methods were added to the index and async index classes:
- list_namespaces(self, **kwargs) -> Iterator[ListNamespacesResponse] (for index.py)
- list_namespaces(self, **kwargs) -> AsyncIterator[ListNamespacesResponse] (for index_asyncio.py)
self, limit: Optional[int] = None, pagination_token: Optional[str] = None
) -> ListNamespacesResponse
Type of Change
Test Plan
Added integration tests for both index and async index classes