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

Updated hello-pinecone-aws example to use pinecone-client==6.0.1 #412

Closed
wants to merge 3 commits into from

Conversation

Joshua-Briggs
Copy link
Collaborator

@Joshua-Briggs Joshua-Briggs commented Feb 25, 2025

Problem

The current notebook hello-pinecone-aws.ipynb uses pinecone-client==3.0.5 which is outdated and therefore some functions needed updating alongside the notebook.

Solution

Updated the notebook to use pinecone-client==6.0.1 and amended functions to use up-to-date parameters, such as:

  • Renaming the index_name parameter to name in the Index method.
  • Adding a spec parameter to the create_index method.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • 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
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc) - Just updating an example notebook
  • None of the above: (explain here)

Test Plan

A quick run through the notebook to check it runs as intended.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@jamescalam jamescalam left a comment

Choose a reason for hiding this comment

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

works for me, tagging @jhamon for PC review

@jhamon you can run in colab https://colab.research.google.com/github/Joshua-Briggs/examples/blob/master/learn/search/semantic-search/hello-pinecone-aws.ipynb -- just reqs an api key near the start

@jhamon
Copy link
Collaborator

jhamon commented Mar 3, 2025

Overall this seems fine to merge. The test run is failing, I think, because the PINECONE_API_KEY secret can't be read because you're external. Running changed notebooks in CI is somewhat experimental anyway and something I'm still experimenting with.

Besides the updates you've made, I'm trying to make the following changes whenever I see an opportunity:

  • Use if pc.has_index(name=index_name): instead of if index_name in pc.list_indexes().names(): blah blah when checking to see if an index exists before creating it.
  • Eliminate post-index wait for ready code like this, which is not needed with serverless indexes. It goes back to a time when pod indexes would falsely claim to be ready when they were not actually ready to receive calls, but that's not relevant for serverless indexes, so we can eliminate these status checks. The SDK already waits until the index is ready unless you specifically tell it not to by passing timeout=-1 to the create_index call:
while not pc.describe_index(index_name).status['ready']:
    time.sleep(1)
  • Consolidate the spec= definition into an inline part of the create_index call and just give one explanation about the arguments to create_index. Also drop any mention of PINECONE_CLOUD and PINECONE_REGION which have never been part of the actual SDK as far as I know and don't seem particularly relevant. See this notebook I recently updated for an example. A lot of these notebooks seem to have gained cells that separately explained spec around the time serverless was introduced, but the flow feels off to me; first of all because "where do I deploy this index i'm creating" is not a difficult concept, and second because those environment variables mentioned above are irrelevant, and third because the spec seems to often get defined a few blocks before it is actually used. Which makes the create_index code a little harder to understand when viewed on its own.
  • Use keyword arguments everywhere, to minimize the impact of future changes to arguments that can happen when something that is currently required becomes optional. So, for instance, I want to use keyword args to identify every name= param that used to just be passed as a positional argument.
  • [Optional] Use enum types where available, e.g. to get a value for AwsRegion.US_EAST_1 instead of "us-east-1".

@jhamon jhamon self-requested a review March 3, 2025 21:09
@jamescalam
Copy link
Collaborator

we'll make these changes @jhamon

@Joshua-Briggs Joshua-Briggs closed this by deleting the head repository Mar 4, 2025
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.

3 participants