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

crud intergration tests: docidcache-centric #1613

Merged
merged 13 commits into from Jan 15, 2021
Merged

Conversation

cristianmtr
Copy link
Contributor

@cristianmtr cristianmtr commented Jan 6, 2021

Main integration tests for CRUD, focusing on using the new DocIDCache functionality

  • Document content_hash incorrectly includes child chunks
  • docs generator and testing it
  • basic structure of tests & parameters
  • UniquePbIndexer and UniqueVectorIndexer do not support delete and update
  • issue with handling missing keys
  • DocIDCache uses savepath / filename differently than most Indexers
  • adding Delete and Update to CacheDriver

@jina-bot jina-bot added size/M area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/executor component/type executor/indexer labels Jan 6, 2021
@github-actions
Copy link

github-actions bot commented Jan 6, 2021

Latency summary

Current PR yields:

  • 😶 index QPS at 1962, delta to last 3 avg.: +3%
  • 😶 query QPS at 36, delta to last 3 avg.: -4%

Breakdown

Version Index QPS Query QPS
current 1962 36
0.9.13 1829 37
0.9.12 1956 37

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

@JoanFM
Copy link
Member

JoanFM commented Jan 6, 2021

Now that I think, DocIDCache does not seem to be a proper name for a class that can cache based on content hash. I think this needs to be renamed DocCache.

@cristianmtr
Copy link
Contributor Author

Now that I think, DocIDCache does not seem to be a proper name for a class that can cache based on content hash. I think this needs to be renamed DocCache.

I'll open a PR to refactor it everywhere and then will rebase

jina/executors/indexers/cache.py Show resolved Hide resolved
jina/types/document/uid.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
@cristianmtr cristianmtr force-pushed the test-cache-crud-tests branch 4 times, most recently from 4295a92 to 3208f86 Compare January 7, 2021 15:05
@jina-bot jina-bot added the area/helper This issue/PR affects the helper functionality label Jan 7, 2021
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 11, 2021

Codecov Report

Merging #1613 (9285166) into master (fa47adb) will decrease coverage by 0.20%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1613      +/-   ##
==========================================
- Coverage   85.03%   84.82%   -0.21%     
==========================================
  Files         133      133              
  Lines        6822     6828       +6     
==========================================
- Hits         5801     5792       -9     
- Misses       1021     1036      +15     
Impacted Files Coverage Δ
jina/executors/indexers/keyvalue.py 98.68% <66.66%> (-1.32%) ⬇️
jina/drivers/cache.py 94.11% <100.00%> (+0.78%) ⬆️
jina/drivers/search.py 96.49% <100.00%> (+1.66%) ⬆️
jina/executors/indexers/cache.py 97.18% <100.00%> (+0.04%) ⬆️
jina/executors/indexers/vector.py 93.12% <100.00%> (ø)
jina/peapods/runtimes/jinad/api.py 75.65% <0.00%> (-11.19%) ⬇️
jina/flow/base.py 87.56% <0.00%> (+0.55%) ⬆️

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 fa47adb...9285166. Read the comment docs.

@JoanFM
Copy link
Member

JoanFM commented Jan 13, 2021

Problem seems to be that update is not clearing properly somehow blob shape. I think it may be more of a primitive types fault than the Update method itself, but it may have exposed the issue and it needs to be solved to continue

@nan-wang nan-wang added this to the v1.0.0 New Features milestone Jan 14, 2021
@cristianmtr cristianmtr force-pushed the test-cache-crud-tests branch 3 times, most recently from b3cde76 to f79a765 Compare January 14, 2021 17:24
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 a minor thing,

However, I still think we should think how to better test for the index size without having such a complex code for testing

tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
tests/integration/docidcache/test_crud_cache.py Outdated Show resolved Hide resolved
@cristianmtr
Copy link
Contributor Author

cristianmtr commented Jan 15, 2021

Just a minor thing,

However, I still think we should think how to better test for the index size without having such a complex code for testing

I agree that the engineering of the tests shouldn't be so complex.

But on the other hand, as an advantage, we can see more clearly how changing some parameters in the cache would affect the size / output of the indexers. Having them as separate tests, with hard-coded apparently magic numbers, would have made the behavior more opaque.

@cristianmtr cristianmtr merged commit 90b779d into master Jan 15, 2021
@cristianmtr cristianmtr deleted the test-cache-crud-tests branch January 15, 2021 09:54
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/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing component/driver component/executor component/resource component/type executor/indexer size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants