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

Support get_by_ids #42

Merged
merged 6 commits into from
Dec 29, 2024
Merged

Conversation

nickderobertis
Copy link
Contributor

Closes #41

  • Implements the get_by_ids method, using JSON.MGET for JSON storage and pipeline + HGETALL for hash storage.
  • The doc on the method is copied straight from the langchain base class
  • Adds integration tests for JSON and hash data types
  • I tried to add a unit test, but seems like I would have to mock a lot of the Redis library to make it work right. I ultimately removed it in the last commit. Let me know if this is a blocker. Maybe we could consider some kind of intermediate layer in between the vector store and Redis to make mocking easier.

@bsbodden
Copy link
Collaborator

@nickderobertis Currently evaluating... We'll let you know soon. Thanks for the contribution

@bsbodden bsbodden self-requested a review December 29, 2024 04:09
Copy link
Collaborator

@bsbodden bsbodden left a comment

Choose a reason for hiding this comment

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

LGTM, we'll work on a proper unit test. Mocking is a PITA. Thanks for the contribution.

@bsbodden bsbodden merged commit 30dfd44 into langchain-ai:main Dec 29, 2024
12 checks passed
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.

Support for get_by_ids
2 participants