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

new: add sparse vectors support in discovery and recommend in local mode #571

Merged
merged 12 commits into from
Apr 5, 2024

Conversation

joein
Copy link
Member

@joein joein commented Apr 3, 2024

more than a half of the PR are tests 🙈

Copy link

netlify bot commented Apr 3, 2024

Deploy Preview for poetic-froyo-8baba7 ready!

Name Link
🔨 Latest commit 50d7b6e
🔍 Latest deploy log https://app.netlify.com/sites/poetic-froyo-8baba7/deploys/660fccb5342a0100089ce8fb
😎 Deploy Preview https://deploy-preview-571--poetic-froyo-8baba7.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@joein joein requested review from agourlay and coszio April 4, 2024 06:46
@@ -47,7 +47,7 @@ def generate_random_sparse_vector_list(


def random_sparse_vectors(
vector_sizes: Union[Dict[str, int], int],
vector_sizes: Union[Dict[str, int]],
Copy link
Member

Choose a reason for hiding this comment

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

can we remove the Union completely then?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we can

Copy link
Member

@agourlay agourlay left a comment

Choose a reason for hiding this comment

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

The test coverage looks excellent!

I did not review in details the scoring implementation but I am confident due to the congruence tests 👍

Copy link
Contributor

@coszio coszio left a comment

Choose a reason for hiding this comment

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

Lots of work in a single PR, but still easy to go through. Nothing to complain aside from some nits

class DistanceOrder(str, Enum):
BIGGER_IS_BETTER = "bigger_is_better"
SMALLER_IS_BETTER = "smaller_is_better"
QueryVector = Union[
Copy link
Contributor

Choose a reason for hiding this comment

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

Only a suggestion, I'll leave it to your criteria

Suggested change
QueryVector = Union[
DenseQueryVector = Union[

List[models.SparseVector],
types.Filter,
]:
def split_examples(
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to read a few times to understand what this was doing, maybe:

Suggested change
def split_examples(
def examples_into_vectors(

?

Comment on lines +795 to +798
else:
query_vector = SparseRecoQuery(
positive=sparse_positive_vectors, negative=sparse_negative_vectors
)
Copy link
Contributor

Choose a reason for hiding this comment

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

what if there are no sparse_positive_vectors nor sparse_negative_vectors either?
Edit: it is already checked in

raise ValueError("No positive or negative examples given")

@joein
Copy link
Member Author

joein commented Apr 5, 2024

tests are failing now due to #580

@joein joein force-pushed the add-sparse-recommendation-and-discovery-local branch from a4f5892 to 50d7b6e Compare April 5, 2024 10:04
@joein joein merged commit c8aa21c into dev Apr 5, 2024
14 checks passed
skvark pushed a commit to skvark/qdrant-client that referenced this pull request Apr 8, 2024
…ode (qdrant#571)

* new: add sparse vectors support in discovery and recommend in local mode

* chore: remove redundant file

* chore: remove another redundant file

* fix: fix type hint for python3.8

* fix: remove redundant types from check

* refactoring: remove redundant union

* refactoring: remove redundant import

* fix: fix type hint

* fix: fix type hint

* refactoring: improve exception description

Co-authored-by: Luis Cossío <luis.cossio@outlook.com>

* refactoring: improve exception description

Co-authored-by: Luis Cossío <luis.cossio@outlook.com>

* refactoring: address review comments

---------

Co-authored-by: Luis Cossío <luis.cossio@outlook.com>
joein added a commit that referenced this pull request Apr 22, 2024
…ode (#571)

* new: add sparse vectors support in discovery and recommend in local mode

* chore: remove redundant file

* chore: remove another redundant file

* fix: fix type hint for python3.8

* fix: remove redundant types from check

* refactoring: remove redundant union

* refactoring: remove redundant import

* fix: fix type hint

* fix: fix type hint

* refactoring: improve exception description

Co-authored-by: Luis Cossío <luis.cossio@outlook.com>

* refactoring: improve exception description

Co-authored-by: Luis Cossío <luis.cossio@outlook.com>

* refactoring: address review comments

---------

Co-authored-by: Luis Cossío <luis.cossio@outlook.com>
@generall generall deleted the add-sparse-recommendation-and-discovery-local branch May 3, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants