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: add _merge_matches and _merge_chunks syntax sugar #1406

Merged
merged 3 commits into from Dec 7, 2020
Merged

fix: add _merge_matches and _merge_chunks syntax sugar #1406

merged 3 commits into from Dec 7, 2020

Conversation

JoanFM
Copy link
Member

@JoanFM JoanFM commented Dec 7, 2020

Fixes #1404

@JoanFM JoanFM requested a review from a team as a code owner December 7, 2020 11:52
@jina-bot jina-bot added size/XS area/core This issue/PR affects the core codebase component/driver labels Dec 7, 2020
@JoanFM JoanFM mentioned this pull request Dec 7, 2020
Closed
cristianmtr
cristianmtr suggested changes Dec 7, 2020
View reviewed changes
jina/drivers/reduce.py Outdated Show resolved Hide resolved
@JoanFM JoanFM changed the title fix: reduce all should work on matches by default fix: add _merge_matches_ executor syntax sugar Dec 7, 2020
@jina-bot jina-bot added size/S and removed size/XS labels Dec 7, 2020
@codecov
Copy link

codecov bot commented Dec 7, 2020 •
edited

Codecov Report

Merging #1406 (6dbf4ac) into master (b2a8f23) will decrease coverage by 1.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1406      +/-   ##
==========================================
- Coverage   83.46%   81.99%   -1.48%     
==========================================
  Files         104      106       +2     
  Lines        6860     7042     +182     
==========================================
+ Hits         5726     5774      +48     
- Misses       1134     1268     +134
Impacted Files Coverage Δ
jina/peapods/grpc_asyncio.py 0.00% <0.00%> (-80.62%) ⬇️
jina/types/sets/chunk_set.py 91.66% <0.00%> (-8.34%) ⬇️
jina/docker/hubio.py 63.15% <0.00%> (-7.76%) ⬇️
jina/docker/hubapi.py 70.31% <0.00%> (-5.47%) ⬇️
jina/types/sets/match_set.py 95.83% <0.00%> (-4.17%) ⬇️
jina/types/ndarray/dense/numpy.py 97.36% <0.00%> (-2.64%) ⬇️
jina/logging/sse.py 92.64% <0.00%> (-2.52%) ⬇️
jina/peapods/container.py 89.36% <0.00%> (-0.97%) ⬇️
jina/logging/profile.py 55.81% <0.00%> (-0.66%) ⬇️
jina/types/document/__init__.py 92.33% <0.00%> (-0.23%) ⬇️
... and 33 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 b2a8f23...6dbf4ac. Read the comment docs.

cristianmtr
cristianmtr previously approved these changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@cristianmtr cristianmtr left a comment

Choose a reason for hiding this comment

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

Would a quick test be needed?

@JoanFM
Copy link
Member Author

JoanFM commented Dec 7, 2020

Would a quick test be needed?

Added some changes in some tests to validate

@jina-bot jina-bot added the area/testing This issue/PR affects testing label Dec 7, 2020
@JoanFM JoanFM changed the title fix: add _merge_matches_ executor syntax sugar fix: add _merge_matches and _merge_chunks executor syntax sugar Dec 7, 2020
@JoanFM JoanFM changed the title fix: add _merge_matches and _merge_chunks executor syntax sugar fix: add _merge_matches and _merge_chunks syntax sugar Dec 7, 2020
maximilianwerk
maximilianwerk approved these changes Dec 7, 2020
View reviewed changes
Copy link
Member

@maximilianwerk maximilianwerk left a comment

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 f3165af into master Dec 7, 2020
@JoanFM JoanFM deleted the fix-reduce-all-driver branch December 7, 2020 21:32
@JoanFM
Copy link
Member Author

JoanFM commented Dec 8, 2020

@nan-wang does this deserve a place in #1337 ?

@nan-wang
Copy link
Member

nan-wang commented Dec 9, 2020

@nan-wang does this deserve a place in #1337 ?

@JoanFM I saw _merge_all is still there. Do we plan to remove it?

@JoanFM
Copy link
Member Author

JoanFM commented Dec 9, 2020

@nan-wang does this deserve a place in #1337 ?

@JoanFM I saw _merge_all is still there. Do we plan to remove it?

I would eventually remove

@JoanFM JoanFM mentioned this pull request Dec 9, 2020
Closed
@nan-wang
Copy link
Member

Gotcha, then it is not a breaking change for the time being

@nan-wang nan-wang added this to the v0.9 Breaking Changes milestone Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximilianwerk maximilianwerk maximilianwerk approved these changes

@cristianmtr cristianmtr cristianmtr left review comments

@CatStark CatStark Awaiting requested review from CatStark

@florian-hoenicke florian-hoenicke Awaiting requested review from florian-hoenicke

Assignees
No one assigned
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/driver component/resource size/S
Projects
None yet
Milestone
v0.9 Breaking Changes
Development

Successfully merging this pull request may close these issues.

top_k is not working on multiple shards
5 participants
@JoanFM @nan-wang @maximilianwerk @cristianmtr @jina-bot