-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: update rag chatbot notebook #434
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
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
jhamon
left a comment
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 few details need adjusting, but overall this is a very nice update. Thanks!
| "id": "CK13RD1jZrwL" | ||
| }, | ||
| "source": [ | ||
| "Then we initialize the index. We will be using OpenAI's `text-embedding-ada-002` model for creating the embeddings, so we set the `dimension` to `1536`." |
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.
You need to update this text to refer to ext-embedding-3-small
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.
added
| "id": "AulXyZWNZrwM" | ||
| }, | ||
| "source": [ | ||
| "Our index is now ready but it's empty. It is a vector index, so it needs vectors. As mentioned, to create these vector embeddings we will OpenAI's `text-embedding-ada-002` model — we can access it via LangChain like so:" |
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.
Model name needs updating here as well
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.
added
| " # if does not exist, create index\n", | ||
| " pc.create_index(\n", | ||
| " index_name,\n", | ||
| " dimension=1536, # dimensionality of embed 3 small\n", |
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 use the actual model name in this comment
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.
updated
| " spec=spec\n", | ||
| " )\n", | ||
| " # wait for index to be initialized\n", | ||
| " while not pc.describe_index(index_name).status['ready']:\n", |
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.
Sleeps are not necessary. This whole cell needs to be updated along the lines of the modernization guide https://www.notion.so/Python-docs-examples-modernization-guide-1ae2b1c5593b80eaaa2bf791bfbe900b#1ae2b1c5593b80eaaa2bf791bfbe900b
Especially:
- Remove unnecessary sleeps
- Use kwarg names for all arguments, including name
- Don't say "connect"
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 can't access the link, but I followed what is in the hello pinecone aws notebook — let me know if the update is correct
|
Looks good, thanks! |
Updates outdated models and Q&A from old RAG chatbot notebook. Also updates the
langchain-pineconepackage to the latest version0.2.5