Repository: jina-ai/serve
Pull request: fix: warn user when calling load config from flow instance
fixing https://github.com/jina-ai/jina/issues/4773

Now when doing 

```python
from jina import Flow

Flow().load_config('config.yaml')
```

you got this warning 

```
UserWarning: Calling load_config from a Flow instance will override all of its initial parameters. You should probably use `Flow.load_config(...)` instead
```

Problem statement:
It shoud not be possible to pass argument to the Flow init function when loading from a yaml file
It shoud not be possible to pass argument to the Flow init function when loading from a yaml file

```python
from jina import Flow
f = Flow(port=8080).load_config('config.yaml')
```

Here port=8080 could be override because load config will override everything.

load config should be a static method, i.e we should not be able to call it on an instance. 

How do we keep backward compatibility ? Maybe deprecated the class method and add a warning

Hints from issue discussion:

Commit message: fix: fix typo

Patch to review:
diff --git a/jina/orchestrate/flow/base.py b/jina/orchestrate/flow/base.py
index af3620dc75d44..97b18393c4f4d 100644
--- a/jina/orchestrate/flow/base.py
+++ b/jina/orchestrate/flow/base.py
@@ -1,6 +1,7 @@
 import argparse
 import base64
 import copy
+import inspect
 import json
 import multiprocessing
 import os
@@ -8,6 +9,7 @@
 import threading
 import time
 import uuid
+import warnings
 from collections import OrderedDict
 from contextlib import ExitStack
 from typing import (
@@ -96,7 +98,11 @@ class FlowType(type(ExitStack), type(JAMLCompatible)):
 ]
 
 
-class Flow(PostMixin, JAMLCompatible, ExitStack, metaclass=FlowType):
+class _BaseClassFlow(PostMixin, JAMLCompatible, ExitStack, metaclass=FlowType):
+    ...
+
+
+class Flow(_BaseClassFlow):
     """Flow is how Jina streamlines and distributes Executors."""
 
     # overload_inject_start_client_flow
