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.

Sign up for GitHub

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

Jump to bottom

test: add tests for sparsity #2316

Merged
merged 3 commits into from
Apr 19, 2021
Merged

test: add tests for sparsity #2316

merged 3 commits into from
Apr 19, 2021

Conversation

JoanFM
Copy link
Member

@JoanFM JoanFM commented Apr 19, 2021

No description provided.

@JoanFM JoanFM requested a review from a team as a code owner April 19, 2021 12:47
@jina-bot jina-bot added size/M area/testing This issue/PR affects testing labels Apr 19, 2021
@codecov
Copy link

codecov bot commented Apr 19, 2021 •
edited

Codecov Report

Merging #2316 (2150561) into master (03e72b1) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 2150561 differs from pull request most recent head 1955ece. Consider uploading reports for the commit 1955ece to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2316      +/-   ##
==========================================
+ Coverage   90.91%   90.94%   +0.02%     
==========================================
  Files         222      222              
  Lines       11792    11792              
==========================================
+ Hits        10721    10724       +3     
+ Misses       1071     1068       -3
Flag Coverage Δ
daemon 51.05% <0.00%> (ø)
jina 91.11% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
jina/drivers/search.py 98.76% <100.00%> (+0.01%) ⬆️
jina/types/sets/document.py 95.32% <100.00%> (+1.71%) ⬆️

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 03e72b1...1955ece. Read the comment docs.

cristianmtr
cristianmtr previously approved these changes Apr 19, 2021
View reviewed changes
@Yongxuanzhang
Copy link
Contributor

There are errors in https://github.com/jina-ai/jina/pull/2316/checks?check_run_id=2380856819

Yongxuanzhang
Yongxuanzhang reviewed Apr 19, 2021
View reviewed changes
Copy link
Contributor

@Yongxuanzhang Yongxuanzhang left a comment

Choose a reason for hiding this comment

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

tests/unit/drivers/test_vector_index_driver.py::test_vector_index_driver_add_bad_docs[tf] FAILED

@github-actions
Copy link

github-actions bot commented Apr 19, 2021 •
edited

Latency summary

Current PR yields:

  • 😶 index QPS at 978, delta to last 3 avg.: -4%
  • 🐢🐢 query QPS at 14, delta to last 3 avg.: -7%

Breakdown

Version Index QPS Query QPS
current 978 14
1.1.6 1026 15
1.1.5 1019 15

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

@jina-bot jina-bot added area/core This issue/PR affects the core codebase component/type labels Apr 19, 2021
Yongxuanzhang
Yongxuanzhang reviewed Apr 19, 2021
View reviewed changes
jina/drivers/search.py Outdated
@@ -139,6 +139,8 @@ def _get_documents_embeddings(self, docs: 'DocumentSet'):
scipy_cls_type = embedding_cls_type.split('_')[1]
embedding_cls_type = 'scipy'

print(f'scipy_cls_type {scipy_cls_type} ')
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use print or logger?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, my bad

@JoanFM JoanFM requested a review from davidbp April 19, 2021 16:17
Yongxuanzhang
Yongxuanzhang reviewed Apr 19, 2021
View reviewed changes
jina/drivers/search.py
@@ -155,12 +155,12 @@ def _fill_matches(self, doc, op_name, topks, scores, topk_embed):
else:
for idx, (numpy_match_id, score) in enumerate(zip(topks, scores)):
vector = None
if topk_embed is not None:
if topk_embed[idx] is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this could work if topk_embed is none?
Will it get this?
TypeError: 'NoneType' object is not subscriptable

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems not to be the case, as it is not considerrd in the case with dense embeddings

Copy link
Contributor

Choose a reason for hiding this comment

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

So this won't happen? topk_embed won't be None but the elements could be None

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, see the caller

Yongxuanzhang
Yongxuanzhang approved these changes Apr 19, 2021
View reviewed changes
Copy link
Contributor

@Yongxuanzhang Yongxuanzhang left a comment •
edited

Choose a reason for hiding this comment

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

LGTM!

@JoanFM JoanFM merged commit 29a9c02 into master Apr 19, 2021
@JoanFM JoanFM deleted the test_index_driver branch April 19, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yongxuanzhang Yongxuanzhang Yongxuanzhang approved these changes

@jacobowitz jacobowitz Awaiting requested review from jacobowitz

@slettner slettner Awaiting requested review from slettner

@cristianmtr cristianmtr Awaiting requested review from cristianmtr

@davidbp davidbp Awaiting requested review from davidbp

Assignees
No one assigned
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/driver component/type size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JoanFM @Yongxuanzhang @cristianmtr @jina-bot