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

Voyager Backend #41

Merged
merged 10 commits into from
Dec 6, 2024
Merged

Voyager Backend #41

merged 10 commits into from
Dec 6, 2024

Conversation

sky-2002
Copy link
Contributor

@sky-2002 sky-2002 commented Dec 4, 2024

This PR adds voyager backend
Related issue: #40

Copy link
Member

@Pringled Pringled left a comment

Choose a reason for hiding this comment

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

Hey @sky-2002 , thanks a lot for making this PR! I think it's almost good to go, just a couple of small comments.

One other comment: could you also add Voyager to the tested backends? It should be as simple as adding Voyager here: https://github.com/MinishLab/vicinity/blob/main/tests/conftest.py#L32, and running make test to see if everything works as expected.

vicinity/backends/voyager.py Show resolved Hide resolved
vicinity/backends/voyager.py Show resolved Hide resolved
@Pringled
Copy link
Member

Pringled commented Dec 4, 2024

A couple of other comments:

Thanks in advance!

@Pringled Pringled self-assigned this Dec 4, 2024
@Pringled Pringled added the enhancement New feature or request label Dec 4, 2024
Copy link
Contributor

@stephantul stephantul 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 adding this 🙏 ! I second @Pringled's comments about the tests, docs and requirements.

vicinity/backends/voyager.py Outdated Show resolved Hide resolved
vicinity/backends/voyager.py Show resolved Hide resolved
vicinity/backends/voyager.py Show resolved Hide resolved
vicinity/backends/voyager.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 97.22222% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
vicinity/backends/voyager.py 96.92% 2 Missing ⚠️
Files with missing lines Coverage Δ
tests/conftest.py 100.00% <100.00%> (ø)
vicinity/backends/__init__.py 100.00% <100.00%> (ø)
vicinity/datatypes.py 100.00% <100.00%> (ø)
vicinity/backends/voyager.py 96.92% <96.92%> (ø)

... and 2 files with indirect coverage changes

@stephantul
Copy link
Contributor

@sky-2002 voyager isn't installed in CI yet, which is why these tests all fail. Could you add it to the list of extra in the Makefile?

@stephantul stephantul self-requested a review December 6, 2024 11:30
Copy link
Member

@Pringled Pringled left a comment

Choose a reason for hiding this comment

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

Approved, thanks for this addition!

@stephantul stephantul merged commit 64a6409 into MinishLab:main Dec 6, 2024
5 checks passed
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.

4 participants