@@ -2037,3 +2043,15 @@ def update_network_interface(self, **kwargs):
         :param kwargs: new network settings
         """
         self._common_kwargs.update(kwargs)
+
+    def __getattribute__(self, item):
+        obj = super(_BaseClassFlow, self).__getattribute__(item)
+
+        if (
+            item == 'load_config' and inspect.ismethod(obj) and obj.__self__ is Flow
+        ):  # check if obj load config call from an instance and not the Class
+            warnings.warn(
+                'Calling load_config from a Flow instance will override all of its initial parameters. You should probably use `Flow.load_config(...)` instead'
+            )
+
+        return obj
diff --git a/tests/integration/issues/github_4773/config.yaml b/tests/integration/issues/github_4773/config.yaml
new file mode 100644
index 0000000000000..3eff2a7147234
--- /dev/null
+++ b/tests/integration/issues/github_4773/config.yaml
@@ -0,0 +1 @@
+jtype: Flow
diff --git a/tests/integration/issues/github_4773/test_load_config.py b/tests/integration/issues/github_4773/test_load_config.py
new file mode 100644
index 0000000000000..0ad13b4771165
--- /dev/null
+++ b/tests/integration/issues/github_4773/test_load_config.py
@@ -0,0 +1,19 @@
+import warnings
+
+import pytest
+
+from jina import Flow
+
+
+def test_load_config_instance():
+    with pytest.warns(UserWarning):  # assert that a warning has been emited
+        f = Flow().load_config('config.yaml')
+    f.build()
+
+
+def test_load_config_class():  # assert that no warnings were emited
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        f = Flow.load_config('config.yaml')
+
+    f.build()
diff --git a/tests/integration/override_config_params/worker/test_override_config_params.py b/tests/integration/override_config_params/worker/test_override_config_params.py
index e1507fa553e4c..8584039c215c7 100644
--- a/tests/integration/override_config_params/worker/test_override_config_params.py
+++ b/tests/integration/override_config_params/worker/test_override_config_params.py
@@ -2,8 +2,7 @@
 
 import pytest
 
-from jina import Executor, Client, requests
-from jina import Flow, Document
+from jina import Client, Document, Executor, Flow, requests
 
 cur_dir = os.path.dirname(os.path.abspath(__file__))
 exposed_port = 12345
@@ -13,7 +12,7 @@
 def flow(request):
     flow_src = request.param
     if flow_src == 'flow-yml':
-        return Flow().load_config(os.path.join(cur_dir, 'flow.yml'))
+        return Flow.load_config(os.path.join(cur_dir, 'flow.yml'))
     elif flow_src == 'uses-yml':
         return Flow(port=exposed_port).add(
             uses=os.path.join(cur_dir, 'default_config.yml'),
diff --git a/tests/unit/parsers/test_warnings.py b/tests/unit/parsers/test_warnings.py
index 1b4777100f4a3..b4fcfc8ddb6ef 100644
--- a/tests/unit/parsers/test_warnings.py
+++ b/tests/unit/parsers/test_warnings.py
@@ -1,6 +1,6 @@
 import pytest
 
-from jina import Flow, Executor
+from jina import Executor, Flow
 
 
 class MyExecutor(Executor):
@@ -16,7 +16,7 @@ def test_flow_with_warnings():
     foo: bar
     '''
     with pytest.warns(UserWarning, match='ignored unknown') as record:
-        Flow().load_config(yaml)
+        Flow.load_config(yaml)
     assert len(record) == 1
     assert record[0].message.args[0].startswith('ignored unknown')
 
@@ -28,7 +28,7 @@ def test_executor_warnings():
     - foo: bar
     '''
     with pytest.warns(UserWarning, match='ignored unknown') as record:
-        Flow().load_config(yaml)
+        Flow.load_config(yaml)
     assert len(record) == 1
     assert record[0].message.args[0].startswith('ignored unknown')
 
@@ -41,7 +41,7 @@ def test_executor_with_warnings():
         foo: bar
     '''
     with pytest.warns(UserWarning, match='ignored unknown') as record:
-        Flow().load_config(yaml)
+        Flow.load_config(yaml)
     assert len(record) == 1
     assert record[0].message.args[0].startswith('ignored unknown')
 
@@ -54,7 +54,7 @@ def test_executor_uses_with_works():
         foo: bar
     '''
     with pytest.warns(None, match='ignored unknown') as record:
-        Flow().load_config(yaml)
+        Flow.load_config(yaml)
     assert len(record) == 0
 
 
@@ -65,7 +65,7 @@ def test_executor_override_with_warnings():
     - override_with: 1
     '''
     with pytest.warns(None, match='ignored unknown') as record:
-        Flow().load_config(yaml)
+        Flow.load_config(yaml)
     assert len(record) == 1
 
 
@@ -79,7 +79,7 @@ def test_executor_metas_works():
             name: MyExecutor
     '''
     with pytest.warns(None, match='ignored unknown') as record:
-        with Flow().load_config(yaml):
+        with Flow.load_config(yaml):
             pass
     assert len(record) == 0
 


Code review comments:
diff_hunk: @@ -0,0 +1,19 @@
+import warnings
+
+import pytest
+
+from jina import Flow
+
+
+def test_load_config_instance():
+    with pytest.warns(UserWarning):  # assert that a warning has been emited
+        f = Flow().load_config('config.yaml')
+    f.build()
path: tests/integration/issues/github_4773/test_load_config.py
text: @user1:
no need to build

diff_hunk: @@ -96,7 +98,11 @@ class FlowType(type(ExitStack), type(JAMLCompatible)):
 ]
 
 
-class Flow(PostMixin, JAMLCompatible, ExitStack, metaclass=FlowType):
+class _BaseClassFlow(PostMixin, JAMLCompatible, ExitStack, metaclass=FlowType):
path: jina/orchestrate/flow/base.py
text: @user1:
why is this new class needed?

@author:
I need this class here : https://github.com/jina-ai/jina/blob/b70aa02900aa3957531719e3b88a00dc25b447de/jina/orchestrate/flow/base.py#L2048

Otherwise I can't have a default `__getattribute__` function which I overload to check the if the function called is `load_config`

@user1:
u can not get it from object? just doing `super().__getattribute__(item)`? It would surprise me this is not possible

@author:
yes ofc

diff_hunk: @@ -0,0 +1,19 @@
+import warnings
+
+import pytest
+
+from jina import Flow
+
+
+def test_load_config_instance():
+    with pytest.warns(UserWarning):  # assert that a warning has been emited
+        f = Flow().load_config('config.yaml')
+    f.build()
+
+
+def test_load_config_class():  # assert that no warnings were emited
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        f = Flow.load_config('config.yaml')
+
+    f.build()
path: tests/integration/issues/github_4773/test_load_config.py
text: @user1:
no need to build