refactor: merge grpc and rest client #2565
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2565 +/- ##
===========================================
+ Coverage 61.73% 87.36% +25.62%
===========================================
Files 135 154 +19
Lines 9022 9649 +627
===========================================
+ Hits 5570 8430 +2860
+ Misses 3452 1219 -2233
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
jina/flow/base.py
Outdated
| @@ -912,7 +914,7 @@ def use_grpc_gateway(self, port: Optional[int] = None): | |||
|
|
|||
| def _switch_gateway(self, gateway: str, port: int): | |||
| restful = gateway == 'RESTRuntime' | |||
| client = WebSocketClient if gateway == 'RESTRuntime' else Client | |||
| client = WebSocketClient if gateway == 'RESTRuntime' else GrpcClient | |||
There was a problem hiding this comment.
Why can we not use Client here passing restful?
There was a problem hiding this comment.
@deepankarm we can, while it does not make a lot difference since Client is actually initialising GrpcClient and WebSocketClient
There was a problem hiding this comment.
then lets have a single poijt of entry, change it to Client with the proper argument, u can use partial functions if needed
There was a problem hiding this comment.
I meant, not using GrpcClient & WebSocketClient at all in flow/base.py and using Client directly. Not important though.
There was a problem hiding this comment.
gave a try, and I broke a lot of tests. I'll look into if I can optimise this part in a future PR.
There was a problem hiding this comment.
and I broke a lot of tests
I believe the reason was because your Client() function does not accept the full arguments that CLI defined (as I mentioned here #2565 (comment)). This oversimplification causes incomplete setup of the client and yields the broken test.
jina/clients/__init__.py
Outdated
| args = set_client_cli_parser().parse_args( | ||
| ['--host', host, '--port-expose', str(port_expose)] | ||
| ) |
There was a problem hiding this comment.
when written in this way, all other client arguments are not passed. better use **kwargs and use ArgNamespace.kwargs2list()
usage: jina client [-h] [--request-size] [--continue-on-error]
[--show-progress] [--return-results] [--host]
[--port-expose] [--proxy]
Start a Python client that connects to a remote Jina gateway
⚙️ Optional Arguments
-h, --help show this help message and exit
⚙️ Client Arguments
--request-size type: int; default: 100
The number of Documents in each Request.
--continue-on-error default: disabled, use "--continue-on-error" to
enable it
If set, a Request that causes error will be logged only
without blocking the further requests.
--show-progress default: disabled, use "--show-progress" to enable
it
If set, client will show a progress bar on receiving
every request.
--return-results default: disabled, use "--return-results" to enable
it
This feature is only used for AsyncClient.
If set, the results of all Requests will be returned as
a list. This is useful when one wants process Responses
in bulk instead of using callback.
⚙️ Expose Arguments
--host type: str; default: 0.0.0.0
The host address of the runtime, by default it is
0.0.0.0.
--port-expose type: int; default: 58570
The port of the host exposed to the public
--proxy default: disabled, use "--proxy" to enable it
If set, respect the http_proxy and https_proxy
environment variables. otherwise, it will unset these
proxy variables before start. gRPC seems to prefer no
proxy
jina/flow/base.py
Outdated
| runtime_cls='GRPCRuntime' | ||
| if self._cls_client == Client | ||
| if self._cls_client == GrpcClient |
There was a problem hiding this comment.
better uniform the case, GRPCRuntime then GRPCClient or GrpcRuntime and GrpcClient
|
Could you inject
|
|
note that, one has to run |
|
applied some changes to |
| @@ -76,37 +87,66 @@ def fill_overload( | |||
| return_type, | |||
| filepath, | |||
| overload_fn, | |||
| class_method, | |||
| indent=' ' * 4, | |||
There was a problem hiding this comment.
wait, isn't the modification the same as fill_overload(..., indent=' '*2)?
There was a problem hiding this comment.
can you revert it and simply pass indent=' '*2
There was a problem hiding this comment.
yes you're right, working on it now
There was a problem hiding this comment.
@hanxiao it's not only about indent, the script will also insert a self inside the method etc..and we have different levels of indent such as no indent (before overload and method name), 4 indents for docstrings etc..
this is what happened after i change indent = ' ' * 2:
This PR aims at simplify C/S mode (Flow as service) Jina Python client usage with a uniform facade, before and after: