feat: Update pinecone client #756
Conversation
|
|
github-actions
bot
added
integration:pinecone
type:documentation
mmurraysans
force-pushed
the
main
branch
2 times, most recently
from
4fca210
to
5b99d3e
Compare
There was a problem hiding this comment.
Hey, @mmurraysans. Thanks for the contribution!
I found some opportunities for improvement.
Also, the tests that require an API key are skipped in community branches.
Feel free to test the changes locally and ping me when you are done (I will do another check).
| # "pinecone-client<3", # our implementation is not compatible with pinecone-client>=3 | ||
| "pinecone-client>3", # our implementation is not compatible with pinecone-client>=3 |
There was a problem hiding this comment.
| # "pinecone-client<3", # our implementation is not compatible with pinecone-client>=3 | |
| "pinecone-client>3", # our implementation is not compatible with pinecone-client>=3 | |
| "pinecone-client>=3", |
| ban-relative-imports = "parents" | ||
|
|
||
| [tool.ruff.per-file-ignores] | ||
| [tool.ruff.lint.per-file-ignores] |
There was a problem hiding this comment.
could you discard the changes in this file?
| :param index_creation_kwargs: Additional keyword arguments to pass to the index creation method. | ||
| You can find the full list of supported arguments in the | ||
| [API reference](https://docs.pinecone.io/reference/create_index). | ||
| In addition the the documented arguments an indexType key specifies the type of index to create. |
There was a problem hiding this comment.
Consistently with the new API, I would:
- remove
environment - add a new parameter called
spec: a dictionary with the same structure shown in the Pinecone API reference.
Two examples of spec:
{
"pod": {
"environment": "us-west-1-gcp",
"pod_type": "p1.x1",
"pods": 1
}
}
{
"serverless": {
"region": "us-east-1",
"cloud": "aws"
}
}
| spec = index_creation_kwargs.pop("spec", None) | ||
| if spec: | ||
| index_spec = ( | ||
| ServerlessSpec(**spec) | ||
| if index_creation_kwargs.pop("indexType", "").lower() == "serverless" | ||
| else PodSpec(**spec) | ||
| ) | ||
| pc.create_index(name=index, dimension=dimension, spec=index_spec, **index_creation_kwargs) | ||
| else: | ||
| pc.create_index(name=index, dimension=dimension, **index_creation_kwargs) |
There was a problem hiding this comment.
Let's change this part according to the comment above (introduction of the spec parameter).
| @pytest.fixture(autouse=True) | ||
| def patch_api_key(monkeypatch): | ||
| monkeypatch.setenv("PINECONE_API_KEY", "env-api-key") |
There was a problem hiding this comment.
I think this change should be discarded.
The API key is already set as a secret in this repository.
| assert document_store.environment == "gcp-starter" | ||
| assert document_store.index == "my_index" | ||
| assert document_store.namespace == "test" | ||
| assert document_store.batch_size == 50 | ||
| assert document_store.dimension == 30 | ||
| assert document_store.index_creation_kwargs == {"metric": "euclidean"} |
There was a problem hiding this comment.
please reintroduce the deleted assertions
masci
changed the title
|
I'm closing this because inactive. |
Related Issues
Pinecone-python-clientv3.0.0 breakspinecone-haystack#223source_tagattribute to PineconeDocumentStore #715Proposed Changes:
This PR requires pinecone_client > 3 and does away with the
pinecone.initmethod in favor of the new class based methods. I aimed to minimize the changes to the codebase as much as possible.How did you test it?
Notes for the reviewer
The new client introduced a
PodSpecand aServerlessPodSpecso I had to add some additional checks for theindex_creation_kwargsargument that is worth a look to make sure this makes sense to everyone.Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.