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

refactor(client): refactor code, style of pyclient #1450

Merged
merged 20 commits into from
Dec 13, 2020
Merged

refactor(client): refactor code, style of pyclient #1450

merged 20 commits into from
Dec 13, 2020

Conversation

hanxiao
Copy link
Member

@hanxiao hanxiao commented Dec 12, 2020 •
edited
Loading

  • Simplify the implementation of PyClient, GatewayPea.
  • Keep async, streaming, prefetch, jupyter-notebook, REST features support.

image

@hanxiao hanxiao requested a review from a team as a code owner December 12, 2020 13:04
@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase component/client labels Dec 12, 2020
@hanxiao hanxiao marked this pull request as draft December 12, 2020 13:05
@hanxiao hanxiao linked an issue Dec 12, 2020 that may be closed by this pull request
PyClientRunTime timeout is not enough (maybe needs to be configurable) #1449
Closed
@github-actions
Copy link

github-actions bot commented Dec 12, 2020 •
edited
Loading

Latency summary

Current PR yields:

  • 😶 index QPS at 1828, delta to last 3 avg.: +2%
  • 🐎🐎🐎🐎 query QPS at 36, delta to last 3 avg.: +12%

Breakdown

Version Index QPS Query QPS
current 1828 36
0.8.3 1763 31
0.8.2 1796 32
0.8.1 1808 31

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

@jina-bot jina-bot added size/M area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/flow component/peapod and removed size/S labels Dec 12, 2020
@jina-bot jina-bot added size/XL area/cli This issue/PR affects the command line interface area/helloworld This issue/PR affects the helloworld component/docker component/logging component/type and removed size/M labels Dec 12, 2020
deepankarm
deepankarm reviewed Dec 13, 2020
View reviewed changes
Copy link
Contributor

@deepankarm deepankarm left a comment

Choose a reason for hiding this comment

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

If PyClientRuntime is removed, how will we work with Jupyter Notebooks?

jina/clients/__init__.py
options=[('grpc.max_send_message_length', -1),
('grpc.max_receive_message_length', -1)]) as channel:
stub = jina_pb2_grpc.JinaRPCStub(channel)
self.logger.success(f'connected to the gateway at {self.args.host}:{self.args.port_expose}!')
Copy link
Contributor

Choose a reason for hiding this comment

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

await channel.ready() here before the success message

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/grpc/grpc/blob/master/examples/python/helloworld/async_greeter_client.py

jina/helper.py Show resolved Hide resolved
jina/peapods/peas/__init__.py Show resolved Hide resolved
bwanglzu
bwanglzu reviewed Dec 13, 2020
View reviewed changes
Copy link
Member

@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

how do you think to maintain pyclient as a separate project outside jina, something like jina-py.

@hanxiao
refactor(client): remove unnecessary runtime
dda4389
@hanxiao
refactor(client): remove unnecessary runtime
381f78b
@hanxiao
refactor(client): remove unnecessary runtime
c9bdd6a
@hanxiao
refactor(types): move append and extend into QueryLangSet
f3c2c7c
@hanxiao
refactor(client): remove unnecessary runtime
6d88c0f
@hanxiao
refactor(types): move append and extend into QueryLangSet
a5918d4
@hanxiao
refactor(types): move append and extend into QueryLangSet
0f94e61
@hanxiao
refactor(types): move append and extend into QueryLangSet
0ac252e
@hanxiao
refactor(types): move append and extend into QueryLangSet
96bbd8b
@hanxiao
refactor(types): move append and extend into QueryLangSet
01ee587
@hanxiao
refactor(types): move append and extend into QueryLangSet
30ac173
@hanxiao
refactor(types): move append and extend into QueryLangSet
8307b14
@hanxiao
refactor(types): move append and extend into QueryLangSet
fac6de4
@hanxiao
refactor(types): move append and extend into QueryLangSet
52b1fef
@hanxiao hanxiao marked this pull request as ready for review December 13, 2020 22:32
@codecov
Copy link

codecov bot commented Dec 13, 2020

Codecov Report

Merging #1450 (52b1fef) into master (46aaa5b) will increase coverage by 1.36%.
The diff coverage is 96.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1450      +/-   ##
==========================================
+ Coverage   81.43%   82.79%   +1.36%     
==========================================
  Files         112      108       -4     
  Lines        6616     6308     -308     
==========================================
- Hits         5388     5223     -165     
+ Misses       1228     1085     -143
Impacted Files Coverage Δ
jina/clients/sugary_io.py 100.00% <ø> (ø)
jina/peapods/peas/gateway/rest/__init__.py 93.75% <93.75%> (ø)
jina/peapods/peas/gateway/grpc/__init__.py 92.15% <94.59%> (+14.60%) ⬆️
jina/logging/profile.py 68.06% <97.05%> (+11.59%) ⬆️
jina/clients/helper.py 79.68% <100.00%> (ø)
jina/clients/request.py 90.76% <100.00%> (ø)
jina/docker/hubio.py 71.84% <100.00%> (-0.08%) ⬇️
jina/excepts.py 100.00% <100.00%> (ø)
jina/helloworld/helper.py 91.48% <100.00%> (ø)
jina/peapods/peas/__init__.py 90.68% <100.00%> (-0.35%) ⬇️
... and 9 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 46aaa5b...52b1fef. Read the comment docs.

@hanxiao hanxiao merged commit 3db7659 into master Dec 13, 2020
@hanxiao hanxiao deleted the refactor-pyclient-wrapper branch December 13, 2020 22:52
@hanxiao hanxiao mentioned this pull request Dec 13, 2020
Merged
This was referenced Dec 14, 2020
Closed
Closed
@maximilianwerk
Copy link
Member

@deepankarm This refactoring broke jinad, which needs to be fixed.

This was referenced Dec 14, 2020
Closed
Closed
@nan-wang nan-wang added this to the v0.9 New Features milestone Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwanglzu bwanglzu bwanglzu left review comments

@deepankarm deepankarm deepankarm left review comments

@imsergiy imsergiy Awaiting requested review from imsergiy

@JoanFM JoanFM Awaiting requested review from JoanFM

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/helloworld This issue/PR affects the helloworld area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/client component/docker component/flow component/logging component/peapod component/type size/XL
Projects
None yet
Milestone
v0.9 New Features
Development

Successfully merging this pull request may close these issues.

PyClientRunTime timeout is not enough (maybe needs to be configurable)
6 participants
@hanxiao @maximilianwerk @deepankarm @bwanglzu @nan-wang @jina-bot