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

fix(ranker): correct column names in ranker score matrix #1973

Merged
merged 2 commits into from Feb 18, 2021
Merged

fix(ranker): correct column names in ranker score matrix #1973

merged 2 commits into from Feb 18, 2021

Conversation

hanxiao
Copy link
Member

@hanxiao hanxiao commented Feb 18, 2021 •
edited by nan-wang

  • column naming was wrong
  • the usage of np.object is deprecated
  • breaking changes introduced, hub-side PR is here:

TODO:

@hanxiao hanxiao requested a review from a team as a code owner February 18, 2021 10:45
@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/driver component/executor executor/ranker labels Feb 18, 2021
@hanxiao hanxiao closed this Feb 18, 2021
@hanxiao hanxiao reopened this Feb 18, 2021
@codecov
Copy link

codecov bot commented Feb 18, 2021 •
edited

Codecov Report

Merging #1973 (9fe8749) into master (9cea5e7) will increase coverage by 3.88%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1973      +/-   ##
==========================================
+ Coverage   82.71%   86.60%   +3.88%     
==========================================
  Files         148      148              
  Lines        7106     7107       +1     
==========================================
+ Hits         5878     6155     +277     
+ Misses       1228      952     -276
Impacted Files Coverage Δ
jina/executors/rankers/__init__.py 92.50% <100.00%> (+0.19%) ⬆️
jina/drivers/control.py 95.08% <0.00%> (+1.63%) ⬆️
jina/clients/base.py 80.83% <0.00%> (+1.66%) ⬆️
jina/peapods/runtimes/jinad/client.py 82.48% <0.00%> (+2.18%) ⬆️
jina/drivers/evaluate.py 98.57% <0.00%> (+2.85%) ⬆️
jina/drivers/encode.py 94.20% <0.00%> (+2.89%) ⬆️
jina/drivers/convertdriver.py 93.93% <0.00%> (+3.03%) ⬆️
jina/flow/mixin/crud.py 89.28% <0.00%> (+3.57%) ⬆️
jina/peapods/runtimes/zmq/zed.py 91.42% <0.00%> (+3.57%) ⬆️
jina/peapods/runtimes/jinad/__init__.py 95.83% <0.00%> (+4.16%) ⬆️
... and 18 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 9cea5e7...a4eeffb. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Feb 18, 2021 •
edited

Latency summary

Current PR yields:

  • 😶 index QPS at 1026, delta to last 3 avg.: +0%
  • 🐢🐢 query QPS at 15, delta to last 3 avg.: -7%

Breakdown

Version Index QPS Query QPS
current 1026 15
1.0.1 1009 15
1.0.0 1032 17

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

CatStark
CatStark reviewed Feb 18, 2021
View reviewed changes
jina/executors/rankers/__init__.py Outdated
@@ -31,18 +33,10 @@ class Chunk2DocRanker(BaseRanker):
"""

required_keys = {'text'} #: a set of ``str``, key-values to extracted from the chunk-level protobuf message
"""set: Set of required keys to be extracted from matches and query to fill the information of `query` and `chunk` meta information.
Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove the documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, copy paste glitch

Yongxuanzhang
Yongxuanzhang approved these changes Feb 18, 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.

LGTM！

@CatStark CatStark merged commit c379b09 into master Feb 18, 2021
@CatStark CatStark deleted the fix-ranker-colnames branch February 18, 2021 15:25
@Yongxuanzhang Yongxuanzhang mentioned this pull request Feb 20, 2021
Closed
4 tasks
@Yongxuanzhang
Copy link
Contributor

Yongxuanzhang commented Feb 21, 2021 •
edited

  • column naming was wrong
  • the usage of np.object is deprecated
  • breaking changes introduced, hub-side PR is here:

TODO:

I will fix the tests @hanxiao

@lusloher lusloher mentioned this pull request Mar 1, 2021
Closed
@nan-wang nan-wang mentioned this pull request Mar 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CatStark CatStark CatStark approved these changes

@Yongxuanzhang Yongxuanzhang Yongxuanzhang approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@hanxiao @Yongxuanzhang @CatStark @jina-bot