8000 feat: connect to pred config by maximilianwerk · Pull Request #3051 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: connect to pred config #3051

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
Aug 3, 2021
Merged

feat: connect to pred config #3051

merged 8 commits into from
Aug 3, 2021

Conversation

maximilianwerk
Copy link
Member

This allows to overwrite our remote-local detection magic explicitly for special use-cases (e.g. running on multiple instances in a VPN and additionally having one remote Pod).

@maximilianwerk maximilianwerk requested a review from a team as a code owner July 30, 2021 08:52
@maximilianwerk maximilianwerk requested review from jacobowitz and cristianmtr July 30, 2021 08:52 8000
@github-actions github-actions bot added size/S area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/flow component/peapod labels Jul 30, 2021
@github-actions
Copy link
github-actions bot commented Jul 30, 2021 •
edited
Loading

Latency summary

Current PR yields:

  • 😶 index QPS at 1396, delta to last 2 avg.: +3%
  • 😶 query QPS at 33, delta to last 2 avg.: +2%
  • 😶 dam extend QPS at 60911, delta to last 2 avg.: +8%
  • 😶 avg flow time within 1.8403 seconds, delta to last 2 avg.: +0%
  • 🐢🐢 import jina within 0.3478 seconds, delta to last 2 avg.: -6%

Breakdown

Version Index QPS Query QPS DAM Extend QPS Avg Flow Time (s) Import Time (s)
current 1396 33 60911 1.8403 0.3478
2.0.15 1282 30 54987 1.8753 0.3822
2.0.14 1411 33 57077 1.8262 0.3653

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

@maximilianwerk maximilianwerk force-pushed the feat-add-connect-pre-config branch 2 times, most recently from 6e61547 to 9a22128 Compare July 30, 2021 13:38
@github-actions github-actions bot added the area/cli This issue/PR affects the command line interface label Jul 30, 2021
@codecov
Copy link
codecov bot commented Jul 30, 2021 •
edited
Loading

Codecov Report

Merging #3051 (773bbce) into master (27d93c2) will increase coverage by 3.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3051      +/-   ##
==========================================
+ Coverage  
8000
 86.21%   89.35%   +3.14%     
==========================================
  Files         142      142              
  Lines        9503     9509       +6     
==========================================
+ Hits         8193     8497     +304     
+ Misses       1310     1012     -298
Flag Coverage Δ
daemon 43.79% <100.00%> (+0.03%) ⬆️
jina 89.34% <100.00%> (+3.14%) ⬆️

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

Impacted Files Coverage Δ
jina/parsers/peapods/runtimes/zed.py 100.00% <ø> (ø)
jina/flow/base.py 89.02% <100.00%> (ø)
jina/parsers/__init__.py 97.80% <100.00%> (+0.04%) ⬆️
jina/parsers/peapods/pod.py 100.00% <100.00%> (ø)
jina/peapods/pods/__init__.py 86.62% <100.00%> (+0.12%) ⬆️
jina/helper.py 79.92% <0.00%> (+0.37%) ⬆️
jina/jaml/parsers/__init__.py 100.00% <0.00%> (+7.31%) ⬆️
jina/optimizers/discovery.py 80.26% <0.00%> (+80.26%) ⬆️
jina/optimizers/__init__.py 97.02% <0.00%> (+97.02%) ⬆️
jina/optimizers/flow_runner.py 97.67% <0.00%> (+97.67%) ⬆️
... and 3 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 27d93c2...773bbce. Read the comment docs.

JoanFM
JoanFM reviewed Aug 2, 2021
View reviewed changes
Copy link
Contributor
@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.

When we were trying to have remote external Pods it was very hard to understand what are the needed arguments related to dynamic routing. I am not sure if this adds yet more noise to this.

In which context have we encountered this need?

@maximilianwerk maximilianwerk force-pushed the feat-add-connect-pre-config branch from 4c92c8f to ea77d89 Compare August 2, 2021 08:06
JoanFM
JoanFM suggested changes Aug 2, 2021
View reviewed changes
JoanFM
JoanFM reviewed Aug 2, 2021
View reviewed changes
@maximilianwerk
Copy link
Member Author

When we were trying to have remote external Pods it was very hard to understand what are the needed arguments related to dynamic routing. I am not sure if this adds yet more noise to this.

In which context have we encountered this need?

We are preparing a Cookbook section for explaining how to setup hosts correctly. This PR is a pre-step for it in order to remove make the setup easier with this Cookbook.

@maximilianwerk maximilianwerk force-pushed the feat-add-connect-pre-config branch from 2e8a2bb to 77b3fab Compare August 2, 2021 08:17
@maximilianwerk maximilianwerk requested a review from JoanFM August 2, 2021 13:53
@maximilianwerk
Copy link
Member Author

@JoanFM the cookbook will be changed in #3066

@maximilianwerk maximilianwerk merged commit 3ff5fe9 into master Aug 3, 2021
@maximilianwerk maximilianwerk deleted the feat-add-connect-pre-config branch August 3, 2021 13:24
davidbp pushed a commit that referenced this pull request Aug 4, 2021
@maximilianwerk @jina-bot @davidbp
feat: connect to pred config (#3051)
3ab64c0 
* feat: connect to pred config

* style: fix overload and cli autocomplete

* fix: use correct property

* fix: removed unnecessary argument

* style: fix overload and cli autocomplete

* fix: adopted test parameters

* fix: docker file for test

* fix: delete dynamic routing arg

Co-authored-by: Jina Dev Bot <dev-bot@jina.ai>
@hanxiao hanxiao mentioned this pull request Aug 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoanFM JoanFM JoanFM approved these changes

@jacobowitz jacobowitz Awaiting requested review from jacobowitz

@cristianmtr cristianmtr Awaiting requested review from cristianmtr

Assignees
No one assigned
Labels
area/cli This issue/PR affects the command line interface area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/flow component/peapod size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maximilianwerk @JoanFM @jina-bot
0