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

PR for merging content on Notebook for RAG with Llama 3 open-source and Elastic #260 #266

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

rishikeshradhakrishnan
Copy link
Contributor

Notebook for RAG with Llama 3 open-source and Elastic #260

Copy link

gitnotebooks bot commented Jun 7, 2024

Found 2 changed notebooks. Review the changes at https://gitnotebooks.com/elastic/elasticsearch-labs/pull/266

@@ -0,0 +1,317 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Commented on notebook notebooks/integrations/llama3/rag-elastic-llama3.ipynb Cell 4 Line 1

!pip install llama-index

nitpick: update this to be one line for pip install, similar to how the other notebooks have done this

Respond and view the context here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code.

@@ -0,0 +1,317 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Commented on notebook notebooks/integrations/llama3/rag-elastic-llama3.ipynb Cell 6 Line 2

from llama_index.core.node_parser import SentenceSplitter
from llama_index.core.ingestion import IngestionPipeline

nitpick: move the imports to where the code being used, so its obvious which cell needs which import.

Respond and view the context here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it accordingly.

@@ -0,0 +1,317 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Commented on notebook notebooks/integrations/llama3/rag-elastic-llama3.ipynb Cell 13 Line 1

## Execute pipeline, 

remove ,

Respond and view the context here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this as well.

@@ -0,0 +1,317 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Commented on notebook notebooks/integrations/llama3/rag-elastic-llama3.ipynb Cell 15 Line 1

The embeddings are stored in a dense vector field of dimension `4096`. The dimension size comes from the size of the embeddings generated from `Llama3`.

is that right? the embedding is 4096 dims? Could we improve this with picking a smaller dim model instead if so.

Respond and view the context here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK and checked, there didnt seem to be any option to change that.

@@ -0,0 +1,317 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Commented on notebook notebooks/integrations/llama3/rag-elastic-llama3.ipynb Cell 1 Line 1

# RAG with Elastic and Llama3

this examples are either LangChain or Llama-index. I would update it like so and be obvious that its using these frameworks behind the scenes.

Respond and view the context here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it.

@rishikeshradhakrishnan rishikeshradhakrishnan merged commit 4c66913 into main Jun 20, 2024
5 checks passed
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.

2 participants