feature: Facets #759
Merged
feature: Facets #759
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
coszio
force-pushed
the
facet-local-mode
branch
from
7f0be8c to
12dc515
Compare
joein
requested changes
Sep 12, 2024
Comment on lines
1789
to
1809
| def convert_facet_value(cls, model: grpc.FacetValue) -> rest.FacetValue: | ||
| name = model.WhichOneof("variant") | ||
| if name is None: | ||
| raise ValueError(f"invalid FacetValue model: {model}") | ||
|
|
||
| val = getattr(model, name) | ||
| return val | ||
|
|
||
| @classmethod | ||
| def convert_facet_value_hit(cls, model: grpc.FacetHit) -> rest.FacetValueHit: | ||
| return rest.FacetValueHit( | ||
| value=cls.convert_facet_value(model.value), | ||
| count=model.count, | ||
| ) | ||
|
|
||
| @classmethod | ||
| def convert_facet_response(cls, model: grpc.FacetResponse) -> rest.FacetResponse: | ||
| return rest.FacetResponse( | ||
| hits=[cls.convert_facet_value_hit(hit) for hit in model.hits], | ||
| ) | ||
|
|
There was a problem hiding this comment.
- lacks conversion tests
- lacks rest to grpc
There was a problem hiding this comment.
We are implementing the inverse conversions even for the structs which we are not using in order to increase test coverage. E.g. scored point, update result
| @@ -1261,7 +1261,7 @@ def recommend_groups( | |||
| with_lookup = RestToGrpc.convert_with_lookup(with_lookup) | |||
|
|
|||
| if isinstance(with_lookup, str): | |||
| with_lookup = grpc.WithLookup(lookup_index=with_lookup) | |||
| with_lookup = grpc.WithLookup(collection=with_lookup) | |||
joein
reviewed
Sep 12, 2024
Comment on lines
+1133
to
+1137
| hits = [ | ||
| models.FacetValueHit(value=value, count=count) | ||
| for value, count in sorted( | ||
| facet_hits.items(), | ||
| # order by count descending, then by value ascending | ||
| key=lambda x: (-x[1], x[0]), | ||
| )[:limit] | ||
| ] |
There was a problem hiding this comment.
if there is a facet request on a field which is not one of supported type (int, str), e.g. with a boolean, then we'll get an ugly pydantic error like
validated_self = self.__pydantic_validator__.validate_python(data, self_instance=self)
pydantic_core._pydantic_core.ValidationError: 2 validation errors for FacetValueHit
value.int
Input should be a valid integer [type=int_type, input_value=True, input_type=bool]
For further information visit https://errors.pydantic.dev/2.9/v/int_type
value.str
Input should be a valid string [type=string_type, input_value=True, input_type=bool]
For further information visit https://errors.pydantic.dev/2.9/v/string_type
There was a problem hiding this comment.
I thought this was already sanitized above. Did you create a failing test?
There was a problem hiding this comment.
I just realized that bool is a subclass of int, which is why it was getting through isinstance.
I now fixed it by using
if type(v) not in get_args_subscribed(types.FacetValue):
continue
joein
force-pushed
the
update-clients
branch
from
d0b9d39 to
1c600f5
Compare
- qdrant base - qdrant client - conversions
coszio
force-pushed
the
facet-local-mode
branch
from
bd123a1 to
d8ccef6
Compare
joein
approved these changes
Sep 16, 2024
|
Just updated for testing bool facets. Right now CI fails because dev build is not yet ready, I'll rerun in a few hours |
joein
added a commit
that referenced
this pull request
Oct 5, 2024
* update rest client * update grpc client with grpcio 1.65.4 * update grpc client with grpcio==1.49.1 * generate grpc client with grpcio==1.48.2 * generate grpc client with grpcio==1.66.1 and mypy-protobuf extension * add mypy-protobuf to pyproject * relax version for grpcio, but keep grpcio-tools at 1.48.2 * improve generation script * fix bash script comment Co-authored-by: George <[email protected]> * use old grpcio to generate client * rollbakc mypy-protobuf * regenerate with distance matrix * new: update clients (#774) * new: update clients * rollback grpcio version * feature: Facets (#759) * facet in local_collection.py * replace usages of calculate_payload_mask * - qdrant remote - qdrant base - qdrant client - conversions * congruence tests + local mode fixes * generate async client * add type stubs and misc fixes * fix mypy in Python 3.8 * generate async client * review remarks * gen async client * update for bool facets * Feature: Distance Matrix API (#769) * add remote impls * add client impl * regen async client * start local * regen async * local mode * start congruence * Fix local mode * regen async of course * test filtering * fix min samples count * simplify comparaison loop * simplify samples loop * add rest/gRPC conversion tests * fix conversions + tests --------- Co-authored-by: George <[email protected]> Co-authored-by: generall <[email protected]> Co-authored-by: Arnaud Gourlay <[email protected]>
joein
added a commit
that referenced
this pull request
Oct 7, 2024
* update rest client * update grpc client with grpcio 1.65.4 * update grpc client with grpcio==1.49.1 * generate grpc client with grpcio==1.48.2 * generate grpc client with grpcio==1.66.1 and mypy-protobuf extension * add mypy-protobuf to pyproject * relax version for grpcio, but keep grpcio-tools at 1.48.2 * improve generation script * fix bash script comment Co-authored-by: George <[email protected]> * use old grpcio to generate client * rollbakc mypy-protobuf * regenerate with distance matrix * new: update clients (#774) * new: update clients * rollback grpcio version * feature: Facets (#759) * facet in local_collection.py * replace usages of calculate_payload_mask * - qdrant remote - qdrant base - qdrant client - conversions * congruence tests + local mode fixes * generate async client * add type stubs and misc fixes * fix mypy in Python 3.8 * generate async client * review remarks * gen async client * update for bool facets * Feature: Distance Matrix API (#769) * add remote impls * add client impl * regen async client * start local * regen async * local mode * start congruence * Fix local mode * regen async of course * test filtering * fix min samples count * simplify comparaison loop * simplify samples loop * add rest/gRPC conversion tests * fix conversions + tests --------- Co-authored-by: George <[email protected]> Co-authored-by: generall <[email protected]> Co-authored-by: Arnaud Gourlay <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Needs #758
Adds
facetfunctionality in both remote and local modeWIP:
All Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
pre-commitwithpip3 install pre-commitand set up hooks withpre-commit install?Changes to Core Features: