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

feat: Qdrant vector search support #2428

Merged
merged 9 commits into from
Sep 10, 2024
Merged

Conversation

Anush008
Copy link
Contributor

@Anush008 Anush008 commented Sep 2, 2024

Description:

This PR adds support for Qdrant to be used for vector search.

Checklist

  • Do new or existing unit or integration tests cover this code?
  • Did you run make unit_testing and make integration-testing successfully?
  • Do new classes, functions, methods and parameters all have docstrings?
  • Were existing docstrings updated, if necessary?

pyproject.toml Outdated Show resolved Hide resolved
superduper/base/config.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@blythed blythed left a comment

Choose a reason for hiding this comment

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

Hi @Anush008 - thank-you for your work and this important contribution to the project!

My main concerns are around configuring the solution (in the config.py module) and
also how we test this.

Could you add comments in the PR on how you tested this? Also, we should somehow test this in the CI too.

@Anush008
Copy link
Contributor Author

Anush008 commented Sep 2, 2024

Hey @blythed. The implementation already undergoes the unit tests..
By default, it uses an :in-memory: instance of the Qdrant API.

@Anush008
Copy link
Contributor Author

Anush008 commented Sep 2, 2024

In-memory mode reference.

Copy link
Collaborator

@jieguangzhou jieguangzhou left a comment

Choose a reason for hiding this comment

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

Great work!

superduper/vector_search/qdrant.py Outdated Show resolved Hide resolved
@blythed
Copy link
Collaborator

blythed commented Sep 6, 2024

@Anush008 could you get the CI passing?

@Anush008
Copy link
Contributor Author

Anush008 commented Sep 6, 2024

Haha. I assumed it was some flaky test. Updated the CHANGELOG. Should be good now.

@Anush008
Copy link
Contributor Author

Anush008 commented Sep 9, 2024

@blythed, could you please approve the CI to check?

@Anush008
Copy link
Contributor Author

Anush008 commented Sep 9, 2024

I am not sure if the failure is related to this PR.

@fnikolai
Copy link
Collaborator

fnikolai commented Sep 9, 2024

Thanks @Anush008 for your PR.

This error usually occurs due to implicit type casting, such as using a string where an int is expected, or vice versa.

It would be very helpful if you could identify where this type casting happens (e.g., in the arguments used when calling qdrant).

If pinpointing the issue is not feasible, as a last resort, you can try increasing the type_ignore value in test/unittest/test_quality.py.

Here's the link to the relevant line:

'type_ignore': 10, # This should only ever increase in obscure edge cases

Copy link
Collaborator

@jieguangzhou jieguangzhou left a comment

Choose a reason for hiding this comment

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

#2428 (comment)

Thanks for your PR @Anush008 .

Please fix the CI, then we can merge it

@Anush008
Copy link
Contributor Author

Anush008 commented Sep 9, 2024

Pushed a fix. Should be green now.

@Anush008
Copy link
Contributor Author

Anush008 commented Sep 9, 2024

cc @fnikolai @jieguangzhou

@jieguangzhou jieguangzhou merged commit ce89712 into superduper-io:main Sep 10, 2024
2 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.

4 participants