feat: native async based grpc & fastapi based rest for gateway #1348

deepankarm · 2020-11-25T07:28:33Z

Following changes are done in this PR

Enable native async based gRPC for gateway server (fixes Refactoring async gRPC servlet from ThreadPoolExecutor to native asyncio support #1287)
Change PyClient to read from grpc.aio.channel
Move from Flask + gevent to FastAPI + uvicorn based REST gateway
Move the gateway logic to a separate process for both gRPC & REST (fixes DELETE /flow with rest based gateway kills jinad server jinad#9)
Move PyClient to a separate process/thread

Side Effects:

gRPC

Moved from custom AsyncExecutor to native asyncio for gRPC
Previously server was async, whereas PyClient was sync. With this PR, PyClient becomes async (call, call_unary, dry_run, index, search, train are now coroutines.

REST

FastAPI & uvicorn support native async capabilities, rather than solving it in a hacky way with gevent.WSGIServer.
FastAPI also claims much higher performance benefits (performance comparison to be done)
With gRPC, we have native streaming + prefetch ability, which was not there with REST. This can now be done wih the help of websockets on FastAPI server.

PyClient

PyClient (and hence py_client) is now an awaitable.
We enable PyClientRuntime, which executes PyClient to a separate process to avoid an event loop in the main process.
PyClient (optionally) sends the gRPC response to a zmq socket, from which main process can read & execute callbacks. (This is the default way of when we do flow.index, flow.search etc)

JinaD

Since gateway now lives on a separate process for both gRPC & REST, flow invocation logic now can move from def to async def for jinad (Difference)
Deleting a Flow wouldn't kill jinad now

CtrlZmqlet

Zmqlet with only PAIR socket (capable of BIND or CONNECT) to be tied with all runtimes that don't have any Peas related logic (e.g. - gateway-grpc, gateway-rest, remote, client)

Jupyter Notebook

We can now build & run Flows in Jupyter. This is possible as we avoid event loops in the main process (avoiding collision with Jupyter's default event loop). Fixes Jupyter notebook & lab kernel restarting without obvious reason when working with Jina #1277

Pending

pytest.asyncio tests for PyClient would still cause segmentation faults, as that brings grpc.aio channel (hence the event loop) on main process. This needs to be handled.
Do performance tests for gRPC + REST based changes
Make changes to jinad (def to async def)
Discuss about async propagation in core (whereto await vs run_until_complete a coroutine)

codecov · 2020-11-25T07:47:09Z

Codecov Report

Merging #1348 (1a866d7) into master (9de90e6) will decrease coverage by 0.83%.
The diff coverage is 88.96%.

@@            Coverage Diff             @@
##           master    #1348      +/-   ##
==========================================
- Coverage   83.33%   82.49%   -0.84%     
==========================================
  Files         104      106       +2     
  Lines        6869     6980     +111     
==========================================
+ Hits         5724     5758      +34     
- Misses       1145     1222      +77

Impacted Files	Coverage Δ
jina/clients/python/grpc.py	`83.09% <85.29%> (+15.30%)`	⬆️
jina/peapods/gateway/grpc.py	`85.91% <85.91%> (ø)`
jina/peapods/gateway/servicer.py	`88.23% <88.23%> (ø)`
jina/peapods/gateway/rest.py	`88.88% <88.88%> (ø)`
jina/peapods/zmq.py	`84.42% <93.02%> (+3.31%)`	⬆️
jina/clients/python/helper.py	`87.00% <100.00%> (+1.44%)`	⬆️
jina/peapods/pea.py	`92.85% <100.00%> (+1.13%)`	⬆️
jina/peapods/pod.py	`83.33% <100.00%> (+0.05%)`	⬆️
jina/peapods/grpc_asyncio.py	`0.00% <0.00%> (-75.52%)`	⬇️
jina/logging/sse.py	`91.93% <0.00%> (-3.23%)`	⬇️
... and 7 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 83edba0...1a866d7. Read the comment docs.

JoanFM · 2020-11-25T08:42:14Z

jina/peapods/gateway.py

+class AsyncGatewayPea:
+    def __init__(self, args):
+        if not args.proxy and os.name != 'nt':
+            os.unsetenv('http_proxy')


This feels like users may get very mad at us?

Why is this needed?

Agree. Unsetting http_proxy and https_proxy sounds really bad.

it is needed since GNES @deepankarm @nan-wang @JoanFM please

read the old code

when you running grpc with http and https proxy you will find grpc won't work anymore.

always keep in mind os.environ is set on process base not globally, there is nothing people get mad at us, because nothing change in global

anytime, when you doubt if it adds any trouble to users, use warning.warn

please always understand grpc is on HTTP2 layer and that's why http proxy setting affects it. if you don't know about this, read grpc doc.

if you really want to solve this elegantly, i suggest looking for issue help/posting issue on grpc github. back in early 2019 i spent a day on this and this was the best solution. things may change, so don't be shy to leverage OSS community.

also please fix the misconception that os.environ changes anything globally. it is a variable shared by that process you spawned and all subprocesses. Fix this misconception is important otherwise all os.environ will look weird to you.

until then, keep this lines. Many companies have internal proxy, without this line, Jina won't work for them.

Are we sure this init lives in another process?

Shouldn't it be moved into start?

JoanFM · 2020-11-25T08:43:58Z

jina/peapods/pod.py

@@ -556,6 +556,25 @@ def start(self) -> 'GatewayPod':
        return self


+class AsyncGatewayPod(BasePod, AsyncExitStack):


Will this play nice with inheriting both ỀxitStack and AsyncExitStack (I think is a good point I may try to have a mixin in the refactor for ExitStack so that it is more separated from BasePod.

class DualContext(ExitStack, AsyncExitStack): def __init__(self) -> None: super(ExitStack, self).__init__() super(AsyncExitStack, self).__init__() def start(self): raise NotImplementedError def close(self): raise NotImplementedError async def astart(self): raise NotImplementedError async def aclose(self): raise NotImplementedError def __enter__(self): return self.start() def __exit__(self, exc_type, exc_val, exc_tb): self.close() super(ExitStack, self).__exit__(exc_type, exc_val, exc_tb) async def __aenter__(self): return await self.astart() async def __aexit__(self, exc_type, exc_val, exc_tb): await self.aclose() await super(AsyncExitStack, self).__aexit__(exc_type, exc_val, exc_tb) class SyncContext(DualContext): def start(self): print('Sync') def close(self): print('Sync close') class AsyncContext(DualContext): async def astart(self): print('Async') async def aclose(self): print('Async close') with SyncContext(): pass async def blah(): async with AsyncContext(): pass asyncio.run(blah())

This works, maybe we can use something similar

JoanFM · 2020-11-29T07:25:52Z

Zmqlet with only ctrl socket (AsyncCtrlZmqlet) -

main process fetches the ctrl address

loop_body in the event loop can await AsyncCtrlZmqlet (which awaits the terminate signal on the ctrl socket)

upon termination signal, loop_termination kicks in (killing grpc/rest servers, sending delete signal to jinad etc)

every jina-process which is not a local pea with zmqstreamlet can inherit this (remote, gateway-grpc, gateway-rest)

ctrl address can be used for termination

doesn't need input/output sockets

i'm assuming, this will always be local. we should be good with ipc only (to avoid clash)?

I think assuming is always local w.r.t the Flow is a valid assumption right now

hanxiao · 2020-11-29T10:14:05Z

the test is broken

* refactor(gateway): clean code, move file structure * refactor(proto): jina primitive types

tests/unit/clients/python/test_client.py

JoanFM · 2020-12-05T12:05:47Z

jina/clients/__init__.py

+                while True:
+                    try:
+                        msg = zmqlet.sock.recv()
+                        if msg == b'TERMINATE':


when is the client sending TERMINATE I haven't seen this happen, it is done between Peas but not from client right?

PyClient would send a terminate signal to PyClientRuntime

it should be the other way around right? PyClient does not know it lives inside a PyClientRuntime? Why don't u use multiprocessing Events?

PyClient receives a stream of responses from gRPC servicer. To make sure the callback functions are executed back in the main process, PyClient sends each such response to PyClientRuntime. Once the stream is completed, PyClient sends a TERMINATE signal to PyClientRuntime. But you're right, this can be handled using an Event.

jina/clients/__init__.py

jina/clients/python/__init__.py

JoanFM

Codecov reports missing test for some cases specially when using ctxt management

JoanFM · 2020-12-06T07:28:18Z

jina/clients/python/grpc.py

-    def __exit__(self, exc_type, exc_val, exc_tb):
-        self.close()
+    async def __aexit__(self, exc_type, exc_val, exc_tb):
+        await self.close()


This aexit is not tested?

JoanFM · 2020-12-06T07:29:01Z

jina/clients/python/grpc.py

-            self.close()
+            # avoid closing a client after a single `index`, `search` or `train` operation
+            if 'close' in kwargs:
+                await self.close()


Test for this part exists?

JoanFM · 2020-12-06T07:30:11Z

jina/peapods/gateway/grpc.py

+        await self.is_gateway_ready.wait()
+        return self
+
+    async def __aenter__(self):


No test using it as ctxt manager

deepankarm · 2020-12-06T07:42:59Z

Codecov reports missing test for some cases specially when using ctxt management

Testing the context manager for AsyncGrpcClient (and PyClient) is not possible until we fix issues with pytest.asyncio. This is because, grpc.aio channel comes to the main process, and creates issues with segmentation faults intermittently. I'll raise an issue for this to keep track.

feat(grpc): async based context managers for gateway

064caa1

jina-bot added size/S area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality component/peapod labels Nov 25, 2020

JoanFM reviewed Nov 25, 2020

View reviewed changes

feat(grpc): async based context managers for gateway

3ad9d5d

jina-bot added size/M and removed size/S labels Nov 26, 2020

refactor(gateway): clean code, move file structure (#1371)

f3699ed

* refactor(gateway): clean code, move file structure * refactor(proto): jina primitive types

jina-bot added size/L area/testing This issue/PR affects testing component/client and removed size/M labels Nov 29, 2020

deepankarm added 3 commits November 30, 2020 18:07

feat(grpc): async based context managers for gateway

8415418

fix: fix return type

ef235d2

feat(rest): fastapi based rest gateway

9e0cfbb

jina-bot added area/setup This issue/PR affects setting up Jina component/resource labels Dec 1, 2020

deepankarm changed the title ~~feat: native async based grpc for gateway~~ feat: native async based grpc & fastapi based rest for gateway Dec 1, 2020

Merge branch 'master' into refactor-async-grpc

9325d6f

deepankarm marked this pull request as ready for review December 1, 2020 15:10

deepankarm requested a review from a team as a code owner December 1, 2020 15:10

deepankarm requested review from CatStark, yuanbit and hanxiao December 1, 2020 15:10

CatStark reviewed Dec 1, 2020

View reviewed changes

tests/unit/clients/python/test_client.py Show resolved Hide resolved

deepankarm added 6 commits December 3, 2020 01:13

fix: remove random import

c3534fc

test: disable pytest asyncio tests

eaf43d3

test: skip py client from tests

cdc613a

feat(client): separate async client to separate process

6dc37f8

refactor(gateway): misc changes

04b7331

test(client): changed to new invocation

283099b

jina-bot added size/XL and removed size/L labels Dec 4, 2020

deepankarm added 7 commits December 5, 2020 04:45

fix: remove random import

f1a381c

fix: issues with callback

d20391d

test: disable pytest asyncio tests

19d256e

test: fix module import

9177bf0

fix: zmq exit only on terminate

7cb2001

fix: pyclient exit logic

8dbc7c7

Merge branch 'master' into refactor-async-grpc

58f25a0

JoanFM reviewed Dec 5, 2020

View reviewed changes

deepankarm added 3 commits December 5, 2020 23:42

fix(zmq): resource unavailable issue

f41658b

fix(zmq): resource unavailable issue

1408645

Merge branch 'master' into refactor-async-grpc

e5fca34

JoanFM reviewed Dec 6, 2020

View reviewed changes

chore: remove debug comments

1a866d7

hanxiao approved these changes Dec 7, 2020

View reviewed changes

hanxiao merged commit 130dfbb into master Dec 7, 2020

hanxiao deleted the refactor-async-grpc branch December 7, 2020 11:40

hanxiao mentioned this pull request Dec 11, 2020

ci: add latency tracking #1437

Merged

nan-wang added this to the v0.9 New Features milestone Dec 23, 2020

This was referenced Feb 11, 2021

Flask is mentioned as an extra dependency when it should be fastapi jina-ai/docs#85

Closed

Flask is mentioned as an extra dependency when it should be fastapi #1931

Closed

numb3r3 mentioned this pull request Jan 19, 2022

fix(runtime): worker grpc proxy env #4189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: native async based grpc & fastapi based rest for gateway #1348

feat: native async based grpc & fastapi based rest for gateway #1348

deepankarm commented Nov 25, 2020 •

edited

Loading

codecov bot commented Nov 25, 2020 •

edited

Loading

JoanFM Nov 25, 2020

JoanFM Nov 25, 2020

nan-wang Nov 26, 2020

hanxiao Nov 26, 2020 •

edited

Loading

hanxiao Nov 26, 2020 •

edited

Loading

JoanFM Nov 26, 2020

JoanFM Nov 26, 2020

JoanFM Nov 25, 2020

deepankarm Nov 25, 2020

deepankarm Nov 25, 2020

JoanFM commented Nov 29, 2020

hanxiao commented Nov 29, 2020

JoanFM Dec 5, 2020

deepankarm Dec 5, 2020

JoanFM Dec 5, 2020

deepankarm Dec 5, 2020 •

edited

Loading

JoanFM left a comment

JoanFM Dec 6, 2020

JoanFM Dec 6, 2020

JoanFM Dec 6, 2020

deepankarm commented Dec 6, 2020

		@@ -556,6 +556,25 @@ def start(self) -> 'GatewayPod':
		return self


		class AsyncGatewayPod(BasePod, AsyncExitStack):

feat: native async based grpc & fastapi based rest for gateway #1348

feat: native async based grpc & fastapi based rest for gateway #1348

Conversation

deepankarm commented Nov 25, 2020 • edited Loading

Following changes are done in this PR

Side Effects:

gRPC

REST

PyClient

JinaD

CtrlZmqlet

Jupyter Notebook

Pending

codecov bot commented Nov 25, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanxiao Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

hanxiao Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanFM commented Nov 29, 2020

hanxiao commented Nov 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepankarm Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

JoanFM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepankarm commented Dec 6, 2020

deepankarm commented Nov 25, 2020 •

edited

Loading

codecov bot commented Nov 25, 2020 •

edited

Loading

hanxiao Nov 26, 2020 •

edited

Loading

hanxiao Nov 26, 2020 •

edited

Loading

deepankarm Dec 5, 2020 •

edited

Loading