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

feat: index drivers update and delete #1460

Merged
merged 34 commits into from
Dec 30, 2020

Conversation

florian-hoenicke
Copy link
Member

Related ticket https://jinaai.atlassian.net/browse/JINACORE-552

  • add UpdateRequest and DeleteRequest
  • add update/delete driver for KVIndexer and VectorIndexer
  • when sharding, errors are not fixed but handled as we currently handle different indexers: include in response, but do not repair them (yet).
  • add tests for the drivers

@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase area/entrypoint This issue/PR affects the entrypoint codebase area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality component/proto labels Dec 14, 2020
jina/proto/jina.proto Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Dec 14, 2020

Latency summary

Current PR yields:

  • 😶 index QPS at 1682, delta to last 3 avg.: +1%
  • 😶 query QPS at 26, delta to last 3 avg.: +0%

Breakdown

Version Index QPS Query QPS
current 1682 26
0.8.16 1667 26
0.8.15 1633 26
0.8.14 1663 26

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

jina/enums.py Outdated Show resolved Hide resolved
Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

watch out! U are bringing changes to the hub submodule in this PR

@florian-hoenicke
Copy link
Member Author

I reverted the changes thanks.

@jina-bot jina-bot added size/L and removed size/M labels Dec 22, 2020
@florian-hoenicke florian-hoenicke marked this pull request as ready for review December 27, 2020 18:07
@florian-hoenicke florian-hoenicke requested a review from a team as a code owner December 27, 2020 18:07
@codecov
Copy link

codecov bot commented Dec 27, 2020

Codecov Report

Merging #1460 (ebd9bbb) into master (afbc44e) will increase coverage by 0.52%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1460      +/-   ##
==========================================
+ Coverage   84.26%   84.79%   +0.52%     
==========================================
  Files         108      108              
  Lines        6336     6417      +81     
==========================================
+ Hits         5339     5441     +102     
+ Misses        997      976      -21     
Impacted Files Coverage Δ
jina/clients/request.py 91.66% <100.00%> (+0.49%) ⬆️
jina/drivers/index.py 100.00% <100.00%> (+5.26%) ⬆️
jina/enums.py 96.57% <100.00%> (+0.14%) ⬆️
jina/executors/indexers/vector.py 88.94% <100.00%> (+0.23%) ⬆️
jina/proto/jina_pb2.py 100.00% <100.00%> (ø)
jina/logging/profile.py 68.33% <0.00%> (-0.58%) ⬇️
jina/drivers/craft.py 100.00% <0.00%> (ø)
jina/types/ndarray/generic.py 100.00% <0.00%> (ø)
jina/drivers/encode.py 94.91% <0.00%> (+0.08%) ⬆️
jina/jaml/__init__.py 95.93% <0.00%> (+0.09%) ⬆️
... and 17 more

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 afbc44e...ebd9bbb. Read the comment docs.

Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

missing unit tests for driver with delete and update method

jina/proto/jina.proto Outdated Show resolved Hide resolved
jina/proto/jina.proto Outdated Show resolved Hide resolved
jina/types/request/__init__.py Outdated Show resolved Hide resolved
jina/proto/jina.proto Outdated Show resolved Hide resolved
jina/proto/jina.proto Outdated Show resolved Hide resolved
Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Watch out there are changes in the hub

Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Just minor things! Did not notice we did not have test_vector_index_driver

@@ -18,6 +18,14 @@ def add(self, keys: Iterator[int], values: Iterator[bytes], *args, **kwargs):
for key, value in zip(keys, values):
self.docs[key] = value

def update(self, keys: Iterator[int], values: Iterator[bytes], *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

u can pass with these, I think these 2 methods won't be used

Copy link
Member Author

Choose a reason for hiding this comment

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

They are used here: SimpleKVIndexDriver(method='update')
However, I van remove the other methods: query, get_query_handler...

for key, value in zip(keys, vectors):
self.docs[key] = value

def update(self, keys: Iterator[int], values: Iterator[bytes], *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

are also these used?

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 in line 63

Copy link
Member

Choose a reason for hiding this comment

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

where?

Copy link
Member Author

@florian-hoenicke florian-hoenicke Dec 30, 2020

Choose a reason for hiding this comment

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

@pytest.fixture(scope='function')
def simple_kv_indexer_driver_update():
    return SimpleVectorIndexDriver(method='update')


@pytest.fixture(scope='function')
def simple_kv_indexer_driver_delete():
    return SimpleVectorIndexDriver(method='delete')

Copy link
Member

Choose a reason for hiding this comment

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

but this is MockGroundTruthVectorIndexer not SimpleVectorIndexDriver

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but the executor is attached to that driver and sets the _exec function accordingly.
simple_vector_indexer_driver_add.attach(executor=mock_groundtruth_indexer, pea=None)

@florian-hoenicke florian-hoenicke merged commit 3fe2145 into master Dec 30, 2020
@florian-hoenicke florian-hoenicke deleted the index-drivers-update-and-delete branch December 30, 2020 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/entrypoint This issue/PR affects the entrypoint codebase area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/client component/driver component/executor component/flow component/proto component/type executor/indexer executor/meta size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants