Skip to content

feat: add json_rag.ipynb, update readme #90

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cormac-rynne
Copy link

Added a notebook to cover a simple technique to apply RAG with JSON data.

Implemented with SentenceTransformer and FAISS to simplify being able to run offline and without API keys.

Update README.md to include references

@NirDiamant
Copy link
Owner

Hi, I'm trying to go over the new notebook, and i'm getting:

image

@cormac-rynne
Copy link
Author

Heya, I think it was because I built it in colab originally then downloaded it. I copied and pasted into a normal notebook and it seems to have fixed the problem

@NirDiamant
Copy link
Owner

Thanks, I can now see it.
Several points to fix:

  • Please try to break down the long cells into smaller, easy-to-understand cells.
  • Elaborate more with each markdown on what is going to appear in that cell (give the user motivation and understanding in advance)
  • Show the results of what we've done. I believe such a short JSON file will make less sense. Maybe load a bigger one and show the user a snippet of the first several elements.
  • Explain it in a way that a junior developer without any prior understanding will be able to grasp it.

thanks!!

@cormac-rynne
Copy link
Author

Show the results of what we've done

I noticed in the contribution file that you said not to include notebook outputs so I've not included these, but happy to include these if needed

@NirDiamant
Copy link
Owner

There should be a making sense balance. we don't want to show garbage, but if it is crucial for the tutorial we want to show it

@cormac-rynne
Copy link
Author

For sure, makes sense. Included the outputs

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