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.

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

feat(async): add AsyncClient and AsyncFlow #1463

Merged
merged 7 commits into from
Dec 15, 2020
Merged

Conversation

hanxiao
Copy link
Member

@hanxiao hanxiao commented Dec 15, 2020

see docstring for details

@hanxiao hanxiao requested a review from a team as a code owner December 15, 2020 10:00
@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase 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/peapod labels Dec 15, 2020
@github-actions
Copy link

github-actions bot commented Dec 15, 2020

Latency summary

Current PR yields:

  • 😶 index QPS at 1909, delta to last 3 avg.: -4%
  • 🐎🐎 query QPS at 37, delta to last 3 avg.: +9%

Breakdown

Version Index QPS Query QPS
current 1909 37
0.8.6 2009 33
0.8.5 2003 33

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

@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #1463 (4b1e4bf) into master (46aaa5b) will increase coverage by 1.36%.
The diff coverage is 90.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1463      +/-   ##
==========================================
+ Coverage   81.43%   82.80%   +1.36%     
==========================================
  Files         112      110       -2     
  Lines        6616     6381     -235     
==========================================
- Hits         5388     5284     -104     
+ Misses       1228     1097     -131     
Impacted Files Coverage Δ
jina/clients/sugary_io.py 100.00% <ø> (ø)
jina/parser.py 86.28% <ø> (ø)
jina/flow/asyncio.py 58.06% <58.06%> (ø)
jina/clients/asyncio.py 71.42% <71.42%> (ø)
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/executors/indexers/vector.py 88.64% <96.66%> (+0.99%) ⬆️
jina/clients/helper.py 79.68% <100.00%> (ø)
jina/clients/request.py 91.30% <100.00%> (ø)
jina/docker/hubio.py 71.84% <100.00%> (-0.08%) ⬇️
... and 19 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 1236a6b...4b1e4bf. Read the comment docs.

jina/helper.py Outdated
from .excepts import BadClient
raise BadClient('something wrong when running the eventloop, result can not be retrieved')
else:
raise RuntimeError('you have an eventloop running but not using jupyter/ipython, '
Copy link
Contributor

Choose a reason for hiding this comment

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

jinad runs inside fastapi+uvicorn which has a default event loop (again based on uvloop). Why do we want to raise a RuntimeError?

@jina-bot jina-bot added size/L area/entrypoint This issue/PR affects the entrypoint codebase and removed size/S labels Dec 15, 2020
@hanxiao hanxiao changed the title fix: jupyter exception handling feat(async): add AsyncClient and AsyncFlow Dec 15, 2020
import numpy as np


class AsyncFlow(Flow):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we implement an async context manager for AsyncFlow?

async with AsyncFlow().add() as f:
   await f.index(['abc'])

Copy link
Member Author

Choose a reason for hiding this comment

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

no. async with is only implemented when one wants to waive the control during __enter__ __exit__. We don't have such needs.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is because we have taken special care not to propagate the async...await to the caller. e.g. - Client and AsyncClient should be context managers, so that they can be used outside a Flow with ease. Similarly GatewayPea & RESTGatewayPea can be an async context manager, even though I don't see heavy usage for the same.

@hanxiao hanxiao merged commit 08fa53e into master Dec 15, 2020
@hanxiao hanxiao deleted the fix-jupyter-except branch December 15, 2020 14:28
@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
Labels
area/core This issue/PR affects the core codebase area/entrypoint This issue/PR affects the entrypoint codebase 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/flow component/peapod size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants