-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add new plugin : Genkit HNSW #51
Conversation
@retzd-tech thanks for the contribution! 🎉 Will review and check what's missing at some point today |
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.
@retzd-tech thank you so much for your contribution! Fixed some dependencies issues here and there, now the build process completes successfully.
@davidoort I suggested some changes, tagging you in the comments. I'd suggest to take those steps before merging. Let me know if you need help.
Given the size of the PR, I advise to first merge this and publish the plugin, then work on the other needed changes (Code Owners, docs, etc.) on a different PR.
plugins/hnsw/README.md
Outdated
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.
@davidoort we should align this to our README template
plugins/hnsw/CONTRIBUTING.md
Outdated
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.
@davidoort the content of this file should be translated to GitHub issues so people can easily spot the missing contributions and help with that. 👍🏻
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.
this is a good idea, but not required for merging imo
"@genkit-ai/flow": "^0.5.0", | ||
"@genkit-ai/googleai": "^0.5.0", | ||
"@langchain/google-genai": "^0.0.11", | ||
"@types/node": "^20.12.12", | ||
"@types/jest": "^29.5.12", | ||
"fs": "^0.0.1-security", | ||
"glob": "^10.4.1", | ||
"hnswlib-node": "^1.3.0", | ||
"langchain": "^0.0.11", | ||
"node-fetch": "^3.2.6", | ||
"redis": "^4.6.13", | ||
"zod": "^3.22.4" |
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.
@davidoort I moved @genkit-ai/ai
and @genkit-ai/core
to peer dependencies, not sure if some of these need to be moved there as well. Maybe worth checking.
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.
sounds good!
plugins/hnsw/CONTRIBUTING.md
Outdated
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.
This is a good idea for some of the other plugins!
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.
Thank you so so much for the contribution and addressing all comments @retzd-tech ! I'm so excited to get this released! 🎉🔥
Before you submit a pull request, please make sure you have read and understood the contribution guidelines and the code of conduct.
This pull request is related to:
I have checked the following:
Description:
Add a new plugin Genkit HNSW