8000 refactor: split segmenter into its own base class by JoanFM · Pull Request #1632 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: split segmenter into its own base class #1632

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
Merged
merged 8 commits into from
Jan 11, 2021
Merged

refactor: split segmenter into its own base class #1632

merged 8 commits into from
Jan 11, 2021

Conversation

JoanFM
Copy link
Contributor
@JoanFM JoanFM commented Jan 8, 2021 •
edited by nan-wang
Loading

Breaking Change #1606

Linked Hub PR jina-ai/jina-hub#2033

@JoanFM JoanFM requested a review from a team as a code owner January 8, 2021 17:53
@jina-bot jina-bot added size/M area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/driver component/executor executor/crafter labels Jan 8, 2021
@JoanFM JoanFM mentioned this pull request Jan 8, 2021
Closed
@github-actions
Copy link
github-actions bot commented Jan 8, 2021 •
edited
Loading

Latency summary

Current PR yields:

  • 😶 index QPS at 1688, delta to last 3 avg.: +0%
  • 😶 query QPS at 31, delta to last 3 avg.: -5%

Breakdown

Version Index QPS Query QPS
current 1688 31
0.9.3 1670 32
0.9.2 1733 32

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

@nan-wang
Copy link
Member
nan-wang commented Jan 9, 2021 •
edited
Loading

part of issue #1618

@codecov
Copy link
codecov bot commented Jan 9, 2021 •
edited
Loading

Codecov Report

Merging #1632 (2e24c3c) into master (921cba3) will increase coverage by 0.11%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1632      +/-   ##
==========================================
+ Coverage   84.66%   84.78%   +0.11%     
==========================================
  Files         128      130       +2     
  Lines        6730     6749      +19     
==========================================
+ Hits         5698     5722      +24     
+ Misses       1032     1027       -5
Flag Coverage Δ
cd ?
ci 84.78% <97.14%> (+40.17%) ⬆️
core 84.78% <97.14%> (+0.80%) ⬆️
integration 42.08% <57.14%> (+0.10%) ⬆️
jinad 44.70% <57.14%> (+0.09%) ⬆️
unit 40.68% <57.14%> (+0.10%) ⬆️

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

Impacted Files Coverage Δ
jina/executors/segmenters/__init__.py 91.66% <91.66%> (ø)
jina/drivers/craft.py 100.00% <100.00%> (ø)
jina/drivers/segment.py 100.00% <100.00%> (ø)
jina/executors/crafters/__init__.py 100.00% <100.00%> (+5.88%) ⬆️
jina/importer.py 67.24% <0.00%> (+2.15%) ⬆️

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 aab258b...2e24c3c. Read the comment docs.

@JoanFM JoanFM force-pushed the refactor-split-segmenter branch from 76517b4 to 0b1f7c2 Compare January 9, 2021 09:17
@JoanFM JoanFM requested a review from nan-wang January 9, 2021 09:18
@JoanFM JoanFM closed this Jan 9, 2021
@JoanFM JoanFM reopened this Jan 9, 2021
@hanxiao
Copy link
Member
hanxiao commented Jan 9, 2021 •
edited
Loading

part of issue #1618

@nan-wang can you use this function to label PR?

image

@JoanFM JoanFM requested a review from hanxiao January 10, 2021 15:56
@nan-wang nan-wang added this to the v1.0.0 Breaking Changes milestone Jan 11, 2021
@nan-wang
Copy link
Member
nan-wang commented Jan 11, 2021 •
edited
Loading

part of issue #1618

@nan-wang can you use this function to label PR?

image

I'm not using the linking function because this will close the issue automatically when the PR is merged. However, issue #1618 contains multiple parts. @hanxiao

hanxiao
hanxiao reviewed Jan 11, 2021
View reviewed changes
tests/integration/issues/github_1546/good3/__init__.py
@@ -25,7 +25,7 @@ def __init__(self,
self.target_size = target_size
self.channel_axis = channel_axis

def craft(self, blob: 'np.ndarray', *args, **kwargs) -> List[Dict]:
def segment(self, blob: 'np.ndarray', *args, **kwargs) -> List[Dict]:
Copy link
Member

Choose a reason for hiding this comment

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

i dont see the need of changing github_1546 test case, but changing also does no harm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was failing the test, I think is because Segmenter does not have craft anymore as method

@hanxiao hanxiao merged commit 3d4c2bb into master Jan 11, 2021
@hanxiao hanxiao deleted the refactor-split-segmenter branch January 11, 2021 13:46
@nan-wang nan-wang mentioned this pull request Feb 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanxiao hanxiao hanxiao approved these changes

@Roshanjossey Roshanjossey Awaiting requested review from Roshanjossey

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

@nan-wang nan-wang Awaiting requested review from nan-wang

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/crafter executor/meta size/M
Projects
None yet
Milestone
v1.0.0 Breaking Changes
Development

Successfully merging this pull request may close these issues.
4 participants
@JoanFM @nan-wang @hanxiao @jina-bot
0