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

Integrate VoyageAI Vectorizer and Reranker class #223

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

tylerhutcherson
Copy link
Collaborator

This PR (thanks to @fzowl) integrates the VoyageAI vectorizer and reranker into the RedisVL client, streamlining access for devs to embed data and rerank search results from Redis.

@tylerhutcherson tylerhutcherson added the enhancement New feature or request label Sep 21, 2024
@fzowl
Copy link

fzowl commented Sep 23, 2024

@tylerhutcherson Thank you very much! Please let me know if i can help you with this.

@fzowl
Copy link

fzowl commented Sep 26, 2024

@tylerhutcherson It seems like there is an error in the rerank code, i suggested a change: https://github.com/redis/redis-vl-python/pull/223/files#r1777636350
Can you please take a look?

@tylerhutcherson
Copy link
Collaborator Author

@fzowl We are so close. I am having a hard time getting this to work with hiredis for some reason as it's part of our testing CI matrix here on github. Seems like there's some dependency issue but the root cause is swallowed by the CI server and it just times out. Will keep trying.

Comment on lines +280 to +283
if isinstance(avectorizer, VoyageAITextVectorizer):
embeddings = await avectorizer.aembed_many(texts)
else:
embeddings = await avectorizer.aembed_many(texts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this if-else block needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure, will remove

Copy link

Choose a reason for hiding this comment

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

@justin-cechmanek Yeah, seems like a mistake. Sorry about that. (i think originally i was planning to add further parameters, but in its state this condition is not needed)

Comment on lines +268 to +271
if isinstance(avectorizer, VoyageAITextVectorizer):
embedding = await avectorizer.aembed(text)
else:
embedding = await avectorizer.aembed(text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same question, why do we need this if-else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ditto!

Copy link

Choose a reason for hiding this comment

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

@justin-cechmanek Same as above, looks like a mistake

@fzowl
Copy link

fzowl commented Oct 14, 2024

@tylerhutcherson @justin-cechmanek Is there a way i can help with this one? I'd happily remove those if-else statements, but i can't push to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants