Repository: jina-ai/serve
Pull request: fix: client default port 
# context:
Currently the port for the client is selected randomly the same way we do for the gateway. It does not makes sense.

https://github.com/jina-ai/jina/issues/4852

# What this pr do

The default port will be 80 unless tls is enable and then 443 should be the default.


Problem statement:
Client port should not be random
Currently the port for the client is selected randomly the same way we do for the gateway. It does not makes sense.

The default port should be 80 unless tls is enable and then 443 should be the default.

Hints from issue discussion:

Commit message: fix: remove random port

Patch to review:
diff --git a/jina/clients/__init__.py b/jina/clients/__init__.py
index 2035f2b8e465b..78abae26c5422 100644
--- a/jina/clients/__init__.py
+++ b/jina/clients/__init__.py
@@ -2,7 +2,7 @@
 import argparse
 from typing import TYPE_CHECKING, Optional, Union, overload
 
-from jina.helper import parse_client
+from jina.clients.helper import parse_client
 
 __all__ = ['Client']
 
diff --git a/jina/clients/base/__init__.py b/jina/clients/base/__init__.py
index b4005b54dab9a..f3e1819e99abb 100644
--- a/jina/clients/base/__init__.py
+++ b/jina/clients/base/__init__.py
@@ -6,8 +6,9 @@
 from abc import ABC
 from typing import TYPE_CHECKING, AsyncIterator, Callable, Iterator, Optional, Union
 
+from jina.clients.helper import parse_client
 from jina.excepts import BadClientInput
-from jina.helper import T, parse_client, typename
+from jina.helper import T, typename
 from jina.logging.logger import JinaLogger
 from jina.logging.predefined import default_logger
 
diff --git a/jina/clients/helper.py b/jina/clients/helper.py
index 365e83e02943d..1c8c28b02cb4d 100644
--- a/jina/clients/helper.py
+++ b/jina/clients/helper.py
@@ -1,11 +1,13 @@
 """Helper functions for clients in Jina."""
 
+from argparse import Namespace
 from functools import wraps
-from typing import Callable, Optional
+from typing import Any, Callable, Dict, Optional, Tuple
 
 from jina.excepts import BadClientCallback
-from jina.helper import get_rich_console
+from jina.helper import ArgNamespace, get_rich_console
 from jina.logging.logger import JinaLogger
+from jina.parsers import set_client_cli_parser
 from jina.proto import jina_pb2
 from jina.types.request.data import Response
 
@@ -105,3 +107,81 @@ def on_error_wrap(resp):
         on_error(resp, exception)
 
     _safe_callback(on_error_wrap, False, logger)(response)
+
+
+def parse_client(kwargs) -> Namespace:
+    """
+    Parse the kwargs for the Client
+
+    :param kwargs: kwargs to be parsed
+
+    :return: parsed argument.
+    """
+    kwargs = _parse_kwargs(kwargs)
+    args = ArgNamespace.kwargs2namespace(
+        kwargs, set_client_cli_parser(), warn_unknown=True
+    )
+
+    if not args.port:
+        args.port = 80 if not args.tls else 443
+
+    return args
+
+
+def _parse_kwargs(kwargs: Dict[str, Any]) -> Dict[str, Any]:
+    if 'host' in kwargs.keys():
+        return_scheme = dict()
+        (
+            kwargs['host'],
+            return_scheme['port'],
+            return_scheme['protocol'],
+            return_scheme['tls'],
+        ) = _parse_host_scheme(kwargs['host'])
+
+        for key, value in return_scheme.items():
+            if value:
+                if key in kwargs:
+                    raise ValueError(
+                        f"You can't have two definitions of {key}: you have one in the host scheme and one in the keyword argument"
+                    )
+                elif value:
+                    kwargs[key] = value
+
+    kwargs = _delete_host_slash(kwargs)
+
+    return kwargs
+
+
+def _delete_host_slash(kwargs: Dict[str, Any]) -> Dict[str, Any]:
+    if 'host' in kwargs:
+        if kwargs['host'][-1] == '/':
+            kwargs['host'] = kwargs['host'][:-1]
+    return kwargs
+
+
+def _parse_host_scheme(host: str) -> Tuple[str, str, str, bool]:
+    scheme, _hostname, port = _parse_url(host)
+
+    tls = None
+    if scheme in ('grpcs', 'https', 'wss'):
+        scheme = scheme[:-1]
+        tls = True
+
+    if scheme == 'ws':
+        scheme = 'websocket'
+
+    return _hostname, port, scheme, tls
+
+
+def _parse_url(host):
+    if '://' in host:
+        scheme, host = host.split('://')
+    else:
+        scheme = None
+
+    if ':' in host:
+        host, port = host.split(':')
+    else:
+        port = None
+
+    return scheme, host, port
diff --git a/jina/helper.py b/jina/helper.py
index f8dfa50eccc08..1261c8545497f 100644
--- a/jina/helper.py
+++ b/jina/helper.py
@@ -1525,82 +1525,6 @@ def get_rich_console():
     )
 
 
