refactor: clarify annotation types to support sparse #2296

JoanFM · 2021-04-09T11:30:16Z

No description provided.

codecov · 2021-04-09T11:34:02Z

Codecov Report

Merging #2296 (d5b1bd1) into master (1825529) will increase coverage by 0.01%.
The diff coverage is 98.76%.

@@            Coverage Diff             @@
##           master    #2296      +/-   ##
==========================================
+ Coverage   90.88%   90.89%   +0.01%     
==========================================
  Files         222      222              
  Lines       11757    11789      +32     
==========================================
+ Hits        10685    10716      +31     
- Misses       1072     1073       +1

Flag	Coverage Δ
daemon	`51.00% <22.22%> (-0.21%)`	⬇️
jina	`91.06% <98.76%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
jina/types/sets/document.py	`93.52% <96.29%> (+0.24%)`	⬆️
jina/drivers/index.py	`97.36% <100.00%> (+0.39%)`	⬆️
jina/drivers/search.py	`98.75% <100.00%> (+0.13%)`	⬆️
jina/executors/encoders/__init__.py	`100.00% <100.00%> (ø)`
jina/executors/indexers/__init__.py	`97.89% <100.00%> (ø)`
jina/types/document/__init__.py	`96.37% <100.00%> (-0.01%)`	⬇️
jina/types/ndarray/generic.py	`100.00% <100.00%> (ø)`

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 1825529...d5b1bd1. Read the comment docs.

github-actions · 2021-04-09T11:44:02Z

Latency summary

Current PR yields:

😶 index QPS at 933, delta to last 3 avg.: -1%
🐢🐢 query QPS at 13, delta to last 3 avg.: -6%

Breakdown

Version	Index QPS	Query QPS
current	933	13
`1.1.5`	955	14
`1.1.4`	945	13

Backed by latency-tracking. Further commits will update this comment.

…notations-update

cristianmtr · 2021-04-15T11:06:55Z

jina/drivers/index.py

@@ -39,8 +39,7 @@ class VectorIndexDriver(BaseIndexDriver):
    If `method` is not 'delete', documents without content are filtered out.
    """

-    @staticmethod
-    def _get_documents_embeddings(docs: 'DocumentSet'):
+    def _get_documents_embeddings(self, docs: 'DocumentSet'):


Why not static anymore?

because the inherited function needs instance attributes

cristianmtr · 2021-04-15T11:08:19Z

jina/drivers/index.py

+    def exec_sparse_cls_type(self) -> str:
+        return self.exec.sparse_cls_type
+
+    def _get_documents_embeddings(self, docs: 'DocumentSet'):


But should the driver be storing this spare class type? Maybe it should still be static and just pass the type as another argument? I don't see it as being part of the Driver's expected knowledge

well the problem is then the signature changes between parent and child

default argument being dense representation class?

It would not be used in the Dense case. It is handy to access dense embbedding via a property

jina/drivers/search.py

jina/types/sets/document.py

cristianmtr · 2021-04-15T11:21:01Z

tests/unit/drivers/test_vector_index_driver.py

@@ -11,6 +11,9 @@
 from jina.types.document import Document


+# TODO: Add tests for sparse vectors index driver.


just a reminder

@bwanglzu I have done changes to avoid having extra Driver classes, so that it is easier for the user to use.

Still the VectorIndexDriver and VectorSearchDriver need to be unit tested

cristianmtr · 2021-04-15T11:21:05Z

tests/unit/drivers/test_vector_search_driver.py

@@ -7,6 +7,7 @@
 from jina.drivers.search import VectorSearchDriver
 from jina.executors.indexers import BaseVectorIndexer

+# TODO: Add tests for sparse vectors search driver.


just a reminder

@bwanglzu I have done changes to avoid having extra Driver classes, so that it is easier for the user to use.

Still the VectorIndexDriver and VectorSearchDriver need to be unit tested

JoanFM · 2021-04-16T06:36:50Z

jina/drivers/search.py

+    def exec_sparse_cls_type(self) -> str:
+        """Get the sparse type from executor.
+
+        :return: Sparse matrix type, default value is `scipy_coo`.


do not put this note here as it is hard to mantain. The default value will be in the base class of executor, not here

JoanFM · 2021-04-16T06:38:33Z

jina/executors/indexers/__init__.py

@@ -270,7 +289,7 @@ def add(self, keys: Iterable[str], vectors: 'np.ndarray', *args, **kwargs) -> No
        raise NotImplementedError

    def query(
-        self, vectors: 'np.ndarray', top_k: int, *args, **kwargs
+        self, vectors: 'EncodingType', top_k: int, *args, **kwargs


would it be more clear if we creatr anotjher type var saying EncodeBatchType that is just an alias?

…notations-update

bwanglzu

LGTM!

refactor: clarify annotation types to support sparse (jina-ai#2296)

refactor: clarify annotation types to support sparse

450d204

JoanFM requested a review from a team as a code owner April 9, 2021 11:30

JoanFM requested review from Roshanjossey and imsergiy April 9, 2021 11:30

jina-bot added size/S area/core This issue/PR affects the core codebase component/executor component/type executor/encoder executor/indexer labels Apr 9, 2021

Merge branch 'master' of https://github.com/jina-ai/jina into type-an…

cc2e563

…notations-update

jina-bot added size/M and removed size/S labels Apr 15, 2021

JoanFM force-pushed the type-annotations-update branch from c8f6089 to 73fd095 Compare April 15, 2021 07:56

jina-bot added the area/testing This issue/PR affects testing label Apr 15, 2021

JoanFM force-pushed the type-annotations-update branch 2 times, most recently from 4497b66 to 2e4ebab Compare April 15, 2021 08:47

jina-bot added the component/driver label Apr 15, 2021

feat: proposal to generalize to multiple sparse types

8e20514

JoanFM force-pushed the type-annotations-update branch from 2e4ebab to 8e20514 Compare April 15, 2021 08:48

cristianmtr reviewed Apr 15, 2021

View reviewed changes

jina/drivers/search.py Outdated Show resolved Hide resolved

cristianmtr reviewed Apr 15, 2021

View reviewed changes

jina/types/sets/document.py Outdated Show resolved Hide resolved

cristianmtr reviewed Apr 15, 2021

View reviewed changes

bwanglzu added 2 commits April 16, 2021 03:58

feat: add sparse cls type property

2cea245

feat: clean up search deriver

69dc079

bwanglzu added 7 commits April 16, 2021 04:08

feat: clean up search deriver

a84f391

feat: add docstring and hints for all sparse emebdfdings

67d4dfb

feat: add docstring and hints for all sparse emebdfdings

1fb6a59

feat: apply black format

c7a4e30

feat: fix docstring and format

adc4905

feat: fix docstring and format

64db57f

feat: fix docstring and format

124bdc3

JoanFM commented Apr 16, 2021

View reviewed changes

bwanglzu added 2 commits April 16, 2021 09:57

feat: fix docstring and format

f5a96a5

feat: fix docstring and format

d8714c1

JoanFM closed this Apr 16, 2021

JoanFM reopened this Apr 16, 2021

JoanFM force-pushed the type-annotations-update branch from e3c31b6 to 24b1ddc Compare April 17, 2021 09:44

feat: avoid having extra classes of drivers and executors

40454d1

JoanFM force-pushed the type-annotations-update branch from 24b1ddc to 40454d1 Compare April 17, 2021 10:07

Merge branch 'master' of https://github.com/jina-ai/jina into type-an…

d5b1bd1

…notations-update

bwanglzu approved these changes Apr 17, 2021

View reviewed changes

JoanFM merged commit 399728f into master Apr 17, 2021

JoanFM deleted the type-annotations-update branch April 17, 2021 14:51

sthagen added a commit to sthagen/jina-ai-jina that referenced this pull request Apr 17, 2021

Merge pull request #8 from jina-ai/master

ae065b6

refactor: clarify annotation types to support sparse (jina-ai#2296)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: clarify annotation types to support sparse #2296

refactor: clarify annotation types to support sparse #2296

JoanFM commented Apr 9, 2021

codecov bot commented Apr 9, 2021 •

edited

github-actions bot commented Apr 9, 2021 •

edited

cristianmtr Apr 15, 2021

JoanFM Apr 15, 2021

cristianmtr Apr 15, 2021

cristianmtr Apr 15, 2021

JoanFM Apr 15, 2021 •

edited

cristianmtr Apr 15, 2021

JoanFM Apr 15, 2021

cristianmtr Apr 15, 2021

JoanFM Apr 17, 2021

cristianmtr Apr 15, 2021

JoanFM Apr 17, 2021

JoanFM Apr 16, 2021

JoanFM Apr 16, 2021

bwanglzu left a comment

		@@ -11,6 +11,9 @@
		from jina.types.document import Document


		# TODO: Add tests for sparse vectors index driver.

refactor: clarify annotation types to support sparse #2296

refactor: clarify annotation types to support sparse #2296

Conversation

JoanFM commented Apr 9, 2021

codecov bot commented Apr 9, 2021 • edited

Codecov Report

github-actions bot commented Apr 9, 2021 • edited

Latency summary

Breakdown

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanFM Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwanglzu left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 9, 2021 •

edited

github-actions bot commented Apr 9, 2021 •

edited

JoanFM Apr 15, 2021 •

edited