-
Notifications
You must be signed in to change notification settings - Fork 121
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
universal-search: Query Group API and local mode #690
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -2523,8 +2529,8 @@ def convert_vector_input(cls, model: rest.VectorInput) -> grpc.VectorInput: | |||
@classmethod | |||
def convert_recommend_input(cls, model: rest.RecommendInput) -> grpc.RecommendInput: | |||
return grpc.RecommendInput( | |||
positive=[cls.convert_vector_input(vector) for vector in model.positive], | |||
negative=[cls.convert_vector_input(vector) for vector in model.negative], | |||
positive=[cls.convert_vector_input(vector) for vector in model.positive] if model.positive is not None else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix existing bug
f15898f
to
9715a26
Compare
4c05a1f
to
de1c9ed
Compare
tests/congruence_tests/test_query.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is still valuable to test grpc too. To edit this easier though, we could set the pytestmark
global variable instead of repeating it for each test
pytestmark = pytest.mark.parametrize("prefer_grpc", [False, True])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, just wanted to take it out of this PR because it triggers issues unrelated to this PR 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the date time bug has been merged I re-pushed a bit of gRPC testing.
7f4f843
It found right away a bug in the with_lookup
implementation :)
prefetch=[set_prefetch_limit_recursively(p, limit) for p in prefetch.prefetch], | ||
) | ||
else: | ||
return types.Prefetch(limit=limit, prefetch=list()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
return types.Prefetch(limit=limit, prefetch=list()) | |
return rest_models.Prefetch(limit=limit, prefetch=list()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not work, it can't find such module 🤔
def set_prefetch_limit_recursively(prefetch: types.Prefetch, limit: int) -> types.Prefetch: | ||
if prefetch is not None: | ||
if isinstance(prefetch.prefetch, list): | ||
return types.Prefetch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
return types.Prefetch( | |
return rest_models.Prefetch( |
with_lookup_collection = self._get_collection(with_lookup) | ||
else: | ||
with_lookup_collection = self._get_collection(with_lookup.collection) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to resolve ids into vectors at this point, considering that we add lookup_from
parameter first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added and tested in 2e52841 :)
d5e5e00
to
0f782d5
Compare
Dear reviewers, I believe I have addressed all your comments 🫡 |
Let's keep this PR open to not make cherry picking too difficult in case of patch release. We could stack other PRs on top of it for the next release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to approve after the description edit :)
Co-authored-by: Luis Cossío <[email protected]>
* pre-implement dbsf * add dbsf congruence tests * mypy lints * add conversions * tests: add test for dbsf conversion --------- Co-authored-by: George Panchuk <[email protected]>
* pre-implement random sampling * generate models * add conversions and tests * fix mypy lints * tests: add test for sample random conversion * use camelcase Sample.Random * review fixes * fix mypy --------- Co-authored-by: George Panchuk <[email protected]>
9f31c70
to
dc585b3
Compare
@joein Thanks for handling the merges 👏 |
* add parametrized fixture for using grpc too * compare grpc and http without running each setup twice * fix: fix exception types in invalid types test * fix: remove random seed which led to a erroneous sequence --------- Co-authored-by: Luis Cossío <[email protected]>
* universal-search: Query Group API and local mode * requires different logic for gRPC error assertion * you come to me at runtime for a compile time issue * suddenly throwing a different error * test more group key types * extend limit of prefetches during group_by * rescoring is the issue * one problem at a time please * code review * regen clients * code review * add lookup_from to query_points_groups * test with_lookup * test and fix gRPC * drop dedicated conversion * Update qdrant_client/qdrant_client.py Co-authored-by: Luis Cossío <[email protected]> * regen async * Distribution-based score fusion in local mode (#703) * pre-implement dbsf * add dbsf congruence tests * mypy lints * add conversions * tests: add test for dbsf conversion --------- Co-authored-by: George Panchuk <[email protected]> * Random sample in local mode (#705) * pre-implement random sampling * generate models * add conversions and tests * fix mypy lints * tests: add test for sample random conversion * use camelcase Sample.Random * review fixes * fix mypy --------- Co-authored-by: George Panchuk <[email protected]> * fix: add type ignore for mypy * fix: fix type hints for 3.8 * fix: do not run mypy on async client generator in CI, simplify condition * Grpc comparison in tests (#726) * add parametrized fixture for using grpc too * compare grpc and http without running each setup twice * fix: fix exception types in invalid types test * fix: remove random seed which led to a erroneous sequence --------- Co-authored-by: Luis Cossío <[email protected]> --------- Co-authored-by: Luis Cossío <[email protected]> Co-authored-by: George Panchuk <[email protected]>
This PR adds the client API and local mode for the new group query API.
I took the opportunity to increase the gRPC coverage but it seems to create issues with newest Python versions.