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(gateway): websocket based streaming and client #1608

Merged
merged 20 commits into from
Jan 9, 2021

Conversation

deepankarm
Copy link
Contributor

@deepankarm deepankarm commented Jan 6, 2021

Recreated duplicate of #1551

Gateway

Added a StreamingEndpoint (WS /stream) to RESTGateway

Client

Added WebSocketClient & AsyncWebSocketClient to stream requests from StreamingEndpoint for Flow & AsyncFlow respectively

Flow

Enabled websocket based clients for REST gateway

image

Notes & Issues:

  • With the current implementation, gateway is blocking, as we wait for each response to come back on the websocket (FIXED)
  • Indexing/searching for flows with needs fail - Flow().add().add(needs='gateway') (TO BE FIXED)
  • Unlike gRPC we cannot send an arbitrary python iterator via websockets, hence every request/response is converted to bytes or json at the gateway & the client. Hence the performance might be poor compared to gRPC.
  • Gateway expects the input to be in bytes only for now. Need to add support for json
  • The reason for having a different way of send & recv in websockets when compared to grpc.async_call - websockets-comment
  • The reason for selecting wsproto library over websockets with UvicornServer, which adds a new dependency to jina - This PR was never merged in uvicorn.

@deepankarm deepankarm requested a review from a team as a code owner January 6, 2021 10:46
@jina-bot jina-bot added size/M area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality area/setup This issue/PR affects setting up Jina component/client component/flow component/peapod labels Jan 6, 2021
@deepankarm deepankarm marked this pull request as draft January 6, 2021 10:48
@github-actions
Copy link

github-actions bot commented Jan 6, 2021

Latency summary

Current PR yields:

  • 😶 index QPS at 1814, delta to last 3 avg.: +0%
  • 😶 query QPS at 34, delta to last 3 avg.: -1%

Breakdown

Version Index QPS Query QPS
current 1814 34
0.9.3 1802 34
0.9.2 1826 34
0.9.1 1834 34

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

@jina-bot jina-bot added size/L area/testing This issue/PR affects testing and removed size/M labels Jan 6, 2021
@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #1608 (f2e866d) into master (2470c38) will decrease coverage by 9.78%.
The diff coverage is 83.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1608      +/-   ##
==========================================
- Coverage   84.82%   75.03%   -9.79%     
==========================================
  Files         126      128       +2     
  Lines        6589     6713     +124     
==========================================
- Hits         5589     5037     -552     
- Misses       1000     1676     +676     
Flag Coverage Δ
cd ?
ci 75.03% <83.81%> (?)
core 75.03% <83.81%> (-9.79%) ⬇️

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

Impacted Files Coverage Δ
jina/clients/request.py 86.11% <0.00%> (-5.56%) ⬇️
jina/docker/hubapi.py 65.40% <0.00%> (-20.76%) ⬇️
jina/executors/indexers/cache.py 77.14% <ø> (-20.00%) ⬇️
jina/peapods/runtimes/jinad/api.py 55.26% <0.00%> (ø)
jina/docker/hubio.py 66.25% <33.33%> (-7.34%) ⬇️
jina/flow/asyncio.py 68.18% <75.00%> (-0.12%) ⬇️
jina/flow/base.py 85.06% <87.50%> (-1.64%) ⬇️
jina/peapods/runtimes/asyncio/rest/app.py 88.23% <88.23%> (ø)
jina/clients/websockets.py 90.47% <90.47%> (ø)
jina/clients/asyncio.py 77.77% <100.00%> (+2.77%) ⬆️
... and 60 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 2470c38...8e64df9. Read the comment docs.

@jina-bot jina-bot added area/cicd This issue/PR affects the cicd pipeline area/housekeeping This issue/PR is housekeeping labels Jan 6, 2021
@nan-wang nan-wang added this to the v1.0.0 New Features milestone Jan 7, 2021
@deepankarm deepankarm marked this pull request as ready for review January 7, 2021 10:19

async def decode(self, websocket: WebSocket, message: Message) -> Any:
if 'text' in message or 'json' in message:
self.client_encoding = 'text'
Copy link
Member

Choose a reason for hiding this comment

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

this could be a good test case to cover

@hanxiao
Copy link
Member

hanxiao commented Jan 9, 2021

im taking this branch

# Conflicts:
#	.github/workflows/ci.yml
#	extra-requirements.txt
#	tests/unit/flow/test_asyncflow.py
@hanxiao hanxiao merged commit e5df803 into master Jan 9, 2021
@hanxiao hanxiao deleted the feat-gateway-ws-streaming branch January 9, 2021 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cicd This issue/PR affects the cicd pipeline area/core This issue/PR affects the core codebase area/housekeeping This issue/PR is housekeeping area/network This issue/PR affects network functionality area/setup This issue/PR affects setting up Jina area/testing This issue/PR affects testing component/client component/docker component/executor component/flow component/peapod component/type executor/indexer executor/meta size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants