-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix embedding types & casing #1032
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
base: main
Are you sure you want to change the base?
Fix embedding types & casing #1032
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Summary of ChangesHello @Surjasa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the "Talk_to_documents_with_embeddings.ipynb" example by rectifying the embedding task type configurations and standardizing the notebook's metadata. The primary goal is to ensure the example accurately demonstrates the usage of "RETRIEVAL_DOCUMENT" and "RETRIEVAL_QUERY" constants, thereby preventing potential user confusion and aligning with SDK expectations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request does a great job of correcting the embedding types and casing in the Talk_to_documents_with_embeddings.ipynb notebook, aligning it with the SDK's best practices. The normalization of notebook metadata is also a good cleanup step. I found one place where a query embedding still uses an incorrect task_type, which I've pointed out in a specific comment. Once that's addressed, this will be a solid improvement.
Wrong Tutorial issue #1030
Summary:
-The notebook
Talk_to_documents_with_embeddings.ipynbused the same embedding type for both query and document, and used the lowercase stringretrieval_document.That is incorrect: document embeddings should use
RETRIEVAL_DOCUMENTand query embeddings should useRETRIEVAL_QUERY(with the exact casing used by the SDK constants).-This change corrects the embedding type names and casing in the notebook and normalizes notebook metadata per the repository style (
execution_count: null and cleared outputs) so diffs are minimal and consistent.What I changed:
Updated the notebook cell(s) that constructed/used embeddings so:
-document embeddings use RETRIEVAL_DOCUMENT
-query embeddings use RETRIEVAL_QUERY (where appropriate)
-corrected any occurrences of the lowercase
retrieval_documentto the proper constant-Ran notebook normalization/formatting to ensure
execution_countis null and outputs are empty (no runtime output saved).Why this fixes the issue:
Using the correct embedding type and casing matches the SDK/guide expectations and prevents incorrect behavior or confusion when users copy the example.
Fixes issue #1030