feat: schema for text index and search by davidbp · Pull Request #267 · docarray/docarray

davidbp · 2022-04-07T13:11:49Z

Expected API examples

q = 'hello' - >  return DocumentArray
q =[ 'hello'] - >  return [DocumentArray] # this can't work unless .find is modfied
q =[ 'hello','hi'] - >  return [DocumentArray, DocumentArray]

The behaviour we want to follow would be analogous to

q = np.array([2,3,4]) - >  return DocumentArray
q = np.array([[2,3,4]]) - >  return [DocumentArray] # this can't work unless .find is modfied
q = np.array([[2,3,4],[2,3,4]]) - >  return [DocumentArray,DocumentArray]

codecov · 2022-04-07T13:17:15Z

Codecov Report

Merging #267 (787421b) into main (5069dc9) will increase coverage by 0.04%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
+ Coverage   86.05%   86.10%   +0.04%     
==========================================
  Files         134      134              
  Lines        6253     6282      +29     
==========================================
+ Hits         5381     5409      +28     
- Misses        872      873       +1

Flag	Coverage Δ
docarray	`86.10% <95.23%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
docarray/array/storage/elastic/seqlike.py	`91.66% <ø> (-0.44%)`	⬇️
docarray/array/mixins/find.py	`88.60% <87.50%> (-0.29%)`	⬇️
docarray/array/storage/elastic/find.py	`89.13% <95.00%> (+0.55%)`	⬆️
docarray/array/storage/elastic/backend.py	`94.33% <100.00%> (+0.28%)`	⬆️
docarray/array/storage/elastic/getsetdel.py	`100.00% <100.00%> (ø)`
docarray/helper.py	`82.40% <0.00%> (+0.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5069dc9...787421b. Read the comment docs.

…na-ai/docarray into feat-elastic-search-with-text

alaeddine-13 · 2022-04-12T23:36:15Z

+    def find(
+        self: 'T',
+        query: Union['DocumentArray', 'Document', 'ArrayType', Dict],
+        metric: Union[
+            str, Callable[['ArrayType', 'ArrayType'], 'np.ndarray']
+        ] = 'cosine',
+        limit: Optional[Union[int, float]] = 20,
+        metric_name: Optional[str] = None,
+        exclude_self: bool = False,
+        only_id: bool = False,
+        index: str = 'text',
+        **kwargs,
+    ) -> Union['DocumentArray', List['DocumentArray']]:
+        """Returns approximate nearest neighbors given an input query.
+
+        :param query: the input query to search by
+        :param limit: the maximum number of matches, when not given defaults to 20.
+        :param metric_name: if provided, then match result will be marked with this string.
+        :param metric: the distance metric.
+        :param exclude_self: if set, Documents in results with same ``id`` as the query values will not be
+                        considered as matches. This is only applied when the input query is Document or DocumentArray.
+        :param only_id: if set, then returning matches will only contain ``id``


At this point, we chose to copy-paste the parent find method code and add more logic to it in order to support finx_by_text.
However, this is not sustainable and a lot of logic (regarding the metrics, matches, limit etc) does not concern the find by text logic. Furthermore, the extended interface (index parameter and the new supported query type) does not appear for the Parent DocumentArray usage in auto-completion.
Therefore, I suggest the following:

applying the extended interface (index parameter and the new supported query type) to the parent DocumentArray class and documenting that these types/params only apply for storage backends that support find by text (might be available in the future for weaviate and qdrant as well)

(option 1) adding an empty handle method to allow extending the behavior and extend it in DocumentArrayElastic

(option 2) adding a method find_by_text to the parent FindMixin which concerns DocumentArray in general which raises a NotImplementedError or a NotSupportedError and add it to the find method in the parent FindMixin. Implement the method for DocumentArrayElastic. That way, only elastic storage backend (and in the future, backends that implement the method) will allow accepting text as input.

Properly documenting that query type text and index parameter only concern storage backends that support find by text

No more re-implementing this find method

the private _find method only handles the find by vector case. We can possible rename it to _find_by_vector

For step 2, I prefer option 2. @JoanFM what is your opinion here ?

makes sense, I've updated find in the parent class to handle strand List[str] and removed it from elastic.find

alaeddine-13 · 2022-04-12T23:38:53Z

+        :param exclude_self: if set, Documents in results with same ``id`` as the query values will not be
+                        considered as matches. This is only applied when the input query is Document or DocumentArray.
+        :param only_id: if set, then returning matches will only contain ``id``
+        :param index: if the query is a string, text search will be performed on `index`, otherwise, ignored. By


again this change should be applied to the parent find mixin, we no more want to do this whole change here

Suggested change

:param index: if the query is a string, text search will be performed on `index`, otherwise, ignored. By

:param index: if the query is a string or list of string, text search will be performed on `index`, otherwise, ignored. By

I see, updated

JoanFM · 2022-04-13T14:15:38Z

Expected API examples

q = 'hello' - >  return DocumentArray
q =[ 'hello'] - >  return [DocumentArray] # this can't work unless .find is modfied
q =[ 'hello','hi'] - >  return [DocumentArray, DocumentArray]

The behaviour we want to follow would be analogous to

q = np.array([2,3,4]) - >  return DocumentArray
q = np.array([[2,3,4]]) - >  return [DocumentArray] # this can't work unless .find is modfied
q = np.array([[2,3,4],[2,3,4]]) - >  return [DocumentArray,DocumentArray]

I think this behavior is not intended. I think we could assume that the query is a DocumentArray in the rest of the interface. I would not vote for mixing interfaces @alaeddine-13

…na-ai/docarray into feat-elastic-search-with-text

JoanFM · 2022-04-13T14:16:40Z

    def find(
        self: 'T',
-        query: Union['DocumentArray', 'Document', 'ArrayType', Dict],
+        query: Union['DocumentArray', 'Document', 'ArrayType', Dict, str, List[str]],


I do not think this is desired. I think Document and DocumentArray are complete enough abstractions to handle what we want.

how would you select how search has to be done if you do da.find(da2) ?

I see two options

add a new field da.find(da2, by='text') or da.find(da2, by='embedding')

have different methods da.find_by_vector(da2) , da.find_by_text(da2)

JoanFM · 2022-04-13T14:18:03Z

        :param exclude_self: if set, Documents in results with same ``id`` as the query values will not be
                        considered as matches. This is only applied when the input query is Document or DocumentArray.
        :param only_id: if set, then returning matches will only contain ``id``
+        :param index:  if the query is a string, text search will be performed on `index`, otherwise, ignored. By


This docstring does not help me understand what this parameter is about or does

alaeddine-13 · 2022-04-13T14:37:04Z

Expected API examples
q = 'hello' - >  return DocumentArray
q =[ 'hello'] - >  return [DocumentArray] # this can't work unless .find is modfied
q =[ 'hello','hi'] - >  return [DocumentArray, DocumentArray]
The behaviour we want to follow would be analogous to
q = np.array([2,3,4]) - >  return DocumentArray
q = np.array([[2,3,4]]) - >  return [DocumentArray] # this can't work unless .find is modfied
q = np.array([[2,3,4],[2,3,4]]) - >  return [DocumentArray,DocumentArray]
I think this behavior is not intended. I think we could assume that the query is a DocumentArray in the rest of the interface. I would not vote for mixing interfaces @alaeddine-13

The API does not behave in that way currently, but we will handle this in a separate PR.
The find method supports various query types here we are just extending the interface.
But in general I think we should have 1 method per input type it's better

Co-authored-by: Han Xiao <han.xiao@jina.ai>

github-actions · 2022-04-14T07:51:23Z

📝 Docs are deployed on https://ft-feat-elastic-search-with-text--jina-docs.netlify.app 🎉

solved

feat: schema for text index and search

0c31227

github-actions Bot added size/s area/core component/array labels Apr 7, 2022

fix: find method overload

d3cbb07

github-actions Bot added size/m and removed size/s labels Apr 8, 2022

davidbp and others added 8 commits April 8, 2022 16:37

fix: logic distpatching find

34e0f4c

fix: add type checking

00397ee

fix: rewrite generic dispatch logic

27d78c8

feat: allow indexing tag fields

197e021

fix: fix indexing tags

a26a20c

fix: accept single string query

de4a46e

test: test find by text

3cf1215

test: test find by tag

b80918d

github-actions Bot added the area/testing label Apr 11, 2022

davidbp and others added 7 commits April 12, 2022 12:58

test: add type tests in find

5bbeed3

test: add type tests in find

f981aa4

Merge branch 'main' into feat-elastic-search-with-text

e607e06

test: remove list test

5b41462

Merge branch 'feat-elastic-search-with-text' of https://github.com/ji…

a079a22

…na-ai/docarray into feat-elastic-search-with-text

test: remove list test

062a71d

fix: adapt text indexing with batch upload in elastic

5d16ead

alaeddine-13 reviewed Apr 12, 2022

View reviewed changes

davidbp added 5 commits April 13, 2022 13:03

refactor: remove find from elastic

d0fa624

fix: corner case query_0 does not exist

06d0e76

refactor: comment

9a42a21

fix: remove breakpoint

40d5835

fix: handle query singletons

6fdb06c

alaeddine-13 reviewed Apr 13, 2022

View reviewed changes

Comment thread docarray/array/mixins/find.py Outdated

Comment thread docarray/array/mixins/find.py Outdated

Comment thread tests/unit/array/mixins/test_find.py Outdated

refactor: add description field

3b85c13

davidbp and others added 3 commits April 13, 2022 16:17

docs: add minimal examples

2131b9b

refactor: separate find by text in its own method

2b7642d

Merge branch 'feat-elastic-search-with-text' of https://github.com/ji…

77a835f

…na-ai/docarray into feat-elastic-search-with-text

JoanFM previously requested changes Apr 13, 2022

View reviewed changes

fix: merge conflict

6321280

github-actions Bot added the area/docs label Apr 13, 2022

alaeddine-13 marked this pull request as ready for review April 13, 2022 14:34

alaeddine-13 and others added 3 commits April 13, 2022 17:15

docs: improve docstring

6115642

docs: add note

b374874

docs: update tag search example

a7cf1cf

hanxiao reviewed Apr 14, 2022

View reviewed changes

Comment thread docs/advanced/document-store/elasticsearch.md Outdated

hanxiao reviewed Apr 14, 2022

View reviewed changes

Comment thread docs/advanced/document-store/elasticsearch.md Outdated

hanxiao reviewed Apr 14, 2022

View reviewed changes

Comment thread docs/advanced/document-store/elasticsearch.md Outdated

hanxiao reviewed Apr 14, 2022

View reviewed changes

Comment thread docs/advanced/document-store/elasticsearch.md Outdated

alaeddine-13 and others added 2 commits April 14, 2022 09:32

chore: apply suggestions

1e979ef

Co-authored-by: Han Xiao <han.xiao@jina.ai>

chore: apply suggestions

a614915

alaeddine-13 reviewed Apr 14, 2022

View reviewed changes

docs: fix typos

787421b

hanxiao approved these changes Apr 14, 2022

View reviewed changes

alaeddine-13 approved these changes Apr 14, 2022

View reviewed changes

alaeddine-13 merged commit d12710c into main Apr 14, 2022

alaeddine-13 deleted the feat-elastic-search-with-text branch April 14, 2022 09:00

	:param index: if the query is a string, text search will be performed on `index`, otherwise, ignored. By
	:param index: if the query is a string or list of string, text search will be performed on `index`, otherwise, ignored. By

Conversation

davidbp commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alaeddine-13 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

davidbp Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alaeddine-13 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

davidbp Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoanFM commented Apr 13, 2022

Uh oh!

JoanFM Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

davidbp Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoanFM Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

alaeddine-13 commented Apr 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidbp commented Apr 7, 2022 •

edited

Loading

codecov Bot commented Apr 7, 2022 •

edited

Loading

davidbp Apr 14, 2022 •

edited

Loading