Skip to content
This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

feat: Qdrant as a supported knowledge base #244

Merged
merged 19 commits into from
Mar 27, 2024

Conversation

Anush008
Copy link
Contributor

@Anush008 Anush008 commented Jan 10, 2024

Description

This PR adds support for Qdrant to be used as a knowledge base.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Test Plan

Tests have been added to tests/system/qdrant for the sync and async implementations and are configured to run using both, an in-memory Qdrant instance and Qdrant server(if running at port 6333, skipped otherwise).

Note

The qdrant_client dependency can be gated behind an extra if needed and CLI support can be added subsequently.

@igiloh-pinecone
Copy link
Contributor

Thank you @Anush008 for the great contribution!!
We are still in the process of reviewing this PR, will publish the review in the coming days.

@Anush008
Copy link
Contributor Author

Thanks @igiloh-pinecone.

@ChristopherDosin
Copy link

We are still in the process of reviewing this PR, will publish the review in the coming days.

Any news on that a month later? :)

@igiloh-pinecone
Copy link
Contributor

We are still in the process of reviewing this PR, will publish the review in the coming days.

Any news on that a month later? :)

Hi @ChristopherDosin,
As I mentioned before, we are considering an internal refactor of the KnowledgeBase class, that could affect the approach taken in this PR. But since this is still in internal discussions and design process, we shouldn't delay merging this PR any longer.
I'll publish a review of the current standing PR in the coming day or two.
Thank you for you patience! 🙏

@ChristopherDosin
Copy link

We are still in the process of reviewing this PR, will publish the review in the coming days.

Any news on that a month later? :)

Hi @ChristopherDosin, As I mentioned before, we are considering an internal refactor of the KnowledgeBase class, that could affect the approach taken in this PR. But since this is still in internal discussions and design process, we shouldn't delay merging this PR any longer. I'll publish a review of the current standing PR in the coming day or two. Thank you for you patience! 🙏

Awesome, thank you my friend - Just wondering which one performs better :)

Copy link
Contributor

@igiloh-pinecone igiloh-pinecone left a comment

Choose a reason for hiding this comment

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

@Anush008 thank you very much for your contribution!!

Apologies for the delayed review.
Please see a few comments. Namely, please try to re-use as much test_ code as possible instead of duplicating it. This will make future changes to KnowledgeBase much more maintainable.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/canopy/knowledge_base/qdrant/constants.py Outdated Show resolved Hide resolved
src/canopy/knowledge_base/qdrant/converter.py Outdated Show resolved Hide resolved
src/canopy/knowledge_base/qdrant/qdrant_knowledge_base.py Outdated Show resolved Hide resolved
Copy link
Contributor

@igiloh-pinecone igiloh-pinecone left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you @Anush008 for this effort

@brightsparc
Copy link

Looking forward to having this land. Anything holding this up?

@igiloh-pinecone igiloh-pinecone added this pull request to the merge queue Mar 10, 2024
@igiloh-pinecone
Copy link
Contributor

Looking forward to having this land. Anything holding this up?

As far as I recall some system test failed in the merge queue 🤔 .
Trying to re-merge this now, let's see.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 10, 2024
@Anush008
Copy link
Contributor Author

@igiloh-pinecone, I've updated the branch. Could you try now please?

@Anush008
Copy link
Contributor Author

Anush008 commented Mar 14, 2024

The flaky CI failures seem to arise from unrelated files.

UPDATE: This should be resolved with #327.

@izellevy izellevy added this pull request to the merge queue Mar 27, 2024
Merged via the queue into pinecone-io:main with commit b835eb2 Mar 27, 2024
7 checks passed
@Anush008 Anush008 deleted the qdrant-knowledge-base branch March 27, 2024 11:34
@brightsparc
Copy link

Nice. When is the next release likely to get cut?

@izellevy
Copy link
Collaborator

We have a new release: https://github.com/pinecone-io/canopy/releases/tag/v0.9.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants