-
Notifications
You must be signed in to change notification settings - Fork 174
feat: Add GoogleAITextEmbedder and GoogleAIDocumentEmbedder components #1783
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
…Embedder and GoogleAIDocumentEmbedder
This might be related' #1694 |
Hi @anakin87, sorry I haven't checked that, but if you can review this and let me know if my PR would be considered or not. |
Hey @garybadwal thanks for your work on this! We actually just created a new integration for Google in this PR which uses the new Could I ask you to do the following with this PR:
Once you've done these, we'll be happy to give a more in depth review! |
Thank you so much @sjrl for your review on this. Sure, I'll make all the changes ASAP and will let you know. Also if you can help a little with tests it would be helpful. If you can provide me some sort of format or standard that specifically being followed for integration, so I can write in the same standards. And thanks once again for your guidance in this. |
Great! For the tests I suggest you follow the ones we have for our OpenAI Text and Document Embedders. They can be found here: |
Thank you so much man @sjrl, I'll complete this over this weekend and you can than give it a review. Thank you so much for your help. |
Sounds good! For the tests make sure to add them to the |
Hi @sjrl, I have added the test cases and also made them run once, those are working fine. |
Hey @garybadwal could you take a look at the failing tests in the CI? It looks like a few of the unit tests are failing. E.g. here and also the linting is failing here. You can run the linting locally by running |
Ok sure @sjrl I'll check. |
…entEmbedder and GoogleAITextEmbedder tests
Hi @sjrl, I fixed the test case and linting problem, but only one linting is failing. Should I fix that too, or will this work? |
...ns/google_genai/src/haystack_integrations/components/embedders/google_genai/text_embedder.py
Outdated
Show resolved
Hide resolved
...ns/google_genai/src/haystack_integrations/components/embedders/google_genai/text_embedder.py
Outdated
Show resolved
Hide resolved
...oogle_genai/src/haystack_integrations/components/embedders/google_genai/document_embedder.py
Outdated
Show resolved
Hide resolved
...oogle_genai/src/haystack_integrations/components/embedders/google_genai/document_embedder.py
Outdated
Show resolved
Hide resolved
Thanks for checking! Go ahead and make the change to see if that helps fix the tests |
...oogle_genai/src/haystack_integrations/components/embedders/google_genai/document_embedder.py
Outdated
Show resolved
Hide resolved
...oogle_genai/src/haystack_integrations/components/embedders/google_genai/document_embedder.py
Outdated
Show resolved
Hide resolved
...oogle_genai/src/haystack_integrations/components/embedders/google_genai/document_embedder.py
Outdated
Show resolved
Hide resolved
@garybadwal looking very good and almost there! |
…der to use private attributes for initialization
…document embedder and tests
Hi @sjrl, |
@sjrl, this is my first open-source contribution. I've often come across tweets and LinkedIn posts from other developers sharing their open-source experiences. One thing I noticed is that they usually include a comment at the top of the file like this: # Author Name: <Name of the contributor>
# Author Email: <Email of the author>
# Author GitHub Username: <GitHub username of the author> Is this considered a valid practice? If so, do we allow it in this project? And would it be okay if I do the same? |
@garybadwal great question! I checked internally and this is how we attribute authorship.
|
Thank you @sjrl! |
Hey @sjrl, If you have any feedback on the work I’ve done in this PR, I’d love to hear it. It’ll really help me improve and refine my approach for future contributions and ensure my work reflects the standards of a good developer. Thanks again! 🙌 |
Hi @sjrl, sorry to bother you — just wanted to check in and ask if you’ll be reviewing this again or planning to merge it today? |
Hi @sjrl, sorry to bother you — just wanted to check in and ask if you’ll be reviewing this again or planning to merge it today? |
Hey @sjrl, just checking in—anything pending for me on this? |
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.
@garybadwal thanks for the contribution!
Related Issues
Proposed Changes:
GoogleAITextEmbedder
: For embedding individual strings.GoogleAIDocumentEmbedder
: For embedding HaystackDocument
objects in batches, with optional metadata handling.google.genai
) and are compatible with Haystack’s embedding interface.How did you test it?
to_dict()
andfrom_dict()
.Notes for the reviewer
_embed_batch()
for performance and error resilience.Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.