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

adds retrieval logic #17

Merged
merged 5 commits into from
Apr 30, 2024
Merged

adds retrieval logic #17

merged 5 commits into from
Apr 30, 2024

Conversation

koleshjr
Copy link
Contributor

@koleshjr koleshjr commented Apr 29, 2024

adds retrieval logic using LCEL
If any changes is required please let me know.
Thank you

Copy link
Owner

@KevKibe KevKibe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
Everything, checks out, don't mind the failed tests, I'll run the tests with my environment variables.
Is there a dependency that needs to be added to use
from operator import itemgetter and from langchain_core.prompts import PromptTemplate imports, if there is kindly add them to the PR in both requirements.txt and setup.cfg.
And could you remove one import of from langchain_pinecone import PineconeVectorStore module, it's imported twice in both files.

@koleshjr
Copy link
Contributor Author

yeah only for langchain core though , I'm on my way home. will fix when I get home

@koleshjr
Copy link
Contributor Author

Fixed it.
Added some flexibility to the retieve method by letting the user specify the models they want
But also adding default models incase they don't provide it
I have not tested them yet, Kindly test and lemme know if there are issues I need to fix

Thanks

Copy link
Owner

@KevKibe KevKibe left a comment

Choose a reason for hiding this comment

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

LGTM

@KevKibe KevKibe merged commit de04360 into KevKibe:master Apr 30, 2024
1 of 2 checks passed
@KevKibe
Copy link
Owner

KevKibe commented Apr 30, 2024

PR Merged🥳

@koleshjr
Copy link
Contributor Author

🥳🥳🥳🥳

@koleshjr
Copy link
Contributor Author

rerankers next. Still reading about them.

@koleshjr
Copy link
Contributor Author

Btw kindly test the method. I just noticed a bug

retriever = vector_store.as_retriver(search_kwargs = {"k": top_k})

fix the above in both openai and google to :

retriever = vector_store.as_retriever(search_kwargs = {"k": top_k})

missed the e in the retriever

@KevKibe
Copy link
Owner

KevKibe commented Apr 30, 2024

Nice.
I'm adding support for cohere embeddings and then do some tests to make sure everything works well, then I draft a release.

@koleshjr
Copy link
Contributor Author

great

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