-from jina.parsers import set_client_cli_parser
-
-
-def parse_client(kwargs):
-    kwargs = _parse_kwargs(kwargs)
-    return ArgNamespace.kwargs2namespace(
-        kwargs, set_client_cli_parser(), warn_unknown=True
-    )
-
-
-def _parse_kwargs(kwargs: Dict[str, Any]) -> Dict[str, Any]:
-    if 'host' in kwargs.keys():
-        return_scheme = dict()
-        (
-            kwargs['host'],
-            return_scheme['port'],
-            return_scheme['protocol'],
-            return_scheme['tls'],
-        ) = _parse_host_scheme(kwargs['host'])
-
-        for key, value in return_scheme.items():
-            if value:
-                if key in kwargs:
-                    raise ValueError(
-                        f"You can't have two definitions of {key}: you have one in the host scheme and one in the keyword argument"
-                    )
-                elif value:
-                    kwargs[key] = value
-
-    kwargs = _add_default_port_tls(kwargs)
-    kwargs = _delete_host_slash(kwargs)
-
-    return kwargs
-
-
-def _add_default_port_tls(kwargs: Dict[str, Any]) -> Dict[str, Any]:
-    if ('tls' in kwargs) and ('port' not in kwargs):
-        kwargs['port'] = 443
-    return kwargs
-
-
-def _delete_host_slash(kwargs: Dict[str, Any]) -> Dict[str, Any]:
-    if 'host' in kwargs:
-        if kwargs['host'][-1] == '/':
-            kwargs['host'] = kwargs['host'][:-1]
-    return kwargs
-
-
-def _parse_host_scheme(host: str) -> Tuple[str, str, str, bool]:
-    scheme, _hostname, port = _parse_url(host)
-
-    tls = None
-    if scheme in ('grpcs', 'https', 'wss'):
-        scheme = scheme[:-1]
-        tls = True
-
-    if scheme == 'ws':
-        scheme = 'websocket'
-
-    return _hostname, port, scheme, tls
-
-
-def _parse_url(host):
-    if '://' in host:
-        scheme, host = host.split('://')
-    else:
-        scheme = None
-
-    if ':' in host:
-        host, port = host.split(':')
-    else:
-        port = None
-
-    return scheme, host, port
-
-
 def is_port_free(host, port):
     with socket(AF_INET, SOCK_STREAM) as session:
         if session.connect_ex((host, port)) == 0:
diff --git a/jina/parsers/orchestrate/runtimes/remote.py b/jina/parsers/orchestrate/runtimes/remote.py
index 530a1b9b45450..0897cc0cb0c33 100644
--- a/jina/parsers/orchestrate/runtimes/remote.py
+++ b/jina/parsers/orchestrate/runtimes/remote.py
@@ -23,7 +23,7 @@ def mixin_client_gateway_parser(parser):
     gp.add_argument(
         '--port',
         type=int,
-        default=helper.random_port(),
+        default=None,
         help='The port of the Gateway, which the client should connect to.',
     )
 
diff --git a/tests/unit/clients/test_interface.py b/tests/unit/clients/test_interface.py
index 9b37f5cdb510a..f01b05d90f07e 100644
--- a/tests/unit/clients/test_interface.py
+++ b/tests/unit/clients/test_interface.py
@@ -37,7 +37,7 @@ def test_host_unpacking(protocol, gateway_type, tls, hostname):
 
 @pytest.mark.parametrize('protocol', ['https', 'grpcs', 'wss'])
 @pytest.mark.parametrize('port', [1234, None])
-def test_host_unpacking(protocol, port):
+def test_host_unpacking_port_tls(protocol, port):
 
     port_scheme = f':{port}' if port else ''
 
@@ -48,9 +48,9 @@ def test_host_unpacking(protocol, port):
     assert c.args.port == port if port else 443
 
 
-@pytest.mark.parametrize('protocol', ['https', 'grpcs', 'wss'])
+@pytest.mark.parametrize('protocol', ['http', 'grpc', 'ws'])
 @pytest.mark.parametrize('port', [1234, None])
-def test_host_unpacking(protocol, port):
+def test_host_unpacking_port(protocol, port):
 
     port_scheme = f':{port}' if port else ''
 
@@ -58,7 +58,7 @@ def test_host_unpacking(protocol, port):
 
     c = Client(host=host)
 
-    assert c.args.port == port if port else 443
+    assert c.args.port == port if port else 80
 
 
 def test_delete_slash_host():
@@ -70,7 +70,7 @@ def test_delete_slash_host():
     assert c.args.host == 'localhost'
 
 
-def test_host_unpacking_port():
+def test_host_unpacking_basic():
 
     protocol = 'http'
     hostname = 'localhost'


Code review comments:
diff_hunk: @@ -105,3 +107,81 @@ def on_error_wrap(resp):
         on_error(resp, exception)
 
     _safe_callback(on_error_wrap, False, logger)(response)
+
+
+def parse_client(kwargs) -> Namespace:
+    """
+    Parse the kwargs for the Client
+
+    :param kwargs: kwargs to be parsed
+
+    :return: parsed argument.
+    """
+    kwargs = _parse_kwargs(kwargs)
+    args = ArgNamespace.kwargs2namespace(
+        kwargs, set_client_cli_parser(), warn_unknown=True
+    )
+
+    if not args.port:
+        args.port = 80 if not args.tls else 443
path: jina/clients/helper.py
text: @user1:
can these magic numbers be specified elsewhere with comment?

@author:
sure

diff_hunk: @@ -105,3 +107,81 @@ def on_error_wrap(resp):
         on_error(resp, exception)
 
     _safe_callback(on_error_wrap, False, logger)(response)
+
path: jina/clients/helper.py
text: @user1:
no need to move this, maybe some other application is using this and we are breaking it for a very small win

@author:
The public function used here is called `parse_client` so it can only be used for the client for me it makes sense that that it belongs to the jina.client.helper

I created this function in the wrong place initially.

What we can keep in the jina.helper is the logic of parsing the host, i.e `host=http://localhost:443` to `port=443`,`host=localhost` and `protocol=http`. But then I should refactor the private functions to make them more general. Right now it mixed client logic in it as well